[Webkit-unassigned] [Bug 103335] [CSS Grid Layout] Support all specified breadth size

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=103335





--- Comment #2 from Julien Chaffraix <jchaffraix at webkit.org>  2012-11-27 10:23:05 PST ---
(From update of attachment 176269)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.