Bug 146020 - min-width/height should default to auto for flexbox items
Summary: min-width/height should default to auto for flexbox items
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sergio Villar Senin
URL:
Keywords: BlinkMergeCandidate, InRadar
Depends on: 149002
Blocks: 146018 146021
  Show dependency treegraph
 
Reported: 2015-06-16 08:40 PDT by Sergio Villar Senin
Modified: 2017-12-01 09:03 PST (History)
16 users (show)

See Also:


Attachments
Patch (53.41 KB, patch)
2015-06-24 11:23 PDT, Sergio Villar Senin
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-mavericks (614.03 KB, application/zip)
2015-06-24 11:57 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews105 for mac-mavericks-wk2 (589.24 KB, application/zip)
2015-06-24 12:06 PDT, Build Bot
no flags Details
Patch (57.73 KB, patch)
2015-06-26 09:16 PDT, Sergio Villar Senin
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews100 for mac-mavericks (545.00 KB, application/zip)
2015-06-26 09:55 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews106 for mac-mavericks-wk2 (662.38 KB, application/zip)
2015-06-26 10:02 PDT, Build Bot
no flags Details
Patch (57.84 KB, patch)
2015-07-01 07:42 PDT, Sergio Villar Senin
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-mavericks (549.01 KB, application/zip)
2015-07-01 08:21 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews104 for mac-mavericks-wk2 (588.33 KB, application/zip)
2015-07-01 08:26 PDT, Build Bot
no flags Details
Patch (57.86 KB, patch)
2015-07-02 00:17 PDT, Sergio Villar Senin
no flags Details | Formatted Diff | Diff
Patch for landing (57.33 KB, patch)
2015-09-10 03:55 PDT, Sergio Villar Senin
no flags Details | Formatted Diff | Diff
Patch for landing v2 (57.35 KB, patch)
2015-09-10 04:02 PDT, Sergio Villar Senin
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sergio Villar Senin 2015-06-16 08:40:37 PDT
As stated here http://dev.w3.org/csswg/css-flexbox/#min-size-auto
Comment 1 Sergio Villar Senin 2015-06-16 08:41:48 PDT
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
Comment 2 Sergio Villar Senin 2015-06-24 11:23:51 PDT
Created attachment 255497 [details]
Patch
Comment 3 Darin Adler 2015-06-24 11:29:37 PDT
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*.
Comment 4 Build Bot 2015-06-24 11:57:31 PDT
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
Comment 5 Build Bot 2015-06-24 11:57:33 PDT
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
Comment 6 Build Bot 2015-06-24 12:06:55 PDT
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
Comment 7 Build Bot 2015-06-24 12:06:58 PDT
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
Comment 8 Sergio Villar Senin 2015-06-26 09:16:47 PDT
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
Comment 9 Build Bot 2015-06-26 09:55:29 PDT
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
Comment 10 Build Bot 2015-06-26 09:55:33 PDT
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
Comment 11 Build Bot 2015-06-26 10:02:02 PDT
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
Comment 12 Build Bot 2015-06-26 10:02:06 PDT
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
Comment 13 Sergio Villar Senin 2015-07-01 07:42:15 PDT
Created attachment 255918 [details]
Patch
Comment 14 Build Bot 2015-07-01 08:21:19 PDT
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
Comment 15 Build Bot 2015-07-01 08:21:23 PDT
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
Comment 16 Build Bot 2015-07-01 08:26:26 PDT
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
Comment 17 Build Bot 2015-07-01 08:26:29 PDT
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
Comment 18 Sergio Villar Senin 2015-07-02 00:17:55 PDT
Created attachment 255998 [details]
Patch
Comment 19 Sergio Villar Senin 2015-07-07 11:32:36 PDT
Hyatt maybe?
Comment 20 Dave Hyatt 2015-08-31 13:49:03 PDT
Comment on attachment 255998 [details]
Patch

r=me
Comment 21 Sergio Villar Senin 2015-09-09 00:37:32 PDT
Committed r189536: <http://trac.webkit.org/changeset/189536>
Comment 22 Alex Christensen 2015-09-09 08:35:38 PDT
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
Comment 23 WebKit Commit Bot 2015-09-09 08:39:50 PDT
Re-opened since this is blocked by bug 149002
Comment 24 Sergio Villar Senin 2015-09-10 03:55:34 PDT
Created attachment 260916 [details]
Patch for landing

Removed some code specific for the gtk port that somehow got into the final patch for review
Comment 25 Sergio Villar Senin 2015-09-10 04:02:16 PDT
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
Comment 26 Sergio Villar Senin 2015-09-10 04:16:00 PDT
Comment on attachment 260917 [details]
Patch for landing v2

Do not set r? as it was reviewed previously
Comment 27 Sergio Villar Senin 2015-09-10 04:58:42 PDT
Committed r189567: <http://trac.webkit.org/changeset/189567>
Comment 28 Simon Fraser (smfr) 2016-05-02 17:27:15 PDT
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?
Comment 29 Sergio Villar Senin 2016-05-03 07:00:02 PDT
(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?
Comment 30 Sergio Villar Senin 2016-05-03 07:05:02 PDT
(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.
Comment 31 Jon Lee 2016-05-16 13:53:56 PDT
Examples appear to include: 156786, 157698
Comment 32 Jon Lee 2016-05-16 14:08:11 PDT
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.
Comment 33 Radar WebKit Bug Importer 2016-08-29 13:57:13 PDT
<rdar://problem/28062217>
Comment 34 Ilya Streltsyn 2017-11-30 07:53:29 PST
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/
Comment 35 Darin Adler 2017-11-30 09:15:23 PST
(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?
Comment 36 Jon Lee 2017-12-01 09:03:33 PST
(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.