CSS3 Selectors failures on css3test.com

[Uday Kiran]

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

[Uday Kiran]

Created attachment 137076 [details]
Proposed patch

[Early Warning System Bot]

Comment on attachment 137076 [details]
Proposed patch

Attachment 137076 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12392916

[Zoltan Herczeg]

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

[Uday Kiran]

Created attachment 137311 [details]
Updated patch

[Uday Kiran]

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.

[Zoltan Herczeg]

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.

[Uday Kiran]

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.

[Uday Kiran]

Created attachment 137342 [details]
Updated patch

Updated patch and added testcases with invalid CSS values

[Zoltan Herczeg]

Comment on attachment 137342 [details]
Updated patch

Excellent patch!

[WebKit Review Bot]

Comment on attachment 137342 [details]
Updated patch

Clearing flags on attachment: 137342

Committed r114261: <http://trac.webkit.org/changeset/114261>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.