Bug 83885 - CSS3 Selectors failures on css3test.com
: CSS3 Selectors failures on css3test.com
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: CSS
: 528+ (Nightly build)
: Unspecified Linux
: P2 Normal
Assigned To: Nobody
http://css3test.com
:
Depends on:
Blocks: 79073
  Show dependency treegraph
 
Reported: 2012-04-13 05:58 PDT by Uday Kiran
Modified: 2012-04-16 08:36 PDT (History)
3 users (show)

See Also:


Attachments
Proposed patch (6.45 KB, patch)
2012-04-13 06:26 PDT, Uday Kiran
no flags Details | Formatted Diff | Diff
Updated patch (6.75 KB, patch)
2012-04-16 04:01 PDT, Uday Kiran
no flags Details | Formatted Diff | Diff
Updated patch (7.59 KB, patch)
2012-04-16 07:13 PDT, Uday Kiran
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Uday Kiran 2012-04-13 05:58:05 PDT
On http://css3test.com, :nth-child(-n-1) :nth-last-child(-n-1) :nth-of-type(-n-1) :nth-last-of-type(-n-1) parsing fails
Comment 1 Uday Kiran 2012-04-13 06:26:07 PDT
Created attachment 137076 [details]
Proposed patch
Comment 2 Early Warning System Bot 2012-04-13 06:35:34 PDT
Comment on attachment 137076 [details]
Proposed patch

Attachment 137076 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12392916
Comment 3 Zoltan Herczeg 2012-04-16 02:06:09 PDT
Comment on attachment 137076 [details]
Proposed patch

Good catch, just a few comments:

View in context: https://bugs.webkit.org/attachment.cgi?id=137076&action=review

> Source/WebCore/ChangeLog:8
> +        Parsing fails for :nth-child(-n-1) :nth-last-child(-n-1) :nth-of-type(-n-1) :nth-last-of-type(-n-1).

Please describe why. (-n-1 also a valid ID, so we need to resume parsing after the 'n' all the time and such)

> Source/WebCore/css/CSSParser.cpp:8543
> +                    // In parseIdentifier, loop doesn't break for numbers.

What do you mean here?

> Source/WebCore/css/CSSParser.cpp:8546
> +                    m_currentCharacter = &m_tokenStart[1];

simply m_tokenStart + 1

> Source/WebCore/css/CSSParser.cpp:8644
> +                    // In parseIdentifier, loop doesn't break for numbers.

ditto.

> Source/WebCore/css/CSSParser.cpp:8647
> +                    m_currentCharacter = &m_tokenStart[2];

ditto
Comment 4 Uday Kiran 2012-04-16 04:01:45 PDT
Created attachment 137311 [details]
Updated patch
Comment 5 Uday Kiran 2012-04-16 04:03:56 PDT
Thanks for reviewing.

(In reply to comment #3)
> (From update of attachment 137076 [details])
> Good catch, just a few comments:
> 
> View in context: https://bugs.webkit.org/attachment.cgi?id=137076&action=review
> 
> > Source/WebCore/ChangeLog:8
> > +        Parsing fails for :nth-child(-n-1) :nth-last-child(-n-1) :nth-of-type(-n-1) :nth-last-of-type(-n-1).
> 
> Please describe why. (-n-1 also a valid ID, so we need to resume parsing after the 'n' all the time and such)
Done.

> 
> > Source/WebCore/css/CSSParser.cpp:8543
> > +                    // In parseIdentifier, loop doesn't break for numbers.
> 
> What do you mean here?
In parseIdentifier function, loop doesn't break for numbers as isCSSLetter(digit) is true, CharacterDash is greater than CharacterNumber.
Moved comment to changelog.

> 
> > Source/WebCore/css/CSSParser.cpp:8546
> > +                    m_currentCharacter = &m_tokenStart[1];
> 
> simply m_tokenStart + 1
> 
Done.

> > Source/WebCore/css/CSSParser.cpp:8644
> > +                    // In parseIdentifier, loop doesn't break for numbers.
> 
> ditto.
> 
Done.

> > Source/WebCore/css/CSSParser.cpp:8647
> > +                    m_currentCharacter = &m_tokenStart[2];
> 
> ditto
Done.
Comment 6 Zoltan Herczeg 2012-04-16 04:30:33 PDT
Comment on attachment 137311 [details]
Updated patch

View in context: https://bugs.webkit.org/attachment.cgi?id=137311&action=review

> Source/WebCore/ChangeLog:14
> +        In parseIdentifier function, loop doesn't break for numbers as isCSSLetter(digit) is true,
> +        CharacterDash is greater than CharacterNumber.
> +        Set m_currentCharacter to '-' and resume parsing of number.

Ok this is somewhat better, but I don't think anyone will understand this except me.

Basically the issue is that n-100 is a valid CSS identifier, since it is built from letters, numbers and dashes. However, in NthChildMode we need to check whether this identifier is a valid nth child descriptor. The original code only checked this if the string was n- but this is not enough. We need to check everything which starts with an n- prefix.

> LayoutTests/css3/parsing-css3-nthchild-expected.txt:8
> +#a:nth-child(n-1) { color: green; }
> +#b:nth-child(n- 10) { color: green; }
> +#c:nth-child(-n-1) { color: green; }
> +#d:nth-child(-n- 10) { color: green; }

I think we should add some invalid forms as well, which enters the new condition, but fails later like: n-a n-1a1 and such.
Comment 7 Uday Kiran 2012-04-16 05:43:15 PDT
Thanks for review.

(In reply to comment #6)
> (From update of attachment 137311 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=137311&action=review
> 
> > Source/WebCore/ChangeLog:14
> > +        In parseIdentifier function, loop doesn't break for numbers as isCSSLetter(digit) is true,
> > +        CharacterDash is greater than CharacterNumber.
> > +        Set m_currentCharacter to '-' and resume parsing of number.
> 
> Ok this is somewhat better, but I don't think anyone will understand this except me.
> 
> Basically the issue is that n-100 is a valid CSS identifier, since it is built from letters, numbers and dashes. However, in NthChildMode we need to check whether this identifier is a valid nth child descriptor. The original code only checked this if the string was n- but this is not enough. We need to check everything which starts with an n- prefix.

Right. I will change that.
> 
> > LayoutTests/css3/parsing-css3-nthchild-expected.txt:8
> > +#a:nth-child(n-1) { color: green; }
> > +#b:nth-child(n- 10) { color: green; }
> > +#c:nth-child(-n-1) { color: green; }
> > +#d:nth-child(-n- 10) { color: green; }
> 
> I think we should add some invalid forms as well, which enters the new condition, but fails later like: n-a n-1a1 and such.

I will add more tests and upload new patch.
Comment 8 Uday Kiran 2012-04-16 07:13:40 PDT
Created attachment 137342 [details]
Updated patch

Updated patch and added testcases with invalid CSS values
Comment 9 Zoltan Herczeg 2012-04-16 07:45:19 PDT
Comment on attachment 137342 [details]
Updated patch

Excellent patch!
Comment 10 WebKit Review Bot 2012-04-16 08:36:44 PDT
Comment on attachment 137342 [details]
Updated patch

Clearing flags on attachment: 137342

Committed r114261: <http://trac.webkit.org/changeset/114261>
Comment 11 WebKit Review Bot 2012-04-16 08:36:49 PDT
All reviewed patches have been landed.  Closing bug.