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.

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.

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

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.

Update parse for INTEGER? {N} => eCSSToken_Number + eCSSToken_Ident or eCSSToken_Symbol + eCSSToken_Number + eCSSToken_Ident

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

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

David, could you address the open review in some fashion?

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.

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

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

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