Bug 38919

Here's the spec: http://dev.w3.org/csswg/css3-box/#the-width-and-height-properties I think intrinsic maps to max-content and min-intrinsic maps to min-content. Adding aliases for this shouldn't be too hard. It also shouldn't be too hard to implement fit-content.

Looking more closely, it looks like min-intrinsic only works with old flexbox, so we would have to implement it.

Looking at the code it appears that intrinsic maps to fit-content (shrink wrapping).

(In reply to comment #3) > Looking at the code it appears that intrinsic maps to fit-content (shrink wrapping). Yes, I think you're right.

(In reply to comment #4) > (In reply to comment #3) > > Looking at the code it appears that intrinsic maps to fit-content (shrink wrapping). > > Yes, I think you're right. This is more complicated than that even. For a regular non-deprecated flex box (RenderBox): intrinsic => fit-content min-intrisic => does nothing For a child of a deprecated flex box (RenderDeprecatedFlexBox): intrinsic => max-content min-intrinsic => min-content Having intrinsic mean "fit-content" outside a flex box seems wrong. Are we sure people depend on this behavior?

Created attachment 147645 [details] Patch Implementation of the writing mode width keywords

Created attachment 147648 [details] Patch Remove the tabs from the changelog. Seems my vim is misconfigured for that kind of file.

(In reply to comment #1) > Here's the spec: > http://dev.w3.org/csswg/css3-box/#the-width-and-height-properties I'm told that the css3-writing-modes spec is more current and we should ignore the css3-box spec: http://dev.w3.org/csswg/css3-writing-modes/#intrinsic-sizing

Comment on attachment 147648 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=147648&action=review We should probably put this behind a compile time flag until it's more complete (implemented for all the properties with tests). Also, please send an email to webkit-dev explaining the new feature. See http://trac.webkit.org/wiki/AddingFeatures and other similar emails. > Source/WebCore/ChangeLog:11 > + Implement the CSS3 writing mode keywords for width and height > + CSS properties (and associated min/max properties). It looks like we're parsing the value for min-{width,height} and width/height (not max), but currently only applying to width. We should be clear about that in the changelog. Since this will take more than one patch and webkit normally does one patch per bug, you may want to file a new bug for what you're implementing and have this bug depend on it (i.e., this bug would be a meta bug for tracking purposes). > Source/WebCore/css/CSSParser.cpp:1906 > + if (id == CSSValueAuto || id == CSSValueIntrinsic || id == CSSValueMinIntrinsic || id == CSSValueWebkitMinContent || id == CSSValueWebkitMaxContent || id == CSSValueWebkitFillAvailable || id == CSSValueWebkitFitContent) Why are we parsing auto on min-width/min-height? This will collide with Ojan's patch on bug 88437 that adds it for real. > Source/WebCore/css/CSSValueKeywords.in:575 > +// CSS3 writing modes keywords Maybe CSS3 intrinsic dimensions? writing modes keywords makes me think of vertical-rl, horizontal-bt, etc. > Source/WebCore/css/LengthFunctions.cpp:110 > case MinIntrinsic: > ASSERT_NOT_REACHED(); > return ZERO_LAYOUT_UNIT; > + case MinContent: > + ASSERT_NOT_REACHED(); > + return ZERO_LAYOUT_UNIT; Can we merge these cases like we do in the other switch statements? > Source/WebCore/css/StyleBuilder.cpp:362 > else if (intrinsicEnabled && primitiveValue->getIdent() == CSSValueIntrinsic) > setValue(styleResolver->style(), Length(Intrinsic)); > - else if (minIntrinsicEnabled && primitiveValue->getIdent() == CSSValueMinIntrinsic) > + else if (intrinsicEnabled && primitiveValue->getIdent() == CSSValueMinIntrinsic) Nit: I would nest ifs so you don't have to check intrinsicEnabled twice. It might also be more readable. > Source/WebCore/css/StyleBuilder.cpp:371 > + else if (fittedEnabled && primitiveValue->getIdent() == CSSValueWebkitMinContent) > + setValue(styleResolver->style(), Length(MinContent)); > + else if (fittedEnabled && primitiveValue->getIdent() == CSSValueWebkitMaxContent) > + setValue(styleResolver->style(), Length(MaxContent)); > + else if (fittedEnabled && primitiveValue->getIdent() == CSSValueWebkitFillAvailable) > + setValue(styleResolver->style(), Length(FillAvailable)); > + else if (fittedEnabled && primitiveValue->getIdent() == CSSValueWebkitFitContent) > + setValue(styleResolver->style(), Length(FitContent)); Nit: I would nest ifs so you don't have to check fittedEnabled twice. > Source/WebCore/platform/Length.h:226 > + // FIXME: This should probably be renamed, but I don't have a better name. > + bool isIntrinsicOrAuto() const { return type() == Auto || isIntrinsic() || isFitted(); } > + bool isIntrinsic() const { return type() == Intrinsic || type() == MinIntrinsic; } > + bool isFitted() const { return type() == MinContent || type() == MaxContent || type() == FillAvailable || type() == FitContent; } How about renaming Intrinsic and MinIntrinsic to LegacyIntrinsic and using Intrinsic for the new values? > LayoutTests/fast/css-writing-modes/style.css:2 > +body * { > + border: 5px solid red; I would just inline the styles in the test and -expected.html files. It will make it more obvious that the -expected.html isn't using the new values you are adding. > LayoutTests/fast/css-writing-modes/style.css:37 > + img where this overrides the intrinsic width to get regular block "auto" Nit: End the sentence with a period. > LayoutTests/fast/css-writing-modes/width.html:5 > + > +<div id="container"> > + Please add a sentence or two describing what the test is testing and how to tell visually if it's passing or failing.

Ah, Elliott explained to me that Mozilla currently only implements this for width. I think it's fine to land full support for this for width/min-width/max-width and file a separate bug for handling it in the height direction. We don't need a compile time flag if we fully support the values for width.

Comment on attachment 147648 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=147648&action=review >> Source/WebCore/ChangeLog:11 >> + CSS properties (and associated min/max properties). > > It looks like we're parsing the value for min-{width,height} and width/height (not max), but currently only applying to width. We should be clear about that in the changelog. > > Since this will take more than one patch and webkit normally does one patch per bug, you may want to file a new bug for what you're implementing and have this bug depend on it (i.e., this bug would be a meta bug for tracking purposes). I changed to only respect on the width now. Previously this patch enabled parsing of the values for height properties with StyleBuilder but shouldn't have. >> Source/WebCore/css/CSSParser.cpp:1906 >> + if (id == CSSValueAuto || id == CSSValueIntrinsic || id == CSSValueMinIntrinsic || id == CSSValueWebkitMinContent || id == CSSValueWebkitMaxContent || id == CSSValueWebkitFillAvailable || id == CSSValueWebkitFitContent) > > Why are we parsing auto on min-width/min-height? This will collide with Ojan's patch on bug 88437 that adds it for real. This was a copy/paste mistake. Removed. >> Source/WebCore/css/LengthFunctions.cpp:110 >> + return ZERO_LAYOUT_UNIT; > > Can we merge these cases like we do in the other switch statements? There's something about debugging above but I'm not sure it's really needed. Combined.

Created attachment 148209 [details] Patch Reworked patch for review comments, lots more tests. Removed code that allowed the new keywords for height properties even though they werent actually enforced.

Attachment 148209 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1 Source/WebCore/css/CSSParser.cpp:1912: One space before end of line comments [whitespace/comments] [5] Source/WebCore/css/CSSParser.cpp:1919: One space before end of line comments [whitespace/comments] [5] Source/WebCore/css/CSSParser.cpp:1952: One space before end of line comments [whitespace/comments] [5] Total errors found: 3 in 29 files If any of these errors are false positives, please file a bug against check-webkit-style.

(In reply to comment #13) > Attachment 148209 [details] did not pass style-queue: > > Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1 > Source/WebCore/css/CSSParser.cpp:1912: One space before end of line comments [whitespace/comments] [5] > Source/WebCore/css/CSSParser.cpp:1919: One space before end of line comments [whitespace/comments] [5] > Source/WebCore/css/CSSParser.cpp:1952: One space before end of line comments [whitespace/comments] [5] > Total errors found: 3 in 29 files > > > If any of these errors are false positives, please file a bug against check-webkit-style. So these are lines of code that already existed that I moved around. Should I be fixing the style of unchanged code that just moved locations in the file?

Comment on attachment 148209 [details] Patch Attachment 148209 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12973564 New failing tests: compositing/repaint/content-into-overflow.html fast/css/shadow-dom-scope.html css3/flexbox/align-absolute-child.html css3/flexbox/child-overflow.html css2.1/20110323/replaced-intrinsic-ratio-001.htm fast/repaint/overflow-into-content.html http/tests/misc/acid2-pixel.html http/tests/misc/acid2.html fast/table/resize-table-row.html fast/repaint/content-into-overflow.html fast/css/acid2-pixel.html fast/css/007.html tables/mozilla_expected_failures/bugs/bug14007-2.html fast/table/resize-table-cell.html svg/zoom/page/zoom-replaced-intrinsic-ratio-001.htm fast/table/resize-table-binding-cell.html css3/flexbox/writing-modes.html svg/as-image/img-preserveAspectRatio-support-2.html fast/css/acid2.html css2.1/t140201-c536-bgpos-00-b-ag.html

Created attachment 148235 [details] Archive of layout-test-results from ec2-cr-linux-04 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-04 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

(In reply to comment #14) > So these are lines of code that already existed that I moved around. Should I be fixing the style of unchanged code that just moved locations in the file? For small changes, I often go ahead and fix the style. If you want, you could do a cleanup patch to fix the style first, but this is trivial enough to just include in this patch. The layout test failures look like they might be related.

Created attachment 148379 [details] Patch Fix broken layout tests where I had disabled the auto keyword for height properties and check style warnings.

Comment on attachment 148379 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=148379&action=review Looks great, just some small nits. If you fix the nits, you can use "webkit-patch land-safely" to upload a new patch with cq? . > Source/WebCore/ChangeLog:25 > + Implement the CSS3 intrinsic dimension keywords for width properties and > + add most of the plumbing for height properties but don't expose them > + yet since this patch doesn't enforce them (matching current Gecko). > + http://dev.w3.org/csswg/css3-writing-modes/#intrinsic-sizing > + > + This patch implements -webkit-min-content, -webkit-max-content, > + -webkit-fill-available and -webkit-fit-content for all width > + properties. Nit: Move this between "Reviewed by ..." and "Tests: ..." > Source/WebCore/css/CSSParser.cpp:1945 > + case CSSPropertyWidth: // <length> | <percentage> | auto | inherit > case CSSPropertyWebkitLogicalWidth: > + if (id == CSSValueAuto || id == CSSValueIntrinsic || id == CSSValueMinIntrinsic || id == CSSValueWebkitMinContent || id == CSSValueWebkitMaxContent || id == CSSValueWebkitFillAvailable || id == CSSValueWebkitFitContent) Can we have width and webkitlogicalwidth check for the new values and fall through (/* no break */) to height/webkitlogicalheight? > LayoutTests/fast/css-intrinsic-dimensions/height-property-value.html:4 > +<!-- > + Tests that the height keywords are not respected by the parser yet. > +--> Put this in description(). > LayoutTests/fast/css-intrinsic-dimensions/width-dynamic-property-value.html:4 > +<!doctype html> > +<script src="../js/resources/js-test-pre.js"></script> > +<script> > + description('Tests that the width keywords are respected when dynamically assigned.'); Nit: Maybe merge the text output tests (tests using js-test-pre) together into a single test? Having fewer test files is a bit easier on to manage (better for svn and easier to mark failing tests). > LayoutTests/fast/css-intrinsic-dimensions/width-keyword-classes.css:2 > +/* > + Take every possible line break so the box width is the width of largest Please move this test into a subdirectory called resources.

Created attachment 148499 [details] Patch for landing

Comment on attachment 148499 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=148499&action=review > Source/WebCore/ChangeLog:6 > + Reviewed by tony@chromium.org. Nit: You didn't need to fill this in manually. 'webkit-patch land-safely' would have done it for you (and put in my name instead of my email).

Comment on attachment 148499 [details] Patch for landing Rejecting attachment 148499 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1 tony@chromium.org found in /mnt/git/webkit-commit-queue/LayoutTests/ChangeLog does not appear to be a valid reviewer according to committers.py. ERROR: /mnt/git/webkit-commit-queue/LayoutTests/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive). Full output: http://queues.webkit.org/results/12996283

Comment on attachment 148499 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=148499&action=review A few nits about the tests. There's no official style guide for tests, but here are some nits that might help in making these tests a little more maintainable. > LayoutTests/fast/css-intrinsic-dimensions/height-property-value.html:4 > +<!-- > + Tests that the height keywords are not respected by the parser yet. > +--> Nit: this is already in the description line. No need to duplicate. > LayoutTests/fast/css-intrinsic-dimensions/max-width-constrained.html:7 > +<!-- > + Tests the behavior of the intrinsic width keywords when applied to max-width > + by placing them inside a constrained container. This test passes if all > + blocks are constrained inside the container since all intrinsic widths > + should be too large to fit. > +--> Nit: since this is a reftest, you could include this in a div on the page without making the test more difficult to maintain. The advantage of a div on the page is it's easy to see what the test is supposed to test without having to view the source. We can't do this for regular pixel tests because antialiasing difference make for many platform specific expected results files.

Created attachment 148610 [details] Patch for landing

Comment on attachment 148610 [details] Patch for landing esprehn, feel free to either remove the cq flag or do a follow up patch to fix Ojan's nits.

Comment on attachment 148610 [details] Patch for landing Clearing flags on attachment: 148610 Committed r120849: <http://trac.webkit.org/changeset/120849>

All reviewed patches have been landed. Closing bug.