Bug 103335 – [CSS Grid Layout] Support <percentage> and viewport-relative breadth sizes

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Julien Chaffraix]

Our breadth resolution is limited for the moment to only fixed lengths:

void RenderGrid::computedUsedBreadthOfGridTracks(TrackSizingDirection direction, Vector<GridTrack>& tracks)
{
   ....
   GridTrack track;
   if (trackStyles[i].isFixed())
       track.m_usedBreadth = trackStyles[i].getFloatValue();
   else
       notImplemented();
   ....
}

We should be able to support <percentage>, calc and viewport percentage (basically anything falling under Length::isSpecified()) very easily.

Note that we wrongly don't allow calc in createGridTrackBreadth (StyleResolver.cpp) which should be fixed and also we should ensure with some test that we don't accept any of the other <length> values (like fill-available, fit-content, ...)

[Xan Lopez]

Created attachment 176269 [details]
all-breadth-size.diff

OK, patch up for RFC. Seems something as simple as this does the trick?

Do we want to thoroughly test all the valid and invalid length types in this same test or should I create another one? Also some calc expressions seem to not work (like calc(100% - 15px) or whatever), but I assume this is because the grid layout implementation is still in the very early stages.

[Julien Chaffraix]

Comment on attachment 176269 [details]
all-breadth-size.diff

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

You need to have some testing for the layout side of your change. This will require a check-layout.js test.

You have several choices to do this:
* indirect method by getting the offset from the grid, see LayoutTests/fast/css-grid-layout/place-cell-by-index.html
* direct method using 100% percent width / height on the grid items so that they match the grid tracks' breadth (which requires bug 102968).

> Do we want to thoroughly test all the valid and invalid length types in this same test or should I create another one?

I would add them to the single value test (LayoutTests/fast/css-grid-layout/grid-columns-rows-get-set.html).

> Also some calc expressions seem to not work (like calc(100% - 15px) or whatever), but I assume this is because the grid layout implementation is still in the very early stages.

That's because the simple calc() you test resolves to a fixed length which we know how to dump. I would check if we don't need to handle calc inside valueForGridTrackBreadth (CSSComputedStyleDeclaration.cpp)

> LayoutTests/fast/css-grid-layout/resources/grid-columns-rows-get-set-multiple.js:26
> +var gridWithCalcElement = document.getElementById("gridWithCalcElement");
> +shouldBe("getComputedStyle(gridWithCalcElement, '').getPropertyValue('-webkit-grid-columns')", "'100px 100px'");
> +shouldBe("getComputedStyle(gridWithCalcElement, '').getPropertyValue('-webkit-grid-rows')", "'100px 100px'");

This testing only validates that we properly apply the calc values properly, which is only half of your change (StyleResolver.cpp).

You are lacking a couple of cases:
* percentage
* viewport percentage
* -webkit-calc(100%)
* -webkit-calc(25% + 25px)
* setting from JavaScript.

Also, I advise people not to use the same value several times or you could miss some variable inversion in the code.

Btw, it's good to have a test for the multiple value case but we should try to also just test parsing the values one by one to have good isolation in case of regression.

> Source/WebCore/rendering/RenderGrid.cpp:147
> +        if (trackStyles[i].isSpecified())
>              track.m_usedBreadth = trackStyles[i].getFloatValue();
>          else
>              notImplemented();

That's one way, the other one is to switch to using trackStyles[i].type() to explicitly say what we support, which are not implemented and also catch what we should never see here.

I prefer the explicit way because of the extra documentation it gives but this works too.

[Julien Chaffraix]

Comment on attachment 176269 [details]
all-breadth-size.diff

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

> Source/WebCore/rendering/RenderGrid.cpp:145
> +        if (trackStyles[i].isSpecified())
>              track.m_usedBreadth = trackStyles[i].getFloatValue();

Actually this is completely wrong sorry for missing that.

You need to use valueForLength (see css/LengthFunctions.h) to do the conversion: this patch would convert 15% into 15px :(.

What's tricky is what to pass in as the |maximumValue| for the conversion. You should have computed your logicalWidth already so it's easy for logical width or column direction. For logical height or row direction, you will need to infer it from the style using computeContentLogicalHeight or some similar function.

[Xan Lopez]

(In reply to comment #2)
> > Also some calc expressions seem to not work (like calc(100% - 15px) or whatever), but I assume this is because the grid layout implementation is still in the very early stages.
> 
> That's because the simple calc() you test resolves to a fixed length which we know how to dump. I would check if we don't need to handle calc inside valueForGridTrackBreadth (CSSComputedStyleDeclaration.cpp)

calc() values were being handled inside the if(length.isPercent()) call, since isPercent() will return true for isCalculated(). So adding an explicit call for isCalculated() before that seems to fix things (at least now I get actual values instead of the empty string for complex calc() expressions).

Question though: it seems the only sane thing to do for isCalculated() is to make the same call than the fallback case, ie, "zoomAdjustedPixelValue(trackLength.value(), style);". So I guess I can change isPercent() to a type() = Percent and let everything else fall through. Or I could also just remove the Percent case and let the fallback handle it, since it will end up doing basically the same minus the adjustment for zoom (so basically this only seems to make sense if we want Percent to not be influenced by zoom if I got it right).

[Xan Lopez]

Created attachment 176754 [details]
breadth-types.diff

OK, another RFC patch.

Updates:

- I came to the conclusion that in CSSComputedStyleDeclaration we *really* had to use valueForLength to get the proper dump results, so I'm doing that (had to pass RenderView through a couple of methods).
- Not using .isPercent() since it also includes the 'Calc' type, which must be handled separately. Added a comment for this.
- Also using valueForLength() in RenderGrid, same reasons. Not sure of whether the max width or height values I'm using make sense, to be honest (I think the width one at least does).
- Tried to add tests for all this as suggested, including a new layout test.

[Julien Chaffraix]

Comment on attachment 176754 [details]
breadth-types.diff

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

Btw, if you sync past r135965, you can use fast/css-grid-layout/percent-grid-item-in-percent-grid-track.html and fast/css-grid-layout/percent-grid-item-in-percent-grid-track-in-percent-grid.html to assess if you are on the right track. They should be passing after this change and only require RenderGrid::computedUsedBreadthOfGridTracks to be properly implemented!

> LayoutTests/fast/css-grid-layout/grid-columns-rows-get-set.html:11
> +    font: 10px Ahem;

I don't see why you need to set the font size here, could you clarify why it's needed outside the 'em' resolution?

> LayoutTests/fast/css-grid-layout/resources/grid-columns-rows-get-set.js:30
> +var gridWithCalcComplexElement = document.getElementById("gridWithCalcComplexElement");
> +shouldBe("getComputedStyle(gridWithCalcComplexElement, '').getPropertyValue('-webkit-grid-columns')", "'150px'");
> +shouldBe("getComputedStyle(gridWithCalcComplexElement, '').getPropertyValue('-webkit-grid-rows')", "'75px'");

These results are wrong AFAICT, you only get the fixed part of your calc expression.

The easiest way to properly check the percent resolution is to have a container with a fixed height and width as that avoids any circular dependency when resolving percent (for 'height', it can mean that we just interpret it as being 'auto', see http://www.w3.org/TR/CSS21/visudet.html#the-height-property).

If you put a containing block of 800 * 600px (which matches the size of DRT's viewport) and if you force all the values to be absolute, you should expect:
* column = 450px (50% of 800 + 150)
* row = 465px (65% * 600 + 75)

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:993
> +    return zoomAdjustedPixelValue(valueForLength(trackLength, 0, renderView), style);

The reason you get the value without resolving the percentage properly above is that you pass '0' as maxValue, which is used to resolve percentage.

> Source/WebCore/rendering/RenderGrid.cpp:145
> +        if (trackStyles[i].isSpecified())
> +            track.m_usedBreadth = valueForLength(trackStyles[i], direction == ForColumns ? m_maxPreferredLogicalWidth : logicalHeight(), view());

This is wrong:
* percentages should be resolved against the logical width (logicalWidth()), not the maximum preferred logical widths. The former is your size as computed during this layout, the latter represents the maximum amount of space a renderer can take (e.g. for text, it would represent the layout with the longest possible lines (ie no extra breaks)).
* using logicalHeight() for the row resolution doesn't work for a good reason: your logical height is usually determined by your children. This means that you are probably using the previous layout's value (or just 0 if we reset it). That's why you have to infer it from your style using computeContentLogicalHeight().

[Xan Lopez]

Created attachment 176987 [details]
breadth-types.diff

OK, another patch. I think I'm making progress but not quite there yet.

- Using zoomAdjustedPixelValueForLength (note the ForLength) now in valueForGridTrackBreadth. This seems to give proper results almost everywhere, and allows us to drop the special case for Percent (so only special case for Auto remains).
- Made the changes requested to RenderGrid. The two testcases you mention (fast/css-grid-layout/percent-grid-item-in-percent-grid-track.html and fast/css-grid-layout/percent-grid-item-in-percent-grid-track-in-percent-grid.html) indeed improved, but still won't work 100%. I'll attach the diffs after.
- The complex calc expressions are still not working, we went back again to no result in the dump. Not sure if I'm not getting what the text should look like here, though.
- Moved the font setting just to the EM class.

[Xan Lopez]

The diff for fast/css-grid-layout/percent-grid-item-in-percent-grid-track-in-percent-grid.html

--- /home/xan/git/webkit/build/normal/layout-test-results/fast/css-grid-layout/percent-grid-item-in-percent-grid-track-in-percent-grid-expected.txt
+++ /home/xan/git/webkit/build/normal/layout-test-results/fast/css-grid-layout/percent-grid-item-in-percent-grid-track-in-percent-grid-actual.txt
@@ -1,12 +1,9 @@
 Test that percentage sized grid items inside percentage sized grid tracks inside a percentage size grid get properly sized.
 
 FAIL:
-Expected 160 for width, but got 0. 
-Expected 90 for height, but got 0. 
-Expected 80 for width, but got 0. 
-Expected 105 for height, but got 0. 
-Expected 240 for width, but got 0. 
-Expected 210 for height, but got 0. 
+Expected 90 for height, but got 54. 
+Expected 105 for height, but got 147. 
+Expected 210 for height, but got 294. 
 
 <div class="container">
 <div class="grid" data-expected-width="400" data-expected-height="300">
@@ -17,12 +14,9 @@
 </div>
 </div>
 FAIL:
-Expected 160 for width, but got 0. 
-Expected 90 for height, but got 0. 
-Expected 80 for width, but got 0. 
-Expected 105 for height, but got 0. 
-Expected 240 for width, but got 0. 
-Expected 210 for height, but got 0. 
+Expected 90 for height, but got 54. 
+Expected 105 for height, but got 147. 
+Expected 210 for height, but got 294. 
 
 <div class="container">
 <div class="grid" style="-webkit-writing-mode: horizontal-bt" data-expected-width="400" data-expected-height="300">
@@ -33,12 +27,9 @@
 </div>
 </div>
 FAIL:
-Expected 120 for width, but got 0. 
-Expected 180 for height, but got 0. 
-Expected 140 for width, but got 0. 
-Expected 60 for height, but got 0. 
-Expected 280 for width, but got 0. 
-Expected 180 for height, but got 0. 
+Expected 120 for width, but got 45. 
+Expected 140 for width, but got 122. 
+Expected 280 for width, but got 245. 
 
 <div class="container">
 <div class="grid" style="-webkit-writing-mode: vertical-rl;" data-expected-width="400" data-expected-height="300">
@@ -49,12 +40,9 @@
 </div>
 </div>
 FAIL:
-Expected 120 for width, but got 0. 
-Expected 180 for height, but got 0. 
-Expected 140 for width, but got 0. 
-Expected 60 for height, but got 0. 
-Expected 280 for width, but got 0. 
-Expected 180 for height, but got 0. 
+Expected 120 for width, but got 45. 
+Expected 140 for width, but got 122. 
+Expected 280 for width, but got 245. 
 
 <div class="container">
 <div class="grid" style="-webkit-writing-mode: vertical-lr;" data-expected-width="400" data-expected-height="300">

--

The diff for fast/css-grid-layout/percent-grid-item-in-percent-grid-track.html:

--- /home/xan/git/webkit/build/normal/layout-test-results/fast/css-grid-layout/percent-grid-item-in-percent-grid-track-expected.txt
+++ /home/xan/git/webkit/build/normal/layout-test-results/fast/css-grid-layout/percent-grid-item-in-percent-grid-track-actual.txt
@@ -1,11 +1,8 @@
 Test that percentage sized grid items inside percentage sized grid tracks get properly sized.
 
 FAIL:
-Expected 160 for width, but got 0. 
 Expected 90 for height, but got 0. 
-Expected 80 for width, but got 0. 
 Expected 105 for height, but got 0. 
-Expected 240 for width, but got 0. 
 Expected 210 for height, but got 0. 
 
 <div class="grid" data-expected-width="400" data-expected-height="300">
@@ -15,11 +12,8 @@
     <div id="d" data-expected-width="240" data-expected-height="210"></div>
 </div>
 FAIL:
-Expected 160 for width, but got 0. 
 Expected 90 for height, but got 0. 
-Expected 80 for width, but got 0. 
 Expected 105 for height, but got 0. 
-Expected 240 for width, but got 0. 
 Expected 210 for height, but got 0. 
 
 <div class="grid" style="-webkit-writing-mode: horizontal-bt" data-expected-width="400" data-expected-height="300">
@@ -29,12 +23,9 @@
     <div id="d" data-expected-width="240" data-expected-height="210"></div>
 </div>
 FAIL:
-Expected 120 for width, but got 0. 
-Expected 180 for height, but got 0. 
-Expected 140 for width, but got 0. 
-Expected 60 for height, but got 0. 
-Expected 280 for width, but got 0. 
-Expected 180 for height, but got 0. 
+Expected 120 for width, but got 69. 
+Expected 140 for width, but got 188. 
+Expected 280 for width, but got 376. 
 
 <div class="grid" style="-webkit-writing-mode: vertical-rl;" data-expected-width="400" data-expected-height="300">
     <div id="a" data-expected-width="120" data-expected-height="15"></div>
@@ -43,12 +34,9 @@
     <div id="d" data-expected-width="280" data-expected-height="180"></div>
 </div>
 FAIL:
-Expected 120 for width, but got 0. 
-Expected 180 for height, but got 0. 
-Expected 140 for width, but got 0. 
-Expected 60 for height, but got 0. 
-Expected 280 for width, but got 0. 
-Expected 180 for height, but got 0. 
+Expected 120 for width, but got 69. 
+Expected 140 for width, but got 188. 
+Expected 280 for width, but got 376. 
 
 <div class="grid" style="-webkit-writing-mode: vertical-lr;" data-expected-width="400" data-expected-height="300">
     <div id="a" data-expected-width="120" data-expected-height="15"></div>

[Julien Chaffraix]

Comment on attachment 176987 [details]
breadth-types.diff

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

We talked about the patch on IRC with Xan and decided to push the calc case to a follow-up bug as it seems to be more involved to get right (especially on the getComputedStyle side).

Btw, don't forget that you need to test the layout side of your change as your grid areas are now properly sized. You can follow the other examples (e.g. fast/css-grid-layout/percent-grid-item-in-percent-grid-track.html) on how to get the grid areas size.

> LayoutTests/fast/css-grid-layout/grid-columns-rows-get-set.html:50
> +.gridWithFitContent {
> +    -webkit-grid-columns: -webkit-fit-content;
> +    -webkit-grid-rows: -webkit-fit-available;
> +}

Probably good to test both values (fit-content, fit-available) for both properties.

> Source/WebCore/rendering/RenderGrid.cpp:145
> +            track.m_usedBreadth = valueForLength(trackStyles[i].length(), direction == ForColumns ? logicalWidth() : computeContentLogicalHeight(MainOrPreferredSize, trackStyles[i].length()), view());

Better! However you are doing the wrong resolution for the rows: you are resolving percentage trackStyle against itself which is not what you want. You want to resolve it against the grid element's style's logical height. I am sure this is why the tests are failing.

[Xan Lopez]

(In reply to comment #9)
> > Source/WebCore/rendering/RenderGrid.cpp:145
> > +            track.m_usedBreadth = valueForLength(trackStyles[i].length(), direction == ForColumns ? logicalWidth() : computeContentLogicalHeight(MainOrPreferredSize, trackStyles[i].length()), view());
> 
> Better! However you are doing the wrong resolution for the rows: you are resolving percentage trackStyle against itself which is not what you want. You want to resolve it against the grid element's style's logical height. I am sure this is why the tests are failing.

Yeah, that nailed it!

I'll upload a new patch without the calc() support, and open a new bug to continue working on calc() after this one.

[Xan Lopez]

Created attachment 177006 [details]
breadth-size.diff

[Xan Lopez]

Comment on attachment 177006 [details]
breadth-size.diff

OK, first version that should be reasonably ready for review.

Dropped calc() support as agreed, added a new layout test, modified all the expected results for passing tests.

[Xan Lopez]

Created attachment 177286 [details]
breadth-sizes.diff

Updated after changes in SVN trunk.

[Julien Chaffraix]

Comment on attachment 177286 [details]
breadth-sizes.diff

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

> LayoutTests/ChangeLog:3
> +        [CSS Grid Layout] Support all specified breadth size

We should probably rename the title to cover only <percentage> and viewport relative sizes.

> LayoutTests/ChangeLog:14
> +        * fast/css-grid-layout/resources/grid-columns-rows-get-set.js: test the new breatdth length types.

Let's add a test for the multiple entry cases to make sure we support it properly too!

> LayoutTests/fast/css-grid-layout/breadth-size-resolution-grid.html:69
> +

You should probably keep some orthogonal writing mode case. Here the grid element and the grid items always have the same writing mode.

Here is an example of orthogonal writing mode:
<div class="grid">
     <div id="a" class="verticalRL" data-expected-width="100" data-expected-height="60"></div>
     <div id="b" data-expected-width="100" data-expected-height="60"></div>
     <div id="c" class="verticalRL" data-expected-width="100" data-expected-height="100"></div>
     <div id="d" data-expected-width="80" data-expected-height="100"></div>
 </div>

(#a and #c have an orthogonal writing mode with respect to their parent)

> LayoutTests/fast/css-grid-layout/resources/grid-columns-rows-get-set.js:3
>  debug("Test getting |display| set through CSS");

This is wrong btw, we should change it while we are touching this code:

debug("Test getting -webkit-grid-columns and -webkit-grid-rows through CSS.");

> LayoutTests/fast/css-grid-layout/resources/grid-columns-rows-get-set.js:28
> +var gridWithFitContentElement = document.getElementById("gridWithFitContentElement");

Adding a debug(...) comment about why those are not valid would be nice as it would make the output more readable. For example:

debug("Test getting wrong values for -webkit-grid-columns and -webkit-grid-rows through CSS (they should resolve to the default: 'none'.");

> Source/WebCore/ChangeLog:10
> +        types. calc() support is still missing, we'll do it in a follow-up
> +        patch.

Would be nice to give the *why* here: calc() support is postponed to a follow-up bug as it seems to require more work to be properly serialized in CSSComputedStyleDeclaration.

> Source/WebCore/ChangeLog:13
> +        (WebCore::valueForGridTrackBreadth): support the new types.

Sentences starts with a capital letter (this applies to all entries).

> Source/WebCore/ChangeLog:14
> +        (WebCore::valueForGridTrackList): modify the call to the upper

s/upper/above/ (upper doesn't sound right here but I am not a native speaker so I could be wrong).

> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:1002
> +static PassRefPtr<CSSValue> valueForGridTrackBreadth(const GridTrackSize& trackSize, const RenderStyle* style, RenderView *renderView)

This could be a const RenderView* if valueForLength wasn't dumb (bug worthy though)

> Source/WebCore/rendering/RenderGrid.cpp:145
> +        if (trackLength.isFixed() || trackLength.isPercent() || trackLength.isViewportPercentage())

Let's add a FIXME about supporting calc.

[Xan Lopez]

Comment on attachment 177286 [details]
breadth-sizes.diff

Just pushed as https://trac.webkit.org/changeset/136432, hopefully with all the changes requested.

[Xan Lopez]

Closing.