Issue 988523003: Reimplement min-width: auto

Issue 988523003: Reimplement min-width: auto (Closed)

Created:
5 years, 9 months ago by cbiesinger

Modified:
5 years, 8 months ago

Reviewers:
leviw_travelin_and_unemployed, yurys, lushnikov, Julien - ping for review, pfeldman, ojan

CC:
darktears, apavlov+blink_chromium.org, blink-reviews, blink-reviews-css, blink-reviews-rendering, blink-reviews-style_chromium.org, dglazkov+blink, Dominik Röttsches, eae+blinkwatch, eae, ed+blinkwatch_opera.com, jchaffraix+rendering, jfernandez, leviw+renderwatch, pdr+renderingwatchlist_chromium.org, Manuel Rego, rwlbuis, svillar, Tab Atkins, zoltan1

Base URL:
svn://svn.chromium.org/blink/trunk

Target Ref:
refs/heads/master

Project:
blink

Visibility:
Public.

More Reviews

Description

[css-flexbox] Reimplement min-width: auto Specified in http://dev.w3.org/csswg/css-flexbox/#min-size-auto We previously had this implemented, but reverted it in: https://bugs.webkit.org/show_bug.cgi?id=111790 The spec has since re-added it with some tweaks. This patch implements the new behavior and updates related tests and devtools UI to match. In case this patch breaks any website or chrome UI, the fix is likely to add: min-width: 0; min-height: 0; to any relevant flexitems. flexbox-flex-direction-column.html is a CSSWG test that also covers min-width. Once we import the official tests, we should remove our local copies. BUG=426898 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=193665

Patch Set 1 #

Patch Set 2 : more min-width #

Patch Set 3 : more min-width 0 #

Patch Set 4 : there's another css file?? #

Patch Set 5 : another min-height #

Patch Set 6 : inspector min-widths #

Patch Set 7 : alternative approach to devtools #

Patch Set 8 : argh #

Patch Set 9 : inspector fixed! hopefully? #

Patch Set 10 : ... #

Patch Set 11 : fully implement min-width spec #

Patch Set 12 : final changes to match spec #

Patch Set 13 : fix issues #

Patch Set 14 : more fixes #

Patch Set 15 : rebased #

Patch Set 16 : bad merge... #

Patch Set 17 : fix bug in percentageMainSizeIsResolvable #

Patch Set 18 : ... #

Patch Set 19 : fix red bots #

Patch Set 20 : rebaselined #

Total comments: 12

Patch Set 21 : review comments #

Patch Set 22 : rebased #

Total comments: 16

Patch Set 23 : review comments #

Patch Set 24 : rebased #

Total comments: 2

Patch Set 25 : review comments #

Created: 5 years, 8 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+237 lines, -48 lines)			Patch
M	LayoutTests/css3/flexbox/flex-one-sets-flex-basis-to-zero-px.html	View		1 chunk	+4 lines, -0 lines	0 comments	Download
M	LayoutTests/css3/flexbox/flexbox-height-with-overflow-auto.html	View		2 chunks	+3 lines, -1 line	0 comments	Download
A	LayoutTests/css3/flexbox/imported/flexbox-flex-direction-column.htm	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20	1 chunk	+57 lines, -0 lines	0 comments	Download
A	LayoutTests/css3/flexbox/imported/flexbox-flex-direction-column-expected.htm	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20	1 chunk	+51 lines, -0 lines	0 comments	Download
M	LayoutTests/css3/flexbox/mozilla/flexbox-items-as-stacking-contexts-2.html	View		1 chunk	+2 lines, -0 lines	0 comments	Download
M	LayoutTests/fast/css/auto-min-size.html	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22	2 chunks	+2 lines, -2 lines	0 comments	Download
M	LayoutTests/fast/css/auto-min-size-expected.txt	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22	1 chunk	+2 lines, -2 lines	0 comments	Download
M	Source/core/css/html.css	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24	3 chunks	+3 lines, -0 lines	0 comments	Download
M	Source/core/css/mediaControls.css	View	1 2 3 4 5 6 7 8 9 10 11 12	1 chunk	+1 line, -0 lines	0 comments	Download
M	Source/core/css/parser/CSSPropertyParser.cpp	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24	1 chunk	+1 line, -1 line	0 comments	Download
M	Source/core/css/themeInputMultipleFields.css	View	1 2 3	1 chunk	+1 line, -0 lines	0 comments	Download
M	Source/core/html/shadow/TextControlInnerElements.cpp	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16	1 chunk	+1 line, -0 lines	0 comments	Download
M	Source/core/layout/LayoutBox.h	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19	2 chunks	+7 lines, -7 lines	0 comments	Download
M	Source/core/layout/LayoutBox.cpp	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24	17 chunks	+49 lines, -28 lines	0 comments	Download
M	Source/core/layout/LayoutButton.cpp	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22	1 chunk	+2 lines, -0 lines	0 comments	Download
M	Source/core/layout/LayoutFlexibleBox.h	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20	1 chunk	+1 line, -0 lines	0 comments	Download
M	Source/core/layout/LayoutFlexibleBox.cpp	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24	4 chunks	+35 lines, -2 lines	0 comments	Download
M	Source/core/layout/LayoutGrid.cpp	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23	1 chunk	+1 line, -1 line	0 comments	Download
M	Source/core/layout/LayoutMenuList.cpp	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24	1 chunk	+2 lines, -0 lines	0 comments	Download
M	Source/core/layout/LayoutReplaced.cpp	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20	2 chunks	+2 lines, -2 lines	0 comments	Download
M	Source/core/layout/MultiColumnFragmentainerGroup.cpp	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16	1 chunk	+1 line, -1 line	0 comments	Download
M	Source/core/style/ComputedStyle.h	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24	1 chunk	+1 line, -1 line	0 comments	Download
M	Source/devtools/front_end/inspectorCommon.css	View	1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22	1 chunk	+8 lines, -0 lines	0 comments	Download

Messages

Total messages: 27 (8 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

cbiesinger

Added yurys for the devtools change. This was the easiest way to ensure that devtools ...

5 years, 9 months ago (2015-03-26 03:55:23 UTC) #4

lushnikov

if this change breaks devtools, it will break entire web. Shouldn't this be discussed first ...

5 years, 8 months ago (2015-03-31 17:45:09 UTC) #7

cbiesinger

Tab advised me that an intent to ship is not necessary because we're just updating ...

5 years, 8 months ago (2015-03-31 21:28:43 UTC) #8

leviw_travelin_and_unemployed

Just a couple nits since I was taking a look. https://codereview.chromium.org/988523003/diff/380001/Source/core/css/html.css File Source/core/css/html.css (right): https://codereview.chromium.org/988523003/diff/380001/Source/core/css/html.css#newcode613 ...

5 years, 8 months ago (2015-04-02 22:14:36 UTC) #10

lushnikov

lgtm per DevTools changes. @cbiesinger: I'm still a bit worried regarding informing web authors about ...

5 years, 8 months ago (2015-04-03 11:15:48 UTC) #11

lushnikov

https://codereview.chromium.org/988523003/diff/380001/Source/devtools/front_end/inspectorCommon.css File Source/devtools/front_end/inspectorCommon.css (right): https://codereview.chromium.org/988523003/diff/380001/Source/devtools/front_end/inspectorCommon.css#newcode1 Source/devtools/front_end/inspectorCommon.css:1: body /deep/ * { We might want to get ...

5 years, 8 months ago (2015-04-03 11:16:11 UTC) #12

cbiesinger

Still investigating that test failure, but the review comments are fixed. https://codereview.chromium.org/988523003/diff/380001/Source/core/css/html.css File Source/core/css/html.css (right): ...

5 years, 8 months ago (2015-04-03 18:49:17 UTC) #13

cbiesinger1

Alright, the test is now green (thanks to https://codereview.chromium.org/1052363002/). Please take another look!

5 years, 8 months ago (2015-04-04 05:17:12 UTC) #14

pfeldman

https://codereview.chromium.org/988523003/diff/420001/Source/devtools/front_end/inspectorCommon.css File Source/devtools/front_end/inspectorCommon.css (right): https://codereview.chromium.org/988523003/diff/420001/Source/devtools/front_end/inspectorCommon.css#newcode1 Source/devtools/front_end/inspectorCommon.css:1: body /deep/ * { - Do you think this ...

5 years, 8 months ago (2015-04-06 10:25:57 UTC) #16

Julien - ping for review

https://codereview.chromium.org/988523003/diff/380001/Source/core/css/parser/CSSPropertyParser.cpp File Source/core/css/parser/CSSPropertyParser.cpp (right): https://codereview.chromium.org/988523003/diff/380001/Source/core/css/parser/CSSPropertyParser.cpp#newcode827 Source/core/css/parser/CSSPropertyParser.cpp:827: validPrimitive = (id == CSSValueAuto || validWidthOrHeight(value, unitless)); Unneeded ...

5 years, 8 months ago (2015-04-06 17:06:47 UTC) #17

cbiesinger

Pavel - I'd be surprised if frameworks had Chrome-specific code that depended on the old ...

5 years, 8 months ago (2015-04-06 19:02:21 UTC) #18

cbiesinger

Please take another look! https://codereview.chromium.org/988523003/diff/380001/Source/core/css/parser/CSSPropertyParser.cpp File Source/core/css/parser/CSSPropertyParser.cpp (right): https://codereview.chromium.org/988523003/diff/380001/Source/core/css/parser/CSSPropertyParser.cpp#newcode827 Source/core/css/parser/CSSPropertyParser.cpp:827: validPrimitive = (id == CSSValueAuto ...

5 years, 8 months ago (2015-04-06 21:34:17 UTC) #19

Julien - ping for review

lgtm with the comments addressed. https://codereview.chromium.org/988523003/diff/380001/Source/core/css/parser/CSSPropertyParser.cpp File Source/core/css/parser/CSSPropertyParser.cpp (right): https://codereview.chromium.org/988523003/diff/380001/Source/core/css/parser/CSSPropertyParser.cpp#newcode827 Source/core/css/parser/CSSPropertyParser.cpp:827: validPrimitive = (id == ...

5 years, 8 months ago (2015-04-13 21:25:59 UTC) #20

lgtm with the comments addressed.

https://codereview.chromium.org/988523003/diff/380001/Source/core/css/parser/...
File Source/core/css/parser/CSSPropertyParser.cpp (right):

https://codereview.chromium.org/988523003/diff/380001/Source/core/css/parser/...
Source/core/css/parser/CSSPropertyParser.cpp:827: validPrimitive = (id ==
CSSValueAuto || validWidthOrHeight(value, unitless));
On 2015/04/06 21:34:16, cbiesinger wrote:
> On 2015/04/06 17:06:47, Julien - PST CC moar reviewers wrote:
> > Unneeded parentheses.
> 
> Yes, but this matches the style in the cases around it, for
> CSSPropertyWebkitMaxLogicalHeight above and CSSPropertyWebkitLogicalHeight
> below.

This goes against our usual convention. I would argue that it doesn't matter
what the rest of the code in the file looks like, we want consistency across the
codebase not a single file.

https://codereview.chromium.org/988523003/diff/420001/Source/core/layout/Layo...
File Source/core/layout/LayoutBox.h (right):

https://codereview.chromium.org/988523003/diff/420001/Source/core/layout/Layo...
Source/core/layout/LayoutBox.h:513: LayoutUnit
computeReplacedLogicalHeightUsing(SizeType, const Length& height) const;
On 2015/04/06 21:34:16, cbiesinger wrote:
> On 2015/04/06 17:06:47, Julien - PST CC moar reviewers wrote:
> > It really seems like SizeType is actually duplicating the following Length.
We
> > could infer the Length in the function above to avoid the duplication of
> > information.
> 
> I'm not sure I follow, could you explain? Are you suggesting that if
> SizeType==MinSize, we look up styleRef().minHeight() inside of this function?
> Maybe that would work for Min/Max, but it would not work for
> MainOrPreferredSize, considering that we want to use the flex basis in some
> cases.
> 
> Also, there is precedent for passing both SizeType and Length - see
> computeLogicalWidthUsing

OK, I missed that flexbox is special when it comes to MainOrPreferredSize.

https://codereview.chromium.org/988523003/diff/420001/Source/core/layout/Layo...
File Source/core/layout/LayoutFlexibleBox.cpp (right):

https://codereview.chromium.org/988523003/diff/420001/Source/core/layout/Layo...
Source/core/layout/LayoutFlexibleBox.cpp:868: LayoutUnit clampedSize =
resolvedFlexBasis;
On 2015/04/06 21:34:16, cbiesinger wrote:
> On 2015/04/06 17:06:47, Julien - PST CC moar reviewers wrote:
> > We don't seem to need |resolvedFlexBasis| as we directly copy its value into
> > |clampedSize|.
> 
> clampedSize is basically std::min(resolvedFlexBasis, maxExtent) -- the if
below
> modified it. Do you think it's too unclear?

I think this would be clearer:

LayoutUnit clampedSize = maxExtent != -1 ? min(resolvedFlexBasis, maxExtent) :
resolvedFlexBasis;

https://codereview.chromium.org/988523003/diff/460001/Source/core/layout/Layo...
File Source/core/layout/LayoutBox.cpp (right):

https://codereview.chromium.org/988523003/diff/460001/Source/core/layout/Layo...
Source/core/layout/LayoutBox.cpp:2552: if (sizeType == MinSize &&
logicalWidth.isAuto())
There is an implicit assumption here. That max-width/max-height don't accept
'auto'. Maybe we should make it explicit by adding:

ASSERT(!logicalWidth.isAuto() || sizeType == MainOrPreferredSize);

wherever the pattern is used.

Julien - ping for review

+Ojan: FYI as you implemented that feature earlier and we are reverting the revert :(

5 years, 8 months ago (2015-04-13 21:26:36 UTC) #22

cbiesinger

5 years, 8 months ago (2015-04-13 23:23:13 UTC) #25

https://codereview.chromium.org/988523003/diff/380001/Source/core/css/parser/...
File Source/core/css/parser/CSSPropertyParser.cpp (right):

https://codereview.chromium.org/988523003/diff/380001/Source/core/css/parser/...
Source/core/css/parser/CSSPropertyParser.cpp:827: validPrimitive = (id ==
CSSValueAuto || validWidthOrHeight(value, unitless));
On 2015/04/13 21:25:59, Julien - OOO 18 Apr - 14 May wrote:
> On 2015/04/06 21:34:16, cbiesinger wrote:
> > On 2015/04/06 17:06:47, Julien - PST CC moar reviewers wrote:
> > > Unneeded parentheses.
> > 
> > Yes, but this matches the style in the cases around it, for
> > CSSPropertyWebkitMaxLogicalHeight above and CSSPropertyWebkitLogicalHeight
> > below.
> 
> This goes against our usual convention. I would argue that it doesn't matter
> what the rest of the code in the file looks like, we want consistency across
the
> codebase not a single file.

Hmm ok, removed the parentheses.

https://codereview.chromium.org/988523003/diff/420001/Source/core/layout/Layo...
File Source/core/layout/LayoutFlexibleBox.cpp (right):

https://codereview.chromium.org/988523003/diff/420001/Source/core/layout/Layo...
Source/core/layout/LayoutFlexibleBox.cpp:868: LayoutUnit clampedSize =
resolvedFlexBasis;
On 2015/04/13 21:25:59, Julien - OOO 18 Apr - 14 May wrote:
> On 2015/04/06 21:34:16, cbiesinger wrote:
> > On 2015/04/06 17:06:47, Julien - PST CC moar reviewers wrote:
> > > We don't seem to need |resolvedFlexBasis| as we directly copy its value
into
> > > |clampedSize|.
> > 
> > clampedSize is basically std::min(resolvedFlexBasis, maxExtent) -- the if
> below
> > modified it. Do you think it's too unclear?
> 
> I think this would be clearer:
> 
> LayoutUnit clampedSize = maxExtent != -1 ? min(resolvedFlexBasis, maxExtent) :
> resolvedFlexBasis;

Done.

https://codereview.chromium.org/988523003/diff/460001/Source/core/layout/Layo...
File Source/core/layout/LayoutBox.cpp (right):

https://codereview.chromium.org/988523003/diff/460001/Source/core/layout/Layo...
Source/core/layout/LayoutBox.cpp:2552: if (sizeType == MinSize &&
logicalWidth.isAuto())
On 2015/04/13 21:25:59, Julien - OOO 18 Apr - 14 May wrote:
> There is an implicit assumption here. That max-width/max-height don't accept
> 'auto'. Maybe we should make it explicit by adding:
> 
> ASSERT(!logicalWidth.isAuto() || sizeType == MainOrPreferredSize);
> 
> wherever the pattern is used.

Added this assert on the line before & in the other places:
  ASSERT(sizeType == MinSize || sizeType == MainOrPreferredSize ||
!logicalWidth.isAuto());

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/988523003/480001

5 years, 8 months ago (2015-04-13 23:24:05 UTC) #26

commit-bot: I haz the power

5 years, 8 months ago (2015-04-14 05:11:15 UTC) #27

Message was sent while issue was closed.

Committed patchset #25 (id:480001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=193665

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages