1455976 - Issue when a display:table element is inside a flex element, happening on oui.sncf.

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[Julien Wajsberg [:julienw]]

STR:
1. Open the URL [1]
=> See how the output seems garbled.

[1] https://agence-voyage.oui.sncf/Flights-Search?trip=roundtrip&leg1=from%3AParis%2C%20France%20(PAR)%2Cto%3ASan%20Francisco%2C%20Californie%2C%20Etats-Unis%20(SFO)%2Cdeparture%3A01%2F07%2F2018TANYT&leg2=from%3ASan%20Francisco%2C%20Californie%2C%20Etats-Unis%20(SFO)%2Cto%3AParis%2C%20France%20(PAR)%2Cdeparture%3A08%2F07%2F2018TANYT&passengers=adults%3A1%2Cchildren%3A0%2Cseniors%3A0%2Cinfantinlap%3AY&options=cabinclass%3Aeconomy&mode=search&origref=agence-voyage.oui.sncf

When I inspect and disable the property `display: table` on the rule for `.pattern-filters label.check` it fixes the issue. `display: table` doesn't seem very useful...

I don't have the error in stable v59 so I ran mozregression and found the following pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=10fab7177ee397815f7b1e0b2c51437466e62268&tochange=4d0b4650e438989ba169b1657b91c5462e9137b6

So I believe this is coming from bug 1374540 so I think it's nightly-only, but I haven't looked at beta myself.

Tracking because this is happening on a real-life website that's used a lot in France, and it breaks its UI to a point that it's barely usable.

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: testcase 1: multi-line flex container

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: testcase 2: single-line flex container

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: (disregard; replacing with clearer testcase)

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: testcase 3: setting flex-basis to explicit value on a table & non-table flex item

Comment 5

[Julien Wajsberg [:julienw]]

Attachment: screenshot of the issue

Comment 6

[Daniel Holbert [:dholbert]]

So there are a few things going on here.
A) The new behavior (from bug 1374540) is to make flex items default to their max-content size.  And here, the max-content size is quite small, because the descendants (the text/labels) are absolutely positioned, so they don't contribute any size to their ancestors' max-content size.  So that's part of why things are ending up ~zero-sized and collapsing on each other.

B) There's also a bit of sizing weirdness around tables, because tables spawn an anonymous wrapper-box (with its own style context).  That wrapper-box is the real flex item, and the table (the thing you set "flex-basis" on) is its child.  Browsers aren't really in agreement about whether/how the table's "flex-basis" makes it out to that wrapper-box.  (Chrome says it does make it to the wrapper-box, Firefox & Edge say it does not, as can be seen via testcase 3.)

This bug arises from both of those ^^ factors, combined. (Normally, "flex-basis:auto;width:100%" on a child of a flex container would give you a flex base size of 100% of the flex container's size -- but here, the flex item is the table wrapper box, which has default "flex-basis:auto; width: auto" and which is unstylable.  So that flex item ends up getting the default flex-base-size behavior, which (as of bug 1374540) is to use the content size, which is ~0 here due to the abspos children.  (Previously we'd use "fit-content" behavior, which in this case apparently honored the "width:100%" on the descendant table-box, whereas 'max-content' sizing does not seem to honor it.)

I'll poke around to figure out what Edge is doing to avoid failing in this way, since they're closest to our behavior here (and since, like us, they implement "flex-basis:content", while Chrome does not).  Perhaps they're treating "flex-basis:auto" on a table-wrapper box as cloning the *table-box*'s main-size property... (rather than the unstylable-and-always-auto table-wrapper box's main-size property...)

Comment 7

[Daniel Holbert [:dholbert]]

Attachment: testcase 4 (flex-basis:auto, width non-auto, comparing px value vs % value

> Perhaps they're treating "flex-basis:auto" on a
> table-wrapper box as cloning the *table-box*'s
> main-size property... (rather than the
> unstylable-and-always-auto table-wrapper
> box's main-size property...)

Hmm, actually, *we* seem to do this too, effectively, as shown by this testcase -- and it works reasonably when the main-size property uses px units, but stuff ends up overlapping when it has % units.

Comment 8

[Daniel Holbert [:dholbert]]

OK, yeah, tables are weird. :)  Here's what's going on in testcase 4. (Interpret "width" as "main-size property", in general; I'll use "width" for concreteness and since that's the property in question in the original site & the testcases here, since we've got horizontal flex containers.)

 - Firefox Nightly is triggering the "IsUsedFlexBasisContent" clause in nsFrame::ComputeSize() for *all table-wrapper boxes* (not just in testcase 4, but everywhere) and doing max-content sizing as a result.  It necessarily always triggers this clause, because table-wrapper boxes always have flex-basis:auto;width:auto;height:auto, and there's no way for authors to change that.

 - For the table that has a px-valued width, it *seems* like flex-basis is cloning that width.  But in fact, it's not! Really, we're doing max-content sizing, and the max-content size of the table-wrapper box takes the fixed px-valued size of its child-box (the table) into account.  So that's how the px value ends up influencing the size of that table flex item.

 - In contrast, for the table that has a percent-valued width: that percent value (on the table child of the table-wrapper-box) *does not influence* the max-content size of the table-wrapper box, because percent widths aren't resolvable on the child of a box whose max-content width we're in the process of measuring.  So we end up using the descendants' content-width (which is skinny) for the flex base size and the size of our flex item.

 - In our final layout of that flex item, though, the table *can* resolve its percent size (against itscontaining block, the flex container) though -- so it does, and so it ends up sticking out of its wrapper.

 - This is how Nightly ends up with overlapping content in testcase 4.


Bottom line: the whole "table as a flex item" story is pretty terrible & behaves a bit differently everywhere, since flex-basis & explicit height/width values aren't specced as "inheriting up" from the table to the table wrapper (and they don't, in Firefox/Edge), which makes for pretty counterintuitive behavior for authors.

I think *technically* Nightly is following the spec here, but I think it's not what anyone actually wants, and we should probably exclude table wrappers from this IsUsedFlexBasisContent() clause so that they end up getting "width:auto" sizing behavior rather than "width:max-content" sizing behavior.  That gives them a chance to resolve percentages on their table-box child, which is what previously happened in Firefox and which is what happens in other browsers.

Comment 9

[Daniel Holbert [:dholbert]]

Attachment: Bug 1455976: Give table wrapper boxes a special case during flex base size resolution, so that percent main-sizes can be respected.

Review commit: https://reviewboard.mozilla.org/r/238806/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/238806/

Comment 10

[Daniel Holbert [:dholbert]]

I still need to add automated tests -- but based on local testing, the attached patch makes us revert to the Firefox 60-and-earlier behavior (i.e. revert to our pre-regression behavior) on the busted URL *and* on all testcases here.

(This makes us go back to agreeing with Edge on all testcases, except for testcase 3, where Edge does its own thing -- it makes the tables overflow [refusing to shrink any of them below their measured 100% flex base size].  In contrast, Firefox 60-and-earlier (and Nightly-with-this-patch) will gracefully shrink the tables there to divide up the space equally [which happens to match Chrome])

Comment 11

[Daniel Holbert [:dholbert]]

Also worth noting: I don't think we need this special case in ComputeSizeWithIntrinsicDimensions(), because this is specific to table-wrapper flex items, and table-wrappers don't have an intrinsic ratio that informs their size calculations and hence don't receive calls to ComputeSizeWithIntrinsicDimensions().

Comment 12

[Mats Palmgren (inactive)]

I tend to think we should always use the table box when testing for "auto-ness"
of style values etc, because it has the true values as specified by the author.
Even if we use the table-wrapper value for layout later.
https://searchfox.org/mozilla-central/rev/fcd5cb3515d0e06592bba42e378f1b9518497e3d/layout/generic/nsFlexContainerFrame.cpp#4457
Granted, it might be a bit tricky to get right in this case though...

What would happen if use the table box styles for the
IsUsedFlexBasisContent calls, and add 'flex-basis:inherit' on
the table-wrapper in the UA sheet?

Comment 13

[Mats Palmgren (inactive)]

> because it has the true values as specified by the author

Or rather, I think those values are the ones that matters from
a CSS specification point of view.  When the spec says "do this
if 'width' is auto", then it's definitely the table box value
that should be tested.

Comment 14

[Daniel Holbert [:dholbert]]

(In reply to Mats Palmgren (:mats) from comment #12)
> What would happen if use the table box styles for the
> IsUsedFlexBasisContent calls, and add 'flex-basis:inherit' on
> the table-wrapper in the UA sheet?

That's  bigger departure from what we've been doing up until now.  And if we were to do that, for consistency, we'd then need to also honor (i.e. "inherit up") the other flex sizing properties (flex-grow and flex-shrink) from the table-box as well.  We do not do that currently, nor does Edge.  Chrome does, but I don't think there's spec support for it, and I think there's been CSSWG discussion about this and the spec editors (at least) prefer our behavior, from a "keep it simple" perspective.  I'll see if I can find a link to that discussion... But anyway, here's a testcase for that behavior (testing whether the table's own "flex-grow" property gets to participate in flex layout): https://jsfiddle.net/q21334g6/

At this point I'd strongly prefer a more surgical go-back-to-what-we-were-doing-for-this-case change (i.e. the attached patch) rather than the larger inherit-all-the-sizing-properties change, seeing as I'm about to be gone for a few weeks (overlapping a merge) and I wouldn't be surprised if the larger change might trigger some unexpected regressions/behavior-changes which I wouldn't be around to help sort through for a little while.

Comment 15

[Daniel Holbert [:dholbert]]

OK, it looks like these days, there's spec text that calls for something kinda in-between. Link: https://drafts.csswg.org/css-flexbox/#flex-items

Quoting:
# In the case of flex items with display: table,
# the table wrapper box becomes the flex item, and
# the order and align-self properties apply to it.
# The contents of any caption boxes contribute
# to the calculation of the table wrapper box’s
# min-content and max-content sizes.

(Interjection: so far, this agrees with what we do.)

# However, like width and height, the flex longhands
# apply to the table box as follows: the flex item’s
# final size is calculated by performing layout as if
# the distance between the table wrapper box’s edges
# and the table box’s content edges were all part of
# the table box’s border+padding area, and the
# table box were the flex item.

This part ^^ does not agree with us (or Edge).  Basically, to match that, we need to add a number of special cases for tables, to make them *behave* as if the table-box were the flex item, and as if the wrapper-box were merely some extra padding around the table-box.  (But really, the table-wrapper box is the true flex item, and it just will get some special-casing.)

Your suggestion in comment 12 - 13 kind of shifts us in this direction, but makes us overstep it a bit.  Given that my patch gets us being back to being interoperable with Edge (and ourselves in the past), and given that this is edge-casey behavior anyway, I'd like to go with my simpler patch and track the doing-what-the-spec-actually-ended-up-calling-for in a separate bug.

WDYT?

Comment 16

[Mats Palmgren (inactive)]

Comment on attachment 8970046 [details]
Bug 1455976: Give table wrapper boxes a special case during flex base size resolution, so that percent main-sizes can be respected.

https://reviewboard.mozilla.org/r/238806/#review244494

Sure, I don't mind doing it like this for a while if you think that's better,
but I'd have to disagree with your "I don't think there's spec support for it".
I believe rather strongly that it's the table box styles that is meant to be
used for this kind of thing, not just in Flexbox, but in general.  I mean if
the table-wrapper box is using an UA default value that is never affected by
what the author specifies then that's clearly not what we should test.
Inheriting the values to the wrapper and the using those is of course fine
too.  So I believe Chrome is the one that's spec compliant here.
Please file a spec issue about this in case there isn't one, so we can get
this sorted out?

Comment 17

[Mats Palmgren (inactive)]

(In reply to Mats Palmgren (:mats) from comment #16)
> Please file a spec issue about this in case there isn't one, so we can get
> this sorted out?

OK, I didn't see your last comment before posting that, so perhaps this
is fine if the Flexbox spec already goes into some detail about how
this case should be handled.  I think in general though, we should assume
that what the spec(s) mean for cases like this is the table box styles.

Comment 18

[Daniel Holbert [:dholbert]]

Yeah, I think the goal of the second half of my spec quote in comment 15 is that we should **kind of** behave like those properties inherit up to the table wrapper box, but with graceful behavior when there are captions, which get subtracted out separately (they'd get space pre-reserved for them, independently from the "flex-basis" space, just like how we reserve space for borders/padding).

(But specifically on the "do-the-properties-inherit" question, the answer is no, for the flex-* subproperties.  The spec quote says those properties "apply to the table box", vs. order and align-self which "apply to [the table wrapper box]".  That language is a bit hand-wavy but I think it's intending to differentiate between which ones target/inherit-up to the table-wrapper vs. which don't.)


Thanks for the review! I'll file a followup to Do The Right Specced Thing here and add some testcases, which I'll post for review before landing.  (I'll put the ones where there's interop & clear spec support in the w3c-css directory, and I'll perhaps put a couple with less spec support in our own layout/reftests directory, just to document our current behavior & catch unexpected changes.)

Comment 19

[Daniel Holbert [:dholbert]]

I filed https://github.com/w3c/csswg-drafts/issues/2604 with a request for clarification in the spec, BTW.

Comment 20

[Daniel Holbert [:dholbert]]

Comment on attachment 8970046 [details]
Bug 1455976: Give table wrapper boxes a special case during flex base size resolution, so that percent main-sizes can be respected.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/238806/diff/1-2/

Comment 21

[Daniel Holbert [:dholbert]]

(Tests added, let me know if you have any feedback on them. I just put them in our own reftest directory, with a note that they should maybe migrate if the spec settles, because the spec situation here is vague right now per my github issue.)

Comment 22

[Mats Palmgren (inactive)]

Sounds good to me.

Comment 23

[Daniel Holbert [:dholbert]]

Thanks! I triggered autoland (and that tree's closed right now, so this will go in whenever it reopens).

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b692f7503da1
Give table wrapper boxes a special case during flex base size resolution, so that percent main-sizes can be respected. r=mats

Comment 25

[Andrei Ciure[:aciure]]

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

Comment 26

[Julien Wajsberg [:julienw]]

Latest nightly fixes the issue for me, thanks for the quick fix, now I'll be able to book my flight ;-)

Comment 27

[roxleitan]

Managed to reproduce the issue on Nightly 61.0a1(2018-04-22) on Windows 10 x64, Mac OS X 10.12 and Ubuntu 16.04 x64 using the link from the description and also with the attached test cases 1-4.

The issues from https://agence-voyage.oui.sncf and from test case 4 are not reproducible on the latest Nightly 61.0a1 (2018-04-29) on Windows 10 x64, Mac OS X 10.12 and Ubuntu 16.04 x64.

The issues from the test cases 1,2 and 3 are still not fixed.
Daniel, should I file a new bug, or those are covered in bug 1456224?

Comment 28

[Daniel Holbert [:dholbert]]

(In reply to [e-mail] from comment #27)
> The issues from https://agence-voyage.oui.sncf and from test case 4 are not
> reproducible on the latest Nightly 61.0a1 (2018-04-29) on Windows 10 x64,
> Mac OS X 10.12 and Ubuntu 16.04 x64.

Great! Thanks.

> The issues from the test cases 1,2 and 3 are still not fixed.
> Daniel, should I file a new bug, or those are covered in bug 1456224?

Those are covered by that other bug, I think - no need for a new one.

(For me on my Android device [only device I have for testing at the moment], testcase 1 is basically fixed, BTW - the expected result is for each color-border flex item to be on its own line. In broken nighties, I think we rendered all items overlapping each other, on the same line.)

Comment 29

[roxleitan]

Based on comment 27 and comment 28 I will mark this bug as Verified fixed.

Comment 30

[cyraid]

This bug is still happening, and there is stack overflow posts about trying to get around it by adding a wrapper around the table element. In chrome it renders fine, but firefox (as per other posts also suggest). Nothing really works in the 'flex world' of css to change the size of the table aside from stretch? (which I think just sets it to 100% instead of filling the remaining space). The CSS has been set to stretch, but the table keeps setting it's width/height to the content.

For an example, please see one of the posts I found on this matter on Stack: https://stackoverflow.com/questions/41421512/why-does-flex-box-work-with-a-div-but-not-a-table

Comment 31

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

Re comment 30:

The example in https://stackoverflow.com/questions/41421512/why-does-flex-box-work-with-a-div-but-not-a-table should be fixed by bug 1674302, targeting Firefox 87.