Bug 36279 - CSS3 :nth-child selector and valid whitespace test fails
Summary: CSS3 :nth-child selector and valid whitespace test fails
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.5
: P2 Normal
Assignee: Simon Fraser (smfr)
URL: http://samples.msdn.microsoft.com/iet...
Keywords:
Depends on:
Blocks:
 
Reported: 2010-03-17 20:24 PDT by Simon Fraser (smfr)
Modified: 2011-01-04 19:51 PST (History)
9 users (show)

See Also:

Attachments
Testcase (1.80 KB, text/html)
2010-03-17 20:24 PDT, Simon Fraser (smfr) 		no flags Details
Patch (4.38 KB, patch)
2011-01-03 18:09 PST, Yael 		no flags Details | Formatted Diff | Diff
View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Fraser (smfr) 2010-03-17 20:24:46 PDT 
Created attachment 51005 [details]
Testcase

This test fails:
http://samples.msdn.microsoft.com/ietestcenter/css3/selectors/ValidWhitespace.htm
Comment 1 Yael 2011-01-03 18:09:04 PST 
Created attachment 77859 [details]
Patch

Teach the tokenizer to allow white spaces around [+/-] signs in nth constructs.
Comment 2 Darin Adler 2011-01-03 21:39:25 PST 
Comment on attachment 77859 [details]
Patch

Changing the tokenizer but not updating CSSSelector::RareData::parseNth is not going to work correctly. It seems the test case here was insufficient to detect that. Please work on a patch that changes both the tokenizer and the parser to allow spaces!
Comment 3 Yael 2011-01-04 07:19:38 PST 
(In reply to comment #2)
> (From update of attachment 77859 [details])
> Changing the tokenizer but not updating CSSSelector::RareData::parseNth is not going to work correctly. It seems the test case here was insufficient to detect that. Please work on a patch that changes both the tokenizer and the parser to allow spaces!

Thank you for your review, Darin.
I was under the impression that CSSSelector::RareData::parseNth already allows the spaces, but I could be wrong.

CSSSelector::RareData::parseNth uses String::find() to find the sign + or -, and does not assume that the sign follows immediately after n.

CSSSelector::RareData::parseNth uses String::toInt() to find the value of b, and that already knows how to skip leading spaces.

Do you have a specific case in mind that could fail ?
Comment 4 Simon Fraser (smfr) 2011-01-04 12:13:59 PST 
RareData::parseNth() looks like it deals with whitespace OK. Darin, are you OK with the patch?
Comment 5 Darin Adler 2011-01-04 13:07:14 PST 
(In reply to comment #3)
> I was under the impression that CSSSelector::RareData::parseNth already allows the spaces

You are right. Sorry, I was wrong!
Comment 6 WebKit Commit Bot 2011-01-04 18:19:24 PST 
The commit-queue encountered the following flaky tests while processing attachment 77859 [details]:

http/tests/appcache/progress-counter.html bug 51913 (author: michaeln@google.com)
The commit-queue is continuing to process your patch.
Comment 7 WebKit Commit Bot 2011-01-04 18:21:25 PST 
Comment on attachment 77859 [details]
Patch

Clearing flags on attachment: 77859

Committed r75039: <http://trac.webkit.org/changeset/75039>
Comment 8 WebKit Commit Bot 2011-01-04 18:21:32 PST 
All reviewed patches have been landed.  Closing bug.
Comment 9 WebKit Review Bot 2011-01-04 19:51:55 PST 
http://trac.webkit.org/changeset/75039 might have broken Leopard Intel Debug (Tests)