925280 - Console object inspector shows wrong sort order for NodeList

Status: RESOLVED FIXED

(DevTools :: Object Inspector, defect)

Description

[piuccio]

Attachment: firefox bug.png

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/31.0.1650.8 Safari/537.36

Steps to reproduce:

* Open Developer Tools
* From the command line, use querySelectorAll to get a NodeList with at least 10 items
document.querySelectorAll("div")
* Click on the result [object NodeList]
* On the right side you'll see a list of nodes


Actual results:

They are displayed like this

0: [objectHTMLDivElement]
1:
10:
100:
101:
102:
..
11:
12:
13:
..
2:

sorted alphabetically


Expected results:

Even if NodeList is an Object and not an Array, it's iterable and Array-like (has the length property and keys are numbers), so I would expect a numeric sort

0:
1:
2:
3:
..
10:
11:

Comment 2

[mjh563]

Attachment: patch

So this is caused (not surprisingly) by object properties being sorted in lexicographic order. There are several ways it could be fixed:

OPTION 1: Stop sorting the properties. This would result in numeric properties being shown in numerical order, followed by all other properties in unsorted order.

OPTION 2: Add array-like object types such as NodeList to the NON_SORTABLE_CLASSES array (http://mxr.mozilla.org/mozilla-central/source/browser/devtools/shared/widgets/VariablesView.jsm#965 ). These would then be shown unsorted (but in the correct numerical order), while other objects would still be sorted the same way as currently.

OPTION 3: Sort using a compare function which sorts numeric properties in numerical order and other properties in alphabetical order.

I don't think Option 1 is an acceptable solution, because named properties of DOM objects would be listed in a seemingly random order. Option 2 is a better solution, but it would fix the issue for specified object types only and might be harder to maintain.

Option 3 is the best solution in my view, and the one implemented in the patch. The main concern is that adding a compare function makes sorting slower, but in the testing I've done this is not a significant problem (even on my old machine, sorting 100K random strings using the compare function in the patch takes < 1s).

Comment 3

[mjh563]

See also bug 828664, where this issue was previously fixed for arrays.

Comment 4

[Rob Campbell [:rc] (:robcee)]

mjh563: I like your reasoning in comment #2. I agree, option 3 sounds like the best solution.

taking a look at your patch...

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 830164 [details] [diff] [review]
patch

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

::: browser/devtools/shared/widgets/VariablesView.jsm
@@ +1243,5 @@
>      // Sort all of the properties before adding them, if preferred.
>      if (aOptions.sorted) {
> +      names.sort(function(aFirst, aSecond) {
> +        // Make sure that numeric properties get sorted in numerical order.
> +        return aFirst - aSecond || aFirst > aSecond;

Driveby, but just want to say that this is very clever and concise.

Comment 6

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 830164 [details] [diff] [review]
patch

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

this also needs a test.

canceling review.

::: browser/devtools/shared/widgets/VariablesView.jsm
@@ +1241,5 @@
>      let names = Object.keys(aItems);
>  
>      // Sort all of the properties before adding them, if preferred.
>      if (aOptions.sorted) {
> +      names.sort(function(aFirst, aSecond) {

good opportunity to use a fat-arrow function here.

e.g., (aFirst, aSecond) => { ...

@@ +1243,5 @@
>      // Sort all of the properties before adding them, if preferred.
>      if (aOptions.sorted) {
> +      names.sort(function(aFirst, aSecond) {
> +        // Make sure that numeric properties get sorted in numerical order.
> +        return aFirst - aSecond || aFirst > aSecond;

This doesn't follow the spec* for Array.prototype.sort which expects a numeric value of -1, 0 or 1 to indicate sort position.

* - http://www.ecma-international.org/ecma-262/5.1/#sec-15.4.4.11

Comment 7

[Brandon Benvie [:benvie]]

(In reply to Rob Campbell [:rc] (:robcee) from comment #6)
> >      // Sort all of the properties before adding them, if preferred.
> >      if (aOptions.sorted) {
> > +      names.sort(function(aFirst, aSecond) {
> > +        // Make sure that numeric properties get sorted in numerical order.
> > +        return aFirst - aSecond || aFirst > aSecond;
> 
> This doesn't follow the spec* for Array.prototype.sort which expects a
> numeric value of -1, 0 or 1 to indicate sort position.

It actually just calls for a negative value, zero, or a positive value, so subtracting the numbers will work. A different problem from this is that it will also sort in decimals as well as negative integers, which is problematic for the goal of separating indexes from normal properties.

To do a sort which only works on indexes, you'd had to do something like:


function isIndex(key) {
  // Filters out non-numbers, decimals, negative numbers, and numbers greater than 1^32
  return (key >>> 0) + "" === key;
}

names.sort((aFirst, aSecond) => {
  if (isIndex(aFirst) && isIndex(aSecond)) {
    return aFirst - aSecond;
  }
  return aFirst > aSecond;
});

Comment 8

[Brandon Benvie [:benvie]]

Actually that wouldn't work either, because the second part of the filter would up interleaving in those properties anyway. You'd have to do something more complex like:

function isIndex(key) {
  return (key >>> 0) + "" === key;
}

let names = Object.keys(aItems);

if (aOptions.sorted) {
  // Check for a length and the presence of the last property
  let isIndexed = typeof aItems.length == "number" && aItems.length - 1 in aItems;

  if (isIndexed) {
    names = names.filter(aName => !isIndex(aName));
  }

  names.sort();

  if (isIndexed) {
    for (let i = 0; i < aItems.length; i++) {
      names.push(i + "");
    }
  }
}

Comment 9

[Dane MacMillan [:danemacmillan]]

Attachment: Unnatural sort order

I had a screenshot and was about to report it, but it's already reported, so here's my screenshot, too.

Comment 11

[Alexandre Poirot [:ochameau] [PTO back August 21]]

It seems to be fixed now, may be thanks to the property iterator introduced in bug 1023386.
At least document.querySelectorAll("div") on this bugzilla page works fine