1700565 - Facebook Pages - likes and follows detail section pushed on 2 rows with default scaling

Status: RESOLVED FIXED

(Core :: Layout: Text and Fonts, defect)

Description

[Cristian Fogel [:cfogel]]

Attachment: 111d.png

Affected versions

• 88.0b1,89.0a1 (2021-03-23);

Affected platforms

• Windows 10

Steps to reproduce

• DEFAULT zoom levels are set up;

1. Launch Firefox, log in with a valid account;
2. Access the Your Pages section;

• can create a new page and add 1-2 likes to it;

3. Observe the PAGES you manage section;

Expected result

• elements are properly displayed;

Actual result

• the value+likes section has a forced overflow even though there is enough space available to be displayed;

Regression range

• First bad: 2020-09-23;
• Last good: 2020-09-22;
• Pushlog: URL;
• Potential regressor: bug 1316534

Additional notes

• set S4 as it might be a flaw in interaction with Facebook;
• can be better observed when changing the zoom levels, as this gets triggered on unexpected values;
• attached screenshot with the issue to best illustrate it.

Comment 1

[Cristian Fogel [:cfogel]]

Updating based on regressor.

Comment 2

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Bug 1316534 fixed a longstanding webcompat issues in Firefox, so maybe it's best to reach out to Facebook team to see if they add workaround for old Firefox version.

In my experience, Facebook usually has complex layout, it would be very helpful to have a minimal testcase in order to be something actionable for our layout team.

Comment 3

[Daniel Holbert [:dholbert]]

I can reproduce on Windows 10, though I can't reproduce on Linux or MacOS.

Here's a reduced testcase, from the Testcase Reducer add-on (again, this only triggers the problem on Win10):
https://jsfiddle.net/y2f5Lthx

The issue seems to be with the "." separator dots -- the layout devtools show that those only have a content size of 3.25px, but they end up with a minimum size of 4.12px for some reason, which then forces them to take that higher amount of width during flex layout. This leaves less width available for the other flex items, and so the "1 like" / "1 follow" tags end up with not-quite-enough-space to fit on one line.

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: testcase 1 (note: requires Segoe UI font, which is available by default only on Windows)

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: testcase 1 (note: requires Segoe UI font, which is available by default only on Windows)

Comment 6

[Daniel Holbert [:dholbert]]

Attachment: screenshot of layout devtools showing discrepancy between content size & minimum size of the "." div

(In reply to Daniel Holbert [:dholbert] from comment #3)

The issue seems to be with the "." separator dots -- the layout devtools show that those only have a content size of 3.25px, but they end up with a minimum size of 4.12px for some reason, which then forces them to take that higher amount of width during flex layout. This leaves less width available for the other flex items, and so the "1 like" / "1 follow" tags end up with not-quite-enough-space to fit on one line.

Here's a screenshot (taken from the attached testcase, on Windows 10) showing this issue, as surfaced by our layout devtools pane.

Comment 7

[Daniel Holbert [:dholbert]]

So this seems to be a discrepancy between:

• the way we do content measurement of flex items when determining the flex container's intrinsic width
• the way we do content measurement of flex items when determining their automatic minimum size

And Segoe UI happens to be hitting some edge case where we come up with a different calculation for some reason (specifically when there is a particular bit of punctuation, surrounded by spaces).

Comment 8

[Daniel Holbert [:dholbert]]

I can reproduce this with the attached testcase on Linux, if I install the segoeui.ttf font (taken from Win10 on the same machine).

(I also had to reset layout.css.font-visibility.level to its default value, in order for this newly-installed font to be discoverable, because I had previously set that pref to 1 at some point in the past to dogfood a font-visibility experiment that jfkthame was working on.)

Comment 9

[Daniel Holbert [:dholbert]]

TYLin: now that we have a testcase, maybe you could take a look here? (Let me know if you don't have Windows machine to test on and/or to locally copy its segoeui.ttf font file from.)

I think this is an unintended behavior-change from Bug 1316534. That bug was focused on fixing something for flex items that have non-default flex-basis values; and here, the flex items have default styling, so I wouldn't have expected this testcase to change its behavior.

(Note: I verified that the reduced testcase does have the same regression range as Facebook does -- the range indicated in comment 0 here. In "good" builds, our flexbox layout devtools show that the content size and final size are both 3.47px; in "bad" builds, the automatic min size is larger and takes over.)

Comment 10

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Attachment: testcase 2 (note: requires Segoe UI font, which is available by default only on Windows)

Re comment 7:

And Segoe UI happens to be hitting some edge case where we come up with a different calculation for some reason (specifically when there is a particular bit of punctuation, surrounded by spaces).

Yes, I can see the discrepancy without using flexbox. Please see the attached testcase, case 2.

It's a <div> containing two characters, Middle Dot and a space. With Segoe UI font and word-break:break-word, it seems the trailing space can contribute to the block container's min-content width, so it's a bit wider than others.

Google Chrome gives all the cases the same width. We also do with font-family: serif or without word-break: break-word.

Jonathan, do you have any ideas?

Comment 11

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Attachment: Screenshot testcase #2.png

Comment 12

[Jonathan Kew [:jfkthame]]

Weird! Experimenting with testcase 1, it seems to occur when the punctuation character's width is narrower than the width of the space glyph. So in Segoe UI, for example, it reproduces with "!" but not with "?". (It also reproduces with "i" or "l"; it's not specific to punctuation.)

Some other fonts such as Tahoma can also reproduce (e.g. with "." or ",", but not with the slightly wider ":").

OK, I guess what's happening is that in the computation of min-content width, word-break: break-word (which is deprecated, fwiw, but overflow-wrap: anywhere results in the same effect) introduces a potential line break after the punctuation character and before the trailing space, which thus goes onto a second line by itself; and its width then contributes to the min-content measurement, ignoring the fact that it would actually hang. (But when the content is actually laid out, the space stays on the first line, mostly hanging beyond the box.)

This all seems a bit confused/broken. It does make me wonder, though: if an element contains only a hang-able space character, what should its min-content width be? Zero, because if the element occurs at line-end, it hangs and effectively occupies no space in its parent? Or the width of the space, because if the element isn't at line-end, it does need that much space to display its content?

Comment 13

[Jonathan Kew [:jfkthame]]

So this seems reminiscent of bug 1665657; perhaps simply a case that wasn't handled properly there?

Comment 14

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Thanks for the info!

Jonathan, do you plan to fix this yourself? If not, I'm happy to dig the code that bug 1665657 touched and learn a bit more myself (hoping enough to fixing this bug).

Comment 15

[Jonathan Kew [:jfkthame]]

I'm not currently working on it, so if you have time to take a look that's fine.

Comment 16

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Attachment: Bug 1700565 - Disallow spaces to contribute to minimum advance width when line breaking is everywhere. r?jfkthame

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a4acaff2d290
Disallow spaces to contribute to minimum advance width when line breaking is everywhere. r=jfkthame

Comment 18

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/28592 for changes under testing/web-platform/tests

Comment 19

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/a4acaff2d290

Comment 20

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot

Comment 21

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:TYLin, is this bug important enough to require an uplift?
If not please set status_beta to wontfix.

For more information, please visit auto_nag documentation.

Comment 22

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

It's ok to let my patch ride the train because this bug actually exists for a while and is uncovered by bug 1316534.