1097984 - flex item with "flex-basis:auto; width:[non-auto]" ends up at its min-content size, instead of its max-content size

Status: RESOLVED WORKSFORME

(Core :: Layout, defect)

Description

[Daniel Holbert [:dholbert]]

Attachment: testcase 1

See attached testcase.

I'd expect both examples to render with no linebreaks (and use the item's preferred width), but for some reason, the second example renders with the item's min-content width.

This is likely a regression from one of the changes to implement a flexbox spec change in the past few months.

Comment 1

[Daniel Holbert [:dholbert]]

Looks like nsContainerFrame::ComputeAutoSize bails out early (and returns an auto-width of "0xdeadbeef" -- that's hardcoded into ComputeAutoSize), if it sees that we have a non-auto "width" property.

In this bug's case, that's bad, because we really do need that auto-size (to resolve "flex-basis:auto") even though our width happens to have a non-auto value.

So right now, we end up resolving our flex-basis:auto to "0xdeadbeef" (a huge negative number), which then gets clamped to the min-width, which is our min-content size.  So that's why we end up being sized to our min-content width.

Anyway, we need to teach that function that our inline style coord is really "flex-basis" if we're a flex item in a horizontal flex container. (much like the code in nsFrame::ComputeSize and  nsLayoutUtils::ComputeSizeWithIntrinsicDimensions().)

Comment 2

[Daniel Holbert [:dholbert]]

Note that this bug is only possible to hit if "flex-basis:auto" really means "auto-size me".  If we back out the flex-basis:main-size rename (due to possible pending spec changes, per bug 1093316) and revert back to the old meaning of "flex-basis:auto", then we won't have this bug anymore.

In that possible future world, we'd still need to fix this for the "content" keyword, though (which is what's been proposed as the alternative way to request auto-sizing behavior from flex-basis).

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: WIP patch

Here's a WIP patch that fixes the testcase. (WIP because this has some copypasted logic that we probably should really factor out)

We also should perhaps consider making "IsFlexItem()" use a frame-state bit, so that we can check that without requiring a virtual function call. (IsFlexItem uses GetParent()->GetType() right now.)  That'd keep this patch from slowing down ComputeAutoSize.

Comment 4

[Liviu Cirdei]

Hi,

Do you have any updates on this? Seems like the attached testcase still shows the issue on latest Nightly (48.0a1).

Comment 5

[Daniel Holbert [:dholbert]]

No updates at the moment; this is indeed still broken.

Comment 6

[Daniel Holbert [:dholbert]]

This is a non-issue at this point. When I originally filed this bug, IIRC the flexbox spec said that flex-basis:auto literally meant "use the item's auto width, and ignore any specified size on it". That behavior was changed and is now behind the content keyword.

We match the expected results if I change the testcase with s/auto/content/, so this was fixed at some point.

Comment 7

[Daniel Holbert [:dholbert]]

Attachment: testcase 2

Here's a testcase with the newer content keyword, to demonstrate that this does indeed work properly.

Comment 8

[Daniel Holbert [:dholbert]]

(For the record: mozregression shows that we stopped collapsing all the way to the min-content width with bug 1093316, so that's essentially what fixed this. We changed behavior later for testcase 2 when we added support for the content keyword.)