667520 - querySelectorAll nth-child returns inaccurate results

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, defect, P1)

Description

[Brian Kardell]

Attachment: Numerous examples illustrating query

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:5.0) Gecko/20100101 Firefox/5.0
Build ID: 20110615151330

Steps to reproduce:

Was experiencing a problem in jquery, but it seems due to delegation to querySelector all. Created a test page and verified that CSS and querySelector all give me the same results for an nth-child(4n) style selector in other browsers, but not in FF 5.


Actual results:

CSS appears to match other browsers (and spec) but querySelectorAll results in different matches in FF5.


Expected results:

CSS and querySelectorAll should have the same result for the same selector.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Argh.  This is a bug in the nth-index cache.  What happens is that we only have  cached values for the <div>s and we end up adding 1 to the previous cached value we find.  That's broken, of course.

In Gecko 2.0, we only looked at the immediate previous sibling, so adding just 1 was correct.

We should get this fixed on aurora (Fx6) in addition to m-c, I think.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Fix nth index cache to work correctly when nth-child selectors are only applied to some elements of a child list.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: Merged to

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 542224 [details] [diff] [review]
Merged to

Wrong bug.

Comment 5

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

Comment on attachment 542219 [details] [diff] [review]
Fix nth index cache to work correctly when nth-child selectors are only applied to some elements of a child list.

r=dbaron

Comment 6

[Boris Zbarsky [:bzbarsky]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/5af231e7a58b

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 542219 [details] [diff] [review]
Fix nth index cache to work correctly when nth-child selectors are only applied to some elements of a child list.

Requesting approval to land this on aurora.  This fixes a regression in querySelector handling of the various nth-child and nth-of-type selectors (as well as handling of actual selector matching in some cases) that we unfortunately shipped in Firefox 5.  The fix is very very safe, and the old code is clearly wrong....

Comment 8

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

Comment on attachment 542219 [details] [diff] [review]
Fix nth index cache to work correctly when nth-child selectors are only applied to some elements of a child list.

approved for aurora (in triage meeting)

Comment 9

[Boris Zbarsky [:bzbarsky]]

http://hg.mozilla.org/releases/mozilla-aurora/rev/d8b00ee31575

Brian, thank you for reporting this!  The fix will ship in Firefox 6 in about 7 weeks.

Comment 10

[Joe Drew (not getting mail)]

http://hg.mozilla.org/mozilla-central/rev/5af231e7a58b

Comment 11

[Brian Kardell]

Thanks guys.

Comment 12

[Virgil Dicu [:virgil] [QA]]

Mozilla/5.0 (X11; Linux x86_64; rv:6.0) Gecko/20100101 Firefox/6.0
Mozilla/5.0 (X11; Linux x86_64; rv:7.0a2) Gecko/20110811 Firefox/7.0a2
Mozilla/5.0 (X11; Linux x86_64; rv:8.0a1) Gecko/20110807 Firefox/8.0a1

Verified on Ubuntu 11.04, MAC OS X 10.6, Windows 7, Windows XP with the attachment test case from the description. The results are now the same as other browsers (Opera, Chrome). The initial incorrect results on Firefox are no longer reproducible.

Setting status to verified Fixed.