Closed Bug 1367430 Opened 7 years ago Closed 7 years ago

Set modifier-key properties in MouseEvents synthesized by performActions command

Categories

(Remote Protocol :: Marionette, enhancement)

Product:
Remote Protocol
Component:
Marionette
Version:
Version 3
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(firefox55 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: impossibus, Assigned: impossibus)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

Bug 1367430 - Set modifier-key properties for mouse action;
59 bytes, text/x-review-board-request
ato
: review+
Details 
See https://github.com/mozilla/geckodriver/issues/645

Example: ctrl+click action should result in a MouseEvent with ctrlKey property set to true because ctrl key is being held down, but that doesn't happen.

I've written a web-platform-test to reproduce the issue. Now I need to dig around in testing/marionette/event.js to see what we're missing.
Summary: Set ctrlKey, altKey, etc. properties in MouseEvents synthesized by performActions command → Set modifier-key properties in MouseEvents synthesized by performActions command
In action.js, dispatchPointerUp/Down synthesizes a MouseEvent based only on the pointer input source's input state, which contains no information about modifier keys. I think this may be missing from the WebDriver spec: look through the input state table for modifier keys pressed, and set the event properties accordingly.
Comment on attachment 8874622 [details]
Bug 1367430 - Set modifier-key properties for mouse action;

https://reviewboard.mozilla.org/r/145968/#review149860

::: commit-message-d69d0:4
(Diff revision 1)
> +Bug 1367430 - Set modifier-key properties for mouse action; r?ato
> +
> +When synthesizing a MouseEvent for the performActions command,
> +set shiftKey, ctrlKey, etc., based on inputStateMap.

Keeping track of modifier keys feels like something the browser should do for us. I looked for an internal API that could check what modifier keys are pressed overall, but that investigation didn't yield anything useful. 

Related: At some point, I should actually learn how everything in event.js works, then burn it to the ground and rewrite it.
Comment on attachment 8874622 [details]
Bug 1367430 - Set modifier-key properties for mouse action;

https://reviewboard.mozilla.org/r/145968/#review149860

> Keeping track of modifier keys feels like something the browser should do for us. I looked for an internal API that could check what modifier keys are pressed overall, but that investigation didn't yield anything useful. 
> 
> Related: At some point, I should actually learn how everything in event.js works, then burn it to the ground and rewrite it.

From what I can tell from https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIDOMWindowUtils, the primitives we have access to require us to keep track of this state ourselves.  I think testing/marionette/event.js is the place where it is expected that we keep track of any state related to input devices, because the nsIDOMWindowUtils API doesn’t really make the distinction between multiple devices.
Comment on attachment 8874622 [details]
Bug 1367430 - Set modifier-key properties for mouse action;

https://reviewboard.mozilla.org/r/145968/#review150140

I think this is fine, but I guess the overall question I’m left with is if this is what the specification intends.  The way I read this, it sets the modifier states of the MouseEvent if any of the keyboard devices have a positive value.  Effectively this means that one keyboards’ modifier state affects _all_ mouse events.

In the real world, I guess this is what you would expect.  If you have two keyboards connected to a computer and either of them hold down control, clicking will be affected by this modifier.

::: testing/marionette/action.js:909
(Diff revision 1)
>    update(inputState) {
>      let allButtons = Array.from(inputState.pressed);
>      this.buttons = allButtons.reduce((a, i) => a + Math.pow(2, i), 0);
>    }
> +
> +  updateModifiers() {

If we don’t need to call this outside of action.Mouse, maybe it should live in the ctor?  

Another concern is the name ‘updateModifiers’, as it really does not tell me anything.

::: testing/marionette/action.js:914
(Diff revision 1)
> +  updateModifiers() {
> +    this.altKey = false;
> +    this.shiftKey = false;
> +    this.metaKey = false;
> +    this.ctrlKey = false;
> +    for (var inputState of action.inputStateMap.values()) {

s/var/let/

::: testing/marionette/action.js:916
(Diff revision 1)
> +          this.altKey = inputState.alt || this.altKey;
> +          this.ctrlKey = inputState.ctrl || this.ctrlKey;
> +          this.metaKey = inputState.meta || this.metaKey;
> +          this.shiftKey = inputState.shift || this.shiftKey;

Indentation is off.

::: testing/marionette/action.js:916
(Diff revision 1)
> +          this.altKey = inputState.alt || this.altKey;
> +          this.ctrlKey = inputState.ctrl || this.ctrlKey;
> +          this.metaKey = inputState.meta || this.metaKey;
> +          this.shiftKey = inputState.shift || this.shiftKey;
Attachment #8874622 - Flags: review?(ato) → review+
Comment on attachment 8874622 [details]
Bug 1367430 - Set modifier-key properties for mouse action;

https://reviewboard.mozilla.org/r/145968/#review150140

> If we don’t need to call this outside of action.Mouse, maybe it should live in the ctor?  
> 
> Another concern is the name ‘updateModifiers’, as it really does not tell me anything.

I agree -- moved to ctor.
Attachment #8874932 - Attachment is obsolete: true
Pushed by mjzffr@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/afcfa17be8ac
Set modifier-key properties for mouse action; r=ato
sorry had to back this out seems this made the failure in bug 1365021 more worse like perma failures -> https://treeherder.mozilla.org/logviewer.html#?job_id=105143585&repo=autoland
Flags: needinfo?(mjzffr)
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/504c134cc674
Backed out changeset afcfa17be8ac for making bug  1365021 worse
Flags: needinfo?(mjzffr)
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/9c79509cd756
Set modifier-key properties for mouse action; r=ato
https://hg.mozilla.org/mozilla-central/rev/9c79509cd756
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Product: Testing → Remote Protocol
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: