Bug 52441 – WebKit flexbox shrink algorithm ignores min-width

• Component: CSS
• Status: RESOLVED WONTFIX

[Oli Studholme]

When a flexible layout box element has a width that is much smaller than the width required by its children, some child boxes may be shrunk past their min-width, even to width:0. Firefox respects content min-width. For more info and an example:
http://oli.jp/bugs/webkit/flexbox-flex-shrinking.html

Checked in WebKit Version 5.0.3 (6533.19.4, r75772) and Chrome 8.0.552.237

[Tony Chang]

Created attachment 91597 [details]
Patch

[Tony Chang]

Hyatt, can you review this?

I'm going to be working with Ojan on the new flexbox spec, but wanted to start off by fixing some old flexbox bugs.

[Tony Chang]

Comment on attachment 91597 [details]
Patch

Further testing shows that this regressed some other tests (a few form controls use -webkit-box and they're not clipping properly).

[Tony Chang]

Created attachment 91766 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 91766 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=91766&action=review

> LayoutTests/platform/chromium-win/media/audio-delete-while-slider-thumb-clicked-expected.txt:-5
> -timeupdate

This seems unrelated.

> Source/WebCore/rendering/RenderBlock.cpp:4521
> +    if (!isTableCell() && !isFlexibleChild() && style()->logicalWidth().isFixed() && style()->logicalWidth().value() > 0)
>          m_minPreferredLogicalWidth = m_maxPreferredLogicalWidth = computeContentBoxLogicalWidth(style()->logicalWidth().value());
>      else {
>          m_minPreferredLogicalWidth = 0;

I guess I don't understand why flex children want no preferrered logical width.

[Tony Chang]

Comment on attachment 91766 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=91766&action=review

>> LayoutTests/platform/chromium-win/media/audio-delete-while-slider-thumb-clicked-expected.txt:-5
>> -timeupdate
> 
> This seems unrelated.

This change happened in http://trac.webkit.org/changeset/78562 for the other ports, but that change caused this test to timeout in Chromium.  Now that the test doesn't timeout anymore, the chromium specific result needs the change too.

>> Source/WebCore/rendering/RenderBlock.cpp:4521
>>          m_minPreferredLogicalWidth = 0;
> 
> I guess I don't understand why flex children want no preferrered logical width.

It still has a preferred logical width, it's just computed (see below the call to computeBlockPreferredLogicalWidths) rather than just using the provided css width.

What this is saying is, "if a flex child has a width specified, allow it to shrink below this width when applying the shrinking algorithm".

An example of this is the 4th case in the layout test.  In that test, the flexbox has a width of 300px and there are three children each with a width of 200px.  The children should shrink below 200px, but instead, they were overflowing.

[Tony Chang]

Created attachment 91939 [details]
Patch

[Tony Chang]

(In reply to comment #7)
> Created an attachment (id=91939) [details]
> Patch

Rebased so it can run on the ews bots.

[Dave Hyatt]

Comment on attachment 91939 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=91939&action=review

I'd strongly recommend just saving algorithmic changes for building a brand new version of flexbox that matches the spec, and leave this deprecated legacy version alone.

> Source/WebCore/rendering/RenderBlock.cpp:629
> +bool RenderBlock::isFlexibleChild()
> +{
> +    return parent() && parent()->isFlexibleBox() && !isPositioned() && style()->boxFlex() > 0.0f;
> +}

I know they don't allow it right now, but just to be forward-thinking, I'd make it !isFloatingOrPositioned instead of just !isPositioned().

Also, this seems incomplete to me.  What about flexing replaced elements?  Seems like all you're fixing here is RenderBlock children of a flexbox, but there can be other types of children like images or replaced elements.

> Source/WebCore/rendering/RenderFlexibleBox.cpp:1055
> +        if (child->style()->minWidth().isFixed() && child->scrollsOverflowX())

I don't completely understand this case.  I don't get why we'd ignore the minWidth of the object when you don't scroll.  I'd need to see the latest flexbox draft to see what they suggest, but this seems suspicious to me.

[Tony Chang]

Not planning on fixing bugs in the old flexbox.