Bug 146020 – min-width/height should default to auto for flexbox items

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Sergio Villar Senin]

As stated here http://dev.w3.org/csswg/css-flexbox/#min-size-auto

[Sergio Villar Senin]

We need to merge these 3:

- https://crrev.com/193665
- https://crrev.com/194062
- https://crrev.com/195930

and part of this one:

https://crrev.com/194887

[Sergio Villar Senin]

Created attachment 255497 [details]
Patch

[Darin Adler]

Comment on attachment 255497 [details]
Patch

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

I’m not the right reviewer for this. Might need to be Hyatt.

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1857
> +inline static bool isFlexOrGrid(Node* element)

Type could be ContainerNode*.

[Build Bot]

Comment on attachment 255497 [details]
Patch

Attachment 255497 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6454581009580032

New failing tests:
mathml/presentation/menclose-notation-default-longdiv.html

[Build Bot]

Created attachment 255499 [details]
Archive of layout-test-results from ews102 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Build Bot]

Comment on attachment 255497 [details]
Patch

Attachment 255497 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6120144791142400

New failing tests:
mathml/presentation/menclose-notation-default-longdiv.html

[Build Bot]

Created attachment 255500 [details]
Archive of layout-test-results from ews105 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Sergio Villar Senin]

Created attachment 255639 [details]
Patch

This new version ensures that the new policy is not applied to RenderFlexibleBox derived classes. Also includes the removal of some hacks added to properly shrink some elements

[Build Bot]

Comment on attachment 255639 [details]
Patch

Attachment 255639 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5371899748024320

New failing tests:
fast/css/auto-min-size.html

[Build Bot]

Created attachment 255642 [details]
Archive of layout-test-results from ews100 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Build Bot]

Comment on attachment 255639 [details]
Patch

Attachment 255639 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5107354190217216

New failing tests:
fast/css/auto-min-size.html

[Build Bot]

Created attachment 255644 [details]
Archive of layout-test-results from ews106 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Sergio Villar Senin]

Created attachment 255918 [details]
Patch

[Build Bot]

Comment on attachment 255918 [details]
Patch

Attachment 255918 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5074698748559360

New failing tests:
fast/css/auto-min-size.html

[Build Bot]

Created attachment 255921 [details]
Archive of layout-test-results from ews102 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Build Bot]

Comment on attachment 255918 [details]
Patch

Attachment 255918 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5477820083994624

New failing tests:
fast/css/auto-min-size.html

[Build Bot]

Created attachment 255922 [details]
Archive of layout-test-results from ews104 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Sergio Villar Senin]

Created attachment 255998 [details]
Patch

[Sergio Villar Senin]

Hyatt maybe?

[Dave Hyatt]

Comment on attachment 255998 [details]
Patch

r=me

[Sergio Villar Senin]

Committed r189536: <http://trac.webkit.org/changeset/189536>

[Alex Christensen]

This caused 4 tests to fail everywhere on mac.  Please check before relanding.
Before: https://build.webkit.org/results/Apple%20Yosemite%20Release%20WK2%20(Tests)/r189535%20(8182)/results.html
After: https://build.webkit.org/results/Apple%20Yosemite%20Release%20WK2%20(Tests)/r189536%20(8183)/results.html

Newly failing tests:
fast/css/auto-min-size.html
+fast/forms/menulist-narrow-width.html
+fast/replaced/width100percent-menulist.html
+fast/replaced/width100percent-searchfield.html

[WebKit Commit Bot]

Re-opened since this is blocked by bug 149002

[Sergio Villar Senin]

Created attachment 260916 [details]
Patch for landing

Removed some code specific for the gtk port that somehow got into the final patch for review

[Sergio Villar Senin]

Created attachment 260917 [details]
Patch for landing v2

Removed some code specific for the gtk port that somehow got into the final patch for review

[Sergio Villar Senin]

Comment on attachment 260917 [details]
Patch for landing v2

Do not set r? as it was reviewed previously

[Sergio Villar Senin]

Committed r189567: <http://trac.webkit.org/changeset/189567>

[Simon Fraser (smfr)]

This broke various internal tools at Apple, and other companies, which now expand a flex box vertically instead of showing overflow-scroll. Were there any follow-up fixes in Blink?

[Sergio Villar Senin]

(In reply to comment #28)
> This broke various internal tools at Apple, and other companies, which now
> expand a flex box vertically instead of showing overflow-scroll. Were there
> any follow-up fixes in Blink?

Flexbox code in Blink has been subject to tons of changes lately. I can try to bisect the fix for that issue but I'd need a reduced test case. Could you try to cook one?

[Sergio Villar Senin]

(In reply to comment #28)
> This broke various internal tools at Apple, and other companies, which now
> expand a flex box vertically instead of showing overflow-scroll. Were there
> any follow-up fixes in Blink?

BTW just in case, as the ChangeLog says, many use cases behave the same just by adding min-width|height: 0px which was the previous default value.

[Jon Lee]

Examples appear to include: 156786, 157698

[Jon Lee]

Of the other bugs I attached, it looks like we definitely behave differently from Firefox and Chrome with 156786 and 157678. The others might be evangelism or behaves correctly.

[Radar WebKit Bug Importer]

<rdar://problem/28062217>

[Ilya Streltsyn]

It seems that under the following conditions:

- the flex container has flex-direction:column and overflow:auto/scroll;
- the flex item is a flex container itself with flex-direction:row

the default min-height:auto still doesn't work correctly (works as 0 instead of the content height). Here is a small self-explaining testcase demonstrating the issue: https://jsfiddle.net/9xzhb6m4/2/

[Darin Adler]

(In reply to Ilya Streltsyn from comment #34)
> It seems that under the following conditions:
> 
> - the flex container has flex-direction:column and overflow:auto/scroll;
> - the flex item is a flex container itself with flex-direction:row
> 
> the default min-height:auto still doesn't work correctly (works as 0 instead
> of the content height). Here is a small self-explaining testcase
> demonstrating the issue: https://jsfiddle.net/9xzhb6m4/2/

Would you be willing to file a new bug report? Or is there some reason that a comment on this existing report is a better way to handle this?

[Jon Lee]

(In reply to Ilya Streltsyn from comment #34)
> It seems that under the following conditions:
> 
> - the flex container has flex-direction:column and overflow:auto/scroll;
> - the flex item is a flex container itself with flex-direction:row
> 
> the default min-height:auto still doesn't work correctly (works as 0 instead
> of the content height). Here is a small self-explaining testcase
> demonstrating the issue: https://jsfiddle.net/9xzhb6m4/2/

Based on the jsfiddle, it looks like we currently agree with Chrome but disagree with Firefox.

Ilya, please file a new bug citing this example.