Bug 14346

Summary: CSS parser accepts unexpected { } in declarations
Product: WebKit Reporter: Gérard Talbot <browserbugs2>
Component: CSSAssignee: Nobody <webkit-unassigned>
Status: VERIFIED FIXED    
Severity: Normal CC: ap, ddkilzer, hyatt, webkit, wilwayco
Priority: P2    
Version: 523.x (Safari 3)   
Hardware: All   
OS: All   
URL: http://www.hixie.ch/tests/evil/css/css21/tests/t0402-syntax-05-f.htm
Bug Depends on:    
Bug Blocks: 12558, 15423, 17913    
Attachments:
Description Flags
patch
hyatt: review+
Patch -- fixed against r35223 none

Description Gérard Talbot 2007-06-23 15:50:19 PDT
<style type="text/css">
p { color:green; color{;color:maroon} } 
</style>

<p>This text should be green</p>

MSIE 7, Opera 9.21, Firefox 2.0.0.4 all pass this test.
This test directly comes from a given example in
CSS 2.1, section 4.2 Rules for handling parsing errors, malformed declarations item:
http://www.w3.org/TR/CSS21/syndata.html#parsing-errors
Comment 1 Gérard Talbot 2007-06-23 15:51:38 PDT
Safari 3.0.2 build 522.13.1 here.
Comment 2 Gérard Talbot 2007-07-15 17:43:15 PDT
Hello,

Please confirm this bug report: the testcase is clear, clean, reduced and comes directly from the latest CSS 2.1 WD in a given example. Thank you.
Comment 3 Alexey Proskuryakov 2007-07-15 21:57:06 PDT
Confirmed with r24287. But please note that fixing this maybe inappropriate during stabilization period anyway, if this problem doesn't affect any sites.
Comment 4 Anatoli Papirovski 2008-05-02 08:19:16 PDT
Created attachment 20925 [details]
patch

(One note to begin with, there's hardly any logic to the formatting of the grammar file, so I just tried to match whatever was there... )

The changelog has most of the fixes listed in it. I'll just provide the test cases not in our test suite that were fixed by this patch:
http://hixie.ch/tests/adhoc/css/parsing/core-syntax/002-demo.html
http://hixie.ch/tests/adhoc/css/parsing/core-syntax/002.html
http://hixie.ch/tests/adhoc/css/parsing/core-syntax/003.html
http://hixie.ch/tests/adhoc/css/parsing/core-syntax/006.html
http://dbaron.org/css/test/parsing4
http://dbaron.org/css/test/parsing5
http://samples.msdn.microsoft.com/csstestpages/Chapter_4/eof-001.htm
http://samples.msdn.microsoft.com/csstestpages/Chapter_4/invalid-decl-at-rule-001.htm (wrong description inside, but a correct test case)
http://annevankesteren.nl/test/css/syntax/007.html
http://annevankesteren.nl/test/css/syntax/018.html

+ the three test cases from the CSS 2.1 test suite which are in LayoutTests.
Comment 5 Dave Hyatt 2008-06-23 00:08:46 PDT
Comment on attachment 20925 [details]
patch

r=me
Comment 6 Robert Blaut 2008-07-18 01:33:17 PDT
*** Bug 19567 has been marked as a duplicate of this bug. ***
Comment 7 Robert Blaut 2008-07-18 14:57:07 PDT
*** Bug 20102 has been marked as a duplicate of this bug. ***
Comment 8 Jacob Refstrup 2008-07-18 20:52:49 PDT
Created attachment 22380 [details]
Patch -- fixed against r35223

Fixed merge conflict against r35223; tested this patch against test case for 15423 and this patch also fixes that defect so I'll close set 15423 as a duplicate.
Comment 9 Jacob Refstrup 2008-07-18 20:59:54 PDT
(In reply to comment #8)
> Created an attachment (id=22380) [edit]
> Patch -- fixed against r35223
> 
> Fixed merge conflict against r35223; tested this patch against test case for
> 15423 and this patch also fixes that defect so I'll close set 15423 as a
> duplicate.
> 

Already marked as a dependent.
Comment 10 David Kilzer (:ddkilzer) 2008-07-20 16:59:25 PDT
(In reply to comment #8)
> Created an attachment (id=22380) [edit]
> Patch -- fixed against r35223
> 
> Fixed merge conflict against r35223; tested this patch against test case for
> 15423 and this patch also fixes that defect so I'll close set 15423 as a
> duplicate.

Merged patch fails building with r35256:

bison -d -p cssyy WebCore/css/CSSGrammar.y -o CSSGrammar.cpp
WebCore/css/CSSGrammar.y:336: type clash (`' `rule') on default action
make: *** [CSSGrammar.cpp] Error 1

Comment 11 David Kilzer (:ddkilzer) 2008-07-20 21:06:00 PDT
(In reply to comment #10)
> Merged patch fails building with r35256:
> 
> bison -d -p cssyy WebCore/css/CSSGrammar.y -o CSSGrammar.cpp
> WebCore/css/CSSGrammar.y:336: type clash (`' `rule') on default action
> make: *** [CSSGrammar.cpp] Error 1

I have a fix for this that reduces the increased shift/reduce count by one (from 3 to 2) for the patch.

Comment 12 David Kilzer (:ddkilzer) 2008-07-21 02:40:59 PDT
Committed revision 35261.

http://trac.webkit.org/changeset/35261

Note that I had to change the patch to CSSGrammar.y to work with trunk:

- Reduced new shift/reduce conflicts by 1 (from 3 to 2)
- Removed "%type <rule> invalid_at_list"
- Moved "%type <ruleList> block_rule_list" to where "%type <ruleList> rule_list" used to be [cosmetic]
- Changed "invalid_at_list" definition to match other *_list definitions (like namespace_list)

Comment 13 Robert Blaut 2008-07-21 05:40:34 PDT
During building Webkit I got now:

bison -d -p cssyy WebCore/css/CSSGrammar.y -o CSSGrammar.cpp
WebCore/css/CSSGrammar.y: conflicts: 46 shift/reduce, 17 reduce/reduce
WebCore/css/CSSGrammar.y: expected 0 reduce/reduce conflicts
WebCore/css/CSSGrammar.y:1319.16: warning: rule never reduced because of conflicts: invalid_at_list: /* empty */
make: *** [CSSGrammar.cpp] Error 1
** BUILD FAILED **
Comment 14 Gavin Barraclough 2008-07-21 07:52:24 PDT
Patch reverted due to build issues (in CSSGrammar.y, as per previous comment).

Sending        LayoutTests/ChangeLog
Sending        LayoutTests/platform/mac/css2.1/t040105-import-01-b-expected.checksum
Deleting       LayoutTests/platform/mac/css2.1/t040105-import-01-b-expected.png
Sending        LayoutTests/platform/mac/css2.1/t040105-import-01-b-expected.txt
Sending        LayoutTests/platform/mac/css2.1/t0402-syntax-05-f-expected.checksum
Deleting       LayoutTests/platform/mac/css2.1/t0402-syntax-05-f-expected.png
Sending        LayoutTests/platform/mac/css2.1/t0402-syntax-05-f-expected.txt
Sending        LayoutTests/platform/mac/css2.1/t0402-syntax-06-f-expected.checksum
Deleting       LayoutTests/platform/mac/css2.1/t0402-syntax-06-f-expected.png
Sending        LayoutTests/platform/mac/css2.1/t0402-syntax-06-f-expected.txt
Sending        WebCore/ChangeLog
Sending        WebCore/css/CSSGrammar.y
Transmitting file data .........
Committed revision 35266.


Comment 15 Gavin Barraclough 2008-07-21 07:55:30 PDT
Reopening bug.
Comment 16 David Kilzer (:ddkilzer) 2008-07-21 08:20:05 PDT
(In reply to comment #13)
> During building Webkit I got now:
> 
> bison -d -p cssyy WebCore/css/CSSGrammar.y -o CSSGrammar.cpp
> WebCore/css/CSSGrammar.y: conflicts: 46 shift/reduce, 17 reduce/reduce
> WebCore/css/CSSGrammar.y: expected 0 reduce/reduce conflicts
> WebCore/css/CSSGrammar.y:1319.16: warning: rule never reduced because of
> conflicts: invalid_at_list: /* empty */
> make: *** [CSSGrammar.cpp] Error 1
> ** BUILD FAILED **

Sorry, I guess Tiger's version of bison isn't as stringent as Leopard's.

Comment 17 David Kilzer (:ddkilzer) 2008-07-23 21:49:03 PDT
(In reply to comment #16)
> (In reply to comment #13)
> > bison -d -p cssyy WebCore/css/CSSGrammar.y -o CSSGrammar.cpp
> > WebCore/css/CSSGrammar.y: conflicts: 46 shift/reduce, 17 reduce/reduce
> > WebCore/css/CSSGrammar.y: expected 0 reduce/reduce conflicts
> > WebCore/css/CSSGrammar.y:1319.16: warning: rule never reduced because of
> > conflicts: invalid_at_list: /* empty */
> > make: *** [CSSGrammar.cpp] Error 1
> > ** BUILD FAILED **
> 
> Sorry, I guess Tiger's version of bison isn't as stringent as Leopard's.

I have a fix for this on Tiger now.  Need to test the fix on Leopard as well.

Comment 18 David Kilzer (:ddkilzer) 2008-07-27 14:09:02 PDT
$ git svn dcommit
Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	LayoutTests/ChangeLog
	M	LayoutTests/platform/mac/css2.1/t040105-import-01-b-expected.checksum
	A	LayoutTests/platform/mac/css2.1/t040105-import-01-b-expected.png
	M	LayoutTests/platform/mac/css2.1/t040105-import-01-b-expected.txt
	M	LayoutTests/platform/mac/css2.1/t0402-syntax-05-f-expected.checksum
	A	LayoutTests/platform/mac/css2.1/t0402-syntax-05-f-expected.png
	M	LayoutTests/platform/mac/css2.1/t0402-syntax-05-f-expected.txt
	M	LayoutTests/platform/mac/css2.1/t0402-syntax-06-f-expected.checksum
	A	LayoutTests/platform/mac/css2.1/t0402-syntax-06-f-expected.png
	M	LayoutTests/platform/mac/css2.1/t0402-syntax-06-f-expected.txt
	M	WebCore/ChangeLog
	M	WebCore/css/CSSGrammar.y
Committed r35403