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

RESOLVED FIXED in Firefox 28

Status

()

defect
P3
normal
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

({testcase})

Trunk
mozilla29
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox27 unaffected, firefox28 fixed, firefox29 fixed)

Details

()

Attachments

(2 attachments, 1 obsolete attachment)

Assignee

Description

6 years ago
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.
Assignee

Updated

6 years ago
Summary: flex items → flex items not wrapping in nested multi-line flexbox with "overflow:hidden"
Posted file 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.
Assignee

Updated

6 years ago
Summary: flex items not wrapping in nested multi-line flexbox with "overflow:hidden" → flex items not wrapping in multi-line flexbox with "overflow:hidden"
Assignee

Comment 2

6 years ago
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.
Assignee

Comment 3

6 years ago
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.
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Assignee

Comment 4

6 years ago
(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.)
Depends on: 702508
No longer depends on: 939901
Assignee

Comment 5

6 years ago
Posted patch fix v1 (obsolete) — Splinter Review
This still needs tests, but this should do it for the fix.
Attachment #8345603 - Flags: review?(matspal)
Assignee

Comment 6

6 years ago
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.
Flags: in-testsuite?
Assignee

Updated

6 years ago
Keywords: testcase
Assignee

Comment 7

6 years ago
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.)
Attachment #8345603 - Attachment is obsolete: true
Attachment #8345603 - Flags: review?(matspal)
Attachment #8345632 - Flags: review?(matspal)

Comment 8

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

r=mats (and I agree we should take this on Aurora)
Attachment #8345632 - Flags: review?(matspal) → review+
Assignee

Comment 9

6 years ago
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.
Flags: needinfo?(dholbert)
Flags: in-testsuite?
Flags: in-testsuite+
Priority: -- → P3
https://hg.mozilla.org/mozilla-central/rev/9a8ca1c0e114
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Assignee

Comment 11

6 years ago
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.
Attachment #8345632 - Flags: approval-mozilla-aurora?
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.
Attachment #8345632 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.