860662 - controller.select() fails to select the option by value

Status: RESOLVED FIXED

(Testing Graveyard :: Mozmill, defect)

Description

[Andrei Eftimie]

Affected tests:

> /testPreferences/testDisableCookies.js
> /testPreferences/testEnableCookies.js
> /testPreferences/testRemoveAllCookies.js

Comment 1

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

With what result? Any reported failure?

Comment 2

[Andrei Eftimie]

These tests fail with:
> "History mode is set to custom"

Watching the test unfold you always see the dropdown being opened, and no option being selected.

Comment 3

[Andrei Eftimie]

Looking more into this (happens on the same element, with trying to select the same option).
This might be fixed at the test-level instead of mozmill controller level.

Comment 4

[cmtalbert]

Need to understand if we are using controller.select properly to see if this is widespread or not. Andrei can you determine if this is specific to the test or if it this is a general problem?

Comment 5

[Andrei Eftimie]

Attachment: Non-final fix

We fail to select the item by failing to correctly click on the desired option.

I've initially thought that we issue the click on the option before the menulist items are completely visible/loaded.
A short sleep after the click to open the menulist and before the click to select the desired option fixes the issue.

However (see attached patch) we succeed even with controller.sleep(0);

Investigating further to see why this is the case.

Comment 6

[Andrei Eftimie]

Comment on attachment 738429 [details] [diff] [review]
Non-final fix

Nevermind, I spoke to soon.

Comment 7

[Andrei Eftimie]

Attachment: potential fix

Ok, attached is a valid patch.
Please see comment 5 for more information.

These 3 tests pass with the patch applied.
I'm not sure why this is.
I guess we "break" the current threading and let the menulist finish opening before selecting the option?

I don't really like this, as its probably not a proper fix.

Comment 8

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 738458 [details] [diff] [review]
potential fix

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

You missed requesting feedback. I compared this with the working code in Mozmill 1.5.x and we have the additional sleeps after the synthesizeMouse calls. Requesting feedback from Henrik to see if this is something we should port to Mozmil 2.0

Where these sleeps were originally introduced:
https://github.com/mozilla/mozmill/commit/38ca2a83952651a51cba56016a805d73d578ae6e
https://github.com/mozilla/mozmill/commit/03c31c2c8d56461702f6385939b93ca9fbc3e0c9

Comment 9

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

Comment on attachment 738458 [details] [diff] [review]
potential fix

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

We might want to check if that helps us in 2.0 even using sleep(0) is less than ideal.

Comment 10

[Andrei Eftimie]

On Windows and on Linux controller.select() has a different effect:
Instead of selecting the requested option (in this particular case "custom") mozmill selects another option (the one above in this case), thus triggering a "Nightly must restart to enable this feature" modal dialog, and failing with "Disconnect Error" since we do not handle that window.

I thought about opening a new bug for this issue at first, but since it is the controller.select() failing, fixing this across all platforms seems appropriate.

Comment 11

[Andrei Eftimie]

Attachment: Patch v2

Fixed 3 issues with this patch:

1) OSX wouldn't select the requested option
2) Windows and Linux would select the wrong option
3) The "press key down until we reach the desired option" also behaves flaky. We replace it with scrollIntoView(). Thus this also fixes bug 852116

Based on the comments from bug 852116 I'm assuming I will also need a mutt test.
I'll have that in a separate patch (or should they be bound together in the same patch?)

We use controller.select() in:
> functional/testPreferences/testDisableCookies.js
> functional/testPreferences/testEnableCookies.js
> functional/testPreferences/testRemoveAllCookies.js
> functional/testPreferences/testRemoveCookie.js
> lib/dom-utils.js

With the 3) problem fixed we should be able to also use it in 
> functional/testPreferences/testPreferredLanguage.js

Going forward with the sleep(0) solution since that is also implemented in hotfix-1.5 as Dave pointed out. Here:
https://github.com/mozilla/mozmill/blob/hotfix-1.5/mozmill/mozmill/extension/resource/modules/controller.js#L855
and here:
https://github.com/mozilla/mozmill/blob/hotfix-1.5/mozmill/mozmill/extension/resource/modules/controller.js#L868


Anyway, some testruns:

OSX
===
Nightly: http://mozmill-crowd.blargon7.com/#/functional/report/49feb8c5f96a46e9037cffdde175a200
Aurora: http://mozmill-crowd.blargon7.com/#/functional/report/49feb8c5f96a46e9037cffdde1768659
Beta: http://mozmill-crowd.blargon7.com/#/functional/report/49feb8c5f96a46e9037cffdde17c68cc
Release: http://mozmill-crowd.blargon7.com/#/functional/report/49feb8c5f96a46e9037cffdde17c7f6b
ESR17: http://mozmill-crowd.blargon7.com/#/functional/report/49feb8c5f96a46e9037cffdde17ce5c6

Windows
=======
Nightly: http://mozmill-crowd.blargon7.com/#/functional/report/49feb8c5f96a46e9037cffdde1880f73
Aurora: http://mozmill-crowd.blargon7.com/#/functional/report/49feb8c5f96a46e9037cffdde188580f
Beta: http://mozmill-crowd.blargon7.com/#/functional/report/49feb8c5f96a46e9037cffdde1885a96

Linux
=====
Nightly: http://mozmill-crowd.blargon7.com/#/functional/report/49feb8c5f96a46e9037cffdde17c5ebb
Aurora: http://mozmill-crowd.blargon7.com/#/functional/report/49feb8c5f96a46e9037cffdde1767b40

Comment 12

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

Given the broken behavior of select() and that all of our preferences tests are failing this should be considered as blocker.

(In reply to Andrei Eftimie from comment #11)
> 1) OSX wouldn't select the requested option
> 2) Windows and Linux would select the wrong option

Yeah. Totally bad behavior we absolutely have to fix.

> 3) The "press key down until we reach the desired option" also behaves
> flaky. We replace it with scrollIntoView(). Thus this also fixes bug 852116

Sounds good.

> Based on the comments from bug 852116 I'm assuming I will also need a mutt
> test.
> I'll have that in a separate patch (or should they be bound together in the
> same patch?)

Everything in the same patch please. That makes it easier to run the tests and verify the functionality.

> Going forward with the sleep(0) solution since that is also implemented in
> hotfix-1.5 as Dave pointed out. Here:

Fine with me until we have a proper solution for that s&$t. :S

Comment 13

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

Comment on attachment 742365 [details] [diff] [review]
Patch v2

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

This patch needs an update given the latest changes to mozelement.js. Also as already said we need tests for it, which include checks for content and chrome instances for all various kinds (by index, option, and value)

Comment 14

[Andreea Matei [:AndreeaMatei]]

Attachment: fix and mutt test

I've added the second sleep and a mutt test that checks the dropdown in a content tab with all options (index, value, option) and one in the prefs dialog that uses scrolling, to see how it behaves when an element is in view and when is not.

Note: without the small sleep after clicking the content pane, we fail, but I think is from the content pane loading, cause if I manually click the pane quickly (without the sleep), all works.

Comment 15

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

Comment on attachment 746979 [details] [diff] [review]
fix and mutt test

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

::: mutt/mutt/tests/js/elementslib/select.js
@@ +17,5 @@
> +
> +  var dropdown = new elementslib.ID(controller.window.document, "state");
> +
> +  // Select item by option
> +  controller.select(dropdown, null, '\n         Alabama\n       ');

Why do we need the line breaks? That should not be necessary at all. Is the form.html broken?

@@ +26,5 @@
> +  assert.equal(dropdown.getNode().value, "AK", "Value has been selected");
> +
> +  // Select item by index
> +  controller.select(dropdown, 3);
> +  assert.equal(dropdown.getNode().selectedIndex, 3, "Value has been selected");

Also check the value here similar to the previous checks.

@@ +28,5 @@
> +  // Select item by index
> +  controller.select(dropdown, 3);
> +  assert.equal(dropdown.getNode().selectedIndex, 3, "Value has been selected");
> +
> +  aController = mozmill.getPreferencesController();

I don't think we want to use the preferences pane for base Mozmill tests. I would propose you create a separate xul file here with the necessary elements identifying menu lists. It can be stored in the Mozmill extension under content/test.

Comment 16

[Andreea Matei [:AndreeaMatei]]

(In reply to Henrik Skupin (:whimboo) from comment #15)
> > +  controller.select(dropdown, null, '\n         Alabama\n       ');
> 
> Why do we need the line breaks? That should not be necessary at all. Is the
> form.html broken?

Yeah, it has line breaks, I'll edit that as well in the next patch.

Comment 17

[Andreea Matei [:AndreeaMatei]]

Attachment: mutt test and chrome page

Created the menu_lists xul page, updated form.html and the test. 
Tested on OS X as well, but I'm having the same issue Daniela mentioned on bug 865640, I'll investigate more, for the moment I have skipped it on os x.

Comment 18

[Andreea Matei [:AndreeaMatei]]

This is the report for Linux, with this patch applied:
http://mozmill-crowd.blargon7.com/#/functional/report/ea82256a8ae9808d91b7e8145e60cef2

Comment 19

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

Comment on attachment 747454 [details] [diff] [review]
mutt test and chrome page

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

::: mozmill/mozmill/extension/content/test/menu_lists.xul
@@ +1,1 @@
> +<?xml version="1.0"?>

I would take a more general name like chrome_elements.xul so we can add more once there is a need to test them.

@@ +27,5 @@
> +      <menuitem label='Kansas' value="KS" /> 
> +      <menuitem label='Kentucky' value="AL" />
> +      <menuitem label='Louisiana' value="LA" />
> +      <menuitem label='Maine' value="ME" />
> +      <menuitem label='Maryland' value="MD" />            

Lots of white-space issues across the whole file.

::: mutt/mutt/tests/js/_files/form.html
@@ +28,5 @@
> +       <option value='Select Your State'>Select your State</option>
> +       <option value='AL'>Alabama</option>
> +       <option value='AK'>Alaska</option>
> +       <option value='AZ'>Arizona</option>
> +       <option value='AR'>Arkansas</option>

Way better! Thanks. Can we take at least that many that a scrolldown appears? We would need many of them so a dynamically generated list might be useful.

::: mutt/mutt/tests/js/elementslib/tests.ini
@@ +5,5 @@
>  [mozelement.js]
>  [mozelement_window.js]
>  [radio_buttons.js]
> +[select.js]
> +skip-if = os == 'mac' = select() is currently failing on OS X

Why? I thought we are fixing it with this patch?

::: mutt/mutt/tests/js/elementslib/select.js
@@ +3,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +const BASE_URL = collector.addHttpResource("../_files/");
> +const TEST_DATA = BASE_URL + "form.html";
> +const TEST_CHROME = "chrome://mozmill/content/test/menu_lists.xul";

Fold this into TEST_DATA so it will become an array.

@@ +26,5 @@
> +  assert.equal(dropdown.getNode().value, "AK", "Value has been selected");
> +
> +  // Select by option
> +  controller.select(dropdown, null, 'Alabama');
> +  assert.equal(dropdown.getNode().value, "AL", "Value has been selected");

For now we do not check what happens if the element is not visible but hidden due to scrollbars. We should make sure to test this.

Also why assert and not equal?

@@ +43,5 @@
> +  
> +  // Select by value
> +  controller.select(menulist, null, null, 'AZ');
> +  assert.equal(menulist.getNode().value, 'AZ', "Value has been selected");
> +  

nit: white-space issues

Comment 20

[Andreea Matei [:AndreeaMatei]]

Attachment: mutt test and chrome page

Updated as requested, added more elements to get the scrollbar and selected in both cases an item that is not visible.

Reports - Nightly:
Linux - http://mozmill-crowd.blargon7.com/#/functional/report/14f8bc4e22e61353662cded4c20b310a
OS X - http://mozmill-crowd.blargon7.com/#/functional/report/14f8bc4e22e61353662cded4c20b4e69

Aurora:
Linux - http://mozmill-crowd.blargon7.com/#/functional/report/14f8bc4e22e61353662cded4c20bd226
OS X - http://mozmill-crowd.blargon7.com/#/functional/report/14f8bc4e22e61353662cded4c20bd604

Comment 21

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

Comment on attachment 749197 [details] [diff] [review]
mutt test and chrome page

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

::: mutt/mutt/tests/js/elementslib/select.js
@@ +3,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +const BASE_URL = collector.addHttpResource("../_files/");
> +const TEST_DATA = [BASE_URL + "form.html",
> +                  "chrome://mozmill/content/test/chrome_elements.xul"];

Please fix the indentation by moving the first entry into it's own line and indent by 2 spaces from the beginning.

@@ +13,5 @@
> +var testContentSelect = function () {
> +  controller.open(TEST_DATA[0]);
> +  controller.waitForPageLoad();
> +
> +  var dropdown = new elementslib.ID(controller.window.document, "state");

This is content so please use controller.tabs.activeTab.

@@ +39,5 @@
> +  // Select by index, item not in view
> +  controller.select(menulist, 20);
> +  utils.waitFor(function () {
> +    return menulist.getNode().selectedIndex === 20;
> +  }, "Index has been selected");

Why we have to wait for the selection? That should be part of the select() method.

::: mutt/mutt/tests/js/elementslib/tests.ini
@@ +5,4 @@
>  [mozelement.js]
>  [mozelement_window.js]
>  [radio_buttons.js]
> +[select.js]

The filename should represent the element and not the method. So please rename it to menulist.js

Comment 22

[Andreea Matei [:AndreeaMatei]]

Attachment: mutt test and chrome page

Included the waitFor() in select as well.
Should we file a bug to remove the waitFors from our tests? It's not affecting us directly, could be a good first bug, but I know we're waiting for the element value to change instead of asserting it.

Comment 23

[Andreea Matei [:AndreeaMatei]]

Attachment: mutt test and chrome page

Re-commited cause not all lines appeared in Bugzilla review page. Hopefully now is shown correctly.

Comment 24

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

Comment on attachment 749311 [details] [diff] [review]
mutt test and chrome page

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

::: mozmill/mozmill/extension/content/test/chrome_elements.xul
@@ +27,5 @@
> +      <menuitem label='Kansas' value="KS" />
> +      <menuitem label='Kentucky' value="KY" />
> +      <menuitem label='Louisiana' value="LA" />
> +      <menuitem label='Maine' value="ME" />
> +      <menuitem label='Maryland' value="MD" /> 

nit: whitespace.

::: mozmill/mozmill/extension/resource/driver/mozelement.js
@@ +652,4 @@
>  
> +          utils.waitFor(function () {
> +            return item.selected;
> +          }, "Item has been selected");

That doesn't say much. Can we make use of the index, value or label attribute of the entry?

Something like:

var name = label || value
-> menuitem selected: index=item.index, name=name

::: mutt/mutt/tests/js/elementslib/menulist.js
@@ +4,5 @@
> +
> +const BASE_URL = collector.addHttpResource("../_files/");
> +const TEST_DATA = [
> +  BASE_URL + "form.html",
> +  "chrome://mozmill/content/test/chrome_elements.xul"];

nit: closing bracket in a new line please.

Comment 25

[Andreea Matei [:AndreeaMatei]]

Attachment: mutt test and chrome page

As confirmed on IRC, now we're waiting for the correct element to be selected in both cases (content and chrome), removing the unnecessary assertions from the test. Checked with the repository as well, by removing the waitFor() in the tests that use select() (testPreferences/testEnableCookie.js for example).

Reports with clean repository:
Linux: 
http://mozmill-crowd.blargon7.com/#/functional/report/90a0b225333a4e0c868fcb2b4455d6c9
OS X:
http://mozmill-crowd.blargon7.com/#/functional/report/90a0b225333a4e0c868fcb2b44560541

NOTE: there might be a loading problem for the mutt test on nightly, on aurora is passing though. It's unrelated to this bug, Andrei has encounter it as well, but I'll investigate more and file a bug if necessary.

Comment 26

[Andreea Matei [:AndreeaMatei]]

Attachment: mutt test and chrome page

Reattaching due to issues from generating the patch from git.

Comment 27

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

Comment on attachment 753178 [details] [diff] [review]
mutt test and chrome page

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

::: mozmill/mozmill/extension/content/test/chrome_elements.xul
@@ +1,5 @@
> +<?xml version="1.0"?>
> +
> +<?xml-stylesheet href="chrome://global/skin/global.css" type="text/css"?>
> +
> +<window id="menu_lists_test"

nit: Please change this to 'mozmill_chrome_elements'.

::: mozmill/mozmill/extension/resource/driver/mozelement.js
@@ +601,5 @@
>            this.dispatchEvent('change', true);
>  
> +          var self = this;
> +          utils.waitFor(function () {
> +            var selected = value || option || self.element.selectedIndex;

I would propose we make a switch statement given which parameter has been set. Otherwise there is a high chance to introduce bad states here and in the following waitFor call.

@@ +603,5 @@
> +          var self = this;
> +          utils.waitFor(function () {
> +            var selected = value || option || self.element.selectedIndex;
> +
> +            return (selected == indx) || (selected == item.label) ||

I know you didn't introduce 'indx' but can we rename it to index for a better understanding?

@@ +651,4 @@
>          // Click the item
>          try {
>            EventUtils.synthesizeMouse(this.element, 1, 1, {}, ownerDoc.defaultView);
> +          utils.sleep(0);

Why do we even need this here?

::: mutt/mutt/tests/js/elementslib/menulist.js
@@ +18,5 @@
> +
> +  var dropdown = new elementslib.ID(controller.tabs.activeTab, "state");
> +
> +  // Select by index, item not in view
> +  dropdown.select(20);

Why have you removed the checks which were in here until the last version of the patch?

Comment 28

[Andreea Matei [:AndreeaMatei]]

(In reply to Henrik Skupin (:whimboo) from comment #27)
> > +  // Select by index, item not in view
> > +  dropdown.select(20);
> 
> Why have you removed the checks which were in here until the last version of
> the patch?

Because we wait in the method now for that change to happen and be correct, we also talked on IRC and I asked if what you meant was to remove the checks from the tests and do it in the method.

With this:
> return (selected == indx) || (selected == item.label) ||..

we ensure the selected item is the one given by the parameters.

Comment 29

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

Sure, but we are testing an API here, so there should be tests which ensure that the code inside the API is doing the right thing. As of now the test does not verify that.

Comment 30

[Andreea Matei [:AndreeaMatei]]

Attachment: mutt test and chrome page

Updated, checked on all platforms.

Comment 31

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

Comment on attachment 757685 [details] [diff] [review]
mutt test and chrome page

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

That works great. So lets get the minor changes fixed and landed as of today! \o/

::: mozmill/mozmill/extension/resource/driver/mozelement.js
@@ +600,5 @@
>            item.selected = true;
>            this.dispatchEvent('change', true);
>  
> +          var self = this;
> +          var selected = value || option || index;

I think i mentioned it before, but we should keep the order of the parameters as given to that method. That will lessen the confusion if something doesn't behave as expected.

@@ +604,5 @@
> +          var selected = value || option || index;
> +          utils.waitFor(function () {
> +            switch (selected) {
> +              case value:
> +                return selected == item.value;

triple signs please for all of those cases and please sort similarly as above.

@@ +666,5 @@
>            EventUtils.synthesizeMouse(item, 1, 1, {}, ownerDoc.defaultView);
> +          utils.sleep(0);
> +
> +          var self = this;
> +          var selected = value || option || index;

Same as above.

Comment 32

[Andreea Matei [:AndreeaMatei]]

Attachment: mutt test and chrome page

Updated to be consistent.

Comment 33

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

Landed on master:
https://github.com/mozilla/mozmill/commit/2684fb7418891cae0aff6c86b1297328d26259b0