Closed Bug 75375 Opened 23 years ago Closed 16 years ago

support for :nth-*() pseudo-classes

Categories

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

enhancement

Tracking

()

VERIFIED FIXED
mozilla1.9.1a1

People

(Reporter: glazou, Assigned: dbaron)

References

Details

(4 keywords, Whiteboard: [Hixie-PF][not needed for 1.9])

Attachments

(5 files, 5 obsolete files)

24.23 KB, patch
Details | Diff | Splinter Review
1.90 KB, patch
bzbarsky
: review+
bzbarsky
: superreview+
beltzner
: approval1.9+
Details | Diff | Splinter Review
8.04 KB, patch
bzbarsky
: review+
bzbarsky
: superreview+
Details | Diff | Splinter Review
12.50 KB, patch
Details | Diff | Splinter Review
35.63 KB, patch
Details | Diff | Splinter Review
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/
Status: NEW → ASSIGNED
Depends on: 75374
Keywords: css3, qawanted
Whiteboard: [Hixie-PF]
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.

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.
moving this out to future to get it off the un-milstoned list, move it back to 
the appropriate milestone when you are ready
Priority: -- → P4
Target Milestone: --- → Future
Keywords: qawanted
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
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
Can we get this working for 1.0, please!  This would just be so infinitely 
useful.

Jake
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
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.
Summary: [CSS3] RFE : support for :nth-*() pseudo-classes → support for :nth-*() pseudo-classes
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).
A single test for a feature passing doesn't imply that any part of the feature
is implemented.  Only all the tests passing does.
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.
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 ;-) .
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
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.
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
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.
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.)
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?
*** Bug 271436 has been marked as a duplicate of this bug. ***
Flags: blocking1.9?
Flags: blocking1.9? → blocking1.9-
Whiteboard: [Hixie-PF] → [Hixie-PF][wanted-1.9]
Any progress on this? There's been no comments in almost 3 years...
QA Contact: ian → style-system
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]
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.
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
It's totally unrelated.  I have a plan for how to do what's needed in bug 73586 comment 22.
Flags: wanted1.9+
Whiteboard: [Hixie-PF][wanted-1.9] → [Hixie-PF]
Flags: blocking1.9?
Flags: wanted1.9+
Flags: wanted-next+
Flags: tracking1.9-
Flags: blocking1.9?
Flags: blocking1.9-
Will this bug fixed before Firefox 3.0.0.0 Release?
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.
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
Assignee: daniel → dbaron
Status: ASSIGNED → NEW
Priority: P4 → P1
Attached patch add stringSplinter Review
Attachment #308111 - Flags: superreview?(bzbarsky)
Attachment #308111 - Flags: review?(bzbarsky)
Comment on attachment 308111 [details] [diff] [review]
add string

Looks good.
Attachment #308111 - Flags: superreview?(bzbarsky)
Attachment #308111 - Flags: superreview+
Attachment #308111 - Flags: review?(bzbarsky)
Attachment #308111 - Flags: review+
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.
Attachment #308111 - Flags: approval1.9?
Attached patch patch (obsolete) — Splinter Review
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.
Updated with improved serialization (thanks partly to the serialization tests in layout/style/test/test_selectors.html that I'm landing right now).
Attachment #308230 - Attachment is obsolete: true
Comment on attachment 308111 [details] [diff] [review]
add string

post-hoc-a
Attachment #308111 - Flags: approval1.9? → approval1.9+
Will these patch included in nightly builds?
If not, can someone do it in a test build?
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.
Attached patch patch (obsolete) — Splinter Review
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.
Attachment #308231 - Attachment is obsolete: true
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...
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.
Yeah, I agree that optimizing that can be a followup.
Attachment #308228 - Flags: superreview?(bzbarsky)
Attachment #308228 - Flags: review?(bzbarsky)
Attachment #308353 - Flags: superreview?(bzbarsky)
Attachment #308353 - Flags: review?(bzbarsky)
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.
Attachment #312072 - Flags: superreview?(bzbarsky)
Attachment #312072 - Flags: review?(bzbarsky)
Comment on attachment 308228 [details] [diff] [review]
pre-patch #1: rename nsAtomStringList to nsPseudoClassList, and remove an unused constructor

r+sr=bzbarsky
Attachment #308228 - Flags: superreview?(bzbarsky)
Attachment #308228 - Flags: superreview+
Attachment #308228 - Flags: review?(bzbarsky)
Attachment #308228 - Flags: review+
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
Attachment #308353 - Flags: superreview?(bzbarsky)
Attachment #308353 - Flags: superreview+
Attachment #308353 - Flags: review?(bzbarsky)
Attachment #308353 - Flags: review+
(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.
This should address bzbarsky's comments.
Attachment #308353 - Attachment is obsolete: true
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.
Attachment #312072 - Flags: superreview?(bzbarsky)
Attachment #312072 - Flags: superreview+
Attachment #312072 - Flags: review?(bzbarsky)
Attachment #312072 - Flags: review+
(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.
Attached patch patch (obsolete) — Splinter Review
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.
Attachment #312072 - Attachment is obsolete: true
> 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?
Attached patch patchSplinter Review
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.
Attachment #314243 - Attachment is obsolete: true
Whiteboard: [Hixie-PF] → [Hixie-PF][not needed for 1.9]
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.*)
Status: NEW → RESOLVED
Closed: 16 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: Future → mozilla2.0
Why don't you include this on Firefox 3.1 ?
Blocks: acid3
Gabriele, the release after Fx 3.0.* is Fx 3.1... (Others, sorry for the spam)
ah ok, I had seen the target milestone that is mozilla2.
Sorry.
marking wanted1.9.1? to get this in the triage queue. 
Flags: wanted1.9.1?
Target Milestone: mozilla2.0 → mozilla1.9.1
Keywords: dev-doc-needed
Target Milestone: mozilla1.9.1 → mozilla1.9.1a1
Flags: wanted1.9.1? → wanted1.9.1+
Keywords: fixed1.9.1
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
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.