Bug 123082

Summary: [CSS Regions] Fix WHITESPACE issues in the CSS grammar.
Product: WebKit Reporter: Mihai Maerean <mmaerean>
Component: WebCore Misc.Assignee: Mihai Maerean <mmaerean>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, darin, esprehn+autocc, glenn, gyuyoung.kim, kling, macpherson, menard, mibalan, mihnea, rune, WebkitBugTracker
Priority: P2 Keywords: AdobeTracked
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
URL: https://codereview.chromium.org/25607005
Bug Depends on:    
Bug Blocks: 57312    
Attachments:
Description Flags
patch
none
patch. incorporates the feedback.
none
patch. incorporates the feedback: add test for the '-' (minus) operator.
none
patch
koivisto: review+, koivisto: commit-queue-
patch. incorporates the feedback: There should be a space between WHITESPACE and | none

Mihai Maerean
Reported 2013-10-20 03:46:06 PDT
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.
Attachments
patch (10.74 KB, patch)
2013-10-21 01:41 PDT, Mihai Maerean 		no flags
DetailsFormatted DiffDiff
patch. incorporates the feedback. (11.28 KB, patch)
2013-10-21 06:39 PDT, Mihai Maerean 		no flags
DetailsFormatted DiffDiff
patch. incorporates the feedback: add test for the '-' (minus) operator. (11.41 KB, patch)
2013-10-21 08:01 PDT, Mihai Maerean 		no flags
DetailsFormatted DiffDiff
patch (11.41 KB, patch)
2013-10-21 08:10 PDT, Mihai Maerean 		koivisto: review+
koivisto: commit-queue-
DetailsFormatted DiffDiff
patch. incorporates the feedback: There should be a space between WHITESPACE and | (11.41 KB, patch)
2013-10-21 09:02 PDT, Mihai Maerean 		no flags
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Mihai Maerean
Comment 1 2013-10-21 01:41:59 PDT
Created attachment 214713 [details] patch
Mihnea Ovidenie
Comment 2 2013-10-21 04:17:47 PDT
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 3 2013-10-21 06:35:21 PDT
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
Comment 4 2013-10-21 06:39:55 PDT
Created attachment 214733 [details] patch. incorporates the feedback.
Mihai Maerean
Comment 5 2013-10-21 08:01:25 PDT
Created attachment 214739 [details] patch. incorporates the feedback: add test for the '-' (minus) operator.
Mihai Maerean
Comment 6 2013-10-21 08:10:37 PDT
Created attachment 214741 [details] patch replace ’ (â) character from the html page with the ' character.
Antti Koivisto
Comment 7 2013-10-21 08:55:39 PDT
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
Comment 8 2013-10-21 09:02:20 PDT
Created attachment 214743 [details] patch. incorporates the feedback: There should be a space between WHITESPACE and |
WebKit Commit Bot
Comment 9 2013-10-21 09:02:52 PDT
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.
Darin Adler
Comment 10 2013-10-21 09:47:34 PDT
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 11 2013-10-21 09:48:57 PDT
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 12 2013-10-21 09:51:43 PDT
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
Comment 13 2013-10-21 09:52:13 PDT
(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
Comment 14 2013-10-21 09:53:17 PDT
I think the patch is fine to land as is, but I do think it’s got some unclear parts.
Mihai Maerean
Comment 15 2013-10-21 12:27:48 PDT
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
Note You need to log in before you can comment on or make changes to this bug.