Flexitems no longer default min-width to min-content

• Component: New Bugs
• Status: RESOLVED FIXED

[Ojan Vafai]

Flexitems no longer default min-width to min-content

[Ojan Vafai]

Created attachment 192102 [details]
Patch

[Ojan Vafai]

*** Bug 97747 has been marked as a duplicate of this bug. ***

[Christian Biesinger]

yay!

this will also fix bug 110913. does nodeFromRect-shadow.html fail for this patch as well?

[WebKit Review Bot]

Comment on attachment 192102 [details]
Patch

Attachment 192102 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17037265

New failing tests:
media/media-document-audio-repaint.html

[Tony Chang]

Comment on attachment 192102 [details]
Patch

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

I had a few questions before I felt comfortable r+'ing this.

> Source/WebCore/ChangeLog:9
> +        The CSSWG has agreed to remove this requirement from the spec because
> +        it added too much confusion. This means we can remove all the places we

Can you link to the mailing list thread where this is agreed upon?

> Source/WebCore/rendering/RenderButton.cpp:116
> +    innerStyle->setFlexShrink(0);

Why wasn't button shrinking a problem before this change?

> LayoutTests/css3/flexbox/box-sizing.html:27
> +    -webkit-flex: 0 0 auto;
> +    flex: 0 0 auto;

Nit: 'none' seems clearer than '0 0 auto'.

> LayoutTests/css3/flexbox/content-height-with-scrollbars.html:12
> +    -webkit-flex: 0 0 auto;
> +    flex: 0 0 auto;

Ditto.

> LayoutTests/css3/flexbox/cross-axis-scrollbar.html:24
> +    -webkit-flex: 0 0 auto;
> +    flex: 0 0 auto;

Ditto.

> LayoutTests/css3/flexbox/cross-axis-scrollbar.html:40
> +    -webkit-flex: 0 0 auto;
> +    flex: 0 0 auto;

Ditto.

> LayoutTests/css3/flexbox/flex-item-min-size-expected.txt:-3
> -Tests that flex items have default min-size to min-content in the main axis direction.
> - PASS
> - PASS

Should we add some tests with min-width: -webkit-min-content?  I suspect we don't have explicit tests for this.  E.g., you could add min-width: -webkit-min-content to the flexitems and keep this test.

> LayoutTests/css3/flexbox/flexbox-baseline.html:25
> +    -webkit-flex: 0 0 auto;
> +    flex: 0 0 auto;

Ditto.

> LayoutTests/css3/flexbox/preferred-widths.html:12
> +    -webkit-flex: 0 0 auto;

Ditto. What about unprefixed?

> LayoutTests/fast/css/auto-min-size.html:16
> +shouldBe('div.style.minWidth', '"0px"');
>  shouldBe('div.style.maxWidth', '""');

Why does minWidth return 0px and maxWidth return an empty string?  Shouldn't these both be empty strings?

[Christian Biesinger]

Comment on attachment 192102 [details]
Patch

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

>> Source/WebCore/rendering/RenderButton.cpp:116
>> +    innerStyle->setFlexShrink(0);
> 
> Why wasn't button shrinking a problem before this change?

I thought the original was correct. Why do you not want the button content to shrink with the button?

>> LayoutTests/css3/flexbox/flexbox-baseline.html:25
>> +    flex: 0 0 auto;
> 
> Ditto.

Maybe add a class in flexbox.css and use that?

[Build Bot]

Comment on attachment 192102 [details]
Patch

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

New failing tests:
media/video-controls-rendering.html
mathml/presentation/fractions.xhtml
mathml/presentation/row-alignment.xhtml
media/controls-strict.html
mathml/presentation/mo.xhtml
mathml/presentation/mo-stretch.html
media/video-display-toggle.html
mathml/presentation/attributes.xhtml
media/nodesFromRect-shadowContent.html
media/audio-controls-rendering.html
fast/forms/control-clip-overflow.html
media/video-no-audio.html
mathml/presentation/subsup.xhtml
media/controls-styling-strict.html
mathml/presentation/fractions-vertical-alignment.xhtml
media/controls-without-preload.html
mathml/presentation/over.xhtml
media/video-playing-and-pause.html
mathml/presentation/roots.xhtml
media/controls-after-reload.html

[Build Bot]

Comment on attachment 192102 [details]
Patch

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

New failing tests:
editing/pasteboard/smart-paste-001.html
mathml/presentation/fractions.xhtml
media/controls-strict.html
http/tests/security/mixedContent/redirect-https-to-http-iframe-in-main-frame.html
fast/events/mouseover-mouseout.html
mathml/presentation/attributes.xhtml
fast/events/autoscroll-in-textarea.html
fast/events/mouse-moved-remove-frame-crash.html
canvas/philip/tests/2d.text.draw.fontface.notinpage.html
media/audio-controls-rendering.html
fast/history/visited-link-background-color.html
fast/events/mouseout-dead-subframe.html
fast/forms/control-clip-overflow.html
fast/writing-mode/vertical-rl-replaced-selection.html
media/controls-styling-strict.html
mathml/presentation/fractions-vertical-alignment.xhtml
fast/events/mouseover-mouseout2.html
fast/events/event-sender-mouse-moved.html
fullscreen/video-cursor-auto-hide.html
http/tests/plugins/third-party-cookie-accept-policy.html

[Christian Biesinger]

Several of those media tests would be fixed by bug 110913 together with bug 112068

[Tony Chang]

Comment on attachment 192102 [details]
Patch

r- due to test failures (to get out of the review queue).  Maybe most will be fixed by re-uploading?

[Ojan Vafai]

(In reply to comment #5)
> (From update of attachment 192102 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=192102&action=review
> 
> > Source/WebCore/ChangeLog:9
> > +        The CSSWG has agreed to remove this requirement from the spec because
> > +        it added too much confusion. This means we can remove all the places we
> 
> Can you link to the mailing list thread where this is agreed upon?

Looks like the spec was just updated.

> > LayoutTests/css3/flexbox/box-sizing.html:27
> > +    -webkit-flex: 0 0 auto;
> > +    flex: 0 0 auto;
> 
> Nit: 'none' seems clearer than '0 0 auto'.

I forgot none existed as a valid value! :)

> > LayoutTests/css3/flexbox/flex-item-min-size-expected.txt:-3
> > -Tests that flex items have default min-size to min-content in the main axis direction.
> > - PASS
> > - PASS
> 
> Should we add some tests with min-width: -webkit-min-content?  I suspect we don't have explicit tests for this.  E.g., you could add min-width: -webkit-min-content to the flexitems and keep this test.

I started with this. It turns out we don't actually support intrinsic sizing keywords as a width value on flex items before or after this patch.

> > LayoutTests/fast/css/auto-min-size.html:16
> > +shouldBe('div.style.minWidth', '"0px"');
> >  shouldBe('div.style.maxWidth', '""');
> 
> Why does minWidth return 0px and maxWidth return an empty string?  Shouldn't these both be empty strings?

Whoops. This was just the test being dumb. We were setting the value to 0 before this and setting it to auto just kept the old value. I reordered the test to make it so that these return the empty string.

(In reply to comment #6)
> (From update of attachment 192102 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=192102&action=review
> 
> >> Source/WebCore/rendering/RenderButton.cpp:116
> >> +    innerStyle->setFlexShrink(0);
> > 
> > Why wasn't button shrinking a problem before this change?
> 
> I thought the original was correct. Why do you not want the button content to shrink with the button?

I was confused. There appears to be a flexbox bug hiding in here. I'll fix that first.

> >> LayoutTests/css3/flexbox/flexbox-baseline.html:25
> >> +    flex: 0 0 auto;
> > 
> > Ditto.
> 
> Maybe add a class in flexbox.css and use that?

That's a good suggestion, but most of these tests are just styling "div" or ".flexbox > div" and I'd rather not modify them a bunch.

[Ojan Vafai]

Button bug 112398.

[Ojan Vafai]

Created attachment 195807 [details]
Patch

[Tony Chang]

Comment on attachment 195807 [details]
Patch

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

> Source/WebCore/rendering/RenderBox.cpp:2204
> +    ASSERT(!logicalWidth.isUndefined());

This was true before this patch, right?

> LayoutTests/ChangeLog:14
> +        * css3/flexbox/flex-item-min-size-expected.txt: Removed.
> +        * css3/flexbox/flex-item-min-size.html: Removed.
> +        These tests are now redundant with tests in fast/css-intrinsic-dimensions.

Ok, although it would be nice to have more tests for flex items with a min-width of -webkit-min-content.

[Ojan Vafai]

Comment on attachment 195807 [details]
Patch

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

>> Source/WebCore/rendering/RenderBox.cpp:2204
>> +    ASSERT(!logicalWidth.isUndefined());
> 
> This was true before this patch, right?

Oh, this was a bad merge. I'll remove this line...this assert isn't really useful.

>> LayoutTests/ChangeLog:14
>> +        These tests are now redundant with tests in fast/css-intrinsic-dimensions.
> 
> Ok, although it would be nice to have more tests for flex items with a min-width of -webkit-min-content.

Yeah...I started converting this test and then it was clear it was going to be very time-consuming and decided it wasn't worth it.

[Ojan Vafai]

Committed r147261: <http://trac.webkit.org/changeset/147261>

[David Kilzer (:ddkilzer)]

(In reply to comment #16)
> Committed r147261: <http://trac.webkit.org/changeset/147261>

This change caused a regression: Bug 114566.

[Simon Fraser (smfr)]

This caused bug 115221

(In reply to comment #16)
> Committed r147261: <http://trac.webkit.org/changeset/147261>

This caused bug 115329.