Bug 16751

Summary: Failing to parse "html*.test" correctly as CSS selector (Acid3 bug)
Product: WebKit Reporter: Eric Seidel (no email) <eric>
Component: CSSAssignee: Dave Hyatt <hyatt>
Status: RESOLVED FIXED    
Severity: Normal CC: webkit
Priority: P2 Keywords: HasReduction
Version: 528+ (Nightly build)   
Hardware: Mac   
OS: OS X 10.4   
URL: http://www.hixie.ch/tests/evil/acid/003/
Bug Depends on: 15508    
Bug Blocks: 17064    
Attachments:
Description Flags
Minimal test case
none
Patch that fixes the bug
none
Shrink the patch a bit. darin: review+

Eric Seidel (no email)
Reported 2008-01-05 14:22:03 PST
Failing to parse "html*.test" correctly as CSS selector (Acid3 bug) Test 44: FAIL (expected 0, got 1 - misparsed selectors) function () { // test 44: selectors without spaces before a "*" selectorTest(function (doc, add, expect) { doc.body.className = "test"; var p = doc.createElement('p'); p.className = "test"; doc.body.appendChild(p); add("html*.test"); expect(doc.body, 0, "misparsed selectors"); expect(p, 0, "really misparsed selectors"); }); return 3; },
Attachments
Minimal test case (319 bytes, text/html)
2008-01-27 15:32 PST, Robert Blaut 		no flags
Details
Patch that fixes the bug (5.52 KB, patch)
2008-02-04 12:00 PST, Dave Hyatt 		no flags
DetailsFormatted DiffDiff
Shrink the patch a bit. (5.29 KB, patch)
2008-02-04 13:15 PST, Dave Hyatt 		darin: review+
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Robert Blaut
Comment 1 2008-01-27 15:32:22 PST
Created attachment 18727 [details] Minimal test case
Dave Hyatt
Comment 2 2008-02-04 12:00:34 PST
Created attachment 18913 [details] Patch that fixes the bug Will land test case from bug as layout test.
Dave Hyatt
Comment 3 2008-02-04 13:15:18 PST
Created attachment 18917 [details] Shrink the patch a bit.
Darin Adler
Comment 4 2008-02-04 13:38:37 PST
Comment on attachment 18917 [details] Shrink the patch a bit. This looks like it relies on how bison handles conflicts. Anywhere we say "selector" it could expand to "selector_with_trailing_whitespace" so it's all about how it reduces. Do we have sufficient test coverage for CSS selector parsing? I'd like to see a lot of tests in this area. r=me, more based on the theory that this is thoroughly tested rather than on my certainty that I can validate this from proofreading the code. I'd also like to see this grammar fixed to remove all the conflicts; that would make it easier to understand the behavior by looking at the code, although it would make the grammar more complex.
Dave Hyatt
Comment 5 2008-02-04 14:10:36 PST
I don't believe it does rely on how Bison handles conflicts. The patch forces whitespace following a selector to resolve into "selector_with_trailing_whitespace." There is no other reduce option for whitespace following a selector.
Dave Hyatt
Comment 6 2008-02-04 14:16:56 PST
Fixed in r29976-8.
Note You need to log in before you can comment on or make changes to this bug.