1469649 - Still possible to get infinitely-sized tables as flex items where it previously wasn't (with flex container, table-layout:fixed, and percent width)

Status: RESOLVED FIXED

(Core :: Layout: Flexbox, defect)

Description

[Daniel Holbert [:dholbert]]

As noted in bug 1461446 comment 35, there are still some cases where fixed-layout tables can be made to explode in size in Firefox 61+, whereas they didn't in Firefox 60.

See attached testcase, which renders with an ~infinitely wide table (labeled "first") in Firefox 61 beta and 62 Nightly.

Firefox 60 and Chrome render the testcase with no gigantic content.

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: testcase 1

Here's a testcase (based on bug 1461446's "testcase 2", with an extra div-wrapped table inserted).

Comment 2

[Daniel Holbert [:dholbert]]

OK, this is kinda terrible.  Basically, it boils down to the fact that we've gone beyond having INT32_MIN available space to distribute, roughly.

Even if we just clamp the total available space to INT32_MIN, to avoid underflow, we just end up distributing "only" INT32_MIN (negative) space to the items, which isn't enough shrinking to actually reach the target size. So we're still left overflowing.

I can think of a few hacky solutions here, but the simplest thing is probably to:
 - clamp available space to a sentinel value.
 - When available space is that sentinel value, just assume we have to shrink everything as much as we possibly can.

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: testcase 2 (partially-reduced Quizlet Live page)

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: reference case 2

Here's a reference case for how I think Quizlet intends for "testcase 2" (and the Quizlet Live layout in general) to look -- three ~equal-width columns.

In Chrome and Firefox 60, testcase 2 matches this reference case. In Edge, they do not match -- Edge expands testcase 2's middle column to absorb basically all of the free space.

One simple solution here (fixing the Firefox issue, as well as that ^^ Edge issue) would be for the site to use
 "flex: 1"
...rather than:
 "flex-grow: 1"

That's closer to what's recommended in the spec, using the "flex: <positive-number>" syntax at https://drafts.csswg.org/css-flexbox-1/#flex-common .  Note the orange/tan note below that saying "Authors are encouraged to control flexibility using the flex shorthand rather than with its longhand properties directly, as the shorthand correctly resets any unspecified components to accommodate common uses."

This avoids issues by no longer relying on what a browser decides the preferred size of a table-layout:fixed element is (which is an absurdly huge number for historical reasons, at least in Firefox, which is what makes it interact poorly in flexbox).

Comment 5

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #4)
> One simple solution here (fixing the Firefox issue, as well as that ^^ Edge
> issue) would be for the site to use
>  "flex: 1"
> ...rather than:
>  "flex-grow: 1"

...on the StudentGameBoard-playerTermGroup element in particular. (This is the change I made to the testcase, to create the reference case.)

Comment 6

[Ryan VanderMeulen [:RyanVM]]

We're already building Fx61 RC builds, so it's going to miss shipping in 61. Is this bug severe enough that you think we should keep this on the radar for a dot release?

Comment 7

[Daniel Holbert [:dholbert]]

Probably not a candidate for a dot release. I'll try to reach out to Quizlet and see if they're up for making the one-line can tweak discussed above.

Comment 8

[Daniel Holbert [:dholbert]]

I'm hoping maybe Jeff Chan[1] from their web team can help apply a workaround on Quizlet's side -- I found his contact info on Twitter and reached out to him.
[1] https://quizlet.com/team/jeff-chan

Comment 9

[bgstandaert]

In case that doesn't work, they also have a contact form at the bottom of this page: https://quizlet.com/help

Comment 10

[Daniel Holbert [:dholbert]]

Thanks! Good news -- I heard back from Jeff, and he says they're looking into this.  So hopefully this won't be a problem for too long, on Quizlet Live at least.  And in the meantime, I've got testcases attached here to let us still trigger the problem even when their site no longer does, so we can still fix this on our end in case other sites are affected as well.

(In reply to Daniel Holbert [:dholbert] from comment #2)
> I can think of a few hacky solutions here, but the simplest thing is
> probably to:
>  - clamp available space to a sentinel value.
>  - When available space is that sentinel value, just assume we have to
> shrink everything as much as we possibly can.

Following up on this -- this ^^ kinda works, but it ends up compressing things too much, such that the three columns end up as small as they possibly can be (with their text content wrapping mid-word), so it's not really a great solution.

So: the next-best option I can think of to address the fundamental issue here is to use an int64_t (with its larger coordinate space) for tracking the flex container's space-to-be-distributed (which is hugely negative value in this case).  This might actually make for more robust results in general, so I'm tempted to do this.

(Or, we could add some sort of hacky special-case to make fixed-layout tables opt out of bug 1374540's changes. But I'd kinda rather not do that.)

Comment 11

[Daniel Holbert [:dholbert]]

Great news -- Jeff & his team deployed the suggested fix on Quizlet's end (using "flex:1" on the element in question), and I verified locally that Quizlet Live now works great in Nightly, in a 6-player game.  (Should work great in Beta as well, I expect.)

bgstandaert and/or qwertyuiopyozo, if one of you wouldn't mind sanity-checking on your end that Quizlet Live now renders as-expected, I'd very much appreciate that extra confirmation.

(In the meantime, I'm still hoping to improve things on our end, probably for Firefox 62. Requesting tracking to be sure this is on the radar there.)

Comment 12

[Daniel Holbert [:dholbert]]

Attachment: testcase 3: series of flex containers with progressively more items that contain 100%-wide fixed-layout tables

Comment 13

[bgstandaert]

I can confirm that Quizlet Live appears to work now in 61.0beta. Thanks for taking the time to investigate this!

Comment 14

[qwertyuiopyozo]

Quizlet live is working fine now. Thanks for your work!

Comment 15

[Liz Henry (:lizzard)]

Tracking for 62, and I'm assuming 63 is also affected.

Comment 16

[Liz Henry (:lizzard)]

Daniel, are you still planning to land a fix, or just tests, here? We could still take them for 63 and uplift to 62.

Comment 17

[Daniel Holbert [:dholbert]]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #16)
> Daniel, are you still planning to land a fix, or just tests, here? We could
> still take them for 63 and uplift to 62.

This is still technically a regression that requires a fix -- though we aren't aware of any sites that are affected (anymore), so this is less of a priority than it initially was.

I'm hoping to come up with a fix soon, but I anticipate it'll be a little ugly.  I suspect this won't (shouldn't try to) make it for 62.

Comment 18

[Pascal Chevrel:pascalc]

(In reply to Daniel Holbert [:dholbert] from comment #17)
> (In reply to Liz Henry (:lizzard) (needinfo? me) from comment #16)
> > Daniel, are you still planning to land a fix, or just tests, here? We could
> > still take them for 63 and uplift to 62.
> 
> This is still technically a regression that requires a fix -- though we
> aren't aware of any sites that are affected (anymore), so this is less of a
> priority than it initially was.
> 
> I'm hoping to come up with a fix soon, but I anticipate it'll be a little
> ugly.  I suspect this won't (shouldn't try to) make it for 62.

Daniel, any news on this front? Thanks

Comment 19

[Daniel Holbert [:dholbert]]

No real updates since comment 17.

Current feeling: I don't think we should try to fix for 62 at this point, given that
- it's late in the cycle (higher risk to take a change now)
- we've already shipped the regression in 61 anyway
- there aren't any known-affected sites (beyond the Quizlet one that's been fixed on their end)

Comment 20

[Pascal Chevrel:pascalc]

Daniel, are you working on this regression and plan to uplift a patch for 63?

Comment 21

[Daniel Holbert [:dholbert]]

I have a rough idea of how to fix it[1], but I'm not actively working on it & it's not at the top of my priority list, given second and third bullet point in comment 19.

[1] (plan is to switch flex algorithms to use 64-bit integers to avoid possibility of overflow from summing up 32-bit input values)

Comment 22

[Pascal Chevrel:pascalc]

Thanks Daniel, I'll stop tracking this bug for 63 based on your assessment in comment #19

Comment 23

[Liz Henry (:lizzard)]

Too late to fix in 64. Marking this issue as fix-optional for 65; if you land a patch in nightly and think it's low-risk for beta, please request uplift.

Comment 24

[ncaq]

Attachment: jobcan-firefox.png

Comment 25

[ncaq]

Attachment: jobcan-chromium.png

Comment 26

[ncaq]

I found known-affected site.
It is https://payroll.jobcan.jp/ 給与明細(pay advice).
It's a site that requires you to login.
I'm new to this, and I don't know how to report this.
Sorry it's too simple.
In the meantime, I'm attaching a screenshot with Firefox and Chromium.
I've confirmed that it reproduces the problem with nightly.

Comment 27

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

Re comment 17:

So: the next-best option I can think of to address the fundamental issue here is to use an int64_t (with its larger coordinate space) for tracking the flex container's space-to-be-distributed (which is hugely negative value in this case). This might actually make for more robust results in general, so I'm tempted to do this.

Do you feel it's worth adding an int64_t strong coordinate type for the above usage? It can also serve the purpose to explore bug 1602669 as its APIs should be similar to strong type nscoord.

Comment 28

[Daniel Holbert [:dholbert]]

Yeah, that seems worth doing (whether via a strong coordinate type or just a typedef -- given that this is relatively localized, I don't have strong feelings & would be fine with either).

Comment 29

[Aleks Totic]

FYI: we are running into "table-layout:fixed;width:100%" issues with Flex/Grid when trying to ship TablesNG in Chrome.

Our test of what we believe is correct behavior is:
wpt/css/css-tables/tentative/table-fixed-minmax.html

Basically:
"table-layout:fixed; width:X%" is a table intrinsic width algorithm exception that reports infinitely wide intrinsic table width.
This works well in block layout, but not if container is flex/grid.
We believe that not doing the exception when inside flex/grid works well, and are trying to ship this in TablesNG.

This is similar to what table layout algorithm does when considering TD's percentages when computing table grid width.
TD's percentages are ignored when computing intrinsic width.

The only difference between TD fix, and table:100% fix is that TD fix applies inside Flex/Grid/Cell, while table:100% fix applies inside Flex/Grid only.

Chrome's code implementing this fix:
https://chromium-review.googlesource.com/c/chromium/src/+/2718436

Comment 31

[Aleks Totic]

FYI: In Chrome, the function that determines whether infinite table sizes are allowed is NGTableNode::AllowColumnPercentages

https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/layout/ng/table/ng_table_node.cc;l=63?q=%20NGTableNode::AllowColumnPercentages

None of this is specified, just leaving it here if you are interested for compatibility reasons.

Comment 32

[Daniel Holbert [:dholbert]]

Thanks! We may need to draw inspiration from that. (I'm not actively working on this at the moment, though.)

Comment 33

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

Attachment: Bug 1469649 Part 1 - Promote floating point precision when computing the weight of flex items.

The precision of double is needed when we are distributing large
sizeDelta via availableFreeSpace * myShareOfRemainingSpace.

Without this patch, the wpt test added in Part 2 will fail.

Depends on D123266

Comment 34

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

Attachment: Bug 1469649 Part 2 - Use 64-bit coord type when resolving the main size for flex items.

The wpt testcase is adapted from bug 1469649 comment 12.

Depends on D123267

Comment 35

[Daniel Holbert [:dholbert]]

Attachment: testcase 4: same as testcase 3, but with `flex-shrink:0` on the flex items

Comment 36

[Daniel Holbert [:dholbert]]

My just-attached "testcase 4" (with flex-shrink:0) has infinitely-sized tables even with the attached patch stack. But it's also infinite in Chrome, so maybe that's interoperable... I had suspected from Aleks's comment that maybe it would be not-infinite in Chrome, but maybe I misunderstood.

Aleks, was your patch meant to address cases like testcase 4? (I thought you intended to constrain the resolved flex base size of flex items with these tables to be non-infinite, but maybe not.)

Just want to be sure we end up resolving this bug in a compatible way. It seems like the current patch does, but I want to be sure I'm not missing something.

Comment 37

[Aleks Totic]

dgrogan is the flex<->table sizing expert, my knowledge of flex is limited.
I am cc'ing him on this bug, see if he can explain this test case.

The infinitely sized table is surprising to me, it might a bug in Chrome too.

Comment 38

[Daniel Holbert [:dholbert]]

I suspect testcase 4 is huge (in both Firefox and Chromium) for the same reason that a similar no-flexbox testcase is huge, with a display:block;width:max-content wrapper instead of a flex-item wrapper): https://jsfiddle.net/dholbert/4gkm05cr/

Comment 39

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

Attachment: Bug 1469649 Part 3 - Upgrade the assertion to MOZ_ASSERT, and update its comments.

The assertion is testing the sign of availableFreeSpace and isUsingFlexGrow.
After we use 64-bit arithmetic, it's likely the stronger assertion holds.

Depends on D123268

Comment 40

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

Testcase 4 is similar to the test I attached in bug 1184430 comment 2 :)

Comment 41

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

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

Comment 42

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/be6f1ddbc743
https://hg.mozilla.org/mozilla-central/rev/f0b07571b559
https://hg.mozilla.org/mozilla-central/rev/abdafabdceec

Comment 43

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

Upstream PR merged by moz-wptsync-bot

Comment 44

[Karl Dubost💡 :karlcow]

This looks a bit similar https://github.com/webcompat/web-bugs/issues/92046
but it should not. it has been fixed.

Comment 45

[Aleks Totic]

https://github.com/webcompat/web-bugs/issues/92046 looks like it is triggered by "table intrinsic size is infinitely wide inside flexbox" issue.

I've looked into it, and I do not think it is just the table. There are also deeply nested sibling flexboxes, and if they are removed, table looks fine.