Bug 88437 – Add support for min-height:auto and min-width:auto

• Component: New Bugs
• Status: RESOLVED FIXED

[Ojan Vafai]

Add support for min-height:auto and min-width:auto

[Ojan Vafai]

Created attachment 146078 [details]
Patch

[Ojan Vafai]

Adding some more people who might have thoughts/objections to this approach.

[Gyuyoung Kim]

Comment on attachment 146078 [details]
Patch

Attachment 146078 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/12896982

[Ojan Vafai]

Created attachment 146108 [details]
Fix EFL compile failure

[Tony Chang]

Comment on attachment 146108 [details]
Fix EFL compile failure

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

The logic seems fine.  Would be nice to get Hyatt to sign off on the design.

> Source/WebCore/rendering/RenderBox.cpp:1741
> +    if ((widthType == MinSize && logicalWidth.isAuto()))

Nit: Extra parentheses.

> Source/WebCore/rendering/RenderBox.cpp:2616
> +        // FIXME: This Length isn't technically a MainSize, but this is the only case where we pass Length that's not directly
> +        // grabbed off the RenderStyle, so it's probably not worth adding an extra case to the enum.

What's to be fixed here?  Maybe the enum value could be MainSizeOrXXX.

> Source/WebCore/rendering/RenderBox.cpp:3005
> +    // FIXME: For non-flexboxes + min-width, this needs to treat non-flexboxes as 0.

I don't understand this comment.  Do you mean min-height?

> Source/WebCore/rendering/RenderDeprecatedFlexibleBox.cpp:1062
> +        else if (child->style()->minWidth().type() == Auto)
> +            minWidth = 0;

Should we have old flexbox test cases with auto?

> Source/WebCore/rendering/RenderFlexibleBox.cpp:158
> +    // FIXME: This should probably be checking for isSpecified since you should be able to use percentage, calc or viewport relative values for width;

Nit: End in a period, not ;.

> Source/WebCore/rendering/RenderFlexibleBox.cpp:208
> +    // FIXME: This should probably be checking for isSpecified since you should be able to use percentage, calc or viewport relative values for min-width;

Nit: End in a period.

> Source/WebCore/rendering/RenderFlexibleBox.cpp:214
> +    // FIXME: This should probably be checking for isSpecified since you should be able to use percentage, calc or viewport relative values for maxWidth;

Nit: End in a period and max-width.

> Source/WebCore/rendering/style/RenderStyle.h:1557
> -    static Length initialMinSize() { return Length(0, Fixed); }
> +    static Length initialMinSize() { return Length(); }

Nit: Length(Auto) just to be clear?

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

Should there be tests for min-height?

[Ojan Vafai]

Created attachment 151478 [details]
Patch

[Ojan Vafai]

Comment on attachment 146108 [details]
Fix EFL compile failure

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

>> Source/WebCore/rendering/RenderBox.cpp:1741
>> +    if ((widthType == MinSize && logicalWidth.isAuto()))
> 
> Nit: Extra parentheses.

Fixed.

>> Source/WebCore/rendering/RenderBox.cpp:2616
>> +        // grabbed off the RenderStyle, so it's probably not worth adding an extra case to the enum.
> 
> What's to be fixed here?  Maybe the enum value could be MainSizeOrXXX.

I did MainOrPreferredSize.

>> Source/WebCore/rendering/RenderBox.cpp:3005
>> +    // FIXME: For non-flexboxes + min-width, this needs to treat non-flexboxes as 0.
> 
> I don't understand this comment.  Do you mean min-height?

Yup.

>> Source/WebCore/rendering/RenderDeprecatedFlexibleBox.cpp:1062
>> +            minWidth = 0;
> 
> Should we have old flexbox test cases with auto?

Done

>> Source/WebCore/rendering/RenderFlexibleBox.cpp:158
>> +    // FIXME: This should probably be checking for isSpecified since you should be able to use percentage, calc or viewport relative values for width;
> 
> Nit: End in a period, not ;.

Done

>> Source/WebCore/rendering/RenderFlexibleBox.cpp:208
>> +    // FIXME: This should probably be checking for isSpecified since you should be able to use percentage, calc or viewport relative values for min-width;
> 
> Nit: End in a period.

done

>> Source/WebCore/rendering/RenderFlexibleBox.cpp:214
>> +    // FIXME: This should probably be checking for isSpecified since you should be able to use percentage, calc or viewport relative values for maxWidth;
> 
> Nit: End in a period and max-width.

done

>> Source/WebCore/rendering/style/RenderStyle.h:1557
>> +    static Length initialMinSize() { return Length(); }
> 
> Nit: Length(Auto) just to be clear?

I was just mimicing initialSize. I could change both?

>> LayoutTests/fast/css/auto-min-size.html:6
>> +shouldBe('div.style.minWidth', '""');
> 
> Should there be tests for min-height?

Yes. Done.

[Tony Chang]

Comment on attachment 151478 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Tests: fast/css/auto-min-size.html

Please include a link to the spec and a description about what 'auto' means for min-{height,width}.

> Source/WebCore/css/CSSParser.cpp:1993
> +        if (id == CSSValueAuto || id == CSSValueIntrinsic || id == CSSValueMinIntrinsic || id == CSSValueWebkitMinContent || id == CSSValueWebkitMaxContent || id == CSSValueWebkitFillAvailable || id == CSSValueWebkitFitContent)

Because of the above case doesn't have a break, this will allow max-width to be auto.  Please add some test cases for this.

> Source/WebCore/css/CSSParser.cpp:2008
> +        if (id == CSSValueAuto || id == CSSValueIntrinsic || id == CSSValueMinIntrinsic)

Same problem with allowing max-height to be auto.

[Ojan Vafai]

Created attachment 151500 [details]
Patch

[Ojan Vafai]

(In reply to comment #8)
> > Source/WebCore/css/CSSParser.cpp:1993
> > +        if (id == CSSValueAuto || id == CSSValueIntrinsic || id == CSSValueMinIntrinsic || id == CSSValueWebkitMinContent || id == CSSValueWebkitMaxContent || id == CSSValueWebkitFillAvailable || id == CSSValueWebkitFitContent)
> 
> Because of the above case doesn't have a break, this will allow max-width to be auto.  Please add some test cases for this.
> 
> > Source/WebCore/css/CSSParser.cpp:2008
> > +        if (id == CSSValueAuto || id == CSSValueIntrinsic || id == CSSValueMinIntrinsic)
> 
> Same problem with allowing max-height to be auto.

Good catch.

[Ojan Vafai]

Committed r122264: <http://trac.webkit.org/changeset/122264>

[Hayato Ito]

FYI.
I've moved the expected.txt from chromium-linux to chromium in http://trac.webkit.org/changeset/122296.

[Ojan Vafai]

(In reply to comment #12)
> FYI.
> I've moved the expected.txt from chromium-linux to chromium in http://trac.webkit.org/changeset/122296.

Whoops. It should just be next to the html file. The results are not platform specific. I accidentally ran this test as a pixel test first, which put it in the chromium-linux directory without me noticing. I'll move it tomorrow if noone else gets around to it first.

[Hayato Ito]

Thank you for letting me know it. Don't worry. Let me do it.

(In reply to comment #13)
> (In reply to comment #12)
> > FYI.
> > I've moved the expected.txt from chromium-linux to chromium in http://trac.webkit.org/changeset/122296.
> 
> Whoops. It should just be next to the html file. The results are not platform specific. I accidentally ran this test as a pixel test first, which put it in the chromium-linux directory without me noticing. I'll move it tomorrow if noone else gets around to it first.

[Hayato Ito]

Done in http://trac.webkit.org/changeset/122297.

[Ojan Vafai]

(In reply to comment #15)
> Done in http://trac.webkit.org/changeset/122297.

Thanks!