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

RESOLVED FIXED in Firefox 28

Status

()

Core
Layout
P3
normal
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

({testcase})

Trunk
mozilla29
testcase
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox27 unaffected, firefox28 fixed, firefox29 fixed)

Details

(URL)

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

3 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

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

Comment 1

3 years ago
Created attachment 8345533 [details]
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

3 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

3 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

3 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

3 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

3 years ago
Created attachment 8345603 [details] [diff] [review]
fix v1

This still needs tests, but this should do it for the fix.
Attachment #8345603 - Flags: review?(matspal)
(Assignee)

Comment 6

3 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.
tracking-firefox28: --- → ?
Flags: in-testsuite?
(Assignee)

Updated

3 years ago
Keywords: testcase
(Assignee)

Comment 7

3 years ago
Created attachment 8345632 [details] [diff] [review]
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.)
Attachment #8345603 - Attachment is obsolete: true
Attachment #8345603 - Flags: review?(matspal)
Attachment #8345632 - Flags: review?(matspal)
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

3 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
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
(Assignee)

Comment 11

3 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+
(Assignee)

Comment 13

3 years ago
Landed on aurora: https://hg.mozilla.org/releases/mozilla-aurora/rev/0fa5f4f29f7f
status-firefox28: --- → fixed
tracking-firefox28: ? → ---
Flags: needinfo?(dholbert)
status-firefox27: --- → unaffected
status-firefox29: --- → fixed
You need to log in before you can comment on or make changes to this bug.