Last Comment Bug 667520 - querySelectorAll nth-child returns inaccurate results
: querySelectorAll nth-child returns inaccurate results
Status: VERIFIED FIXED
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: P1 normal with 3 votes (vote)
: mozilla7
Assigned To: Boris Zbarsky [:bz]
:
Mentors:
Depends on:
Blocks: 598832
  Show dependency treegraph
 
Reported: 2011-06-27 10:51 PDT by Brian Kardell
Modified: 2011-08-11 06:26 PDT (History)
8 users (show)
bzbarsky: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
affected
+
fixed


Attachments
Numerous examples illustrating query (1.23 KB, text/html)
2011-06-27 10:51 PDT, Brian Kardell
no flags Details
Fix nth index cache to work correctly when nth-child selectors are only applied to some elements of a child list. (3.71 KB, patch)
2011-06-27 11:57 PDT, Boris Zbarsky [:bz]
dbaron: review+
dbaron: approval‑mozilla‑aurora+
Details | Diff | Review
Merged to (6.94 KB, patch)
2011-06-27 12:08 PDT, Boris Zbarsky [:bz]
no flags Details | Diff | Review

Description Brian Kardell 2011-06-27 10:51:44 PDT
Created attachment 542207 [details]
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 [:bz] 2011-06-27 11:14:32 PDT
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 [:bz] 2011-06-27 11:57:51 PDT
Created 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.
Comment 3 Boris Zbarsky [:bz] 2011-06-27 12:08:10 PDT
Created attachment 542224 [details] [diff] [review]
Merged to
Comment 4 Boris Zbarsky [:bz] 2011-06-27 12:10:11 PDT
Comment on attachment 542224 [details] [diff] [review]
Merged to

Wrong bug.
Comment 5 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2011-06-27 14:57:31 PDT
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 7 Boris Zbarsky [:bz] 2011-06-28 08:26:34 PDT
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-7 (review requests must explain patch) 2011-06-28 14:56:03 PDT
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 [:bz] 2011-06-28 18:12:34 PDT
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) 2011-06-28 18:56:14 PDT
http://hg.mozilla.org/mozilla-central/rev/5af231e7a58b
Comment 11 Brian Kardell 2011-06-29 06:32:19 PDT
Thanks guys.
Comment 12 Virgil Dicu [:virgil] [QA] 2011-08-11 06:26:06 PDT
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.

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