Open
Bug 1440958
Opened 6 years ago
Updated 2 years ago
www.bloomberg.com layout broken when font size is odd
Categories
(Core :: Layout, defect, P3)
Tracking
()
REOPENED
People
(Reporter: euthanasia_waltz, Unassigned)
Details
Attachments
(4 files)
Bad case : font-size=15
STR: 1. Set default font size to 15 (font for your locale) 2. Open https://www.bloomberg.com/ (redirects due to your locale) AR: The layout is broken. Article tiles are overlapped. ER: Articles are tiled. Another STR: 1. Go to https://www.bloomberg.com/ 2. Open DevTools/Inspector 3. Select <html> and add "font-size:15px" to its style 4. Also apply other font sizes, 14, 16, 17, ... AR: Tiles are overlapped when font-size is odd(13, 15, 17, 19...) Other browsers(Edge, Vivaldi) does not have this problem.(confirmed by each developer tools on them)
|
||
Comment 2•6 years ago
|
||
I can repro. They use a Flexbox container with items sized with REM units, so tentatively ni?ing Daniel :).
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(dholbert)
|
||
Comment 3•6 years ago
|
||
I can reproduce. Here's a somewhat reduced testcase, taken from the current bloomberg site.
|
|
||
Comment 4•6 years ago
|
||
This is just the fact that we don't represent lengths precisely -- we use a fixed-point representation with 60 "nscoord" units per CSS pixel (typically), and in this case, the rem units and an odd font-size end up producing something like 178.125px, which can't be represented exactly in our 60-nscoord-units-per-pixel representation.
More detail:
The flex container here has "flex-direction: column wrap", with the intent that the first flex item will occupy one whole "column", and the second and third items will *exactly fill* the second column. But in reality, we end up thinking the second and third don't quite fit -- we have to do some rounding under the hood, which means the second and third item don't quite fit, and the third item gets pushed to its own third flex-column (which is off the right edge of the container).
The sizes here are as follows:
- The container has
height: 25rem
- The second and third child *each* have:
height: 11.875rem; (and there's padding included in this height, due to 'box-sizing:border-box')
- ...and the third child also has:
margin-top: 1.25rem;
And as it happens, the second and third child's sizes here are 11.875 + 11.875 + 1.25, which is *exactly* 25 rem. So they're depending on us representing all of these quantities precisely correctly, without any allowance for rounding.
Unfortunately, with an odd font-size, we don't represent them all precisely. 11.875 * 15px = 178.125px, and converted to nscoord (60 per pixel), that is 10687.5 nscoord units, which we'll internally round up to 10688. We have two elements that have that size, so we round up twice, and we end up being 1 full nscoord unit larger than the site is expecting. So the math doesn't quite work out for these items to both fit, so we push the third one to its own column.Flags: needinfo?(dholbert)
|
|
||
Comment 5•6 years ago
|
||
So this is a version of bug 265084 (in the "consider making nscoords be floats" sense). We may take action there someday, but it's a pretty serious architectural change, and it'll undoubtedly have fallout, so it's probably not a priority in the near term. And I don't think there's much else besides that bug that we can do to fix this in Firefox itself. It's possible for bloomberg to work around this, by e.g. using the "flex" property on these children to make them cooperate to exactly fill the container's height (rather than using hardcoded zero-tolerance REM sizes). But I'll bet Bloomberg wouldn't prioritize that especially high either, since it could add room for things to break and this issue isn't even reproducible unless you make local hacks/changes to their content. (whether via devtools or a mandatory font tweak) In the meantime: atlanto, if you want to avoid this sort of problem, I'd suggest you stick with even font-size values in your local overrides -- this will reduce the likelihood of a fractional rem/em lengths being resolved to fractional nscoord units which then trigger rounding & possible-overflow.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → DUPLICATE
(In reply to Daniel Holbert [:dholbert] from comment #4) > Unfortunately, with an odd font-size, we don't represent them all precisely. > 11.875 * 15px = 178.125px, and converted to nscoord (60 per pixel), that is > 10687.5 nscoord units, which we'll internally round up to 10688. We have > two elements that have that size, so we round up twice, and we end up being > 1 full nscoord unit larger than the site is expecting. So the math doesn't > quite work out for these items to both fit, so we push the third one to its > own column. I believe there are contexts where we intentionally round *down* rather than to *nearest* so that we avoid this sort of problem. (For example, we do this when processing % values in CSS, at least in the old style system; see nsStyleCoord::ComputeCoordPercentCalc and nsStyleCoord::ComputeComputedCalc.) Maybe this should be another such case?
Flags: needinfo?(dholbert)
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
|
|
||
Comment 7•6 years ago
|
||
Oh, good point... that would avoid this. For Gecko's style system, I think that happens in nsRuleNode.cpp's "CalcLengthWith" function: * For 'em' units: https://dxr.mozilla.org/mozilla-central/rev/df0e967f8e8fa30a69338008b6ee296bf5e9c545/layout/style/nsRuleNode.cpp#590,598 * For 'rem' units: https://dxr.mozilla.org/mozilla-central/rev/df0e967f8e8fa30a69338008b6ee296bf5e9c545/layout/style/nsRuleNode.cpp#573 * (and similar for "ch", "ex", and other font-dependent metrics) Though I don't know if that resolution happens elsewhere in a post-Stylo world (emilio, do you know?) I imagine we'd want to replace our calls to "ScaleCoordRound" with some new (similar) function called ScaleCoordRoundDown() or somesuch. (either here, or wherever the Stylo equivalent of this code is.)
Flags: needinfo?(dholbert) → needinfo?(emilio)
|
|
||
Comment 8•6 years ago
|
||
So, Stylo uses floating point arithmetic for all lengths already, up until the point we set those in style structs, see bug 1392161 / https://github.com/servo/servo/pull/18381. This was done initially only to deal with transform arithmetic, but then we saw that it was just cleaner to use it everywhere instead, and just convert at the end of the pipeline. We have some app-units for the device viewport size and such that predate that change and such, but mostly minor. Everything should be CssPixelLength (read: f32) already. Curiously, the viewport percentages go through app units, and have some round-down to prevent this kind of issues, which is weird, and could maybe be somewhat simplified, since also predates that change (see https://github.com/servo/servo/pull/18448 / bug 1396045): https://searchfox.org/mozilla-central/rev/908e3662388a96673a0fc00b84c47f832a5bea7c/servo/components/style/values/specified/length.rs#253 That points to bug 989802, and so on... So, from the Stylo's perspective, switching to floats in style structs should actually be somewhat painless, contrary to what one could have expected. Did that answer the question? I may have deviated a bit :)
Flags: needinfo?(emilio)
|
|
||
Comment 9•6 years ago
|
||
(In reply to Daniel Holbert [:dholbert] from comment #7) > Oh, good point... that would avoid this. > > For Gecko's style system, I think that happens in nsRuleNode.cpp's > "CalcLengthWith" function: Also note that that function only applies to calc() as far as I can tell. Bug 989802 is the only place I'm aware of Gecko intentionally rounding down here.
nsStyleCoord::ComputeCoordPercentCalc should apply pretty broadly, and I think should still be in use *with* stylo (e.g., for percentages on properties like 'width', both simple percentages and percentages inside calc()). I was thinking based on comment 4 that there might be some rounding in the flex layout code that should be doing floor rather than round. But I didn't go through it that closely.
|
|
||
Comment 11•6 years ago
|
||
(In reply to David Baron :dbaron: ⌚️UTC-8 from comment #10) > I was thinking based on comment 4 that there might be some rounding in the > flex layout code that should be doing floor rather than round. But I didn't > go through it that closely. Gotcha -- yeah, there's nothing flex-specific here. This rounding (during "em"/"rem" to nscoord conversion) happens when we produce style structs, IIRC, and layout only sees nsStyleCoord with nscoord values, percentages, and enum values. Here's a testcase that demonstrates the bug, with no flex involved and with "inline-block" linewrapping instead. If you compare this to Chrome, you'll see that the pink boxes are wrapped in Firefox (for the odd font-sizes) but none are wrapped in Chrome. If we adjusted ComputeCoordPercentCalc's em/rem rounding to consistently round down, that'd fix both testcases here. (Technically we'd end making the children a little too small rather than a little too big in the odd-font-size cases, so there'd be 1px of incorrectly-unfilled-space at the end of the container.) That's clearly an improvement in this case, but I'm not entirely confident that it'd be an improvement in all cases... e.g. I could probably come up with a pathological case where the container is "em"-sized and the child is "px"-sized, and the rounded-up container is large enough for the children, but the rounded-down container is not.
|
||
Updated•2 years ago
|
Severity: normal → S3
You need to log in
before you can comment on or make changes to this bug.
Description
•