977500 - document.querySelectorAll NodeList items are sorted lexicographically instead of numerically

Status: RESOLVED FIXED

(DevTools :: Object Inspector, defect)

Description

[PK]

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

Comment 1

[Cork]

Please provide the html testcase not an image.

Comment 2

[PK]

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

Comment 3

[Cork]

(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.

Comment 4

[Cork]

Attachment: simplified testcase without any dom interaction

Comment 5

[PK]

(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.

Comment 6

[Sami Jaktholm]

Attachment: variablesview-dont-sort-nodelist.patch

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.

Comment 7

[Panos Astithas (he/him) [:past] (please ni?)]

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

Comment 8

[Sami Jaktholm]

Attachment: variablesview-dont-sort-nodelist-v2.patch

Here's a patch with the correct definition for "function* runner". Hopefully the long name doesn't cause any issues.

Comment 9

[Sami Jaktholm]

Attachment: variablesview-dont-sort-nodelist.patch

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.

Comment 10

[Panos Astithas (he/him) [:past] (please ni?)]

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.

Comment 11

[Panos Astithas (he/him) [:past] (please ni?)]

Try was green, landed:
https://hg.mozilla.org/integration/fx-team/rev/a8f1318174d0

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/a8f1318174d0