Bug 103313 – [CSS Grid Layout] Implement support for grid-template

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Julien Chaffraix]

grid-template allows the grid to be defined graphically using identifiers to name grid areas:

grid-template = [ " [ <identifier> | . ]+ " ]+ | none

We also need to be careful with the interaction with grid-definition-{rows|columns}.

[Sergio Villar Senin]

Fixing this involves merging 3 different CLs from Blink:

*   Implement 'grid-template' parsing

    This change makes us recognize grid-template, parse it, store it and
    return properly to JavaScript.

    The specification mandates to validate to check that the areas defined
    by 'grid-template' are rectangular which involves a lot of the
    CSSParser code.

    Review URL: https://chromiumcodereview.appspot.com/18532004

*  [CSS Grid Layout] Fix grid-template serialization

    This is a follow-up of https://codereview.chromium.org/18532004/#msg6

    We didn't know if serializing strings was specified and it turns out
    that it is: http://dev.w3.org/csswg/cssom/#serialize-a-string


*  Move GridCoordinate and GridSpan to a separate file

    In order to implement 'grid-template', we need a way to describe a
    grid area's coordinates. That's what GridCoordinate is about and
    instead of duplicating code, splitting the classes will enable code
    sharing.

    While moving the code, added some comments about the 2 classes.

    Review URL: https://chromiumcodereview.appspot.com/18345011

[Sergio Villar Senin]

Created attachment 213696 [details]
Patch

[Sergio Villar Senin]

Created attachment 213699 [details]
Patch

[Build Bot]

Comment on attachment 213699 [details]
Patch

Attachment 213699 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/3758003

[EFL EWS Bot]

Comment on attachment 213699 [details]
Patch

Attachment 213699 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/3758006

[Build Bot]

Comment on attachment 213699 [details]
Patch

Attachment 213699 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/3714254

[EFL EWS Bot]

Comment on attachment 213699 [details]
Patch

Attachment 213699 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/3716246

[Dean Jackson]

Comment on attachment 213699 [details]
Patch

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

Nearly ready once build errors are fixed.

> Source/WebCore/css/CSSGridTemplateValue.cpp:54
> +    NamedGridAreaMap::const_iterator end = gridAreaMap.end();
> +    for (NamedGridAreaMap::const_iterator it = gridAreaMap.begin(); it != end; ++it) {

You can use "auto" here now. for (auto it = gridAreaMap.begin(), end = gridAreaMap.end(); it != end; ++it)

> Source/WebCore/css/CSSGridTemplateValue.cpp:61
> +    end = gridAreaMap.end();
> +    for (NamedGridAreaMap::const_iterator it = gridAreaMap.begin(); it != end; ++it) {

Ditto.

> Source/WebCore/css/CSSParser.cpp:5173
> +        if (!columnCount) {
> +            columnCount = columnNames.size();
> +            ASSERT(columnCount);
> +        } else if (columnCount != columnNames.size()) {
> +            // The declaration is invalid is all the rows don't have the number of columns.
> +            return 0;
> +        }

Please write this with the return first.

if (columnCount != columnNames.size()) {
  // Comment, note typo is -> if
  return 0;
}

Although I guess that assumes that columnCount is non-zero too.

> Source/WebCore/css/CSSParser.cpp:5189
> +            NamedGridAreaMap::iterator gridAreaIt = gridAreaMap.find(gridAreaName);

Maybe auto gridAreaIterator?

[Sergio Villar Senin]

Comment on attachment 213699 [details]
Patch

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

Thx for the review dino, new patch upcoming

> Source/WebCore/css/CSSGridTemplateValue.h:72
> +

Actually I'm also removing these three and using CSS_VALUE_TYPE_CASTS that does the same (and more) and it's more WebKit-style

[Sergio Villar Senin]

Created attachment 213758 [details]
Patch

[Dean Jackson]

Comment on attachment 213758 [details]
Patch

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

> Source/WebCore/css/CSSGridTemplateValue.cpp:2
> + * Copyright (C) 2013 Google Inc. All rights reserved.

Do you really mean to assign Google the copyright? Why not your own name? (do you work for them?)

> Source/WebCore/css/CSSGridTemplateValue.cpp:14
> + *     * Neither the name of Google Inc. nor the names of its

DItto.

> Source/WebCore/css/CSSGridTemplateValue.h:2
> + * Copyright (C) 2013 Google Inc. All rights reserved.

Ditto. And other places too.

> Source/WebCore/css/CSSParser.cpp:5177
> +        for (size_t currentCol = 0; currentCol < columnCount; ++currentCol) {

Could you expand Col to Column? I don't see the need to shorten (there are a few instances of this)

> LayoutTests/fast/css-grid-layout/grid-template-get-set.html:67
> +    shouldBeEqualToString("window.getComputedStyle(gridWithSpanningRowsDotTemplate).getPropertyValue('-webkit-grid-template')", '"span" "."')

Please extract window.getComputedStyle(gridWithSpanningRowsDotTemplate).getPropertyValue('-webkit-grid-template') into a helper function.

In fact, the function could wrap the getElementById + shouldBeEqualToString part as well, so it is just something like testGridTemplateValue("gridWithDefaultTemplate", "none");

> LayoutTests/fast/css-grid-layout/grid-template-get-set.html:76
> +    element.style.webkitGridTemplate = "'foobar'";
> +    shouldBeEqualToString("window.getComputedStyle(element).getPropertyValue('-webkit-grid-template')", '"foobar"')

Hmm... maybe not the getElementById part then, but at least it could take an element as a parameter.

[Ryosuke Niwa]

We need to keep the Google copyright notice if the code was copied from Blink or if the author looked at Blink code to write this.

[Sergio Villar Senin]

(In reply to comment #11)
> (From update of attachment 213758 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=213758&action=review
> 
> > Source/WebCore/css/CSSGridTemplateValue.cpp:2
> > + * Copyright (C) 2013 Google Inc. All rights reserved.
> 
> Do you really mean to assign Google the copyright? Why not your own name? (do you work for them?)

As Ryosuke mentions this work is based on some patches from Blink so it's fair/correct/legal to keep the copyright notice as is. I might add the Igalia copyright too.

> > Source/WebCore/css/CSSParser.cpp:5177
> > +        for (size_t currentCol = 0; currentCol < columnCount; ++currentCol) {
> 
> Could you expand Col to Column? I don't see the need to shorten (there are a few instances of this)

Also expanded lookAheadCol -> lookAheadColumn
 
> > LayoutTests/fast/css-grid-layout/grid-template-get-set.html:76
> > +    element.style.webkitGridTemplate = "'foobar'";
> > +    shouldBeEqualToString("window.getComputedStyle(element).getPropertyValue('-webkit-grid-template')", '"foobar"')
> 
> Hmm... maybe not the getElementById part then, but at least it could take an element as a parameter.

Done

[Sergio Villar Senin]

Committed r157211: <http://trac.webkit.org/changeset/157211>

[Sergio Villar Senin]

*** Bug 119922 has been marked as a duplicate of this bug. ***