Use truncation instead of rounding to resolve percentages into app units #102

Asun0204 · 2024-12-26T02:54:36Z

change conversion to appunit from float of percentage calcuation, prevent child box size is calculated too large.

Servo PR: servo/servo#34714

Loirooriol · 2025-01-08T14:01:13Z

Since we want to test this before publishing a new release of app units, let's inline the code here for now.

Loirooriol · 2025-01-08T14:02:41Z

BTW, when creating a PR it's better not to use your main branch.

Signed-off-by: Oriol Brufau <[email protected]>

Loirooriol

It may make more sense to modify this instead:

stylo/style/values/computed/length.rs

Lines 496 to 501 in 7e529e8

    
           impl From<CSSPixelLength> for Au { 
        
               #[inline] 
        
               fn from(len: CSSPixelLength) -> Self { 
        
                   Au::from_f32_px(len.0) 
        
               } 
        
           }

I thought that would be more likely to affect Firefox, but I have tried commenting that (and its callers) out, and Firefox compiles fine, so it's not used.

Asun0204 · 2025-02-10T03:36:13Z

I'm sorry that the code you commit is overwitten by mistake, can you push the code again? @Loirooriol

Loirooriol · 2025-02-10T06:57:57Z

What code, 83dad25 ?

Asun0204 · 2025-02-10T07:33:05Z

Yes. And this opration that truncate in au construction not only in the percentage will expose another problem of #servo34714.

The parameter of Timing Function style is f32 while it is f64 data type used in bezier function calculation. Loss can be produced in this progress. Could we just use truncate method in percentage, just like firefox?

Loirooriol · 2025-02-10T07:40:47Z

Sure, you can try that. But currently the logic first resolves LengthPercentage into a Length and then to an Au. You will need to remove the middle step.

Signed-off-by: Asun0204 <[email protected]>

{"fail_fast": false, "matrix": [{"name": "Linux", "workflow": "linux", "wpt_layout": "none", "profile": "release", "unit_tests": false, "build_libservo": false, "bencher": false, "wpt_args": ""}]}

Signed-off-by: Oriol Brufau <[email protected]>

{"fail_fast": false, "matrix": [{"name": "Linux", "workflow": "linux", "wpt_layout": "none", "profile": "release", "unit_tests": false, "build_libservo": false, "bencher": false, "wpt_args": ""}]}

{"fail_fast": false, "matrix": [{"name": "Linux (WPT)", "workflow": "linux", "wpt_layout": "2020", "profile": "release", "unit_tests": false, "build_libservo": false, "bencher": false, "wpt_args": ""}]}

Signed-off-by: Oriol Brufau <[email protected]>

{"fail_fast": false, "matrix": [{"name": "Linux (WPT)", "workflow": "linux", "wpt_layout": "2020", "profile": "release", "unit_tests": false, "build_libservo": false, "bencher": false, "wpt_args": ""}]}

Signed-off-by: Oriol Brufau <[email protected]>

Loirooriol

Don't revert e2c385d

Loirooriol

The PR is still including unrelated commits.

Asun0204 · 2025-03-04T13:10:18Z

Do we need to test the code? change truncate conversion to appunit from float in percentage calcuation is the final version code and the other commits are used fo testing temporary.

Loirooriol · 2025-03-04T13:17:09Z

Remove f588e85

Asun0204 · 2025-03-04T13:21:35Z

I don't know what's going on with f588e85 but there is a lot of crates in this commit. It can not compile without it.

Loirooriol · 2025-03-04T13:24:51Z

You need to rebase your changes on top of the latest main (0190fff).

Asun0204 · 2025-03-04T13:38:02Z

I completed it. There is only one commit base on the latest main. Do we need to import app_units pr?

Loirooriol · 2025-03-04T13:52:42Z

style/values/computed/length_percentage.rs

-            .map(Au::from)
+        if let Unpacked::Percentage(_) = self.unpack() {
+            return self.maybe_percentage_relative_to(container_len.map(Length::from))
+                .map(|length| Au::from_f32_px_trunc(length.px()));


This won't compile because the app_units PR hasn't landed.
Please make sure everything compiles so that I can run tests.

I push a commit to import app_units pr for compiling it.
It's ok now.

Loirooriol · 2025-03-05T00:38:57Z

style/values/computed/length_percentage.rs

+        if let Unpacked::Percentage(_) = self.unpack() {
+            return self.maybe_percentage_relative_to(container_len.map(Length::from))
+                .map(|length| Au::from_f32_px_trunc(length.px()));
+        }
+        self.maybe_percentage_relative_to(container_len.map(Length::from)).map(Au::from)


No need to repeat self.maybe_percentage_relative_to(container_len.map(Length::from))

Suggested change

if let Unpacked::Percentage(_) = self.unpack() {

return self.maybe_percentage_relative_to(container_len.map(Length::from))

.map(|length| Au::from_f32_px_trunc(length.px()));

}

self.maybe_percentage_relative_to(container_len.map(Length::from)).map(Au::from)

self.maybe_percentage_relative_to(container_len.map(Length::from))

.map(if let Unpacked::Percentage(_) = self.unpack() {

|length| Au::from_f32_px_trunc(length.px())

} else {

Length::from

})

}

Loirooriol

Also you should cover to_used_value for consistency.

Loirooriol · 2025-03-05T13:36:43Z

style/values/computed/length_percentage.rs

+        if let Unpacked::Percentage(_) = self.unpack() {
+            return Au::from_f32_px_trunc(self.to_pixel_length(containing_length).px());
+        }
        Au::from(self.to_pixel_length(containing_length))


I would also avoid repeating self.to_pixel_length(containing_length) here:

Suggested change

if let Unpacked::Percentage(_) = self.unpack() {

return Au::from_f32_px_trunc(self.to_pixel_length(containing_length).px());

}

Au::from(self.to_pixel_length(containing_length))

let value = self.to_pixel_length(containing_length);

if let Unpacked::Percentage(_) = self.unpack() {

Au::from_f32_px_trunc(value.px());

} else {

Au::from(value)

}

Loirooriol

app_units 0.7.8 has been released, so you can upgrade to it.

Also please use a clearer title like "Use truncation instead of rounding to resolve percentages into app units" or similar.

Loirooriol · 2025-03-11T11:01:17Z

You keep using "calcuation" instead of "calculation".
But just avoid this word since it seems to refer to calc() and this is about plain percentages.
Also "float of percentage" is confusing.

Signed-off-by: Asun0204 <[email protected]>

servo/stylo@a93e7ef...4add86f - servo/stylo#134 Remove unnecessary imports - servo/stylo#133 Reinstate missing gecko-specific import - servo/stylo#135 fix: add atoms "show" - servo/stylo#102 Use truncation instead of rounding to resolve percentages into app units Signed-off-by: Oriol Brufau <[email protected]>

servo/stylo@a93e7ef...4add86f - servo/stylo#134 Remove unnecessary imports - servo/stylo#133 Reinstate missing gecko-specific import - servo/stylo#135 fix: add atoms "show" - servo/stylo#102 Use truncation instead of rounding to resolve percentages into app units Signed-off-by: Oriol Brufau <[email protected]> Co-authored-by: Oriol Brufau <[email protected]>

Asun0204 changed the title ~~[fixbug] change conversion to appunit from float of percentage calcuation~~ change conversion to appunit from float of percentage calcuation Dec 26, 2024

Asun0204 mentioned this pull request Dec 26, 2024

Fix the incorrect percentage calculation of width (#34665) servo/servo#34714

Merged

4 tasks

Asun0204 force-pushed the main branch from 2784f36 to 2e235b1 Compare December 26, 2024 04:03

Loirooriol force-pushed the main branch from 2e235b1 to 83dad25 Compare January 8, 2025 14:00

Loirooriol added a commit to Asun0204/servo that referenced this pull request Jan 8, 2025

Use servo/stylo#102

fb37859

Signed-off-by: Oriol Brufau <[email protected]>

Loirooriol added a commit to Asun0204/servo that referenced this pull request Jan 8, 2025

Use servo/stylo#102

c26cf65

Signed-off-by: Oriol Brufau <[email protected]>

Loirooriol reviewed Jan 8, 2025

View reviewed changes

Loirooriol force-pushed the main branch from fbd6a14 to dfed17b Compare January 8, 2025 18:41

Asun0204 force-pushed the main branch 2 times, most recently from 6259dd8 to 8fd98ee Compare January 10, 2025 01:40

Loirooriol force-pushed the main branch from 816c355 to 4359cb6 Compare February 5, 2025 02:01

Asun0204 force-pushed the main branch from 8fd98ee to fb255ae Compare February 10, 2025 02:46

Asun0204 force-pushed the main branch from fb255ae to 5138dbc Compare February 11, 2025 07:19

Asun0204 added a commit to Asun0204/servo that referenced this pull request Feb 11, 2025

Use servo/stylo#102

abedef6

Signed-off-by: Asun0204 <[email protected]>

Asun0204 pushed a commit to Asun0204/servo that referenced this pull request Feb 11, 2025

Use servo/stylo#102

962f16f

Signed-off-by: Oriol Brufau <[email protected]>

Asun0204 pushed a commit to Asun0204/servo that referenced this pull request Feb 11, 2025

Use servo/stylo#102

d3a6998

Signed-off-by: Oriol Brufau <[email protected]>

Asun0204 force-pushed the main branch from 5138dbc to b6f986e Compare February 11, 2025 11:38

Asun0204 pushed a commit to Asun0204/servo that referenced this pull request Feb 11, 2025

Use servo/stylo#102

d4153ea

Signed-off-by: Oriol Brufau <[email protected]>

Asun0204 force-pushed the main branch 3 times, most recently from 6cf7f42 to 5e04a73 Compare March 4, 2025 09:05

Loirooriol requested changes Mar 4, 2025

View reviewed changes

Asun0204 force-pushed the main branch from 844799d to c297870 Compare March 4, 2025 12:31

Loirooriol requested changes Mar 4, 2025

View reviewed changes

Asun0204 force-pushed the main branch from c297870 to 64eebc6 Compare March 4, 2025 13:34

Loirooriol requested changes Mar 4, 2025

View reviewed changes

Loirooriol mentioned this pull request Mar 5, 2025

layout: support CSS will-change servo/servo#35787

Merged

4 tasks

Loirooriol reviewed Mar 5, 2025

View reviewed changes

Loirooriol requested changes Mar 5, 2025

View reviewed changes

Asun0204 force-pushed the main branch from b14888d to 9d3059d Compare March 5, 2025 06:21

Loirooriol reviewed Mar 5, 2025

View reviewed changes

Asun0204 force-pushed the main branch from 9d3059d to 440dfbd Compare March 6, 2025 01:39

Loirooriol approved these changes Mar 10, 2025

View reviewed changes

Asun0204 force-pushed the main branch from 440dfbd to 5fff3ef Compare March 11, 2025 10:48

Use truncation instead of rounding to resolve percentages into app units

900ba88

Signed-off-by: Asun0204 <[email protected]>

Asun0204 force-pushed the main branch from 5fff3ef to 900ba88 Compare March 11, 2025 11:03

Loirooriol changed the title ~~change conversion to appunit from float of percentage calcuation~~ Use truncation instead of rounding to resolve percentages into app units Mar 11, 2025

Loirooriol added this pull request to the merge queue Mar 11, 2025

Merged via the queue into servo:main with commit 4add86f Mar 11, 2025
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use truncation instead of rounding to resolve percentages into app units #102

Use truncation instead of rounding to resolve percentages into app units #102

Asun0204 commented Dec 26, 2024 •

edited by Loirooriol

Loading

Loirooriol commented Jan 8, 2025

Loirooriol commented Jan 8, 2025

Loirooriol left a comment

Asun0204 commented Feb 10, 2025 •

edited

Loading

Loirooriol commented Feb 10, 2025

Asun0204 commented Feb 10, 2025

Loirooriol commented Feb 10, 2025

Loirooriol left a comment

Loirooriol left a comment

Asun0204 commented Mar 4, 2025

Loirooriol commented Mar 4, 2025

Asun0204 commented Mar 4, 2025

Loirooriol commented Mar 4, 2025

Asun0204 commented Mar 4, 2025 •

edited

Loading

Loirooriol Mar 4, 2025

Asun0204 Mar 4, 2025

Loirooriol Mar 5, 2025

Asun0204 Mar 5, 2025

Loirooriol left a comment

Loirooriol Mar 5, 2025

Asun0204 Mar 6, 2025

Loirooriol left a comment

Loirooriol commented Mar 11, 2025

	impl From<CSSPixelLength> for Au {
	#[inline]
	fn from(len: CSSPixelLength) -> Self {
	Au::from_f32_px(len.0)
	}
	}

Use truncation instead of rounding to resolve percentages into app units #102

Use truncation instead of rounding to resolve percentages into app units #102

Conversation

Asun0204 commented Dec 26, 2024 • edited by Loirooriol Loading

Loirooriol commented Jan 8, 2025

Loirooriol commented Jan 8, 2025

Loirooriol left a comment

Choose a reason for hiding this comment

Asun0204 commented Feb 10, 2025 • edited Loading

Loirooriol commented Feb 10, 2025

Asun0204 commented Feb 10, 2025

Loirooriol commented Feb 10, 2025

Loirooriol left a comment

Choose a reason for hiding this comment

Loirooriol left a comment

Choose a reason for hiding this comment

Asun0204 commented Mar 4, 2025

Loirooriol commented Mar 4, 2025

Asun0204 commented Mar 4, 2025

Loirooriol commented Mar 4, 2025

Asun0204 commented Mar 4, 2025 • edited Loading

Loirooriol Mar 4, 2025

Choose a reason for hiding this comment

Asun0204 Mar 4, 2025

Choose a reason for hiding this comment

Loirooriol Mar 5, 2025

Choose a reason for hiding this comment

Asun0204 Mar 5, 2025

Choose a reason for hiding this comment

Loirooriol left a comment

Choose a reason for hiding this comment

Loirooriol Mar 5, 2025

Choose a reason for hiding this comment

Asun0204 Mar 6, 2025

Choose a reason for hiding this comment

Loirooriol left a comment

Choose a reason for hiding this comment

Loirooriol commented Mar 11, 2025

Asun0204 commented Dec 26, 2024 •

edited by Loirooriol

Loading

Asun0204 commented Feb 10, 2025 •

edited

Loading

Asun0204 commented Mar 4, 2025 •

edited

Loading