Bug 83885 - CSS3 Selectors failures on css3test.com
: CSS3 Selectors failures on css3test.com
Status: RESOLVED FIXED
: WebKit
CSS
: 528+ (Nightly build)
: Unspecified Linux
: P2 Normal
Assigned To:
: http://css3test.com
:
:
: 79073
  Show dependency treegraph
 
Reported: 2012-04-13 05:58 PST by
Modified: 2012-04-16 08:36 PST (History)


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


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2012-04-13 05:58:05 PST
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 From 2012-04-13 06:26:07 PST -------
Created an attachment (id=137076) [details]
Proposed patch
------- Comment #2 From 2012-04-13 06:35:34 PST -------
(From update of attachment 137076 [details])
Attachment 137076 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/12392916
------- Comment #3 From 2012-04-16 02:06:09 PST -------
(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)

> 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 From 2012-04-16 04:01:45 PST -------
Created an attachment (id=137311) [details]
Updated patch
------- Comment #5 From 2012-04-16 04:03:56 PST -------
Thanks for reviewing.

(In reply to comment #3)
> (From update of attachment 137076 [details] [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 From 2012-04-16 04:30:33 PST -------
(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.

> 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 From 2012-04-16 05:43:15 PST -------
Thanks for review.

(In reply to comment #6)
> (From update of attachment 137311 [details] [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 From 2012-04-16 07:13:40 PST -------
Created an attachment (id=137342) [details]
Updated patch

Updated patch and added testcases with invalid CSS values
------- Comment #9 From 2012-04-16 07:45:19 PST -------
(From update of attachment 137342 [details])
Excellent patch!
------- Comment #10 From 2012-04-16 08:36:44 PST -------
(From update of attachment 137342 [details])
Clearing flags on attachment: 137342

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