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.
Created attachment 214713 [details] patch
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?
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.
Created attachment 214733 [details] patch. incorporates the feedback.
Created attachment 214739 [details] patch. incorporates the feedback: add test for the '-' (minus) operator.
Created attachment 214741 [details] patch replace ’ (â) character from the html page with the ' character.
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 |
Created attachment 214743 [details] patch. incorporates the feedback: There should be a space between WHITESPACE and |
Comment on attachment 214743 [details] patch. incorporates the feedback: There should be a space between WHITESPACE and | Rejecting attachment 214743 [details] from commit-queue. mmaerean@adobe.com 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.
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.
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.
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>
(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.
I think the patch is fine to land as is, but I do think it’s got some unclear parts.
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