The following example is parsed incorrectly: body { background-color: red; {invalid block}# } body { background-color: greed; } Parser ignoring the opening brace of the invalid block and considers its closing brace as a closing for the declaration list. Only happens if 1) invalid block follows another declaration and 2) followed by any character (including whitespace). In this example second rule is ignored.
Created attachment 194190 [details] Patch
Created attachment 194496 [details] Patch
Created attachment 194900 [details] Patch
Comment on attachment 194900 [details] Patch Attachment 194900 [details] did not pass win-ews (win): Output: http://webkit-commit-queue.appspot.com/results/17132913
Created attachment 195044 [details] Patch
Comment on attachment 195044 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=195044&action=review > Source/WebCore/ChangeLog:10 > + The rgammar rule changed to handle invalid tokens before and after invalid block. rgammar -> grammar
Created attachment 195320 [details] Patch
I haven't looked at your patch to see what code changes have been made, but the correct behavior here (assuming "greed" is a typo for "green") is described here: http://dev.w3.org/csswg/css3-syntax/#consume-a-list-of-declarations In summary, while you're inside of a style block, and thus in the "consume a list of declarations" mode, encountering the {invalid block} hits the "anything else" clause, which is a syntax error. You then seek forward, throwing away everything, until you hit a ; or the end of the style block. So, the page should be green, obviously.
(In reply to comment #8) > I haven't looked at your patch to see what code changes have been made, but the correct behavior here (assuming "greed" is a typo for "green") is described here: http://dev.w3.org/csswg/css3-syntax/#consume-a-list-of-declarations There also is a section that clarifies error recovery mechanism: http://www.w3.org/TR/css3-syntax/#error-handling. It states that the parser must observe the rule "matching pairs of (), [], {}, "", and '', and correctly handling escapes". > In summary, while you're inside of a style block, and thus in the "consume a list of declarations" mode, encountering the {invalid block} hits the "anything else" clause, which is a syntax error. You then seek forward, throwing away everything, until you hit a ; or the end of the style block. > > So, the page should be green, obviously. The test from the description doesn't fail any longer (because of https://bugs.webkit.org/show_bug.cgi?id=113142) but other tests still do. There is a result of running the test on TOT: FAIL: Test 6 FAIL: Test 8 FAIL: Test 10 FAIL: Test 12 FAIL: Test 13 FAIL: Test 15 For instance: .malformed6 {color: red; #{}} #test6 { display:none; } Parser log shows it considers "#{" as a syntax error and drops it. First closing brace is considered as closing brace for the whole rule. Next closing brace is prevents the second selector from being parsed correctly.
(In reply to comment #9) > (In reply to comment #8) > > I haven't looked at your patch to see what code changes have been made, but the correct behavior here (assuming "greed" is a typo for "green") is described here: http://dev.w3.org/csswg/css3-syntax/#consume-a-list-of-declarations > > There also is a section that clarifies error recovery mechanism: http://www.w3.org/TR/css3-syntax/#error-handling. It states that the parser must observe the rule "matching pairs of (), [], {}, "", and '', and correctly handling escapes". Don't look at that spec - it's from 2003 and very obsolete and wrong. Multiple syntax changes that have been made to CSS 2.1 never got applied to that spec. Only look at the Editor's draft at the link I gave you instead, at least until I actually get my draft published as a Working Draft. > > In summary, while you're inside of a style block, and thus in the "consume a list of declarations" mode, encountering the {invalid block} hits the "anything else" clause, which is a syntax error. You then seek forward, throwing away everything, until you hit a ; or the end of the style block. > > > > So, the page should be green, obviously. > > The test from the description doesn't fail any longer (because of https://bugs.webkit.org/show_bug.cgi?id=113142) but other tests still do. There is a result of running the test on TOT: > FAIL: Test 6 > FAIL: Test 8 > FAIL: Test 10 > FAIL: Test 12 > FAIL: Test 13 > FAIL: Test 15 > > For instance: > .malformed6 {color: red; #{}} > #test6 { > display:none; > } > > Parser log shows it considers "#{" as a syntax error and drops it. First closing brace is considered as closing brace for the whole rule. Next closing brace is prevents the second selector from being parsed correctly. Yes, those are definitely wrong and should be processed as you think - the "#" should parse as a DELIM character, and the {} following that is a simple block.
> > For instance: > > .malformed6 {color: red; #{}} > > #test6 { > > display:none; > > } > > > > Parser log shows it considers "#{" as a syntax error and drops it. First closing brace is considered as closing brace for the whole rule. Next closing brace is prevents the second selector from being parsed correctly. > > Yes, those are definitely wrong and should be processed as you think - the "#" should parse as a DELIM character, and the {} following that is a simple block. This example is parsed wrongly from common sense (as far as I understand invalid_blocks are ignored for future syntax compatibility) and consistency (because the current grammar throws away such blocks in some contexts and doesn't in others) and the old spec's points of view. But the new draft doesn't say that. In this example the parser works exactly as you said: 1. Sees #. It is not a valid declaration, not EOF or a block end. So it's a syntax error. 2. Reads the next token '{'. It is not an end of the block, not EOF, not a semicolon. So it moves forward. 3. It sees the '}' token. This is the first closing brace it met so it closes the declaration list.
(In reply to comment #11) > > > For instance: > > > .malformed6 {color: red; #{}} > > > #test6 { > > > display:none; > > > } > > > > > > Parser log shows it considers "#{" as a syntax error and drops it. First closing brace is considered as closing brace for the whole rule. Next closing brace is prevents the second selector from being parsed correctly. > > > > Yes, those are definitely wrong and should be processed as you think - the "#" should parse as a DELIM character, and the {} following that is a simple block. > > This example is parsed wrongly from common sense (as far as I understand invalid_blocks are ignored for future syntax compatibility) and consistency (because the current grammar throws away such blocks in some contexts and doesn't in others) and the old spec's points of view. But the new draft doesn't say that. In this example the parser works exactly as you said: > 1. Sees #. It is not a valid declaration, not EOF or a block end. So it's a syntax error. > 2. Reads the next token '{'. It is not an end of the block, not EOF, not a semicolon. So it moves forward. > 3. It sees the '}' token. This is the first closing brace it met so it closes the declaration list. Are you trying to say that those steps are what the current Syntax spec (at http://dev.w3.org/csswg/css-syntax/) says? Because it doesn't - it does what I said, instead, where the # is a syntax error, it consumes the {} whole as a simple block, and then recovers when it sees the } closing the style rule. (It knows that, because the style rule has already found its closing brace, and is just parsing its contents, so there's no chance of misparsing on that.)
Bug 112986 (currently being reviewed) introduces a bracket stack keeping track of matching parentheses. That patch is fixing error handling for media queries specifically, but error recovery for other parts of the grammar needs to take balancing of parentheses into account too. I just didn't want to make that patch too large.
Webkit CSS Parser was updated with Blink one, this patch will not apply and the test pass any way in Safari 16: Link - https://jsfiddle.net/t6repdLk/ Marking this as "RESOLVED CONFIGURATION CHANGED". Thanks!