972595 - Flex containers should use "flex-basis" instead of "width" for computing intrinsic widths of flex items

Status: RESOLVED DUPLICATE of bug 1055887

(Core :: Layout, defect, P4)

Description

[Daniel Holbert [:dholbert]]

Per http://lists.w3.org/Archives/Public/www-style/2014Feb/0483.html , the flexbox spec section on "Intrinsic Sizes" [1] says that each flex item's contribution is its "hypothetical main size".

And the definition of "hypothetical main size"[2] is based on flex-basis, rather than width/height. (though "auto" may fall back to the width/height value)

Currently, we use IntrinsicForContainer to compute each item's contribution, and that does use the "width" property. But we should change that to use flex-basis for horizontal flex items (with non-'auto' flex-basis).

[1] http://dev.w3.org/csswg/css-flexbox/#intrinsic-sizes
[2] http://dev.w3.org/csswg/css-flexbox/#hypothetical-main-size

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: testcase 1

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: fix v1 (wip)

Here's a first-pass fix; this fixes the attached testcase.  I'll bet we can more elegantly share code with the existing "width or flex-basis" chunks of nsLayoutUtils.cpp, though, so this stands to be improved a bit.

Comment 3

[Daniel Holbert [:dholbert]]

Note: Chrome/Blink matches our (broken) current rendering, incorrectly relying on 'width'.

Opera 12.16. (from local testing) and IE (according to the www-style post) rely on 'flex-basis' here and hence do not match our current rendering.

Comment 4

[Greg Whitworth]

If I am correct Daniel, would this change make this test work correctly in FF and match Blink/IE: http://jsfiddle.net/944tL115/4/ 

Also, we have seen Blink at times take flex-basis into account while others they do not.

Comment 5

[Daniel Holbert [:dholbert]]

I don't think that's related to this bug -- I think we're overflowing there because of "min-width:auto" imposing a large minimum width on the <input>. It's basically an instance of bug 1088586.

Blink matches us on that testcase if I add this CSS rule:
  input { min-width: -webkit-min-content }
which makes it "fake" min-width:auto support.

Comment 6

[Greg Whitworth]

We have implemented all of the aspects of the latest spec and due to the width of the input the flex container should most definitely include that width of the input (whether the input is large or not) to calculate the hypothetical main-size of the items. I don't want to hijack this thread for this bug so if you could follow up with me in email.

Comment 7

[Daniel Holbert [:dholbert]]

Attachment: testcase 2 (variant of Greg's jsfiddle)

Ah, I see what you mean. tl;dr: yes, that testcase would be fixed (in that we'd grow the containers to be large enough for the contents) if we fixed this bug.

I was confused because I only had Blink to compare to locally (since I run Linux), and Blink does *not* grow the containers -- instead, it shrinks the <input> to accomodate the incorrectly-too-small container. (as I noted in comment 5)  So it sort of looks right, but it's not (and I imagine it differs from IE, if IE does this right).

I'm attaching a testcase that's based on your fiddle, but with "flex:none" on the <input>, to prevent Blink from shrinking it & looking correct. This testcase compares "flex:0 0 80px" to "flex:none;width:80px", which *should* be equivalent, but aren't currently in Firefox or Chrome.

Can you confirm that the two examples in this testcase look the same (and don't overflow) in IE?

Comment 8

[Greg Whitworth]

Yes, IE12 renders per spec in this test case while Chrome/FF allow the input to overflow. Glad you are aware of it. Thanks.

Comment 9

[ettavolt]

Attachment: Shows flex-basis ignored on oversized content when overflow is hidden.

I would like to add one more test case.
From it you can see, that 0px flex-basis is ignored, if flex item has oversized content, unless overflow:auto is specified (uncomment corresponding css on .flex2 to see correction). Blink cuts the overflowed content as expected. Presto is bugged.
Spec says (http://www.w3.org/TR/css-flexbox-1/#hypothetical-main-size), that hypothetic main size is flex-basis if it is defined.

Comment 10

[Daniel Holbert [:dholbert]]

@ettavolt: Thanks for the testcase.  It's actually unrelated to this bug, though, and Firefox is doing the right thing there. What you're running into is the flexbox spec's "implied minimum size of flex items", described here: http://dev.w3.org/csswg/css-flexbox/#min-size-auto

"overflow:auto" does indeed disable that feature (as required by the spec); but a simpler way to disable it (if you want to disable it) is to simply set "min-height:0" or "min-width:0".

(Microsoft Edge matches Firefox's rendering of your testcase, too, which increases my confidence that we're doing the right thing & Blink's the odd one out. :))

Comment 11

[Greg Whitworth]

@dholbert

I opened a bug against Blink for this: https://code.google.com/p/chromium/issues/detail?id=504931&thanks=504931&ts=1435338782

Feel free to chime in if necessary, I'll ping Christian offline to ensure that he knows about this interop difference.

Comment 12

[Daniel Holbert [:dholbert]]

Thanks, Greg!

Comment 13

[ettavolt]

Aha, seems I've missed the statement "hypothetical main size is the item’s flex base size _clamped_ according to its _min_ and _max_ main size properties.".
So the new meaning for min and max sizes is the most confusing thing in this case.

Comment 14

[Daniel Holbert [:dholbert]]

Resolving as INVALID, since it seems there's no Firefox bug here. Thanks for filing, in any case; hopefully we'll get some investigation/interop on the Chrome/blink side here, via Greg's bug.

Comment 15

[Daniel Holbert [:dholbert]]

Sorry, my bad -- I resolved this as INVALID with only comment 9 through 13 in mind -- but those comments are about something different from what was originally being discussed here.

The discussion before comment 9 still represents a firefox bug.  Though it's ultimately about the "intrinsic sizes" section of the flexbox spec, which has been rewritten since this bug was filed.

So: I'm duping this forward to bug bug 1055887, which is on implementing the newer language there.