Last Comment Bug 750388 - Parsing of :nth-* pseudo-classes should accept strings starting with "+n"
: Parsing of :nth-* pseudo-classes should accept strings starting with "+n"
Status: RESOLVED FIXED
[mentor=bz][mentor=kennyluck][lang=c++]
:
Product: Core
Classification: Components
Component: CSS Parsing and Computation (show other bugs)
: unspecified
: x86 All
: -- normal (vote)
: mozilla24
Assigned To: Thomasy
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-04-30 12:02 PDT by Boris Zbarsky [:bz]
Modified: 2013-05-29 07:14 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch for +n case (2.13 KB, patch)
2012-08-09 06:33 PDT, Thomasy
no flags Details | Diff | Review
Patch for +n case and +(space)n with test case (6.14 KB, patch)
2012-08-10 01:44 PDT, Thomasy
no flags Details | Diff | Review
Patch for a/**/n... case (9.74 KB, patch)
2012-08-10 22:00 PDT, Thomasy
no flags Details | Diff | Review
Test csae for nth-* pseudo-classes (4.40 KB, text/html)
2012-08-11 05:14 PDT, Thomasy
no flags Details
Fix for 1/**/n-10 and -1/**/n-10 case (10.11 KB, patch)
2012-08-12 03:48 PDT, Thomasy
dbaron: review+
Details | Diff | Review

Description Boris Zbarsky [:bz] 2012-04-30 12:02:08 PDT
E.g. "+n" or "+n+2" or "+n-2".

Right now we just don't handle the case when the first token is a symbol token for '+'.  Should be somewhat easy to fix, I'd think.  We just need to check for '+' and set a flag to not allow leading '-', then parse as we do now, I think.
Comment 1 Josh Matthews [:jdm] 2012-04-30 15:56:57 PDT
Can you drop a link to a file/name a function where this code lives?
Comment 2 Kang-Hao (Kenny) Lu [:kennyluck] 2012-04-30 16:03:59 PDT
(In reply to Josh Matthews [:jdm] (travelling until June 25th, not reading non-CCed bugmail) from comment #1)
> Can you drop a link to a file/name a function where this code lives?

http://hg.mozilla.org/mozilla-central/annotate/992588c2eab6/layout/style/nsCSSParser.cpp#l3438

I would like to offer to be a mentor too.

/me is now writing a mail to mozilla.mozillians about this.
Comment 3 Boris Zbarsky [:bz] 2012-04-30 18:02:18 PDT
Yep, comment 2 is right on the money.  Sorry for not putting that in initially.
Comment 4 Kang-Hao (Kenny) Lu [:kennyluck] 2012-05-02 13:35:43 PDT
Some of the other corner cases in an+b that fail Gecko:

1) "N-1" and similarly "-N-1" should not be ignored.
2) "n\-1" and "\-n-1" should be ignored.

1) was already discovered in bug 543151. For example, patch v1 has a two-liner for this. To fix 2) completely, I think it would be better to have a flag like mSVGmode in the scanner that treats "-" as a DELIM instead of a {nmstart} but I am not sure we should bother doing this. At data point, IE9 doesn't parse CSS escape sequences in :nth-child() so although it passes 2) it fails to parse ":nth-child(\n)".

On the other hand, the same code path could be useful if we allows optional spaces around '+' and '-' in calc() instead of strictly requiring it. I can't find working group discussions that led to [1]. Have we got author complaints about this ever since -moz-calc() was released? 

[1] http://lists.w3.org/Archives/Public/www-style/2008Nov/0147
Comment 5 Po-chiang Chao [:bobchao] 2012-08-08 22:01:24 PDT
Thomas would like to take this bug, with help from Kenny.
Assign -> Thomas
Comment 6 Thomasy 2012-08-09 06:33:59 PDT
Created attachment 650529 [details] [diff] [review]
Patch for +n case
Comment 7 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-08-09 08:05:55 PDT
Comment on attachment 650529 [details] [diff] [review]
Patch for +n case

I don't see why this requires so much code to be duplicated.  Can't you put an mToken.IsSymbol('+') check earlier, set a 'hadSign' variable to true, and then continue on (failing in some of the other cases if hadSign is true)?  And, in fact, I think it needs to happen earlier since it needs to happen before the block of code just above the one that you modified that pushes back characters after n- or -n-; otherwise you won't correctly handle cases like :nth-child(+n-3).

Finally, this also needs some tests.  You should add them next to the other :nth-child tests in layout/style/test/test_selectors.html.   You should make sure to test that + and - are allowed before the INTEGER? {N}, but also that they are not allowed when there's a space in-between.  You should also make sure that cases with two signs aren't allowed in more combinations.  And then you should make sure the tests pass with your patch.
Comment 8 Thomasy 2012-08-10 01:44:27 PDT
Created attachment 650828 [details] [diff] [review]
Patch for +n case and +(space)n with test case
Comment 9 Kang-Hao (Kenny) Lu [:kennyluck] 2012-08-10 03:00:28 PDT
Comment on attachment 650828 [details] [diff] [review]
Patch for +n case and +(space)n with test case

Review of attachment 650828 [details] [diff] [review]:
-----------------------------------------------------------------

As a starter, if you want to get a formal review. You should set the review flag to r? and the reviewer to David. But here are my random inputs:

::: layout/style/nsCSSParser.cpp
@@ +3620,5 @@
>                                                nsCSSPseudoClasses::Type aType)
>  {
>    PRInt32 numbers[2] = { 0, 0 };
>    bool lookForB = true;
> +  bool hadSign = false;

If "hadSign" is true only if you get a "+" symbol. I suggest you rename it to "hadPlusSign". However, you can make the code accept the "-" symbol too. Opera does this. If I recall correctly, IE does this too. Test case:

data:text/html,<style>:nth-child(-/**/n+3) { background: green; }</style>

"/**/" makes a COMMENT token. If you have problem about how CSS tokenization works. Feel free to ping me on IRC.

@@ +3632,5 @@
> +  // Begin with +
> +  if (mToken.IsSymbol('+')) {
> +    hadSign = true;
> +    if(RequireWhitespace()){
> +      REPORT_UNEXPECTED_EOF(PEPseudoClassArgEOF);

This is not an unexpected eof, but I actually know very littele about the error reporting codes.
Comment 10 Kang-Hao (Kenny) Lu [:kennyluck] 2012-08-10 03:06:41 PDT
Oh, you should also obsolete your previous patch.
Comment 11 Thomasy 2012-08-10 22:00:28 PDT
Created attachment 651092 [details] [diff] [review]
Patch for a/**/n...  case

Update parse for INTEGER? {N} => eCSSToken_Number + eCSSToken_Ident or eCSSToken_Symbol + eCSSToken_Number + eCSSToken_Ident
Comment 12 Kang-Hao (Kenny) Lu [:kennyluck] 2012-08-11 00:57:45 PDT
(In reply to Thomasy from comment #11)
> Update parse for INTEGER? {N} => eCSSToken_Number + eCSSToken_Ident or
> eCSSToken_Symbol + eCSSToken_Number + eCSSToken_Ident

Oh, huh. You implemented all possible cases! I'd note that the spec (css3-selector) is quite crappy here (see [1] for some discussions), and I can't really predict what browsers will converge to. Last time I check, IE and Opera don't support "a/**/n", so...

If I were you, I would just do attachment 650828 [details] [diff] [review] + mToken.IsSymbol('+') => mToken.IsSymbol('+') || mToken.IsSymbol('-'), but let's ask for David's opinion here.

[1] http://lists.w3.org/Archives/Public/www-style/2012Apr/thread#msg805
Comment 13 Thomasy 2012-08-11 05:14:30 PDT
Created attachment 651118 [details]
Test csae for nth-* pseudo-classes

I test the browser using attached test case

_Not_ support IE9 (9.0.8)

span:nth-child(+1/**/n-1)  
span:nth-child(-1/**/n+10)  
span:nth-child(1/**/n-1)  

Chrome 21.0.1180.75 m Only _support_ 

span:nth-child(1n-1)
span:nth-child(+1n-1)
span:nth-child(-1n+10)
span:nth-child(-1n+10)
span:nth-child(-n+10)

Opera 12.00 1467 _Not_ support 
span:nth-child(+1/**/n-1)
span:nth-child(-1/**/n+10)
span:nth-child(1/**/n-1)

In the proposed patch _Not_ support

span:nth-child(+1/**/n-1) 
span:nth-child(1/**/n-1)

because /**/ and white-space are taken as eCSSToken_WhiteSpace in CSS Scanner, and number+white-space+n is illegal for a space in-between.
Comment 14 Kang-Hao (Kenny) Lu [:kennyluck] 2012-08-11 05:44:33 PDT
(In reply to Thomasy from comment #13)
> I test the browser using attached test case

Creating tests is usually a good thing to do. Well done!

> In the proposed patch _Not_ support
> 
> span:nth-child(+1/**/n-1) 
> span:nth-child(1/**/n-1)

Which patch? attachment 650828 [details] [diff] [review] ? Not supporting "span:nth-child(-1/**/n+10)" but these two seems like a bug to me.

> because /**/ and white-space are taken as eCSSToken_WhiteSpace in CSS
> Scanner

Note that /**/ doesn't create eCSSToken_WhiteSpace. It is simply a token separator.

> , and number+white-space+n is illegal for a space in-between.

Yep.
Comment 15 Thomasy 2012-08-11 16:05:15 PDT
(In reply to Kang-Hao (Kenny) Lu [:kennyluck] from comment #14)
> (In reply to Thomasy from comment #13)
> > I test the browser using attached test case
> 
> Creating tests is usually a good thing to do. Well done!
> 
> > In the proposed patch _Not_ support
> > 
> > span:nth-child(+1/**/n-1) 
> > span:nth-child(1/**/n-1)
> 
> Which patch? attachment 650828 [details] [diff] [review] ? Not supporting
attachment 651092 [details] [diff] [review]

> "span:nth-child(-1/**/n+10)" but these two seems like a bug to me.
The proposed patch _do_ support "span:nth-child(-1/**/n+10)" but I will look into why it not work for nth-child(+1/**/n-1) and span:nth-child(1/**/n-1)

It is something around the code below, any idea?

   else if (eCSSToken_Number == mToken.mType) {
     if (!mToken.mIntegerValid) {
       REPORT_UNEXPECTED_TOKEN(PEPseudoClassArgNotNth);
       return eSelectorParsingStatus_Error; // our caller calls SkipUntil(')')
     }
-    numbers[1] = mToken.mInteger;
-    lookForB = false;
+    // for +-an case
+    if ( mToken.mHasSign && hasSign[0] ) {
+      REPORT_UNEXPECTED_TOKEN(PEPseudoClassArgNotNth);
+      return eSelectorParsingStatus_Error; // our caller calls SkipUntil(')')
+    }
+    PRInt32 intValue = mToken.mInteger * sign[0];
+    // for -a/**/n case
+    if (! GetToken(false)) {
+      numbers[1] = intValue;
+      lookForB = false;
+    }else {
+      if (eCSSToken_Ident == mToken.mType && mToken.mIdent.LowerCaseEqualsLiteral("n")) {
+        numbers[0] = intValue;
+      }else {
+        UngetToken();
+        numbers[1] = intValue;
+        lookForB = false;
+      }
+      
+    }
+    
   }
 
> > because /**/ and white-space are taken as eCSSToken_WhiteSpace in CSS
> > Scanner
> 
> Note that /**/ doesn't create eCSSToken_WhiteSpace. It is simply a token
> separator.
> 
> > , and number+white-space+n is illegal for a space in-between.
> 
> Yep.
Comment 16 Kang-Hao (Kenny) Lu [:kennyluck] 2012-08-12 01:15:09 PDT
(In reply to Thomasy from comment #15)
> > "span:nth-child(-1/**/n+10)" but these two seems like a bug to me.
> The proposed patch _do_ support "span:nth-child(-1/**/n+10)" but I will look
> into why it not work for nth-child(+1/**/n-1) and span:nth-child(1/**/n-1)

Oh, yeah. My statement was exactly the opposite. Sorry about that.
 
> It is something around the code below, any idea?

Yes. :)

> +    }else {
> +      if (eCSSToken_Ident == mToken.mType &&
> mToken.mIdent.LowerCaseEqualsLiteral("n")) {
> +        numbers[0] = intValue;

The token list made of "-1/**/n-1" is NUMBER(-1) IDENT(n-1), not NUMBER(-1) IDENT(n) NUMBER(-1) like some people might think. CSS tokenization is quite weird here, but get over it.

"-1/**/n+10" makes NUMBER(-1) IDENT(n) NUMBER(+10) because the plus sign is not a identifier character.
Comment 17 Thomasy 2012-08-12 03:48:27 PDT
Created attachment 651181 [details] [diff] [review]
Fix for 1/**/n-10  and  -1/**/n-10  case

Get it. Fix for  nth-child(+1/**/n-1) and nth-child(1/**/n-1) case.
Comment 18 Josh Matthews [:jdm] 2013-04-24 12:21:39 PDT
David, could you address the open review in some fashion?
Comment 19 Simon Sapin (:SimonSapin) 2013-05-27 19:48:53 PDT
I’d wait for CSS WG to figure out exactly what syntax we want to allow or not before making our implementation conform to a spec that might change.
Comment 20 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2013-05-27 20:15:12 PDT
I'm actually inclined the other way -- I'm nearly done reviewing, and I think we should probably take the patch now.  I think likely all of these changes will end up matching what the WG agrees, and all but one of them (making +n work) are obscure enough we can revert them later if needed.

(While reviewing, I also noticed bug 543151 comment 81.)
Comment 21 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2013-05-27 20:23:05 PDT
Comment on attachment 651181 [details] [diff] [review]
Fix for 1/**/n-10  and  -1/**/n-10  case

r=dbaron with these blank lines removed:

>+      }
>+      
>+    }
>+    

and the trailing whitespace after some of the test lines removed, and some additional tests added (mostly to test the n-... cases as much as the n+... cases).

I have the patch merged and fixed up locally; I'll land on mozilla-inbound after I'm confident I've added the necessary test coverage.



Sorry for taking so long to get to this review; I really messed up there.
Comment 22 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2013-05-27 20:44:05 PDT
Also, for reference, I was looking at three different specs:
http://www.w3.org/TR/css3-selectors/#nth-child-pseudo
http://dev.w3.org/csswg/selectors4/#anb
http://dev.w3.org/csswg/css-syntax/#anb
Comment 23 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2013-05-27 21:00:15 PDT
While writing the additional tests, I found an additional (existing) bug and filed bug 876570.
Comment 24 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2013-05-27 21:13:27 PDT
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/91ba33ab8437
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/47870c0ef43b

Thanks again for the patch, and sorry for taking so long to get to it.

(I hope the commit message is accurate.  In the future, it's best to include commit messages on patches that you submit for review.)
Comment 26 Thomasy 2013-05-29 07:14:23 PDT
(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #24)
> remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/91ba33ab8437
> remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/47870c0ef43b
> 
> Thanks again for the patch, and sorry for taking so long to get to it.
> 
> (I hope the commit message is accurate.  In the future, it's best to include
> commit messages on patches that you submit for review.)

Get it. Thanks David Baron [:dbaron] for reviewing the patch.

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