782441 - support "overflow:hidden|scroll|auto" on flex containers

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Daniel Holbert [:dholbert]]

Quoting dbaron from bug 666041 comment 97:
> So a few thoughts on things that should be followups after the initial
> landing:
[...]
>  (2) We should make sure 'overflow' works correctly on flex containers
>      and flex items.  This is likely to take a bit of work.

Comment 1

[David Bolter [:davidb] (NeedInfo me for attention)]

Would be great to get an assignee here.

Comment 2

[Daniel Holbert [:dholbert]]

NOTE: The spec used to imply that flex containers shouldn't honor the "overflow" property:
 http://lists.w3.org/Archives/Public/www-style/2013Jan/0465.html
...but that was apparently accidental.  The spec has since been updated to explicitly say "The ‘overflow’ property applies to flex containers":
 http://lists.w3.org/Archives/Public/www-style/2013Feb/0001.html

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: part 1: Add FCDATA_MAY_NEED_SCROLLFRAME

I believe this is *almost* as simple as just adding FCDATA_MAY_NEED_SCROLLFRAME.  (though a few additional tweaks are needed, which I'll post in subsequent patches)

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: (ignore)

This followup addresses the only issue that I've found so far.

The issue was basically this: With *just* part 1 applied, then style-recomputation can end up incorrectly applying flex-item fixup to the scrollbars (since their content-nodes are children of a content-node that has "display:flex"), so we end up converting them to display:block during the style-recomputation and reconstructing their frames as block-frames.

I'm not 100% sure that this is the right place to fix this, but this WIP does seem to fix the problem.

Comment 5

[Daniel Holbert [:dholbert]]

Comment on attachment 722924 [details] [diff] [review]
(ignore)

oops, attached the wrong "part 2" patch -- but I also realized that we *already* are hitting a version of the "part 2" issue that I described above, for
  <legend style="overflow:scroll;display:flex">
so I just spun part 2 off into its own bug: bug 849407

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 722922 [details] [diff] [review]
part 1: Add FCDATA_MAY_NEED_SCROLLFRAME

r=me

Comment 7

[Daniel Holbert [:dholbert]]

Attachment: part 2: Make flex container properties inherit to scrolled frame

When (belatedly) writing tests for this to prepare for landing, I noticed that e.g. "flex-direction" wasn't getting honored on flex containers w/ overflow:hidden|scroll|auto.

This is because the scrolled frame has its style context, which has all of these properties set to their default values.  We have to explicitly tell it to inherit these properties from its parent, the scrollframe, in order for them to get seen by nsFlexContainerFrame.

To make sure I wasn't forgetting anything, I cribbed from the property index at http://www.w3.org/TR/css3-flexbox/#property and listed all of the longhand properties that are marked as applying to flex containers (as opposed to those that apply to flex items).  This includes two properties we don't yet support, for multi-line flexbox. (bug 702508)

Comment 8

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #7)
> This is because the scrolled frame has its style context

er, "its _own_ style context"

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 727996 [details] [diff] [review]
part 2: Make flex container properties inherit to scrolled frame

r=me

Comment 10

[Daniel Holbert [:dholbert]]

Attachment: part 3: reftests

Here are some reftests.

* The "-1.html" reftests check that "overflow:hidden|scroll|auto" render correctly on a flex container, using "flex" to size the children (to be sure we're actually getting a flex container rather than a block), and using suitably-positioned/sized blocks in the reference case w/ "overflow" set to the same values.

* The "-2.html" reftests check that "flex-align:center" is honored on an overflow:hidden flex container. (That's one of the properties we're adding a ua.css pass-through for)

* The "-3.html" reftests check that "justify-content:space-around" is honored on an overflow:hidden flex container. (That's another of the properties we're adding a ua.css pass-through for.)

And there are "vert" and "horiz" versions of all of the above tests, checking that "flex-direction: column" is honored. (That's the last of the properties we're adding a ua.css pass-through for.)

Each of the tests also has a copy of whatever we're testing with "overflow" at its initial value, to demonstrate that the flex items do stick out of their container in that one. (That lets us know we're really getting clipping in the chunks that have "overflow:hidden".)

Comment 11

[Daniel Holbert [:dholbert]]

(Basically, each "vert-N" and "horiz-N" test are identical aside from the orientation -- flex-direction:column in the vertical one, to give us a vertical flexbox, and heights/widths swapped.)

Comment 12

[Daniel Holbert [:dholbert]]

Try run: https://tbpl.mozilla.org/?tree=Try&rev=0e36c0d9d8dd

No failures in these tests or any related tests. (just intermittents, looks like)

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 728100 [details] [diff] [review]
part 3: reftests

r=me

Comment 14

[Daniel Holbert [:dholbert]]

Landed:
part 1: https://hg.mozilla.org/integration/mozilla-inbound/rev/bdf0d126c1b8
part 2: https://hg.mozilla.org/integration/mozilla-inbound/rev/472f6793e512
part 3: https://hg.mozilla.org/integration/mozilla-inbound/rev/1d7b2eafeebc

Comment 15

[Joe Drew (not getting mail)]

https://hg.mozilla.org/mozilla-central/rev/bdf0d126c1b8
https://hg.mozilla.org/mozilla-central/rev/472f6793e512
https://hg.mozilla.org/mozilla-central/rev/1d7b2eafeebc