Bug 145860 – [CSS Grid Layout] Fix grid-template-areas parsing to avoid spaces

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Manuel Rego Casasnovas]

We should allow to remove the spaces between the dots and the idents (for reference see discussion at CSS WG mailing list: https://lists.w3.org/Archives/Public/www-style/2015May/0239.html).

So, the following example:
  grid-template-areas: ". title ."
                       "nav main aside";
should be equivalent to:
  grid-template-areas: "....title....."
                       "nav main aside";
and:
  grid-template-areas: ".title."
                       "nav main aside";

[Manuel Rego Casasnovas]

Created attachment 254686 [details]
Patch

[Sergio Villar Senin]

Comment on attachment 254686 [details]
Patch

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

Looking good but I think we can improve the tokenization...

> Source/WebCore/css/CSSParser.cpp:5994
> +        areaName.append(text[i]);

Instead of appending one by one, I think it would be better to keep track of the index of the start of the area name, and then just simply do a substring() call (perhaps even substringSharingImpl()) as gridRowNames will be valid throughout the whole life of columnNames I think)

> LayoutTests/fast/css-grid-layout/grid-template-areas-get-set.html:80
> +}

Although you already consider some of them I think we should have separate test cases for the following situations:
a) ....A
b) A..
c) ...A..
d) A.B
e) ..A.B
f) A.B..
g) ..A..B...

[Manuel Rego Casasnovas]

Created attachment 254720 [details]
Patch

[Manuel Rego Casasnovas]

Thanks for the review!

(In reply to comment #2)
> > Source/WebCore/css/CSSParser.cpp:5994
> > +        areaName.append(text[i]);
> 
> Instead of appending one by one, I think it would be better to keep track of
> the index of the start of the area name, and then just simply do a
> substring() call (perhaps even substringSharingImpl()) as gridRowNames will
> be valid throughout the whole life of columnNames I think)

Yeah I think this approach makes the code a bit simpler.

> > LayoutTests/fast/css-grid-layout/grid-template-areas-get-set.html:80
> > +}
> 
> Although you already consider some of them I think we should have separate
> test cases for the following situations:
> a) ....A
> b) A..
> c) ...A..
> d) A.B
> e) ..A.B
> f) A.B..
> g) ..A..B...

I've added more cases like these ones and other too.

[Sergio Villar Senin]

Comment on attachment 254720 [details]
Patch

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

> Source/WebCore/css/CSSParser.cpp:5971
> +    while (index < text.length()) {

let's cache text.length() in a local variable in order not to repeat the same call over and over again.

> Source/WebCore/css/CSSParser.cpp:5975
> +                index++;

I think it's ok to use a single line for this.

> Source/WebCore/css/CSSParser.cpp:5982
> +                index++;

Ditto.

> LayoutTests/fast/css-grid-layout/grid-template-areas-get-set-expected.txt:21
> +PASS getComputedStyle(gridWithDotsNoSpace).getPropertyValue('-webkit-grid-template-areas') is "\". title . nav . . main test\""

Seems like you forgot to upload all the new expectations?

[Build Bot]

Comment on attachment 254720 [details]
Patch

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

New failing tests:
fast/css-grid-layout/grid-template-areas-get-set.html

[Build Bot]

Created attachment 254722 [details]
Archive of layout-test-results from ews107 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Build Bot]

Comment on attachment 254720 [details]
Patch

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

New failing tests:
fast/css-grid-layout/grid-template-areas-get-set.html

[Build Bot]

Created attachment 254723 [details]
Archive of layout-test-results from ews101 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Manuel Rego Casasnovas]

Created attachment 254735 [details]
Patch

[WebKit Commit Bot]

Attachment 254735 [details] did not pass style-queue:


ERROR: Source/WebCore/css/CSSParser.cpp:5975:  More than one command on the same line in while  [whitespace/parens] [4]
ERROR: Source/WebCore/css/CSSParser.cpp:5981:  More than one command on the same line in while  [whitespace/parens] [4]
Total errors found: 2 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Manuel Rego Casasnovas]

Created attachment 254754 [details]
Patch

[Manuel Rego Casasnovas]

Thanks for the review.

(In reply to comment #5)
> Comment on attachment 254720 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=254720&action=review
> 
> > Source/WebCore/css/CSSParser.cpp:5971
> > +    while (index < text.length()) {
> 
> let's cache text.length() in a local variable in order not to repeat the
> same call over and over again.

Done.

> > Source/WebCore/css/CSSParser.cpp:5975
> > +                index++;
> 
> I think it's ok to use a single line for this.

It seems the style checker didn't like it.

> > LayoutTests/fast/css-grid-layout/grid-template-areas-get-set-expected.txt:21
> > +PASS getComputedStyle(gridWithDotsNoSpace).getPropertyValue('-webkit-grid-template-areas') is "\". title . nav . . main test\""
> 
> Seems like you forgot to upload all the new expectations?

True :-)

[WebKit Commit Bot]

Comment on attachment 254754 [details]
Patch

Clearing flags on attachment: 254754

Committed r185492: <http://trac.webkit.org/changeset/185492>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Csaba Osztrogonác]

(In reply to comment #14)
> Comment on attachment 254754 [details]
> Patch
> 
> Clearing flags on attachment: 254754
> 
> Committed r185492: <http://trac.webkit.org/changeset/185492>

It caused 11 assertions on Apple's debug bots:
https://build.webkit.org/builders/Apple%20Mavericks%20Debug%20WK2%20%28Tests%29/builds/12379

[WebKit Commit Bot]

Re-opened since this is blocked by bug 145927

[Manuel Rego Casasnovas]

Created attachment 254817 [details]
Patch

[Manuel Rego Casasnovas]

(In reply to comment #16)
> (In reply to comment #14)
> > Comment on attachment 254754 [details]
> > Patch
> > 
> > Clearing flags on attachment: 254754
> > 
> > Committed r185492: <http://trac.webkit.org/changeset/185492>
> 
> It caused 11 assertions on Apple's debug bots:
> https://build.webkit.org/builders/
> Apple%20Mavericks%20Debug%20WK2%20%28Tests%29/builds/12379

Yes it was a silly mistake "index < length" was the last condition at:
  while (text[index] != ' ' && text[index] != '.' && index < length)

When it should be the first one. :-(

[Manuel Rego Casasnovas]

Comment on attachment 254817 [details]
Patch

Clearing flags on attachment: 254817

Committed r185520: <http://trac.webkit.org/changeset/185520>

[Manuel Rego Casasnovas]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Comment on attachment 254817 [details]
Patch

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

> Source/WebCore/css/CSSParser.cpp:5964
> +static Vector<String> parseGridTemplateAreasColumnNames(const String& gridRowNames)

Could save some memory allocation by making this take a StringView and return a Vector<StringView>. Then can convert to a string when we need to. Probably a bit of a project, though.