852116 - controller.select() fails to click on selected options if outside of visible viewport

Status: RESOLVED FIXED

(Testing Graveyard :: Mozmill, defect)

Description

[Andrei Eftimie]

Selecting an option from a MenuList fails if the Option is located outside the visible viewport.

It appears the code that fails is the following:

> // Scroll down until item is visible
> for (var i = 0; i <= menuitems.length; ++i) {
>   var selected = this.element.boxObject.QueryInterface(Ci.nsIMenuBoxObject).activeChild;
>   if (item == selected) {
>     break;
>   }
>   EventUtils.synthesizeKey("VK_DOWN", {}, ownerDoc.defaultView);
> }

The MenuList is not scrolled as it should.

One possible workaround is to use scrollIntoView() on the option element.
(Tested with great success on the 1.5 hotfix branch)

Comment 1

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

Why does the summary say "click()" and the example snippet here is about synthesizeKey? Are both affected or what do you mean?

Comment 2

[Andrei Eftimie]

More context (this is from mozmill master (2.0) branch, comment 1 code is from hotfix-1.5):
The "click" is just below the snippet with a synthesizeMouse.

The "scrolling" is done by pressing VK_DOWN the number of items that are before the one we want to click.

> // Click the item
> try {
>   EventUtils.synthesizeMouse(element, 1, 1, {}, ownerDoc.defaultView);
>   this.sleep(0);
> 
>   // Scroll down until item is visible
>   for (var i = s = element.selectedIndex; i <= element.itemCount + s; ++i) {
>     var entry = element.getItemAtIndex((i + 1) % element.itemCount);
>     EventUtils.synthesizeKey("VK_DOWN", {}, ownerDoc.defaultView);
>     if (entry.label == item.label) {
>       break;
>     }
>     else if (entry.label == "") i += 1;
>   }
> 
>   EventUtils.synthesizeMouse(item, 1, 1, {}, ownerDoc.defaultView);
>   this.sleep(0);
> 
>   frame.events.pass({'function':'Controller.select()'});
>   return true;
> } catch (ex) {
>   throw new Error('No item selected for element ' + el.getInfo());
>   return false;
> }

Comment 3

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

Please give us an example as best with a minimized testcase for the option element. I still can't imagine that it doesn't work. We haven't seen this before. Thanks.

Comment 4

[Andrei Eftimie]

Attachment: Testcase

Attached is a testcase.

It makes 2 basic tests, selecting an item that is initially visible, then an item under the fold (which requires scrolling).
1) It tests a regular html <select> element. Works fine.
2) It tests a <menulist> item (I used the Language Selector). It works on the initially visible item. It fails on the initially hidden item. (You can watch the test and see that it fails to scroll & select the element)

Here is the relevant pull request for patching mozmill (hotfix-1.5) https://github.com/mozilla/mozmill/pull/115
With this patch applied run the tests again. All will pass.

======

I can mark this as a blocker for bug 826251 (and use controller.select() instead of manual clicks for selecting the value) but that will delay landing that until we patch and release a new version of mozmill.

I agree this would be cleaner, but I also know we are reluctant to patch mozmill on the hotfix branch.
Please advise on how to proceed.

Comment 5

[Andrei Eftimie]

Attachment: hotfix-1.5 patch

I've added the patch here as instructed.
(I hope the format is good, have not yet created patches out of a git commit).

Created with > git show
APplies cleanly (on a clean branch) with > git apply

Comment 6

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

Comment on attachment 730721 [details] [diff] [review]
hotfix-1.5 patch

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

The way how we should work on that patch is to first upload a patch for master, including a test. That would make it way more comfortable for me to review the hotfix-1.5 branch (which we are even not sure will ever be active).

Comment 7

[Andrei Eftimie]

I've initially uploaded a patch for the hotfix-1.5 branch since we arrived at this issue trying to land bug 826251.

If you're saying this might never hit hotfix-1.5 we should then take out the dependency, and move forward on bug 826251 without using controller.select() (since this fails in that particular instance) and we should add this bug to a sprint to make it proper on master and make a mutt test for it.

Comment 8

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

Andrei, is this fixed now?

Comment 9

[Andrei Eftimie]

Yep, been implemented as part of bug 852116
We probably don't want to patch 1.5 with this