Closed Bug 750388 Opened 12 years ago Closed 11 years ago

Parsing of :nth-* pseudo-classes should accept strings starting with "+n"

Categories

(Core :: CSS Parsing and Computation, defect)

x86
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla24

People

(Reporter: bzbarsky, Assigned: thomas)

Details

(Whiteboard: [mentor=bz][mentor=kennyluck][lang=c++])

Attachments

(2 files, 3 obsolete files)

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.
Whiteboard: [mentor=bz] → [mentor=bz][lang=c++]
Can you drop a link to a file/name a function where this code lives?
(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.
Whiteboard: [mentor=bz][lang=c++] → [mentor=bz][mentor=kennyluck][lang=c++]
Yep, comment 2 is right on the money.  Sorry for not putting that in initially.
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
Thomas would like to take this bug, with help from Kenny.
Assign -> Thomas
Assignee: nobody → thomas
Status: NEW → ASSIGNED
Attached patch Patch for +n case (obsolete) — Splinter Review
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 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.
Oh, you should also obsolete your previous patch.
Attachment #650529 - Attachment is obsolete: true
Attachment #650828 - Attachment is obsolete: true
Attached patch Patch for a/**/n... case (obsolete) — Splinter Review
Update parse for INTEGER? {N} => eCSSToken_Number + eCSSToken_Ident or eCSSToken_Symbol + eCSSToken_Number + eCSSToken_Ident
Attachment #651092 - Flags: review?(dbaron)
(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
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.
Attachment #651118 - Attachment mime type: text/plain → text/html
(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.
(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.
(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.
Get it. Fix for  nth-child(+1/**/n-1) and nth-child(1/**/n-1) case.
Attachment #651092 - Attachment is obsolete: true
Attachment #651092 - Flags: review?(dbaron)
Attachment #651181 - Flags: review?(dbaron)
OS: Mac OS X → All
David, could you address the open review in some fashion?
Flags: needinfo?(dbaron)
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.
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 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.
Attachment #651181 - Flags: review?(dbaron) → review+
While writing the additional tests, I found an additional (existing) bug and filed bug 876570.
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.)
https://hg.mozilla.org/mozilla-central/rev/91ba33ab8437
https://hg.mozilla.org/mozilla-central/rev/47870c0ef43b
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
(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.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: