Fix the incorrect percentage calculation of width (#34665) #34714
+21
−21
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jschwe
reviewed
Dec 20, 2024
components/layout_2020/geom.rs
Outdated
Comment on lines
761
to
764
| if v.to_percentage().is_some() && basis.inline.is_some() { | ||
| Some(Au::from_f32_px( | ||
| v.to_pixel_length(basis.inline?).px().floor(), | ||
| )) |
There was a problem hiding this comment.
Suggested change
| if v.to_percentage().is_some() && basis.inline.is_some() { | |
| Some(Au::from_f32_px( | |
| v.to_pixel_length(basis.inline?).px().floor(), | |
| )) | |
| if v.to_percentage().is_some() { | |
| basis.inline.map(|inline| Au::from_f32_px(v.to_pixel_length(inline).px().floor())) |
I'm not familiar with this code, so I can't comment on the correctness, but I believe we can simplify your code by using map
There was a problem hiding this comment.
I'm not familiar with rust, do you mean the following usage?
v.to_percentage().map_or_else(
|| v.maybe_to_used_value(basis.inline),
|_| {
Some(Au::from_f32_px(
v.to_pixel_length(basis.inline?).px().floor(),
))
},
)
Loirooriol
requested changes
Dec 20, 2024
There was a problem hiding this comment.
This would force percentages to resolve to a whole number of app units, which is bad.
Also, this isn't covering all cases of to_used_value and maybe_to_used_value, which is inconsistent.
It would be better to either:
- Add a new method in Au which is like
from _f32_pxbut truncating instead of rounding. Then modifyto_used_valueandmaybe_to_used_valuein Stylo to use that. - Modify
from_f32_pxitself, if that doesn't break anything in Firefox nor Servo.
|
Also, the current behavior isn't technically "incorrect". It indeed seems beneficial to change it, but the specs basically leave subpixel arithmetic up to the UAs. |
|
If I'm not mistaken, I think this issue is causing the mobile jingdong website https://m.jingdong.com/ to have an issue where the bar at the bottom to overflows in servo, but not in Firefox or chrome. Edit:
|
|
Althrough subpixel arithmetic is not defined in W3C, the current behavior has something wrong obviously (#34665). Truncate method is used in percentage calculation of firefox and chromium. It is the code in firefox. static inline nscoord DefaultPercentLengthToAppUnits(float aPixelLength) {
return NSToCoordTruncClamped(aPixelLength);
}It is the code in chromium. LayoutUnit MinimumValueForLengthInternal(const Length& length,
LayoutUnit maximum_value) {
switch (length.GetType()) {
case Length::kPercent:
// Don't remove the extra cast to float. It is needed for rounding on
// 32-bit Intel machines that use the FPU stack.
return LayoutUnit(
static_cast<float>(maximum_value * length.Percent() / 100.0f));
// ...
}
constexpr explicit LayoutUnit(float value)
: value_(base::saturated_cast<int>(value * kFixedPointDenominator)) {}So should we only modify the implementation of |
I would say undesirable instead of wrong.
No, that's the wrong place, as I said in #34714 (review) |
|
Another reason is that |
|
I omplete the code but don't know how to merge it. What to do next? @Loirooriol |
Signed-off-by: Oriol Brufau <obrufau@igalia.com>
|
🔨 Triggering try run (#12672394680) for Linux WPT |
|
Test results for linux-wpt-layout-2020 from try job (#12672394680): Flaky unexpected result (25)
Stable unexpected results that are known to be intermittent (6)
Stable unexpected results (3)
|
|
|
Please investigate the failures. Interpolating from an inherited 100px to 40px at 1.5 should be |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The conversion from percentage * basis to Au is unreasonably. Round method is used in this process and the size of box is larger.
./mach build -ddoes not report any errors./mach test-tidydoes not report any errors