Closed Bug 1469649 Opened 3 years ago Closed 2 months ago

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

Categories

(Core :: Layout: Flexbox, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
93 Branch
Tracking Status
firefox-esr52 --- unaffected
firefox-esr60 --- unaffected
firefox-esr78 --- wontfix
firefox-esr91 --- wontfix
firefox60 --- unaffected
firefox61 --- wontfix
firefox62 + wontfix
firefox63 - wontfix
firefox64 --- wontfix
firefox65 --- wontfix
firefox91 --- wontfix
firefox92 --- wontfix
firefox93 --- fixed

People

(Reporter: dholbert, Assigned: TYLin)

References

Details

(Keywords: regression)

Attachments

(10 files)

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.
Attached file testcase 1
Here's a testcase (based on bug 1461446's "testcase 2", with an extra div-wrapped table inserted).
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
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.
Attached file 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).
(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.)
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?
Flags: needinfo?(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.
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
In case that doesn't work, they also have a contact form at the bottom of this page: https://quizlet.com/help
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.)
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.)
Flags: needinfo?(dholbert)
I can confirm that Quizlet Live appears to work now in 61.0beta. Thanks for taking the time to investigate this!
Quizlet live is working fine now. Thanks for your work!
Tracking for 62, and I'm assuming 63 is also affected.
Daniel, are you still planning to land a fix, or just tests, here? We could still take them for 63 and uplift to 62.
Flags: needinfo?(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.
Flags: needinfo?(dholbert)
(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
Flags: needinfo?(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)
Flags: needinfo?(dholbert)
Daniel, are you working on this regression and plan to uplift a patch for 63?
Flags: needinfo?(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)
Flags: needinfo?(dholbert)
Thanks Daniel, I'll stop tracking this bug for 63 based on your assessment in comment #19
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.
Attached image jobcan-firefox.png
Attached image jobcan-chromium.png

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.

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.

Flags: needinfo?(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).

Flags: needinfo?(dholbert)
Component: Layout → Layout: Flexbox

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

Duplicate of this bug: 1723348

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.

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

Depends on: 1726620

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

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

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.

Flags: needinfo?(a)

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.

Flags: needinfo?(a)

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/

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

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

See Also: → 1184430
Assignee: dholbert → aethanyc
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/30167 for changes under testing/web-platform/tests
Status: ASSIGNED → RESOLVED
Closed: 2 months ago
Resolution: --- → FIXED
Target Milestone: --- → 93 Branch
Blocks: 1727614
Upstream PR merged by moz-wptsync-bot
Regressions: 1728319
Regressions: 1730506
You need to log in before you can comment on or make changes to this bug.