[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


Julien Chaffraix <jchaffraix at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #177286|review?                     |review+, commit-queue-
               Flag|                            |




--- Comment #14 from Julien Chaffraix <jchaffraix at webkit.org>  2012-12-03 11:31:08 PST ---
(From update of attachment 177286)
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.

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