Bug 38919 – Add support for fit-content etc

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Erik Arvidsson]

Mozilla supports the following CSS length values for width and height (as well as for min and max versions of those)

fit-content
max-content
min-content
available

WebKit supports instrinsic, which I believe maps to fit-content. It would be nice if we allowed the name fit-contents as well as added support for the others.

[Tony Chang]

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.

[Tony Chang]

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

[Elliott Sprehn]

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

[Tony Chang]

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

[Elliott Sprehn]

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

[Elliott Sprehn]

Created attachment 147645 [details]
Patch

Implementation of the writing mode width keywords

[Elliott Sprehn]

Created attachment 147648 [details]
Patch

Remove the tabs from the changelog. Seems my vim is misconfigured for that kind of file.

[Tony Chang]

(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

[Tony Chang]

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.

[Tony Chang]

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.

[Elliott Sprehn]

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.

[Elliott Sprehn]

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.

[WebKit Review Bot]

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.

[Elliott Sprehn]

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

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Tony Chang]

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

[Elliott Sprehn]

Created attachment 148379 [details]
Patch

Fix broken layout tests where I had disabled the auto keyword for height properties and check style warnings.

[Tony Chang]

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.

[Elliott Sprehn]

Created attachment 148499 [details]
Patch for landing

[Tony Chang]

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 [e-mail].

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

[WebKit Review Bot]

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

[e-mail] 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

[Ojan Vafai]

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.

[Elliott Sprehn]

Created attachment 148610 [details]
Patch for landing

[Tony Chang]

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.

[WebKit Review Bot]

Comment on attachment 148610 [details]
Patch for landing

Clearing flags on attachment: 148610

Committed r120849: <http://trac.webkit.org/changeset/120849>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.