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

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[simonpatp]

Attachment: 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.

Comment 1

[Daniel Holbert [:dholbert]]

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.

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: reduced testcase 1

Comment 3

[Daniel Holbert [:dholbert]]

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.

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: 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.)

Comment 5

[Daniel Holbert [:dholbert]]

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

Comment 6

[Daniel Holbert [:dholbert]]

(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.

Comment 7

[Daniel Holbert [:dholbert]]

Attachment: 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).

Comment 8

[Daniel Holbert [:dholbert]]

Attachment: part 2: make abspos frames return false from IsFlexItem

Comment 9

[Daniel Holbert [:dholbert]]

Attachment: 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.

Comment 10

[Mats Palmgren (inactive)]

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

Comment 11

[Daniel Holbert [:dholbert]]

(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

Comment 12

[Daniel Holbert [:dholbert]]

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

Comment 13

[Daniel Holbert [:dholbert]]

Landed:
part 1: https://hg.mozilla.org/integration/mozilla-inbound/rev/455784759138
part 2: https://hg.mozilla.org/integration/mozilla-inbound/rev/15afc631509c

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/455784759138
https://hg.mozilla.org/mozilla-central/rev/15afc631509c