Bug 112652 – Make intrinsic size keywords on flexboxes work

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Ojan Vafai]

Make intrinsic size keywords on flexboxes work

[Ojan Vafai]

Created attachment 193720 [details]
Patch

[Build Bot]

Comment on attachment 193720 [details]
Patch

Attachment 193720 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/17136597

New failing tests:
css3/flexbox/flexbox-baseline.html

[Build Bot]

Comment on attachment 193720 [details]
Patch

Attachment 193720 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/17132297

New failing tests:
css3/flexbox/flexbox-baseline.html

[Tony Chang]

Comment on attachment 193720 [details]
Patch

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

> LayoutTests/ChangeLog:15
> +        * platform/chromium-linux/css3/flexbox/flexbox-baseline-expected.png:
> +        * platform/chromium-win/css3/flexbox/flexbox-baseline-expected.txt:
> +        This looks like a rounding difference. The new result matches the non-column result
> +        in this same test, so it looks more correct to me.

Yeah, this change seems fine to me.  You probably need TestExpectation suppressions for text failures on Mac and other non-chromium ports.  You probably need image suppressions for chromium mac and chromium win.

> LayoutTests/fast/css-intrinsic-dimensions/intrinsic-sized-column-flex-items.html:10
> +    display: -webkit-flex;
> +    display: flex;
> +    -webkit-flex-direction: column;
> +    flex-direction: column;

Can we use resources/flexbox.css for these?

> LayoutTests/fast/css-intrinsic-dimensions/intrinsic-sized-column-flex-items.html:17
> +    -webkit-flex: none;
> +    flex: none;
> +    display: -webkit-flex;
> +    display: flex;

Ditto.

> LayoutTests/fast/css-intrinsic-dimensions/intrinsic-sized-column-flex-items.html:31
> +    <div class="child" style="width: -webkit-max-content;" data-expected-width="210">

Nit: I would probably make a class for the width so when we drop the prefix, it'll be easy to update the test.

> LayoutTests/fast/css-intrinsic-dimensions/intrinsic-sized-flex-items.html:8
> +    display: -webkit-flex;
> +    display: flex;

Ditto.

> LayoutTests/fast/css-intrinsic-dimensions/intrinsic-sized-flex-items.html:15
> +    display: -webkit-flex;
> +    display: flex;

Ditto.

[Christian Biesinger]

Comment on attachment 193720 [details]
Patch

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

> Source/WebCore/rendering/RenderFlexibleBox.cpp:929
>      // FIXME: Support intrinsic min/max lengths.

Is this fixed now?

[Ojan Vafai]

Comment on attachment 193720 [details]
Patch

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

>> Source/WebCore/rendering/RenderFlexibleBox.cpp:929
>>      // FIXME: Support intrinsic min/max lengths.
> 
> Is this fixed now?

Yes! Didn't notice the FIXME. :)

[Ojan Vafai]

> > LayoutTests/fast/css-intrinsic-dimensions/intrinsic-sized-column-flex-items.html:10
> > +    display: -webkit-flex;
> > +    display: flex;
> > +    -webkit-flex-direction: column;
> > +    flex-direction: column;
> 
> Can we use resources/flexbox.css for these?

As discussed in person, it's a bit weird to have these tests reach out into a different (non-ancestor) directory for their CSS. I don't feel strongly, but I lean towards leaving it as is.

> 
> > LayoutTests/fast/css-intrinsic-dimensions/intrinsic-sized-column-flex-items.html:17
> > +    -webkit-flex: none;
> > +    flex: none;
> > +    display: -webkit-flex;
> > +    display: flex;
> 
> Ditto.
> 
> > LayoutTests/fast/css-intrinsic-dimensions/intrinsic-sized-column-flex-items.html:31
> > +    <div class="child" style="width: -webkit-max-content;" data-expected-width="210">
> 
> Nit: I would probably make a class for the width so when we drop the prefix, it'll be easy to update the test.

Whoops. Meant to do this. There's already a shared CSS file. Changed these tests to use it.

[Ojan Vafai]

Committed r146272: <http://trac.webkit.org/changeset/146272>