Closed Bug 782441 Opened 12 years ago Closed 11 years ago

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


(Core :: Layout, defect)

Not set





(Reporter: dholbert, Assigned: dholbert)




(3 files, 1 obsolete file)

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.
Blocks: 812822
Would be great to get an assignee here.
Assignee: nobody → dholbert
No longer blocks: 812822
Summary: Make sure "overflow" works correctly on flex containers and flex items → support "overflow:hidden|scroll|auto" on flex containers
NOTE: The spec used to imply that flex containers shouldn't honor the "overflow" property:
...but that was apparently accidental.  The spec has since been updated to explicitly say "The ‘overflow’ property applies to flex containers":
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)
Attachment #722922 - Flags: review?(bzbarsky)
Attached patch (ignore) (obsolete) — Splinter Review
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.
Depends on: 849407
Comment on attachment 722924 [details] [diff] [review]

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
Attachment #722924 - Attachment description: part 2 (wip): Use AutoFlexItemStyleFixupSkipper for anonymous subtrees in ReResolveStyleContext → (ignore)
Attachment #722924 - Attachment is obsolete: true
Blocks: 841876
Comment on attachment 722922 [details] [diff] [review]

Attachment #722922 - Flags: review?(bzbarsky) → review+
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 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)
Attachment #727996 - Flags: review?(bzbarsky)
(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 on attachment 727996 [details] [diff] [review]
part 2: Make flex container properties inherit to scrolled frame

Attachment #727996 - Flags: review?(bzbarsky) → review+
Attached patch part 3: reftestsSplinter Review
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".)
Attachment #728100 - Flags: review?(bzbarsky)
(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.)
Try run:

No failures in these tests or any related tests. (just intermittents, looks like)
Comment on attachment 728100 [details] [diff] [review]
part 3: reftests

Attachment #728100 - Flags: review?(bzbarsky) → review+
Blocks: 1036404
You need to log in before you can comment on or make changes to this bug.