967319 - Show a nodesList result with natural order

Status: RESOLVED FIXED

(DevTools :: Object Inspector, defect)

Description

[Fernando García Gómez, stripTM]

The results of a nodeList / HTMLCollection / ... are alphanumerical sorted. For example
document.getElementsByTagName("a");
show: 0, 1, 10, 11, 12...

It would be desirable to shew in natural order: 0, 1, 2, 3...

Comment 1

[Patrick Brosset <:pbro>]

STR:
- Open the webconsole on this bugzilla page
- Evaluate document.getElementsByTagName("a");
- Click on the output link to open the variablesview

=> The list of <a> DOMNodes is sorted like 0,1,10,11,12,...
=> It would indeed be better to have it sorted by numerical index. Especially that we now have the highlight on hover in the variablesview.

However, if you evaluate [1,2,3,4,5,6,7,8,9,10,11,12] in the webconsole and click on the output link, the result in the variablesview is sorted as expected.

Comment 4

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

To fix this bug, you will need to edit http://mxr.mozilla.org/mozilla-central/source/browser/devtools/shared/widgets/VariablesView.jsm?rev=0f7b2674c64a#1398 and http://mxr.mozilla.org/mozilla-central/source/browser/devtools/shared/widgets/VariablesView.jsm?rev=0f7b2674c64a#2235 to take a custom sorting function that will order properties that parse as numbers numerically (in increasing order), and properties that do not parse as numbers lexicographically.

Comment 5

[bmax1337]

Attachment: 230924.diff

Fixed bug by implementing a function naturalSort, also wrote a test. Semi-confused on why there was a test that checks to make sure the keys are "not-sorted" but yet still passes with my fix.

Comment 6

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

Comment on attachment 8571586 [details] [diff] [review]
230924.diff

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

This looks really good, just some minor style issues to address and a couple more cases to add to the test.

::: browser/devtools/shared/widgets/VariablesView.jsm
@@ +1396,5 @@
>      // Sort all of the properties before adding them, if preferred.
>      if (aOptions.sorted && aKeysType != "just-numbers") {
> +      names.sort(
> +          this._naturalSort
> +      );

We can put this all on one line, just `names.sort(this._naturalSort);`

@@ +1606,5 @@
>    /**
> +   * Sort in ascending order.
> +   * @param string a 
> +   * @param string b 
> +   * @return boolean

This should be:
@return number -1 if a is less than b, 0 if order no change in order, +1 if a is greater than 0

@@ +1609,5 @@
> +   * @param string b 
> +   * @return boolean
> +   */
> +  _naturalSort: function(a,b) 
> +   { 

nit, please follow the style of how functions are defined in this file, by placing the opening curly bracket on the same line as the function signature. Please also remove the trailing whitespace on this line and the lines above it.

@@ +1611,5 @@
> +   */
> +  _naturalSort: function(a,b) 
> +   { 
> +    if (isNaN(parseFloat(a)) && isNaN(parseFloat(b)))
> +      return (a<b ? -1 : 1);

another thing that this file is doing is wrapping all if-blocks with curly brackets even if they only have one line in their body.

@@ +2247,5 @@
>      // Sort all of the properties before adding them, if preferred.
>      if (aOptions.sorted) {
> +      propertyNames.sort(
> +          this._naturalSort
> +      );

Ditto about putting this all in one line.

::: browser/devtools/webconsole/test/browser_console_variables_view_dont_sort_non_sortable_classes_properties.js
@@ +121,5 @@
> +    is(keyIterator.next().value, "1", "First key should be 1");
> +    is(keyIterator.next().value, "4", "Second key should be 4");
> +
> +    // If the properties are sorted, the next one will be 10.
> +    is(keyIterator.next().value, "10", "Third key is not 10");

Please also add two more checks to make sure that the next one from here is "abc" and the one following that is "hello".

Comment 7

[bmax1337]

Attachment: 230924.diff

New patch matching current code conventions and better test.

Comment 8

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

Comment on attachment 8572400 [details] [diff] [review]
230924.diff

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

Sorry, I should have given some of these comments in the previous review.

With the following changed this should be ready for check-in.

::: browser/devtools/shared/widgets/VariablesView.jsm
@@ +1604,5 @@
>    /**
> +   * Sort in ascending order.
> +   * @param string a
> +   * @param string b
> +   * @return number -1 if a is less than b, 0 if order no change in order, +1 if a is greater than 0

replace "0 if order no change in order" with "0 if no change in order" :)

@@ +1608,5 @@
> +   * @return number -1 if a is less than b, 0 if order no change in order, +1 if a is greater than 0
> +   */
> +  _naturalSort: function(a,b) {
> +    if (isNaN(parseFloat(a)) && isNaN(parseFloat(b))) {
> +      return (a<b ? -1 : 1);

Can you please add a comment saying that this only needs to handle non-numbers since it is used with an array which has numeric-based indices placed in order?

Also, put spaces around the less-than operator and remove the parentheses.

Comment 9

[bmax1337]

Attachment: 230924.diff

Comment 10

[bmax1337]

Attachment: 230924.diff

Comment 11

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

Comment on attachment 8572428 [details] [diff] [review]
230924.diff

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

Just noticed something here before marking this as r+,

::: browser/devtools/webconsole/test/browser_console_variables_view_dont_sort_non_sortable_classes_properties.js
@@ +124,5 @@
> +    // If the properties are sorted, the next one will be 10.
> +    is(keyIterator.next().value, "10", "Third key is not 10");
> +    // If sorted next properties should be "abc" then "hello"
> +    is(keyIterator.next().value, "abc", "Fourth key is not abc");
> +    is(keyIterator.next().value, "hello", "Fifth key is not hello");

The descriptions for the last three test-cases all say "is not *" but it should be "is *"

Comment 12

[bmax1337]

Attachment: 230924.diff

Fixed some english in tests.

Comment 13

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

Pushed to tryserver,
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8f85913967eb

Comment 14

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

Attachment: Patch for check-in

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/a4a679316000

Comment 16

[Ryan VanderMeulen [:RyanVM]]

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

Comment 17

[Fernando García Gómez, stripTM]

The patch landing to Firefox Nightly and works fine.
Thanks!