1801165 - Chrome and Firefox place text differently in a given test-case reduced from an nsa.gov webcompat issue (with negative margin-bottom, overflow:hidden, and implicit baseline alignment)

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Thomas Wisniewski [:twisniewski]]

Attachment: testcase.html

In the attached test-case, Firefox and Chrome/Safari differ in where they place the green header text. This is ultimately the root cause of a webcompat issue with nsa.gov's website, causing their header to be misaligned. It appears to be flex-related, but I'm not quite sure what the root cause is.

Comment 1

[Daniel Holbert [:dholbert]]

This seems like it's probably a Firefox bug.

Something seems to be blocking .container2's negative margin from collapsing/propagating outwards in this case. And it seems to have something to do with baseline alignment, per (C)/(D) below.

Any of the following tweaks will fix the issue, FWIW (noting since each one is a bit of a clue about what's wrong):
(A) Changing overflow:hidden to clip (or visible) on .container2
(B) Changing display:flex to block on .container2. (If I instead use grid then it's broken, though; so the brokenness isn't flex-specific, but rather it's specific to non-blocks)
(C) Adding a single character (e.g. a) as the next sibling of .container2
(D) Adding vertical-align:top to .container
(E) Removing display:inline-block from .container

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: testcase 2

Here's a testcase with a bunch of variants. The first box here is a reduced testcase where we're non-interoperable; the remaining boxes are cases where we're interoperable.

In the first part of this testcase (the first orange/gray area):

• Firefox draws the orange border around the whole gray element (with a bit of space at the bottom), with the gray element aligned to the top of the orange border.
• Chrome draws the orange border ~1em tall with the gray element shifted down by about ~1em (as if its baseline is all the way at its top).

In all other cases, the orange border is ~1em tall, and Firefox/Chrome are interoperable.

Comment 3

[Daniel Holbert [:dholbert]]

(To be clear: each of the orange/gray sections in testcase 2 are nearly the same as the first, but each has one small tweak -- some of the tweaks from comment 1, not necessarily in order, and one or two others.)

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: testcase 3

And for simplicity/debugability, here's just a version of the non-interoperable part from testcase 2.

(This is essentially a reduced version of the original testcase here.).

Comment 5

[Daniel Holbert [:dholbert]]

This was a regression from bug 969874.

INFO: Last good revision: 476293c6700fb45da40b8096184211432a89be57 (2019-02-05)
INFO: First bad revision: de51545099a617602be187d1c0f68ff2a87d6fb2 (2019-02-06)
INFO: Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=476293c6700fb45da40b8096184211432a89be57&tochange=de51545099a617602be187d1c0f68ff2a87d6fb2

In the last-good-build 2019-02-05:

• testcase 1 and testcase 3 match Chrome's current behavior.
• testcase 2 mostly matches Chrome's current behavior, except that we align the 5th section (the first "a" section, where "a" is inside the flex container) a bit further down. That's the thing that bug 969874 was fixing.

In the first-bad build 2019-02-06, the rendering of all three testcase matches current Firefox (with one exception that's not particularly interesting -- the 3rd section in testcase 2 differs from current Nightly, only because we didn't support overflow:clip back then. If I add overflow:-moz-hidden-unscrollable which was our old name for that keyword, then that 3rd section matches current Nightly.)

Comment 6

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

Set release status flags based on info from the regressing bug 969874

Comment 7

[Ksenia Berezina [:ksenia]]

This is no longer an issue on the website and the testcases appear to work the same in both Firefox and Chrome, so I think we can close this.

Comment 8

[Daniel Holbert [:dholbert]]

(In reply to Ksenia Berezina [:ksenia] from comment #7)

This is no longer an issue on the website and the testcases appear to work the same in both Firefox and Chrome, so I think we can close this.

nit: in cases where things start working but we're not exactly sure when/why, it's best to use RESOLVED|WORKSFORME, to capture the uncertainty. The "FIXED" status is reserved for cases where there's a known/identified patch that landed to fix it.

But we can leave this as FIXED, because I think I identified what fixed this, with mozregression --find-fix which gave me this fix range:

Fix range:
First good revision: 25a8668d92431d469b443f0f2030328bab754aea (2023-02-24)
Last bad revision: 8abe8c3a623300b21387637f5c6471ebf1b3e9ae (2023-02-23)
Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=8abe8c3a623300b21387637f5c6471ebf1b3e9ae&tochange=25a8668d92431d469b443f0f2030328bab754aea

In that range, I suspect this would've been fixed by bug 1811311, which refactored/unified some baseline-determination code (which is the relevant code here, per comment 5) to avoid inconsistencies between different codepaths.

Comment 9

[Ksenia Berezina [:ksenia]]

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

(In reply to Ksenia Berezina [:ksenia] from comment #7)

This is no longer an issue on the website and the testcases appear to work the same in both Firefox and Chrome, so I think we can close this.

nit: in cases where things start working but we're not exactly sure when/why, it's best to use RESOLVED|WORKSFORME, to capture the uncertainty. The "FIXED" status is reserved for cases where there's a known/identified patch that landed to fix it.

But we can leave this as FIXED, because I think I identified what fixed this, with mozregression --find-fix which gave me this fix range:

Ah yes, good point. Thanks for finding the fixing range!

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Is there a testcase worth landing here still or do we already have similar coverage elsewhere?

Comment 11

[Daniel Holbert [:dholbert]]

Probably worth adding a test, yeah. [keeping ni open to do that]

It doesn't look like bug 1811311 added tests or removed failure annotations for preexisting tests that were failing at the time, so it appears we didn't have test coverage for this at that point, at least (and probably still don't).

Comment 12

[Daniel Holbert [:dholbert]]

(Replacing RyanVM's ni=me with a ni=me from myself, per slack, to reduce noise in his needinfo list. We should still circle back to this, but it's not a high-priority task.)