1694729 - display: table-cell on fieldset changed in Firefox 86

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[Thomas]

Attachment: Screen Shot 2021-02-24 at 12.22.34 PM.png

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.16; rv:86.0) Gecko/20100101 Firefox/86.0

Steps to reproduce:

After upgrading to Firefox 86, some of our layouts became smaller than what they were in Firefox 85. The elements have common CSS in display: table-cell.

When I remove this CSS, things go back to the correct width.

Demo: https://codepen.io/TwisterMc/pen/WNodGGw

Actual results:

Items became smaller in Firefox 86. This broke existing sites.

Expected results:

The layout should not have changed.

Comment 1

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

The Bugbug bot thinks this bug should belong to the 'Core::Layout' component, and is moving the bug to that component. Please revert this change in case you think the bot is wrong.

Comment 2

[Thomas]

I'm beginning to wonder if Mozilla fixed a long standing bug which we had a work around for. Now that the bug is fixed, our work around breaks things. However, I don't see anything in the release notes that can prove or disprove my theory so I'm confused.

Comment 3

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

Thanks for the report and a testcase!

It was bug 1686310 that caused this behavior change. The bug was a refactor (maybe with some minor behavior changes), so it won't appear on the release note.

I can confirm that removing this part of the CSS yields the expected result.

fieldset { /* This is the problem area. Remove to see it become the expected width. */
  display: table-cell;
}

Mats, any ideas?

Comment 4

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

Attachment: 1694729.html

Attached the reporter's codepen testcase in a standalone file.

Comment 5

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

Attachment: 1694729.html

Attached the reporter's codepen testcase in a standalone file.

Comment 6

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

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

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Curious, what was the purpose of specifying display: table-cell on a fieldset to begin with? :)

Comment 8

[Thomas]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #7)

Curious, what was the purpose of specifying display: table-cell on a fieldset to begin with? :)

That is a great question. The original dev is no longer here, but the comment in the code says:

// Enforce consistent behavior of the fieldset
fieldset {
    min-width: 0;
}

@-moz-document url-prefix() {
    fieldset {
        display: table-cell;
    }
}

I can remove that code and test, but I'm not sure what it was solving unfortunately. If I had to guess, I'd say the code was for bug 504622 which has been fixed for a while now. That doesn't really explain why it renders differently all of a sudden, but maybe my solution is to just remove our code.

Comment 9

[Mats Palmgren (inactive)]

This line now checks for actual table frames:
https://searchfox.org/mozilla-central/rev/df23c6e58c6be1eb8399e475878f73d4867bef87/layout/generic/ReflowInput.cpp#2213
whereas before it just checked for display which made <fieldset> enter that block (wrongly). By coincidence it did the right thing.
Now, we instead fall through to this check:
https://searchfox.org/mozilla-central/rev/df23c6e58c6be1eb8399e475878f73d4867bef87/layout/generic/ReflowInput.cpp#2275-2283
which does the wrong thing in this case...

Comment 10

[Mats Palmgren (inactive)]

Attachment: Bug 1694729 - Fix sizing issues for display:table-* on <fieldset> and replaced elements. r=emilio

Comment 11

[Mats Palmgren (inactive)]

The "table-cell/table-column on replaced elements behaves as inline" is a bit odd - I wasn't aware that was a thing.
Actually, I just found the spec section that WPT refers to, at the end of the section it says:
https://drafts.csswg.org/css-tables-3/#table-structure

Authors should not assign a display type from the previous list to replaced elements (eg: input fields or images). When the display property of a replaced element computes to one of these values, it is handled instead as though the author had declared either block (for table display) or inline (for all other values). Whitespace collapsing and box generation must happen around those replaced elements like if they never had any table-internal display value applied to them, and had always been block or inline.

So, hmm, it seems the WPT and spec disagrees... I'll check what other UAs are doing on that test...

Comment 12

[Mats Palmgren (inactive)]

So the WPT /css/css-tables/table-model-fixup-2.html is bogus and doesn't reflect the spec...
The CSSWG resolved to standardize Gecko's behavior here (back in 2017!):
https://github.com/w3c/csswg-drafts/issues/508#issuecomment-319891999
which is what the css-tables quote above correctly reflects.
(The test also doesn't test what it claims to be testing so I'll amend it so that it actually does. The reason the "Replaced elements inside a table" replaced elements ends up on "different lines" is that they are actually in different table rows, not that they behave as blocks.)

I see that emilio already filed a WPT issue about that test here:
https://github.com/web-platform-tests/wpt/issues/10165

Comment 13

[Mats Palmgren (inactive)]

Maybe the intermittent failure issue is solved now?
This commit seems related to that:

changeset: 546136:813a7e625678
user: Sam Sneddon <[e-mail]>
date: Fri Aug 21 09:31:01 2020 +0000
summary: Bug 1659601 [wpt PR 25059] - Make table-model-fixup-2 wait for load, a=testonly

and @FremyCompany closed the WPT issue above on Aug 18, 2020. So presumably it should be fixed?

I'll remove the "disabled" annotations in the meta file and see if that sticks...

Comment 14

[Mats Palmgren (inactive)]

I filed a Chrome bug: https://bugs.chromium.org/p/chromium/issues/detail?id=1182918

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2f7f2d2cec3b
Fix sizing issues for display:table-* on <fieldset> and replaced elements.  r=emilio

Comment 16

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

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

Comment 17

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/2f7f2d2cec3b

Comment 18

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

Upstream PR merged by moz-wptsync-bot

Comment 19

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

The patch landed in nightly and beta is affected.
:mats, 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 20

[Julien Cristau [:jcristau]]

Ping timeout.

Comment 21

[Sören Hentzschel]

It's probably too late for Firefox 87, but I suggest to consider this bugfix as a ride-along if there will be a Firefox 87.0.1. Bug 1686310 caused serious visual regressions. It affects all forms in every forum that uses a software called WBB in versions 4.0 or 4.1. I don't know the international forum software market but in Germany it's a very popular software that is used by hundreds if not thousands websites. Version 4.x already reached EOL and I don't know how common these versions still are but I already found four affected forums, it was also already reported in the German Firefox support forum and there won't be an update by the vendor of the forum software due to the EOL (newer versions are not affected).

Comment 22

[Mike Conley (:mconley) (:⚙️)]

ni?ing jcristau for comment 21.

Comment 23

[Julien Cristau [:jcristau]]

Somebody (who knows this code) would have to actually request uplift on the patch.

Comment 24

[Mike Conley (:mconley) (:⚙️)]

Okay, ni'ing emilio who reviewed for comment 21.

Comment 25

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 9205772 [details]
Bug 1694729 - Fix sizing issues for display:table-* on <fieldset> and replaced elements. r=emilio

Beta/Release Uplift Approval Request

• User impact if declined: according to comment 21, a lot, but this was a regression from 86 so beware.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: comment 0
• List of other uplifts needed: none
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Relatively simple layout fix, affecting specifically stuff with table display that aren't tables, and which has been on nightly and beta for a while now.
• String changes made/needed: none

Comment 26

[Sören Hentzschel]

Attachment: screenshot

Maybe it helps with the uplift decision: I attached a screenshot to show you how one example form looks in affected forums. The correct version is on top, the broken version on the bottom.

Comment 27

[Alexandru Trif, Desktop Test Engineering [:atrif]]

Reproduced the issue with Firefox 88.0a1 (20210224215151) on Windows 10x64.
Verified fixed with 88.0b8 (20210406185740) and 89.0a1 (20210407212527) on Windows 10x64, macOS 10.12 and Ubuntu 20. The layouts are displayed as stated in comment 0 on the attached test case.

Comment 28

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9205772 [details]
Bug 1694729 - Fix sizing issues for display:table-* on <fieldset> and replaced elements. r=emilio

88 is in RC now and there are no plans for an 87 dot release.