"min-width" not fully honored on absolutely-positioned element in flexbox

RESOLVED FIXED in mozilla29

Status

()

Core
Layout
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: simonpatp, Assigned: dholbert)

Tracking

Trunk
mozilla29
All
Linux
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 2 obsolete attachments)

(Reporter)

Description

4 years ago
Created attachment 8362058 [details]
original testcase

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux i686; rv:26.0) Gecko/20100101 Firefox/26.0 (Beta/Release)
Build ID: 20131206152524

Steps to reproduce:

View attachment


Actual results:

red box (ul) is not covered by blue boxes (li)


Expected results:

blue boxes (li) should have covered the red ul box so the entire very long string is visible. Removing the "width: 100%" rule on the ul works causes it to work correctly if the lime flexbox (div) is smaller than the ul, but it then does not expand as intended if the lime flexbox is larger (Ex: 300em should make all three elements 300em wide approximatly). It looks correct when (div.width is larger than the li text) or (when ul.width is unset and div.width is smaller than the li text). It behaves correctly when the absolute/relative positioning information and lime div height is removed and the lime div's width is varied.
(Assignee)

Comment 1

4 years ago
Seems like we're clamping the grandchildren based on the "width: 100%", even though the min-width is larger.

From tweaking the testcase in dev tools, it looks like this happens even if you drop the complexity of the percentage / min-content sizing and just use something simpler like "width: 10px; min-width: 100px" on the ul element.
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Updated

4 years ago
Hardware: x86 → All
(Assignee)

Updated

4 years ago
Attachment #8362058 - Attachment description: test.html → original testcase
(Assignee)

Comment 2

4 years ago
Created attachment 8362065 [details]
reduced testcase 1
(Assignee)

Comment 3

4 years ago
Ah, of course -- when we compute the abspos frame's size, we trigger the code path for "isFlexItem", here:
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsFrame.cpp?rev=0f0b3d2a7af5&mark=3975-3996#3975
because the frame's parent is a flex container.

This makes us ignore "min-width", because we're expecting that the flex layout algorithm will take min-width into account at the appropriate time later.

However, absolutely positioned items don't get sized with the flex layout algorithm (and aren't true flex items). So it never gets the min-width applied; we just behave as if there's no min-width.

The fix is simple -- we shouldn't treat abspos elements as flex items in that code.
(Assignee)

Updated

4 years ago
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
(Assignee)

Updated

4 years ago
Summary: Incorrect width of absolute element in relative flexbox → "min-width" not fully honored on absolutely-positioned element in flexbox
(Assignee)

Comment 4

4 years ago
Created attachment 8362072 [details] [diff] [review]
fix v1

This patch fixes both testcases for me.

(Automated tests still needed; I won't get to adding those tonight, but I'm requesting review ahead of time, in case you want to look at this before I get to adding them.)
Attachment #8362072 - Flags: review?(matspal)
(Assignee)

Comment 5

4 years ago
Note: This patch moves ::IsFlexItem to be non-inline, with its definition in nsFrame.cpp.

This is because it now depends on nsIFrame::IsAbsolutelyPositioned(), which is defined in nsIFrameInlines.h, which some of our callers (e.g. nsHTMLReflowState.h [1]) does not include.

(And that caller doesn't want to include nsIFrameInlines, either, because it's a .h file, which means it shouldn't depend on any inline-definition headers. IIUC, inline-definition headers are only supposed to be included by .cpp files -- not by other .h files)

[1] http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsHTMLReflowState.h#164
(Assignee)

Comment 6

4 years ago
(In reply to Daniel Holbert [:dholbert] from comment #5)
> Note: This patch moves ::IsFlexItem to be non-inline, with its definition in
> nsFrame.cpp.

Actually, this isn't technically necessary, and it's probably nice to keep ::IsFlexItem() inline and cheap. New patch coming up that does this.
(Assignee)

Comment 7

4 years ago
Created attachment 8362287 [details] [diff] [review]
part 1: Move nsCSSOffsetState constructor defn to .cpp file

This moves the nsCSSOffsetState constructor definition (and in particular, its IsFlexItem invocation) out of nsHTMLReflowState.h and into the .cpp file.

This will let us add code to IsFlexItem without breaking everything that #includes nsHTMLReflowState.h (the problem I was getting around in comment 5).
Attachment #8362072 - Attachment is obsolete: true
Attachment #8362072 - Flags: review?(matspal)
(Assignee)

Updated

4 years ago
Attachment #8362287 - Flags: review?(matspal)
(Assignee)

Comment 8

4 years ago
Created attachment 8362296 [details] [diff] [review]
part 2: make abspos frames return false from IsFlexItem
Attachment #8362296 - Flags: review?(matspal)
(Assignee)

Comment 9

4 years ago
Created attachment 8362346 [details] [diff] [review]
part 2 v2: Make abspos frames return false from IsFlexItem

Same patch, but now including a vertical reftest variant ("b" to the existing "a"), with the same reference case.
Attachment #8362296 - Attachment is obsolete: true
Attachment #8362296 - Flags: review?(matspal)
Attachment #8362346 - Flags: review?(matspal)
Comment on attachment 8362346 [details] [diff] [review]
part 2 v2: Make abspos frames return false from IsFlexItem

"(Is the parent a flex container frame, and are we not
an absolutely-positioned element?)" seems like it could be better phrased.
How about "(i.e. a non-abs-pos child of a flex container)"?
I don't think the comment needs to be more than a hint - people who
need the exact definition should look at the source.

r=mats either way
Attachment #8362346 - Flags: review?(matspal) → review+

Updated

4 years ago
Attachment #8362287 - Flags: review?(matspal) → review+
(Assignee)

Comment 11

4 years ago
(In reply to Mats Palmgren (:mats) from comment #10)
> How about "(i.e. a non-abs-pos child of a flex container)"?

Thanks, that's much better.

Try run: https://tbpl.mozilla.org/?tree=Try&rev=66ea727f21ab
(Assignee)

Comment 12

4 years ago
Previous patch was missing an #include for the inlines file, which caused build failures on Try (though strangely not on my local machine).

Try run with that fixed: https://tbpl.mozilla.org/?tree=Try&rev=6f0c988debf6
(Assignee)

Comment 13

4 years ago
Landed:
part 1: https://hg.mozilla.org/integration/mozilla-inbound/rev/455784759138
part 2: https://hg.mozilla.org/integration/mozilla-inbound/rev/15afc631509c
Flags: in-testsuite+
https://hg.mozilla.org/mozilla-central/rev/455784759138
https://hg.mozilla.org/mozilla-central/rev/15afc631509c
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
You need to log in before you can comment on or make changes to this bug.