1053150 - elementslib.Lookup() can return an array, but should only return a single value

Status: RESOLVED WONTFIX

(Testing Graveyard :: Mozmill, defect)

Description

[Cosmin Malutan, [:cosmin-malutan]]

Right now, elementslib.Lookup() might returns an array, it should return a single node, perhaps it would be best to make a check similar we do for Selector class and return an index or the first found element. 
https://github.com/mozilla/mozmill/blob/hotfix-2.0/mozmill/mozmill/extension/resource/driver/elementslib.js#L152

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Magic behavior is bad. Especially when all the other methods only return a single element. There are also downsides when changing this behavior, but in general it would be consistent.

Comment 2

[hharchani]

I want to work on this bug. Please assign this to me, and give info required, i.e. what should it return instead, when its an array.

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+2]]

You are welcome to work on this bug! So I assigned it to you. Please have a look at the URL field, which explains how to setup the development environment for Mozmill, and how to get started.

In regards of what it should return, it should be the first found element.

Comment 4

[hharchani]

diff --git a/mozmill/mozmill/extension/resource/driver/elementslib.js b/mozmill/mozmill/extension/resource/driver/elementslib.js
index 6578f1a..93eb883 100644
--- a/mozmill/mozmill/extension/resource/driver/elementslib.js
+++ b/mozmill/mozmill/extension/resource/driver/elementslib.js
@@ -149,7 +149,7 @@ function Selector(_document, selector, index) {
 
   var nodes = nodeSearch(_document, this.getNodeForDocument, this.selector);
 
-  return nodes ? nodes[index || 0] : null;
+  return nodes ? ( Array.isArray( nodes[0] ) ? nodes[0][index || 0] : nodes[index || 0] ): null;
 };
 

Can this be a solution?
Array.isArray() [1]


[1] https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/isArray

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to hharchani from comment #4)
> @@ -149,7 +149,7 @@ function Selector(_document, selector, index) {

You are modifying Selector here but not Lookup. But interestingly it seems to suffer from the same problem. So may have to change both of them. 

> -  return nodes ? nodes[index || 0] : null;
> +  return nodes ? ( Array.isArray( nodes[0] ) ? nodes[0][index || 0] :
> nodes[index || 0] ): null;

First we should get rid of the index parameter. It's not necessary anymore given that we always want to return the first entry. 

> Can this be a solution?
> Array.isArray() [1]

Here we seem to always have an array or null returned. So an extra check would not be necessary.

For the changes to Lookup look here:
https://github.com/mozilla/mozmill/blob/master/mozmill/mozmill/extension/resource/driver/elementslib.js#L438

Comment 6

[hharchani]

I have made following two changes.

    diff --git a/mozmill/mozmill/extension/resource/driver/elementslib.js b/mozmill/mozmill/extension/resource/driver/elementslib.js
    index 6578f1a..1e5bdbb 100644
    --- a/mozmill/mozmill/extension/resource/driver/elementslib.js
    +++ b/mozmill/mozmill/extension/resource/driver/elementslib.js
    @@ -136,7 +136,7 @@ function nodeSearch(doc, func, string) {
      *
      * Finds an element by selector string
      */
    -function Selector(_document, selector, index) {
    +function Selector(_document, selector) {
       if (selector == undefined) {
         throw new Error('Selector constructor did not recieve enough arguments.');
       }
    @@ -149,7 +149,7 @@ function Selector(_document, selector, index) {
     
       var nodes = nodeSearch(_document, this.getNodeForDocument, this.selector);
     
    -  return nodes ? nodes[index || 0] : null;
    +  return nodes ? ( Array.isArray(nodes) ? nodes[0] : nodes) : null;
     };
     
     /**
    @@ -533,5 +533,6 @@ function Lookup(_document, expression) {
         return false;
       };
     
    -  return expSplit.reduce(reduceLookup);
    +  var nodes = expSplit.reduce(reduceLookup);
    +  return nodes ? ( Array.isArray(nodes) ? nodes[0] : nodes ): null;
     };


Except for Selector, all other functions return directly the return value of nodeSearch, without any check. Can't it be an Array?
Is this the right way I'm doing this.

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Can you please create a real patch and attach it to this bug? I would like to test this change on my own, given that this is kinda complicated code around in this file. Once uploaded please request feedback from me. Thanks.

Comment 8

[hharchani]

Attachment: 0001-Returning-single-value-instead-of-an-array.patch

created patch.

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8489354 [details] [diff] [review]
0001-Returning-single-value-instead-of-an-array.patch

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

::: mozmill/mozmill/extension/resource/driver/elementslib.js
@@ +136,4 @@
>   *
>   * Finds an element by selector string
>   */
> +function Selector(_document, selector) {

Please keep the index here.

@@ +149,4 @@
>  
>    var nodes = nodeSearch(_document, this.getNodeForDocument, this.selector);
>  
> +  return nodes ? ( Array.isArray(nodes) ? nodes[0] : nodes) : null;

This all is a bit more complicated. So if you want you may want to check the stack, you will be able to find the following:

Given by the current code `nodes` will always be an array or null. So why? It's the return value of the nodeSearch() call, and in there `e` is assigned when an element has been found. In case of the Selector class the callback function is this.getNodeForDocument(), which itself returns the result from `this.document.querySelectorAll(s)`. Here querySelectorAll always returns an array or null.

So what I would suggest for now is to keep the code as we have. It works perfectly fine in those cases you expect multiple elements to be found.  Instead lets get the comment in the jsdoc string right above this method updated. It should include that we return the first found element only, in case multiple elements have been found and no index has been specified.

@@ +533,4 @@
>      return false;
>    };
>  
> +  var nodes = expSplit.reduce(reduceLookup);

Given by the following specification, the reduce() call should return a single element: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/Reduce

If that doesn't happen here, something is wrong in the reduceLookup method.

I think the best here would be to ask Cosmin if he can give you an example when this happens. Usually a testcase should be attached when filing a bug. Once we have this testcase, you could run Mozmill with it, and check with `dump("what_ever_string")` the reason for returning an array.

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Cosmin, do you still know when you hit this problem? A testcase would be kinda appreciated.

Comment 11

[Cosmin Malutan, [:cosmin-malutan]]

This was filed while investigating bug 1050253.
So what actually happened there: we had a duplicate in access key which resulted in duplicated lookup expressions which returned an array.
I will provide a tc in a bit.
Thanks

Comment 12

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: 0001-Bug-1053150-Mutt-test-to-check-that-elementslib.Look.patch

Henrik, here is a mutt test for this issue.

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Comment on attachment 8492965 [details] [diff] [review]
0001-Bug-1053150-Mutt-test-to-check-that-elementslib.Look.patch

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

That looks fine and should give a good start in testing the changes as done by hharchani.

Comment 14

[hharchani]

I am unassigning myself from this.

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Cosmin, anything left to do here? Mind checking it? If we can get this in with nearly no additional work, it would be fine. Otherwise I would wontfix it.

Comment 16

[Cosmin Malutan, [:cosmin-malutan]]

The patch was never tested, honestly  don't think in helps, but as stated upper in bug the issue has to be investigated whit the testcase I provided, now considering that we will completely remove the lookup expressions soon, I wouldn't invest any more time here.