Bug 123082 – [CSS Regions] Fix WHITESPACE issues in the CSS grammar.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Mihai Maerean]

Fix WHITESPACE issues in the CSS grammar.

A single WHITESPACE token consumes consecutive spaces, but does not consume
spaces separated by comments. That means S* and S+ in CSS grammars need to
accept multiple WHITESPACE tokens. Additionally, white spaces are not
mandatory to separate an @-symbol and the rest of the prelude.

Use space non-terminal instead of WHITESPACE for S+ in calc expressions.

Use maybe_space non-terminal instead of WHITESPACE for S* after @-webkit-filter
and @-webkit-region.

This is a port from https://codereview.chromium.org/25607005, the patch by Rune Lillesveen.

[Mihai Maerean]

Created attachment 214713 [details]
patch

[Mihnea Ovidenie]

Comment on attachment 214713 [details]
patch

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

> Source/WebCore/css/CSSGrammar.y.in:1589
> +    space '+' space {

Why are '+' and '-' treated differently than '*' and '/' below? Do you need to add space: definition above?

[Mihai Maerean]

Comment on attachment 214713 [details]
patch

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

>> Source/WebCore/css/CSSGrammar.y.in:1589
>> +    space '+' space {
> 
> Why are '+' and '-' treated differently than '*' and '/' below? Do you need to add space: definition above?

The grammar requires spaces around binary ‘+’ and ‘-’ operators. The ‘*’ and ‘/’ operators do not require spaces. http://www.w3.org/TR/css3-values/#calc-syntax

An expression like 70px+40px should return an error.
Using the same form as for * and / leads to the calc expressions without the whitespaces to succeed instead of returning an error, which breaks the css3/calc/calc-errors.html test.

I will upload a new patch that includes these comments to why explain the changes are needed.

[Mihai Maerean]

Created attachment 214733 [details]
patch. incorporates the feedback.

[Mihai Maerean]

Created attachment 214739 [details]
patch. incorporates the feedback: add test for the '-' (minus) operator.

[Mihai Maerean]

Created attachment 214741 [details]
patch

replace ’ (â) character from the html page with the ' character.

[Antti Koivisto]

Comment on attachment 214741 [details]
patch

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

> Source/WebCore/css/CSSGrammar.y.in:373
> +space: WHITESPACE| space WHITESPACE ;

There should be a space between WHITESPACE and |

[Mihai Maerean]

Created attachment 214743 [details]
patch. incorporates the feedback: There should be a space between WHITESPACE and |

[WebKit Commit Bot]

Comment on attachment 214743 [details]
patch. incorporates the feedback: There should be a space between WHITESPACE and |

Rejecting attachment 214743 [details] from commit-queue.

[e-mail] does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/contributors.json.

- If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.

- If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/contributors.json by adding yourself to the file (no review needed).  The commit-queue restarts itself every 2 hours.  After restart the commit-queue will correctly respect your committer rights.

[Darin Adler]

Comment on attachment 214743 [details]
patch. incorporates the feedback: There should be a space between WHITESPACE and |

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

> Source/WebCore/css/CSSGrammar.y.in:373
> +/* for expressions that require at least one whitespace to be present, like the + and - operators in calc expressions */
> +space: WHITESPACE | space WHITESPACE ;

I don’t understand why this is needed. WHITESPACE should match an arbitrarily long run of whitespace, so this “space” should be no different than just WHITESPACE. Can you construct a test that fails if we use WHITESPACE instead of space in calc_func_operator? If not, please don’t add this production.

[Darin Adler]

Comment on attachment 214743 [details]
patch. incorporates the feedback: There should be a space between WHITESPACE and |

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

> Source/WebCore/css/CSSGrammar.y.in:1592
> + * The grammar requires spaces around binary â+â and â-â operators.
> + * The '*' and '/' operators do not require spaces.
> + * http://www.w3.org/TR/css3-values/#calc-syntax

Where’s the test coverage for this? I’d rather omit this comment but include thorough test coverage demonstrating parse failure when the + and - don’t have spaces on either side.

[WebKit Commit Bot]

Comment on attachment 214743 [details]
patch. incorporates the feedback: There should be a space between WHITESPACE and |

Clearing flags on attachment: 214743

Committed r157720: <http://trac.webkit.org/changeset/157720>

[Darin Adler]

(In reply to comment #0)
> A single WHITESPACE token consumes consecutive spaces, but does not consume
> spaces separated by comments.

So this secret is the reason we can never use WHITESPACE. But this is only in the bug report, not in the comments. That’s unnecessarily mysterious.

[Darin Adler]

I think the patch is fine to land as is, but I do think it’s got some unclear parts.

[Mihai Maerean]

Comment on attachment 214743 [details]
patch. incorporates the feedback: There should be a space between WHITESPACE and |

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

>> Source/WebCore/css/CSSGrammar.y.in:1592
>> + * http://www.w3.org/TR/css3-values/#calc-syntax
> 
> Where’s the test coverage for this? I’d rather omit this comment but include thorough test coverage demonstrating parse failure when the + and - don’t have spaces on either side.

The test you mention already exists: http://trac.webkit.org/browser/trunk/LayoutTests/css3/calc/calc-errors.html