Created attachment 137076 [details] Proposed patch

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

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

Created attachment 137311 [details] Updated patch

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 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.

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.

Created attachment 137342 [details] Updated patch Updated patch and added testcases with invalid CSS values

Comment on attachment 137342 [details] Updated patch Excellent patch!

Comment on attachment 137342 [details] Updated patch Clearing flags on attachment: 137342 Committed r114261: <http://trac.webkit.org/changeset/114261>

All reviewed patches have been landed. Closing bug.