1091662 - Ensure shortcuts work in Marionette tests

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect)

Description

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

While working on shortcut examples (see URL field for test file) for Marionette I have seen a couple of issues:

* Shortcuts with modifiers are not working when sent to the browser window
* There is a missing Accel key, which we were able to use before to circumvent the CONTROL (Linux/Windows) and COMMAND (OS X) modifier. We may want to request this feature in Marionette client
* No send_keys support in Actions (see bug 1090925)

As talked on IRC, Chris is most likely going to work on this.

Comment 1

[Chris Manchester (limited bugmail, email directly)]

There's backdoor to EventUtils.js through the chrome sanbox that exposes a synthesizeKeys function that looks a lot like what mozmill uses. I have a working version of the test at https://github.com/chmanchester/firefox-greenlight-tests/tree/keys that uses this. Using this may end up being more expedient than fixing bug 1090925, but I'll check that out tomorrow.

Comment 2

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

As David told me send_keys should perfectly work with the modifiers. So this method might not correctly send it.

Comment 3

[Chris Manchester (limited bugmail, email directly)]

(In reply to Henrik Skupin (:whimboo) from comment #2)
> As David told me send_keys should perfectly work with the modifiers. So this
> method might not correctly send it.

send_keys is implemented in terms of synthesizeKeys, so I'm not sure about that (although the accel keys was not working when I tried to use it due to navigator not being defined in the execution sandbox). send_keys with modifiers doesn't seem to be working as expected, I'll look into that a further.

Comment 4

[Chris Manchester (limited bugmail, email directly)]

In chrome context the marionette server implements send_keys in terms of EventUtils' sendString, which has in a comment (apparently accurate):

 * For now this method only works for English letters (lower and upper case)
 * and the digits 0-9.

So I don't think this will be sufficient as-is.

Comment 5

[Chris Manchester (limited bugmail, email directly)]

I was able to get something like the test linked in the url field of this bug by using the content scope and making a few modifications (that I think are bug fixes; I'll file that separately) to marionette-sendkeys.js.

I think our choice now is:

Develop a python library based on synthesizeKeys through testUtils mentioned in comment 1

or

Get marionette to be where we need it, which would involve fixing bug 1090925 and/or working around comment 4.

Comment 6

[David Burns :automatedtester]

We need to fix Marionette here. It looks like we have 2 ways of typing in Marionette and we need to move the Chrome way of testing to be the same as 

http://dxr.mozilla.org/mozilla-central/source/testing/marionette/marionette-server.js#2051 needs to be more like http://dxr.mozilla.org/mozilla-central/source/testing/marionette/marionette-listener.js#1620

The Content way uses http://dxr.mozilla.org/mozilla-central/source/testing/marionette/marionette-sendkeys.js which understands(or at least should) modifiers

Comment 7

[Chris Manchester (limited bugmail, email directly)]

Attachment: Add a test for shortcut keys in marionette

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=580e3479cb09

Comment 8

[Chris Manchester (limited bugmail, email directly)]

Attachment: Support for modifier keys in chrome scope in marionette.

Comment 9

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

Comment on attachment 8516707 [details] [diff] [review]
Support for modifier keys in chrome scope in marionette.

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

::: testing/marionette/marionette-sendkeys.js
@@ -70,4 @@
>            break;
>          case '\uE03D':
>            keyCode = "VK_META";
> -          metaKey = !metaKey;

What about accelKey? This would be a helpful addition in terms of the command key on OS X and ctrl on Linux/Windows.

::: testing/marionette/marionette-server.js
@@ +2049,5 @@
> +      let el = this.curBrowser.elementManager.getKnownElement(
> +        aRequest.parameters.id, currentWindow);
> +      utils.sendKeysToElement(currentWindow, el, aRequest.parameters.value,
> +                              this.sendOk.bind(this), this.sendError.bind(this),
> +                              command_id, "chrome");

Maybe better to pass this.context here.

Comment 10

[Chris Manchester (limited bugmail, email directly)]

(In reply to Henrik Skupin (:whimboo) from comment #9)
> Comment on attachment 8516707 [details] [diff] [review]
> Support for modifier keys in chrome scope in marionette.
> 
> Review of attachment 8516707 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: testing/marionette/marionette-sendkeys.js
> @@ -70,4 @@
> >            break;
> >          case '\uE03D':
> >            keyCode = "VK_META";
> > -          metaKey = !metaKey;
> 
> What about accelKey? This would be a helpful addition in terms of the
> command key on OS X and ctrl on Linux/Windows.

I was thinking we can add this in our own library as a shortcut. I didn't see a reference to it in the character types table in the webdriver spec, but I could have missed it under an alias.

> 
> ::: testing/marionette/marionette-server.js
> @@ +2049,5 @@
> > +      let el = this.curBrowser.elementManager.getKnownElement(
> > +        aRequest.parameters.id, currentWindow);
> > +      utils.sendKeysToElement(currentWindow, el, aRequest.parameters.value,
> > +                              this.sendOk.bind(this), this.sendError.bind(this),
> > +                              command_id, "chrome");
> 
> Maybe better to pass this.context here.

Comment 11

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

(In reply to Chris Manchester [:chmanchester] from comment #10)
> > What about accelKey? This would be a helpful addition in terms of the
> > command key on OS X and ctrl on Linux/Windows.
> 
> I was thinking we can add this in our own library as a shortcut. I didn't
> see a reference to it in the character types table in the webdriver spec,
> but I could have missed it under an alias.

It's more like a meta key set to the appropriate command key. So it's not real. Having that would help a lot in reducing if/else blocks. Having that in general would be a great enhancement for desktop applications, but yeah if it is not in the spec.... David, what do you think?

Comment 12

[David Burns :automatedtester]

(In reply to Henrik Skupin (:whimboo) from comment #11)
> (In reply to Chris Manchester [:chmanchester] from comment #10)
> > > What about accelKey? This would be a helpful addition in terms of the
> > > command key on OS X and ctrl on Linux/Windows.
> > 
> > I was thinking we can add this in our own library as a shortcut. I didn't
> > see a reference to it in the character types table in the webdriver spec,
> > but I could have missed it under an alias.
> 
> It's more like a meta key set to the appropriate command key. So it's not
> real. Having that would help a lot in reducing if/else blocks. Having that
> in general would be a great enhancement for desktop applications, but yeah
> if it is not in the spec.... David, what do you think?

Sounds like it would be useful. You can handle this on the local by checking the platform that is returned when you start a new session.

I suggest raising a bug against https://www.w3.org/Bugs/Public/enter_bug.cgi?comment=&blocked=20860&short_desc=%5BWebDriver%20Spec%5D%3A%20&product=Browser%20Test%2FTools%20WG&component=WebDriver for AccelKeys to get added and I will put it forward for discussion within the group

Comment 13

[David Burns :automatedtester]

Comment on attachment 8516707 [details] [diff] [review]
Support for modifier keys in chrome scope in marionette.

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

As discussed on IRC, it would be great to have a test for this

::: testing/marionette/marionette-server.js
@@ +2049,5 @@
> +      let el = this.curBrowser.elementManager.getKnownElement(
> +        aRequest.parameters.id, currentWindow);
> +      utils.sendKeysToElement(currentWindow, el, aRequest.parameters.value,
> +                              this.sendOk.bind(this), this.sendError.bind(this),
> +                              command_id, "chrome");

yea, please change this to this.context

Comment 14

[Chris Manchester (limited bugmail, email directly)]

Attachment: Support for modifier keys in chrome scope in marionette.

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=caae36243aa6

Comment 15

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

(In reply to David Burns :automatedtester from comment #12)
> I suggest raising a bug against
[..]
> I will put it forward for discussion within the group

Thank's David! I filed it as https://www.w3.org/Bugs/Public/show_bug.cgi?id=27244.

Comment 16

[Chris Manchester (limited bugmail, email directly)]

Comment on attachment 8517087 [details] [diff] [review]
Support for modifier keys in chrome scope in marionette.

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

Try looks good with the copy/paste test. This is also passing the greenlight test linked in the url field of the bug.

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6a21dd57eeb3

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6a21dd57eeb3

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6a21dd57eeb3