Bug 38919 - Add support for fit-content etc
: Add support for fit-content etc
Status: RESOLVED FIXED
: WebKit
CSS
: 528+ (Nightly build)
: All All
: P2 Enhancement
Assigned To:
: https://developer.mozilla.org/en/CSS/...
:
: 130910
: 86154
  Show dependency treegraph
 
Reported: 2010-05-11 11:17 PST by
Modified: 2014-03-28 12:34 PST (History)


Attachments
Patch (26.35 KB, patch)
2012-06-14 13:54 PST, Elliott Sprehn
no flags Review Patch | Details | Formatted Diff | Diff
Patch (26.38 KB, patch)
2012-06-14 13:57 PST, Elliott Sprehn
no flags Review Patch | Details | Formatted Diff | Diff
Patch (53.90 KB, patch)
2012-06-18 17:32 PST, Elliott Sprehn
no flags Review Patch | Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-04 (979.52 KB, application/zip)
2012-06-18 20:36 PST, WebKit Review Bot
no flags Details
Patch (53.67 KB, patch)
2012-06-19 11:45 PST, Elliott Sprehn
no flags Review Patch | Details | Formatted Diff | Diff
Patch for landing (51.87 KB, patch)
2012-06-19 21:24 PST, Elliott Sprehn
no flags Review Patch | Details | Formatted Diff | Diff
Patch for landing (51.86 KB, patch)
2012-06-20 11:24 PST, Elliott Sprehn
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2010-05-11 11:17:57 PST
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.
------- Comment #1 From 2012-05-11 10:39:33 PST -------
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.
------- Comment #2 From 2012-05-11 10:49:01 PST -------
Looking more closely, it looks like min-intrinsic only works with old flexbox, so we would have to implement it.
------- Comment #3 From 2012-06-13 12:26:59 PST -------
Looking at the code it appears that intrinsic maps to fit-content (shrink wrapping).
------- Comment #4 From 2012-06-13 13:30:54 PST -------
(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.
------- Comment #5 From 2012-06-13 15:17:24 PST -------
(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?
------- Comment #6 From 2012-06-14 13:54:12 PST -------
Created an attachment (id=147645) [details]
Patch

Implementation of the writing mode width keywords
------- Comment #7 From 2012-06-14 13:57:52 PST -------
Created an attachment (id=147648) [details]
Patch

Remove the tabs from the changelog. Seems my vim is misconfigured for that kind of file.
------- Comment #8 From 2012-06-15 11:59:28 PST -------
(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 #9 From 2012-06-15 13:27:40 PST -------
(From update of attachment 147648 [details])
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.
------- Comment #10 From 2012-06-15 13:39:43 PST -------
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 #11 From 2012-06-18 17:17:55 PST -------
(From update of attachment 147648 [details])
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.
------- Comment #12 From 2012-06-18 17:32:42 PST -------
Created an attachment (id=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.
------- Comment #13 From 2012-06-18 17:39:10 PST -------
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.
------- Comment #14 From 2012-06-18 17:46:02 PST -------
(In reply to comment #13)
> Attachment 148209 [details] [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 #15 From 2012-06-18 20:36:25 PST -------
(From update of attachment 148209 [details])
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
------- Comment #16 From 2012-06-18 20:36:31 PST -------
Created an attachment (id=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
------- Comment #17 From 2012-06-19 09:52:58 PST -------
(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.
------- Comment #18 From 2012-06-19 11:45:11 PST -------
Created an attachment (id=148379) [details]
Patch

Fix broken layout tests where I had disabled the auto keyword for height properties and check style warnings.
------- Comment #19 From 2012-06-19 13:07:13 PST -------
(From update of attachment 148379 [details])
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.
------- Comment #20 From 2012-06-19 21:24:18 PST -------
Created an attachment (id=148499) [details]
Patch for landing
------- Comment #21 From 2012-06-20 10:05:52 PST -------
(From update of attachment 148499 [details])
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 #22 From 2012-06-20 10:18:50 PST -------
(From update of attachment 148499 [details])
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 #23 From 2012-06-20 10:59:38 PST -------
(From update of attachment 148499 [details])
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.
------- Comment #24 From 2012-06-20 11:24:16 PST -------
Created an attachment (id=148610) [details]
Patch for landing
------- Comment #25 From 2012-06-20 11:43:53 PST -------
(From update of attachment 148610 [details])
esprehn, feel free to either remove the cq flag or do a follow up patch to fix Ojan's nits.
------- Comment #26 From 2012-06-20 12:29:05 PST -------
(From update of attachment 148610 [details])
Clearing flags on attachment: 148610

Committed r120849: <http://trac.webkit.org/changeset/120849>
------- Comment #27 From 2012-06-20 12:29:17 PST -------
All reviewed patches have been landed.  Closing bug.