Last Comment Bug 75375 - support for :nth-*() pseudo-classes
: support for :nth-*() pseudo-classes
Status: VERIFIED FIXED
[Hixie-PF][not needed for 1.9]
: css3, dev-doc-complete, late-l10n, verified1.9.1
Product: Core
Classification: Components
Component: CSS Parsing and Computation (show other bugs)
: Trunk
: All All
: P1 enhancement with 55 votes (vote)
: mozilla1.9.1a1
Assigned To: David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch)
:
Mentors:
: 271436 419274 419276 (view as bug list)
Depends on: 75374 401291
Blocks: 65133 acid3 419276
  Show dependency treegraph
 
Reported: 2001-04-10 07:01 PDT by Daniel Glazman (:glazou)
Modified: 2011-08-25 08:03 PDT (History)
93 users (show)
roc: wanted‑next+
roc: wanted1.9.1+
roc: blocking1.9-
dbaron: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch showing work in progress (24.23 KB, patch)
2001-04-23 03:05 PDT, Daniel Glazman (:glazou)
no flags Details | Diff | Splinter Review
add string (1.90 KB, patch)
2008-03-07 23:27 PST, David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch)
bzbarsky: review+
bzbarsky: superreview+
mbeltzner: approval1.9+
Details | Diff | Splinter Review
pre-patch #1: rename nsAtomStringList to nsPseudoClassList, and remove an unused constructor (8.04 KB, patch)
2008-03-08 15:10 PST, David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch)
bzbarsky: review+
bzbarsky: superreview+
Details | Diff | Splinter Review
pre-patch #2: make nsPseudoClassList store integer pairs too (12.04 KB, patch)
2008-03-08 15:11 PST, David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch)
no flags Details | Diff | Splinter Review
patch (30.22 KB, patch)
2008-03-08 15:14 PST, David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch)
no flags Details | Diff | Splinter Review
pre-patch #2: make nsPseudoClassList store integer pairs too (12.16 KB, patch)
2008-03-09 18:09 PDT, David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch)
bzbarsky: review+
bzbarsky: superreview+
Details | Diff | Splinter Review
patch (31.38 KB, patch)
2008-03-27 10:49 PDT, David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch)
bzbarsky: review+
bzbarsky: superreview+
Details | Diff | Splinter Review
pre-patch #2: make nsPseudoClassList store integer pairs too (12.50 KB, patch)
2008-04-05 00:03 PDT, David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch)
no flags Details | Diff | Splinter Review
patch (32.10 KB, patch)
2008-04-07 21:02 PDT, David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch)
no flags Details | Diff | Splinter Review
patch (35.63 KB, patch)
2008-05-01 16:57 PDT, David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch)
no flags Details | Diff | Splinter Review

Description Daniel Glazman (:glazou) 2001-04-10 07:01:21 PDT
Add support for  :nth-child(), :nth-last-child(), :nth-of-type(),
:nth-last-of-type() pseudo-classes.

Reference material : http://www.w3.org/TR/css3-selectors/
Comment 1 Daniel Glazman (:glazou) 2001-04-23 03:05:32 PDT
Created attachment 31818 [details] [diff] [review]
patch showing work in progress
Comment 2 Daniel Glazman (:glazou) 2001-04-23 03:08:01 PDT
WORK IN PROGRESS : patch showing a way to add support for the new css 3 pseudos

  :nth-child()
  :nth-last-child()
  :nth-of-type()
  :nth-last-of-type()

I did not try to optimize for the moment so all your suggestions and comments
are **highly** welcome.

Just an extra note :  all relevant tests available at

    http://www.geocities.com/glazou_2000/css3-modsel-tests/index.htm

are green.

Comment 3 Daniel Glazman (:glazou) 2001-04-23 04:01:13 PDT
Just forgot to say that the patch added here contains all the modifications for
bug 75374... I did not have the will/courage to split everything.
Comment 4 rubydoo123 2001-05-03 10:58:21 PDT
moving this out to future to get it off the un-milstoned list, move it back to 
the appropriate milestone when you are ready
Comment 5 Jacob Kjome 2001-08-02 22:51:33 PDT
I can't wait to be able to use this stuff!  The closest thing I can do 
to "dynamically" color alternate table rows is:

tr:first-child {
  color: inherit;
  background: white none;
}
/* Good for 6 rows including the :first-child above */
tr + tr, tr + tr + tr + tr, tr + tr + tr + tr + tr + tr { color: inherit; 
background: green none; }
tr + tr + tr, tr + tr + tr + tr + tr { color: inherit; background: orange 
none; } 

but that is so damned limited and ugly.

This stuff is going to be so cool!

Comments on this seem to have stopped in April/May.  Is this still being worked 
on?  If so, any idea when we might start being able to use it???

Jake
Comment 6 Jonas Sicking (:sicking) PTO Until July 5th 2001-09-16 19:19:54 PDT
Please try to make this work for dynamic content too. There are currently bugs 
with the implemented structural pseudo-classes and dynamic content, see bug 
73586 and bug 98997
Comment 7 Jacob Kjome 2002-03-14 01:23:54 PST
Can we get this working for 1.0, please!  This would just be so infinitely 
useful.

Jake
Comment 8 Max Kanat-Alexander 2003-06-20 21:10:48 PDT
This might just be one of the most useful new features in CSS3, as far as the
current web developer is concerned. Even just :nth-child(even) and
:nth-child(odd) (if they were made known) might actually encourage major
Intranet use of Mozilla, which can be key for adoption. I can't even begin to
recount all the times I've seen 

There are perhaps tens of thousands of intranet and Internet pages that would
benefit greatly from Mozilla implementing this CSS3 selector.

I think that perhaps this should be moved from "Future" to Mozilla 1.6alpha,
which, based on the roadmap, seems like a good place for it.

Of course, anyone can feel free to implement it earlier...

So, what's the status?

-M
Comment 9 Ernest Cline 2004-01-07 15:28:54 PST
Is there any possibility that the priority of this bug could get bumped up?

If :nth-child() in particular were implemented, then it could be used as a
workaround for Bug 915, which for many users is one of the most irriating bugs
of Mozilla and one of the top 50 as far votes and dupes are considered.
Assuming that the table does not use the rowspan or colspan attributes (or at
least doesn't use them very much) then the selector "tr > *:nth-child(k)" could
be used to supply styling to the kth column.  Since fixing Bug 915 is likely to
require a major effort to fix (as it will require going outside the rules of CSS
inheritance to fix) I doubt that it will get fixed anytime soon.

The selector "tr > :first-child + * + * + ... + *"  where there are k-1
occurences of "+ *" is a substitute, but hardly an elegant one, particularly for
large tables.
Comment 10 Frank Fridlund 2004-01-26 16:03:50 PST
Hmmm, I was just browsing the W3C tests (at
http://www.w3.org/Style/CSS/Test/CSS3/Selectors/current/xhtml/full/flat/index.html
) and I found that the :nth-child and the :nth-last-child does apparantly works
somewhat on Firebird (Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.5)
Gecko/20031007 Firebird/0.7).  The tests that Firebird pass relevant to the
:nth-child class are #28b and #29b, although it doesn't pass all the tests
related to this bug (particularly #28-#31).
Comment 11 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2004-01-26 16:05:42 PST
A single test for a feature passing doesn't imply that any part of the feature
is implemented.  Only all the tests passing does.
Comment 12 Hixie (not reading bugmail) 2004-01-26 16:08:15 PST
Those are the error handling tests checking that browsers don't mis-parse the
selectors (for UAs that don't support them at all) or don't apply them to too
much (for those that do). So all UAs should pass those particular tests.
Comment 13 Kevin Newman 2004-09-07 15:10:35 PDT
I just wanted to point out that IE7 (http://dean.edwards.name/IE7/) currently
supports this selector, and does so (probably) so that the guy that added it can
use it in his intranet app ;-) .
Comment 14 William J. Edney 2004-09-07 17:18:24 PDT
As 'the guy' mentioned previously, I guess I should respond :-).

I added :nth-child() capability to the IE7 project partly as a programming 
challenge and partly as functionality needed for coloring alternating table 
rows, etc. which I used to do manually before I saw the power of this selector.

The IE7 code isn't perfect (still a few bugs, need to implement the 'of type' 
ones), but I'm committed to making it work. Being a big Mozilla supporter, I'd 
love to see this work for Mozilla.

How about it guys? You've got C at your disposal - I only had JS :-).

I'd offer a case of beer to Daniel as I've done for Mozilla bug fixage in the 
past (still owe jst for onbeforeunload() :-) ), but since Daniel's in France, 
maybe he'd prefer wine instead?

Cheers,

- Bill
Comment 15 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2004-09-07 18:09:35 PDT
Implementing it is easy.  Getting it to dynamically update when the content tree
changes is hard -- and that's particularly important for the *last-child
variants, since the content tree arrives in pieces.
Comment 16 William J. Edney 2004-09-08 07:34:17 PDT
Hmmm... sounds nasty.

We don't really deal with this situation on IE7 since the user must manually
make a call to update the document after changing the markup structure.

Ok, so maybe this would need 2 cases of beer (wine?) :-)

Cheers,

- Bill
Comment 17 Ernest Cline 2004-09-08 08:16:32 PDT
Might I suggest that during the initial build from the content tree that the
various *-last-* variants simply not be applied until the end tag for the parent
has been reveived?  Since I don't know the code, I can't say if thsi would be
easy to implement or if the code would need severe modification to do this. 
This would still cause problems for content generated dynamically client side,
but that's a secondary problem and if it can be reduced to simply a performance
issue for dynamic content, it could be addressed by a note to content developers
suggesting that they deactivate *-last-* rules befre making a lot of changes and
then reactivating the rules once they are done.
Comment 18 Hixie (not reading bugmail) 2004-09-08 10:42:56 PDT
That would just be a hack that doesn't solve the true problem. (The true 
solution would also address handling DOM changes, :hover rules, etc.)
Comment 19 Ernest Cline 2004-09-09 05:57:07 PDT
It might be just a hack, but it would also speed document rendering on initial
load, which to my mind is the most important case (but not the only important
case), so it would be a hack that would be useful even after dynamic handling
were  optimized in other ways.  Besides, does it even make logical sense to
apply *-last-* rules until we know where the end of the parent is?
Comment 20 Richard Brodie 2004-11-23 12:55:22 PST
*** Bug 271436 has been marked as a duplicate of this bug. ***
Comment 21 Neil Skrypuch 2007-04-29 11:52:55 PDT
Any progress on this? There's been no comments in almost 3 years...
Comment 22 YUKI "Piro" Hiroshi 2007-10-26 11:47:56 PDT
How about converting the selector to XPath internallay? It will help you to
collect target elements or verify the element is matched to the selector
or not. It is a major method for JavaScript developers to use XPath as
complex conditions finding elements. Actually, my Firefox extension
"Text Shadow ( https://addons.mozilla.org/firefox/addon/5410 )" uses it.

This is an example to convert CSS3 ":nth-***" selectors to a location step of XPath expressions, from "Text Shadow".


function getXPathFragment(target) {
  var stepNamePart;
  var tagName = target.split(':')[0];
  var pseud   = target.split(':')[1];
  if (tagName && tagName != '*') {
    stepNamePart = '*[local-name() = "'+tagName.toLowerCase()+'"'+
                      ' or local-name() = "'+tagName.toUpperCase()+'"]';
  }
  else {
    stepNamePart = tagName || '*';
  }

  var axis = /^nth-last-/.test(pseud) ?
               'following-sibling' : 'preceding-sibling' ;
  var condition = /^nth-(last-)?of-type/.test(pseud) ?
               stepNamePart : '*' ;

  var nthPattern;

  if (/nth-(last-)?(child|of-type)\(\s*([0-9]+)\s*\)/.test(pseud)) {
    nthPattern = 'count('+axis+'::'+condition+') = '+
                   (parseInt(RegExp.$3)-1);
  }
  else if (/nth-(last-)?(child|of-type)\(\s*([0-9]+)n\s*(([\+\-])\s*([0-9]+)\s*)?\)/.test(pseud)) {
    nthPattern = '(count('+axis+'::'+condition+')'+
                   (RegExp.$6 ? ' '+RegExp.$5+' '+RegExp.$6 : '' )+')'+
                   ' mod '+RegExp.$3+' = 1';
  }
  else if (/nth-(last-)?(child|of-type)\(\s*(odd|even)\s*\)/.test(pseud)) {
    nthPattern = 'count('+axis+'::'+condition+') mod 2 = '+
                   (RegExp.$3 == 'even' ? '1' : '0' );
  }

  return stepNamePart + '[' + nthPattern + ']';
}

alert(getXPathFragment('tr:nth-child(2n+1)'));
// => *[local-name() = "tr" or local-name() = "TR"][(count(preceding-sibling::*) + 1) mod 2 = 1]
Comment 23 Jonas Sicking (:sicking) PTO Until July 5th 2007-10-26 11:57:21 PDT
The problem isn't finding the correct elements. The problem is making the selector still work when the DOM is modified. If the first child matching the selector is removed a whole new set of elements are selected, and another set of elements are unselected.

Figuring out which, without making adding and removing children extremely slow, is the tricky part.

Reevaluating an XPath expression for every document mutation would be way too slow. And I say that having done a lot of work on the XPath engine.
Comment 24 William J. Edney 2007-10-26 12:24:15 PDT
Jonas -

Knowing nothing about how the reflow engine works, would the new reflow stuff in Moz 1.9 help the performance here, such that we could hold out hope for an implementation in the near future?

Maybe dbaron can chime in.

Having nth-* pseudoclasses is incredibly powerful.

Cheers,

- Bill
Comment 25 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2007-10-26 12:42:18 PDT
It's totally unrelated.  I have a plan for how to do what's needed in bug 73586 comment 22.
Comment 26 :Gavin Sharp [email: gavin@gavinsharp.com] 2008-03-06 08:18:47 PST
*** Bug 419274 has been marked as a duplicate of this bug. ***
Comment 27 Gabriele Best [:Gabri] 2008-03-07 07:24:14 PST
Will this bug fixed before Firefox 3.0.0.0 Release?
Comment 28 :Aryeh Gregor (away until August 15) 2008-03-07 10:29:44 PST
Robert O'Callahan has unset wanted1.9+ and set wanted-next+, so I interpret that as meaning it's too late for it to go into Gecko 1.9 (FF3), but it's wanted for the release after that.  I don't see any obvious documentation about the exact meanings of the flags, though.
Comment 29 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2008-03-07 11:12:20 PST
Re comment 27: probably not, and please don't ask in bugs when they will be fixed; it clutters them up with questions and makes it hard to find the useful bits.

Taking from Daniel, with his permission.  I think what needs to happen here is:

 * we use Daniel's parsing code (above), merged to trunk (I still need to review the details, though)

 * instead of the storage approach in the patch above, rename nsAtomStringList to nsPseudoClassList (since that patch, we have a generic mechanism for pseudo-classes with arguments, but it's for string arguments) and then extend nsPseudoClassList to take integer pairs as well

 * change the matching code to cache more information in SelectorMatchesData
Comment 30 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2008-03-07 23:27:57 PST
Created attachment 308111 [details] [diff] [review]
add string
Comment 31 Boris Zbarsky [:bz] 2008-03-07 23:29:32 PST
Comment on attachment 308111 [details] [diff] [review]
add string

Looks good.
Comment 32 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2008-03-07 23:33:00 PST
Comment on attachment 308111 [details] [diff] [review]
add string

I want to get this string in now (i.e., next 28 minutes) in case the post-1.9 releases plan involves features slipped in to 1.9.0.x releases (an idea that first appeared on dev-planning in the past day or two).

So I'm just going to land it shortly, but requesting after-the-fact approval.
Comment 33 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2008-03-08 15:10:51 PST
Created attachment 308228 [details] [diff] [review]
pre-patch #1: rename nsAtomStringList to nsPseudoClassList, and remove an unused constructor
Comment 34 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2008-03-08 15:11:32 PST
Created attachment 308230 [details] [diff] [review]
pre-patch #2: make nsPseudoClassList store integer pairs too

Applies on top of attachment 308228 [details] [diff] [review].
Comment 35 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2008-03-08 15:14:40 PST
Created attachment 308231 [details] [diff] [review]
patch

This makes some assumptions about the various questions I sent to www-style in the past few days: http://lists.w3.org/Archives/Public/www-style/2008Mar/

It basically assumes that everything in the examples is intended to be syntactically valid, assumes that :nth-child() is not valid (both as Daniel's patch did).  But it changes to allowing no whitespace at all inside :nth-child(), which matches KHTML and WebKit trunk, although that's still open for discussion.

I had to expose the scanner's Pushback method to the parser to get :nth-child(2n-1) or :nth-child(n-1) to parse, since "2n-1" is a dimension and "n-1" is an identifier.  (That was by far the easiest way, anyway.)  That was a bug in Daniel's parsing code.
Comment 36 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2008-03-09 18:09:38 PDT
Created attachment 308353 [details] [diff] [review]
pre-patch #2: make nsPseudoClassList store integer pairs too

Updated with improved serialization (thanks partly to the serialization tests in layout/style/test/test_selectors.html that I'm landing right now).
Comment 37 Mike Beltzner [:beltzner, not reading bugmail] 2008-03-10 08:17:26 PDT
Comment on attachment 308111 [details] [diff] [review]
add string

post-hoc-a
Comment 38 Gabriele Best [:Gabri] 2008-03-10 10:20:18 PDT
Will these patch included in nightly builds?
If not, can someone do it in a test build?
Comment 39 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2008-03-22 23:56:36 PDT
*** Bug 419274 has been marked as a duplicate of this bug. ***
Comment 40 Boris Zbarsky [:bz] 2008-03-23 12:33:19 PDT
Note to self: I tried replacing this part:

+          const PRInt32 n = (index - b) / a;
+          result = n >= 0 && (a * n == index - b);

with:

  div_t division_result = div(index - b, a);
  result = division_result.rem == 0 && division_result.quot >= 0;

because shark was flagging those two lines as a hotspot in a profile I was doing, but it doesn't seem to be faster, and might even be a little slower.  The function call wasn't being inlined, for one thing.
Comment 41 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2008-03-27 10:49:46 PDT
Created attachment 312072 [details] [diff] [review]
patch

This version of the patch allows whitespace in more places (still hasn't fully been decided by the WG, I think) and has a few more tests.
Comment 42 Boris Zbarsky [:bz] 2008-04-01 13:04:00 PDT
I'm a little worried by one thing here.  Consider a table with a bunch of rows, and the following selector:

  tr:nth-child(even)

This will involve computing the nth index for every row, and since the computation is O(N) in number of previous siblings, the overall computation would be O(N^2) in number of rows.  At the same time, during pageload we would be resolving style on a bunch of rows at once, and on any mutation we would restyle all the rows.  In all situations, we will have resolved style on earlier rows before resolving style on later ones.

Would it make sense to either cache the resulting RuleProcessorDatas or nth indices or something?

This still wouldn't help with :nth-last-child(even), but that seems like a rarer case...
Comment 43 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2008-04-01 13:10:56 PDT
I was thinking of something like that as a followup optimization -- i.e., passing data between sibling RuleProcessorData objects (perhaps even via an LRU cache).  But I don't think it needs to be part of this patch.
Comment 44 Boris Zbarsky [:bz] 2008-04-01 13:29:00 PDT
Yeah, I agree that optimizing that can be a followup.
Comment 45 dolphinling 2008-04-02 15:05:38 PDT
*** Bug 419276 has been marked as a duplicate of this bug. ***
Comment 46 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2008-04-02 16:37:39 PDT
Comment on attachment 312072 [details] [diff] [review]
patch

I think my proposal in http://lists.w3.org/Archives/Public/www-style/2008Mar/0111.html was adopted at the CSS face-to-face meeting (my memory is a little fuzzy, but fantasai confirmed on IRC), so this should be good to go.
Comment 47 Boris Zbarsky [:bz] 2008-04-03 21:15:19 PDT
Comment on attachment 308228 [details] [diff] [review]
pre-patch #1: rename nsAtomStringList to nsPseudoClassList, and remove an unused constructor

r+sr=bzbarsky
Comment 48 Boris Zbarsky [:bz] 2008-04-03 21:31:00 PDT
Comment on attachment 308353 [details] [diff] [review]
pre-patch #2: make nsPseudoClassList store integer pairs too

>+nsPseudoClassList::nsPseudoClassList(nsIAtom* aAtom)
>+  // We can't assert that it doesn't have a string arg
>+  NS_ASSERTION(!nsCSSPseudoClasses::HasStringArg(aAtom) &&
>+               !nsCSSPseudoClasses::HasNthPairArg(aAtom),
>+               "unexpected pseudo-class");

I'm not sure how to reconcile the code and comment there....  see below.

>+  nsPseudoClassList *result = new nsPseudoClassList(mAtom);
>+  if (u.mMemory) {
>+    if (nsCSSPseudoClasses::HasStringArg(mAtom)) {
>+      result->u.mString = NS_strdup(u.mString);
>+    } else {
>+      NS_ASSERTION(nsCSSPseudoClasses::HasNthPairArg(mAtom), "unexpected pseudo-class");
>+      result->u.mNumbers = static_cast<PRInt32*>(
>+        nsMemory::Clone(u.mNumbers, sizeof(PRInt32) * 2));
>+    }
>+  }

I would somewhat prefer just using different constructors here for the different cases instead of duplicating the cloning code...  So something like:

  if (!u.mMemory) {
    result = new nsPseudoClassList(mAtom);
  } else if (nsCSSPseudoClasses::HasStringArg(mAtom)) {
    result = new nsPseudoClassList(mAtom, u.mString);
  } else {
    NS_ASSERTION(nsCSSPseudoClasses::HasNthPairArg(mAtom),
                 "unexpected pseudo-class");
    result = new nsPseudoClassList(mAtom, u.mNumbers);
  }

Then we could leave the above assert in the bare-atom constructor and remove the "can't assert" comment, right?

>+++ b/layout/style/nsICSSStyleRule.h
>+  union {
>+    void*           mMemory; // all pointer types use NS_Alloc/NS_Free
>+    // Which of the following is used depend on mAtom.

Worth documenting that for some mAtoms this is guaranteed null.

With those changes, r+sr=bzbarsky
Comment 49 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2008-04-03 22:11:45 PDT
(In reply to comment #48)
> (From update of attachment 308353 [details] [diff] [review])
> >+nsPseudoClassList::nsPseudoClassList(nsIAtom* aAtom)
> >+  // We can't assert that it doesn't have a string arg
> >+  NS_ASSERTION(!nsCSSPseudoClasses::HasStringArg(aAtom) &&
> >+               !nsCSSPseudoClasses::HasNthPairArg(aAtom),
> >+               "unexpected pseudo-class");
> 
> I'm not sure how to reconcile the code and comment there....  see below.

Ah, right.  When I started out I was trying to handle the tree pseudos that had optional string arguments... but at some point I realized that those where all pseudo-*elements* rather than pseudo-classes so I didn't need to worry.  I suppose I didn't quite undo all the damage from that.

The other changes suggested sound fine at first glance -- I'll look more closely when I make them.
Comment 50 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2008-04-05 00:03:15 PDT
Created attachment 313775 [details] [diff] [review]
pre-patch #2: make nsPseudoClassList store integer pairs too

This should address bzbarsky's comments.
Comment 51 Boris Zbarsky [:bz] 2008-04-06 22:09:46 PDT
Comment on attachment 312072 [details] [diff] [review]
patch

>+++ b/layout/style/nsCSSParser.cpp
>@@ -2767,6 +2778,7 @@ CSSParserImpl::ParsePseudoClassWithIdent
>+    // XXX Call SkipUntil to the next ")"?

File a followup to investigate this, and the other such XXX comments here?

>+CSSParserImpl::ParsePseudoClassWithNthPairArg(nsCSSSelector& aSelector,
>+  PRInt32 numbers[2];
>+  numbers[0] = 0;
>+  numbers[1] = 0;

Why not

  PRInt32 numbers[2] = { 0, 0 };
  
?

>+  // Check if we have the first parenthesis

s/if/whether/

>+    // The '+' or '-' sign can separated by whitespace or not.

"can be optionally separated by whitespace"?

>+++ b/layout/style/nsCSSRuleProcessor.cpp
>+RuleProcessorData::GetNthIndex(PRBool aIsOfType, PRBool aIsFromEnd)
>+    nsIContent* child = parent->GetChildAt(cur);
>+    NS_ASSERTION(child, "couldn't find node in its parent's child list");

I don't think this is assertable as written.  mContent could be anonymous, in which case we'd walk off the parent's child list in this loop.

We should probably just go ahead and IndexOf() on the kid to check that it's in fact in the parent's child list.

I'm not sure which behavior we want here when it's not.  Simplest seems to be to not match these selectors for anonymous content subtree roots, just like we don't match :first-child.

Given the future optimization thoughts, it might also make sense to write this loop with the walk starting at mContent's index (which we're having to get anyway, for the anonymous content check) and walk to the edge of the parent's child list.  That way when we go to optimize we can optimize by breaking out of the loop early.  Either way works fine for purposes of this patch, though.

>+    if (nsStyleUtil::IsSignificantChild(child, PR_FALSE, PR_FALSE) &&

Would it make sense to just explicitly do a IsContentOfType(eELEMENT) here?  IsSignificantChild() makes three IsContentOfType() calls that end up being equivalent to that one call, given that this is an nsIContent.  Perhaps we should siply change IsSignificantChild() to do an eELEMENT check up front instead...  But that still seems like an unnecessary function call here.

>@@ -1135,6 +1186,43 @@ static PRBool SelectorMatches(RuleProces
>+        // Integer division in C does truncation towards 0.  So check
>+        // that the result is nonnegative, and that there was no
>+        // truncation.

The truncation direction doesn't seem relevant, and mentioning it confused me for a bit as I searched through my C books to verify the direction for negative numbers..  We really just want to check that |index-b)| is evenly divisible by |a| and that the quotient is nonnegative, right?  I'd drop mention of the direction in this comment.  And perhaps move it into the a != 0 "else" case.

>+          const PRInt32 n = (index - b) / a;
>+          result = n >= 0 && (a * n == index - b);

Perhaps put |index-b| into a local?

>+++ b/layout/style/nsCSSScanner.cpp
>@@ -983,7 +983,8 @@ PRBool nsCSSScanner::ParseNumber(nsresul
>+  // Set mIntegerValid for all cases (except %, below) because we need
>+  // it for the "2n" in :nth-child(2n).

So this makes % parsing a tad slower due to the extra ToInteger() call, right?  I guess that should be OK.

STILL NEED TO REVIEW THE TESTS....

>+++ b/layout/style/test/test_of_type_selectors.xhtml

>+        for (var i in match_indices) {
>+            var e = elements[match_indices[i]];

Could use |for each (var i in match_indices)| to avoid having to do the indexing yourself.  Either way.

>+        for (var i in notmatch_indices) {

This redeclares 'i'.  Probably doesn't matter much, in any case.

I assume the reason this isn't in test_selectors.html is because of the namespace usage?

>+++ b/layout/style/test/test_selectors.html

>-        ifdoc.body.innerHTML = body_contents;
>+        if (typeof(body_contents) == "string") {

This looks fine, but I don't see your new tests using it.  Is it left over from an earlier iteration?

>+    // :nth-child(), etc.

Can you add:

  test_balanced_unparseable(":nth-child(2-n)");
  test_balanced_unparseable(":nth-child(2-n-1)");
  test_balanced_unparseable(":nth-child(n-2-1)");

?  Those took me some work in terms of sorting through the parsing code and why it discards them...

r+sr=bzbarsky once the anonymous content issue is fixed.  And perhaps we should add a test for that.
Comment 52 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2008-04-07 21:01:39 PDT
(In reply to comment #51)
> (From update of attachment 312072 [details] [diff] [review])
> >+    // XXX Call SkipUntil to the next ")"?
> 
> File a followup to investigate this, and the other such XXX comments here?

Filed bug 427688.

> "can be optionally separated by whitespace"?

I chose "can optionally be separated", which I think is clearer.

> >+++ b/layout/style/nsCSSRuleProcessor.cpp
> >+RuleProcessorData::GetNthIndex(PRBool aIsOfType, PRBool aIsFromEnd)
> >+    nsIContent* child = parent->GetChildAt(cur);
> >+    NS_ASSERTION(child, "couldn't find node in its parent's child list");
> 
> I don't think this is assertable as written.  mContent could be anonymous, in
> which case we'd walk off the parent's child list in this loop.
> 
> We should probably just go ahead and IndexOf() on the kid to check that it's in
> fact in the parent's child list.

I don't see the point in calling IndexOf -- anonymous content is relatively rare (and particularly so on elements with lots of children).  We may as well just walk the whole list and find out that it's not there.

So what I did is I made GetNthIndex return 0 when a node is not in its parent's child list (it's using 1-based indices, after all).  And I changed the special value for completely uninitialized to -2.  (I left -1 as the half-uninitialized value introduced by the patch in bug 12858.)

> I'm not sure which behavior we want here when it's not.  Simplest seems to be
> to not match these selectors for anonymous content subtree roots, just like we
> don't match :first-child.

Agreed.  That's what I've done (I think).  (Does this problem go away with XBL2?)

> Given the future optimization thoughts, it might also make sense to write this
> loop with the walk starting at mContent's index (which we're having to get
> anyway, for the anonymous content check) and walk to the edge of the parent's
> child list.  That way when we go to optimize we can optimize by breaking out of
> the loop early.  Either way works fine for purposes of this patch, though.

I left things as they are, given the changes in bug 128585.

> >+    if (nsStyleUtil::IsSignificantChild(child, PR_FALSE, PR_FALSE) &&
> 
> Would it make sense to just explicitly do a IsContentOfType(eELEMENT) here? 
> IsSignificantChild() makes three IsContentOfType() calls that end up being
> equivalent to that one call, given that this is an nsIContent.  Perhaps we
> should siply change IsSignificantChild() to do an eELEMENT check up front
> instead...  But that still seems like an unnecessary function call here.

I replaced the IsSignificantChild call with an IsContentOfType(eELEMENT) call, which seems more clearly correct to me as well.

> >@@ -1135,6 +1186,43 @@ static PRBool SelectorMatches(RuleProces
> >+        // Integer division in C does truncation towards 0.  So check
> >+        // that the result is nonnegative, and that there was no
> >+        // truncation.
> 
> The truncation direction doesn't seem relevant, and mentioning it confused me
> for a bit as I searched through my C books to verify the direction for negative
> numbers..  We really just want to check that |index-b)| is evenly divisible by
> |a| and that the quotient is nonnegative, right?  I'd drop mention of the
> direction in this comment.  And perhaps move it into the a != 0 "else" case.

I moved the comment and put the "towards 0" in parentheses.

> >+          const PRInt32 n = (index - b) / a;
> >+          result = n >= 0 && (a * n == index - b);
> 
> Perhaps put |index-b| into a local?

I'd rather not; compilers should be able to optimize and I think it's clearer without.

> >-        ifdoc.body.innerHTML = body_contents;
> >+        if (typeof(body_contents) == "string") {
> 
> This looks fine, but I don't see your new tests using it.  Is it left over from
> an earlier iteration?

I never ended up using it, but I didn't see any reason to remove it, either.
Comment 53 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2008-04-07 21:02:38 PDT
Created attachment 314243 [details] [diff] [review]
patch

I think this addresses all of Boris's review comments (some as described in the previous comment) except for adding a test for the anonymous content issue.
Comment 54 Boris Zbarsky [:bz] 2008-04-08 09:57:12 PDT
> anonymous content is relatively rare (and particularly so on elements with lots 
> of children)

This last is probably true, yeah.

> Does this problem go away with XBL2?

I don't know.

In that last patch, don't you need to set |result| to PR_FALSE if index <= 0?
Comment 55 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2008-05-01 16:57:38 PDT
Created attachment 318935 [details] [diff] [review]
patch

This adds the test I mentioned in comment 53 and fixes the bug Boris pointed out in comment 54 (which causes the added test to fail).  I added the test in a separate file because I couldn't figure out how to deal with issues of waiting for a binding document to load within the framework of test_selectors.html.
Comment 56 David Baron :dbaron: ⌚️UTC+2 (mostly busy through August 4; review requests must explain patch) 2008-06-02 20:21:11 PDT
http://hg.mozilla.org/mozilla-central/index.cgi/rev/c5b6d415d822
http://hg.mozilla.org/mozilla-central/index.cgi/rev/6ea3a422f95e
http://hg.mozilla.org/mozilla-central/index.cgi/rev/dc3c9abdd272

->fixed (for the release after Firefox 3.0.*)
Comment 57 Gabriele Best [:Gabri] 2008-06-03 03:07:53 PDT
Why don't you include this on Firefox 3.1 ?
Comment 58 Jean-Yves Perrier [:teoli] 2008-06-03 04:47:03 PDT
Gabriele, the release after Fx 3.0.* is Fx 3.1... (Others, sorry for the spam)
Comment 59 Gabriele Best [:Gabri] 2008-06-03 05:12:03 PDT
ah ok, I had seen the target milestone that is mozilla2.
Sorry.
Comment 60 Damon Sicore (:damons) 2008-06-04 16:51:33 PDT
marking wanted1.9.1? to get this in the triage queue. 
Comment 62 Henrik Skupin (:whimboo) 2009-02-04 16:10:06 PST
Verified with the following builds by doing the W3C test:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b3pre) Gecko/20090204 Shiretoko/3.1b3pre Ubiquity/0.1.5 ID:20090204020327

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1b3pre) Gecko/20090204 Shiretoko/3.1b3pre

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