948654 - flex items not wrapping in multi-line flexbox with "overflow:hidden"

Status: RESOLVED FIXED

(Core :: Layout, defect, P3)

Description

[Daniel Holbert [:dholbert]]

This was originally reported in bug 939901 comment 48.

STR: Load https://bugzilla.mozilla.org/attachment.cgi?id=8345511 in Firefox Nightly.

EXPECTED RESULTS: Bar with "EDIT" & "DELETE"  at the bottom of each "card".

ACTUAL RESULTS: That bar is not visible; instead, it's rendered as a partially-visible gray bar at the right edge of each card, clipped from "overflow:hidden".

As noted in the testcase's source, you'll get EXPECTED RESULTS if you remove "overflow:hidden".

Chrome (Blink) 32.0.1685.0 dev and Opera (Presto) 12.16 both show EXPECTED RESULTS.

Comment 1

[Patrick Westerhoff]

Attachment: testcase.html

A simpler test case, i.e. stripped down to the core problem which is a *single* wrapping flex box layout, not wrapping with hidden overflow.

Comment 2

[Daniel Holbert [:dholbert]]

Thanks, Patrick! That's helpful.

Even further-reduced testcase:

data:text/html,<div style="display: flex; flex-wrap: wrap; overflow: hidden; width: 30px; border: 1px solid black;"><div>a</div><div>b</div><div>c</div><div>d</div>

Reference case:
data:text/html,<div style="display: flex; flex-wrap: wrap; width: 30px; border: 1px solid black;"><div>a</div><div>b</div><div>c</div><div>d</div>

I think the problem here is that we need to inherit "flex-wrap" on -moz-scrolled-content, here:
  http://mxr.mozilla.org/mozilla-central/source/layout/style/ua.css#128

Otherwise, the "flex-wrap" property is only seen on the "outer" scrollframe, and not on the inner scrolled flexbox, which means "flex-wrap" doesn't actually end up having any effect.

Comment 3

[Daniel Holbert [:dholbert]]

Ah yes, I even have a commented-out line there :)
> 148   /* flex-wrap: inherit; FIXME: not yet supported (bug 702508) */
http://mxr.mozilla.org/mozilla-central/source/layout/style/ua.css#148

That just needs uncommenting.

Comment 4

[Daniel Holbert [:dholbert]]

(Fortunately, that line (and the 'align-content' line a bit upwards from it) are the only instances of '702508' in the codebase -- so I don't have any other straggling TODOs associated with that bug.)

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: fix v1

This still needs tests, but this should do it for the fix.

Comment 6

[Daniel Holbert [:dholbert]]

I intend to request backport approval to land this (trivial) patch on aurora, too.
 --> flagging as tracking-firefox28 to be sure it doesn't slip off the radar.

Comment 7

[Daniel Holbert [:dholbert]]

Attachment: fix v1 (now with reftests)

Here's the same fix, now with reftests for [flex-wrap, align-content] in both vertical and horizontal flex containers.

(These tests are just hg cp'd from existing reftests that check for other flex-container properties being inherited through a scroll frame. The testcase has content with & without 'overflow' set; the reference case does not use 'overflow' at all, but instead tweaks the sizing to match the testcase's clipped overflow-region.)

Comment 8

[Mats Palmgren (inactive)]

Comment on attachment 8345632 [details] [diff] [review]
fix v1 (now with reftests)

r=mats (and I agree we should take this on Aurora)

Comment 9

[Daniel Holbert [:dholbert]]

Landed: https://hg.mozilla.org/integration/mozilla-inbound/rev/9a8ca1c0e114

needinfo=me to request aurora approval once this has stuck on inbound and/or central.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9a8ca1c0e114

Comment 11

[Daniel Holbert [:dholbert]]

Comment on attachment 8345632 [details] [diff] [review]
fix v1 (now with reftests)

Requesting aurora approval. This is a very minimal change (just uncommenting two lines of CSS that I intended to uncomment as part of fixing bug 702508)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 702508 (feature)

User impact if declined: Misrendered web content (Two CSS properties ignored, if they're on an "overflow:hidden" element)

Testing completed (on m-c, etc.): local testing, 2 days on inbound, 1 day on m-c

Risk to taking this patch (and alternatives if risky): Very low.

String or IDL/UUID changes made by this patch: None.

Comment 12

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 8345632 [details] [diff] [review]
fix v1 (now with reftests)

low risk, approving for landing, since it's trivial i'm going to take off the tracking flag and just set status flags.

Comment 13

[Daniel Holbert [:dholbert]]

Landed on aurora: https://hg.mozilla.org/releases/mozilla-aurora/rev/0fa5f4f29f7f