If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Ensure shortcuts work in Marionette tests

RESOLVED FIXED in mozilla36

Status

Testing
Marionette
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: whimboo, Assigned: chmanchester)

Tracking

Trunk
mozilla36
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

3 years ago
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.
(Assignee)

Comment 1

3 years ago
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.
(Reporter)

Comment 2

3 years ago
As David told me send_keys should perfectly work with the modifiers. So this method might not correctly send it.
(Assignee)

Comment 3

3 years ago
(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.
(Assignee)

Comment 4

3 years ago
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.
(Assignee)

Comment 5

3 years ago
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.
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
(Assignee)

Updated

3 years ago
Assignee: nobody → cmanchester
(Assignee)

Comment 7

3 years ago
Created attachment 8516437 [details] [diff] [review]
Add a test for shortcut keys in marionette

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

Comment 8

3 years ago
Created attachment 8516707 [details] [diff] [review]
Support for modifier keys in chrome scope in marionette.
Attachment #8516707 - Flags: review?(dburns)
(Assignee)

Updated

3 years ago
Attachment #8516437 - Attachment is obsolete: true
(Reporter)

Comment 9

3 years ago
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.
(Assignee)

Comment 10

3 years ago
(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.
(Reporter)

Comment 11

3 years ago
(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?
(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 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
(Assignee)

Comment 14

3 years ago
Created attachment 8517087 [details] [diff] [review]
Support for modifier keys in chrome scope in marionette.

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

Updated

3 years ago
Attachment #8516707 - Attachment is obsolete: true
Attachment #8516707 - Flags: review?(dburns)
(Reporter)

Comment 15

3 years ago
(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.
(Assignee)

Comment 16

3 years ago
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.
Attachment #8517087 - Flags: review?(dburns)
Attachment #8517087 - Flags: review?(dburns) → review+
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/6a21dd57eeb3
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/6a21dd57eeb3
https://hg.mozilla.org/mozilla-central/rev/6a21dd57eeb3
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.