Closed Bug 977500 Opened 7 years ago Closed 7 years ago

document.querySelectorAll NodeList items are sorted lexicographically instead of numerically

Categories

(DevTools :: Object Inspector, defect)

27 Branch
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 32

People

(Reporter: pavel.kostenkov, Assigned: sjakthol)

Details

Attachments

(4 files, 2 obsolete files)

Attached image ffBug.jpg
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:27.0) Gecko/20100101 Firefox/27.0 (Beta/Release)
Build ID: 20140212131424

Steps to reproduce:

Create at least 11 DOM nodes. (30 on the screen but I guess this will work for any count)
Get a NodeList of these nodes either by document.querySelectorAll or $$ or jQuery (works for any of this tools).
In developer tools add a watch for those nodes or simply view that array.




Actual results:

Results numeration is wrong:
0,1,10,11,2,3 etc


Expected results:

0,1,2,3,4...,10,11 etc if more nodes.
Component: Untriaged → Developer Tools
Please provide the html testcase not an image.
Flags: needinfo?(pavel.kostenkov)
Component: Developer Tools → Developer Tools: Object Inspector
Summary: document.querySelectorAll NodeList items are sorted in improper way → document.querySelectorAll NodeList items are sorted lexicographically instead of numerically
Keywords: testcase-wanted
Attached file bug977500.html
Here's an example HTML - actually it might work with any doctype and html, i didn't check. But for this attachment it works anyway.
Flags: needinfo?(pavel.kostenkov)
(In reply to pavel.kostenkov from comment #2)
> Created attachment 8383774 [details]
> bug977500.html
> 
> Here's an example HTML - actually it might work with any doctype and html, i
> didn't check. But for this attachment it works anyway.

Ok, if you mean the overlay when you hover the variable, it doesn't look like this has anything to do with html at all and is just about the variable inspector.
(In reply to Cork from comment #3)
> (In reply to pavel.kostenkov from comment #2)
> > Created attachment 8383774 [details]
> > bug977500.html
> > 
> > Here's an example HTML - actually it might work with any doctype and html, i
> > didn't check. But for this attachment it works anyway.
> 
> Ok, if you mean the overlay when you hover the variable, it doesn't look
> like this has anything to do with html at all and is just about the variable
> inspector.

Yes, html does not matter much in the case. The case you've provided is using simple object - for object it's ok to not arrange its keys and I'm not sure why are the arranged for this case. Maybe this is another bug which is not related to pseudoarrays maybe not. For NodeList or jQuery collections they are pseudo-arrays and their numeric keys are expected to be arranged. 

I've tested the same behavior (both cases - simple object and NodeList or jQuery collection) - for Blink and IE the keys are 0,1,2,3,4....,10,11.
Here's a patch that fixes the issue for NodeList. It also contains a test case that ensures none of the specified array types (VariablesView.NON_SORTABLE_CLASSES in browser/devtools/shared/widget/VariablesView.jsm) are not sorted.
Attachment #8428364 - Flags: review?(past)
Comment on attachment 8428364 [details] [diff] [review]
variablesview-dont-sort-nodelist.patch

Review of attachment 8428364 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with a green try run.

::: browser/devtools/webconsole/test/browser.ini
@@ +142,5 @@
>  [browser_console_nsiconsolemessage.js]
>  [browser_console_optimized_out_vars.js]
>  [browser_console_private_browsing.js]
>  [browser_console_variables_view.js]
> +[browser_console_variables_view_dont_sort_non_sortable_classes_properties.js]

Hm, that is a really long file name.

::: browser/devtools/webconsole/test/browser_console_variables_view_dont_sort_non_sortable_classes_properties.js
@@ +44,5 @@
> +    </html>";
> +
> +  let jsterm;
> +
> +    function *runner() {

This should be: function* runner
Attachment #8428364 - Flags: review?(past) → review+
Assignee: nobody → sjakthol
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Here's a patch with the correct definition for "function* runner". Hopefully the long name doesn't cause any issues.
Here's a revised patch that calls the correct clean up function (finishTest instead of finish) in the attached test case. Sorry for the inconvenience.
Attachment #8428364 - Attachment is obsolete: true
Attachment #8428741 - Attachment is obsolete: true
Attachment #8430573 - Flags: review?(past)
Comment on attachment 8430573 [details] [diff] [review]
variablesview-dont-sort-nodelist.patch

Review of attachment 8430573 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the updated patch. I'll land it when the try run below finishes successfully:
https://tbpl.mozilla.org/?tree=Try&rev=f21f6a5e7a8d

::: browser/devtools/webconsole/test/browser_console_variables_view_dont_sort_non_sortable_classes_properties.js
@@ +75,5 @@
> +   * specified by aObject is inspected.
> +   *
> +   * @param string aObject
> +   *               A string once executed creates and returns the object to
> +   *               inspect.

Indentation is off in these 2 lines, should have been indented up to "string" in the line above. I'll take care of that before landing.
Attachment #8430573 - Flags: review?(past) → review+
Keywords: testcase-wanted
OS: Windows 7 → All
Hardware: x86_64 → All
https://hg.mozilla.org/mozilla-central/rev/a8f1318174d0
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 32
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.