Bug 52441 - WebKit flexbox shrink algorithm ignores min-width
: WebKit flexbox shrink algorithm ignores min-width
Status: RESOLVED WONTFIX
: WebKit
CSS
: 528+ (Nightly build)
: PC Mac OS X 10.5
: P2 Normal
Assigned To:
: http://oli.jp/bugs/webkit/flexbox-fle...
:
:
:
  Show dependency treegraph
 
Reported: 2011-01-14 03:50 PST by
Modified: 2011-10-06 09:54 PST (History)


Attachments
Patch (57.66 KB, patch)
2011-04-28 17:13 PST, Tony Chang
no flags Review Patch | Details | Formatted Diff | Diff
Patch (64.97 KB, patch)
2011-04-29 16:50 PST, Tony Chang
no flags Review Patch | Details | Formatted Diff | Diff
Patch (64.39 KB, patch)
2011-05-02 10:43 PST, Tony Chang
hyatt: review-
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2011-01-14 03:50:59 PST
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
------- Comment #1 From 2011-04-28 17:13:33 PST -------
Created an attachment (id=91597) [details]
Patch
------- Comment #2 From 2011-04-28 17:14:11 PST -------
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.
------- Comment #3 From 2011-04-29 10:59:56 PST -------
(From update of attachment 91597 [details])
Further testing shows that this regressed some other tests (a few form controls use -webkit-box and they're not clipping properly).
------- Comment #4 From 2011-04-29 16:50:28 PST -------
Created an attachment (id=91766) [details]
Patch
------- Comment #5 From 2011-05-01 09:56:48 PST -------
(From update of attachment 91766 [details])
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.
------- Comment #6 From 2011-05-02 10:22:34 PST -------
(From update of attachment 91766 [details])
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.
------- Comment #7 From 2011-05-02 10:43:35 PST -------
Created an attachment (id=91939) [details]
Patch
------- Comment #8 From 2011-05-02 10:44:11 PST -------
(In reply to comment #7)
> Created an attachment (id=91939) [details] [details]
> Patch

Rebased so it can run on the ews bots.
------- Comment #9 From 2011-05-04 15:13:14 PST -------
(From update of attachment 91939 [details])
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.
------- Comment #10 From 2011-10-06 09:54:24 PST -------
Not planning on fixing bugs in the old flexbox.