Bug 88437

Created attachment 146078 [details] Patch

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

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

Created attachment 146108 [details] Fix EFL compile failure

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?

Created attachment 151478 [details] Patch

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.

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.

Created attachment 151500 [details] Patch

(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.

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

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

(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.

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.

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

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