Closed Bug 972595 Opened 8 years ago Closed 7 years ago

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

Categories

(Core :: Layout, defect, P4)

defect

Tracking

()

RESOLVED DUPLICATE of bug 1055887

People

(Reporter: dholbert, Assigned: dholbert)

References

Details

Attachments

(3 files, 1 obsolete file)

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
Assignee: nobody → dholbert
Priority: -- → P4
Attached file testcase 1
Attached patch fix v1 (wip)Splinter Review
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.
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.
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.
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.
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.
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?
Flags: needinfo?(gwhit)
Yes, IE12 renders per spec in this test case while Chrome/FF allow the input to overflow. Glad you are aware of it. Thanks.
Flags: needinfo?(gwhit)
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.
@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. :))
Attachment #8626503 - Attachment is obsolete: true
@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.
Thanks, Greg!
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.
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.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → INVALID
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.
Resolution: INVALID → DUPLICATE
Duplicate of bug: 1055887
You need to log in before you can comment on or make changes to this bug.