75375 - support for :nth-*() pseudo-classes

Status: VERIFIED FIXED

(Core :: CSS Parsing and Computation, enhancement, P1)

Description

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

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) (not active in Mozilla any more)]

Attachment: patch showing work in progress

Comment 2

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

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) (not active in Mozilla any more)]

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]

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]

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) No longer reading bugmail consistently]

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]

Can we get this working for 1.0, please!  This would just be so infinitely 
useful.

Jake

Comment 8

[Max Kanat-Alexander]

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]

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]

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-4, no longer working on Mozilla)]

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)]

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]

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]

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-4, no longer working on Mozilla)]

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]

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]

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)]

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]

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]

*** Bug 271436 has been marked as a duplicate of this bug. ***

Comment 21

[Neil Skrypuch]

Any progress on this? There's been no comments in almost 3 years...

Comment 22

[YUKI "Piro" Hiroshi]

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) No longer reading bugmail consistently]

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]

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-4, no longer working on Mozilla)]

It's totally unrelated.  I have a plan for how to do what's needed in bug 73586 comment 22.

Comment 27

[Gabriele Best [:Gabri]]

Will this bug fixed before Firefox 3.0.0.0 Release?

Comment 28

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

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-4, no longer working on Mozilla)]

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-4, no longer working on Mozilla)]

Attachment: add string

Comment 31

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 308111 [details] [diff] [review]
add string

Looks good.

Comment 32

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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-4, no longer working on Mozilla)]

Attachment: pre-patch #1: rename nsAtomStringList to nsPseudoClassList, and remove an unused constructor

Comment 34

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: pre-patch #2: make nsPseudoClassList store integer pairs too

Applies on top of attachment 308228 [details] [diff] [review].

Comment 35

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: 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-4, no longer working on Mozilla)]

Attachment: 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]]

Comment on attachment 308111 [details] [diff] [review]
add string

post-hoc-a

Comment 38

[Gabriele Best [:Gabri]]

Will these patch included in nightly builds?
If not, can someone do it in a test build?

Comment 40

[Boris Zbarsky [:bzbarsky]]

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-4, no longer working on Mozilla)]

Attachment: 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 [:bzbarsky]]

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-4, no longer working on Mozilla)]

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 [:bzbarsky]]

Yeah, I agree that optimizing that can be a followup.

Comment 46

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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 [:bzbarsky]]

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 [:bzbarsky]]

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-4, no longer working on Mozilla)]

(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-4, no longer working on Mozilla)]

Attachment: pre-patch #2: make nsPseudoClassList store integer pairs too

This should address bzbarsky's comments.

Comment 51

[Boris Zbarsky [:bzbarsky]]

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-4, no longer working on Mozilla)]

(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-4, no longer working on Mozilla)]

Attachment: 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 [:bzbarsky]]

> 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-4, no longer working on Mozilla)]

Attachment: 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-4, no longer working on Mozilla)]

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]]

Why don't you include this on Firefox 3.1 ?

Comment 58

[Jean-Yves Perrier [:teoli]]

Gabriele, the release after Fx 3.0.* is Fx 3.1... (Others, sorry for the spam)

Comment 59

[Gabriele Best [:Gabri]]

ah ok, I had seen the target milestone that is mozilla2.
Sorry.

Comment 60

[Damon Sicore (:damons)]

marking wanted1.9.1? to get this in the triage queue. 

Comment 61

[Eric Shepherd [:sheppy]]

These are now documented:

http://developer.mozilla.org/en/docs/CSS::nth-child
http://developer.mozilla.org/en/docs/CSS::nth-last-child
http://developer.mozilla.org/en/docs/CSS::nth-of-type
http://developer.mozilla.org/en/docs/CSS::nth-last-of-type

Comment 62

[Henrik Skupin [:whimboo][⌚️UTC+2]]

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