1370403 - Synthesize contextmenu MouseEvent when performing webdriver actions

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, enhancement)

Description

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Follow-up from Bug #1367430:

When we perform a ctrl+click via performActions in Marionette, we should see a contextmenu MouseEvent but we don't.

Comment 1

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

This is should work in a similar way to what we get from event.js when we perform a mousedown followed by mouseup: in that case, a "click" MouseEvent is also emitted.

Comment 2

[muthuraj90ec]

Hi I have a possible fix for this, could some one with more privileges and expertise help reviewing the patch i have?

Comment 3

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Of course. Please go ahead and upload a patch. I will take a look.

Comment 4

[muthuraj90ec]

Attachment: action.js in mozilla-central/testing/marionette/action.js

This patch is to trigger contextmenu event when geckodriver sends pointer down, pointer up event

Comment 5

[muthuraj90ec]

Attachment: patch action.js in mozilla-central/testing/marionette/action.js

Marionette does not send a contextmenu event when selenium tries a context click (gecko driver sends pointer down and pointer up events).

Comment 6

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8890113 [details] [diff] [review]
patch action.js in  mozilla-central/testing/marionette/action.js

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

Thanks for the patch. Next time you submit a patch, please request review by setting the r? flag with my name so I get a notification.

This is on the right track, but I'd like to see the logic moved to testing/marionette/event.js.

One issue to fix: that this patch will result in mousedown,mouseup,(click),contextmenu, but we want to match actual Firefox behaviour which is mousedown,contextmenu,mouseup.

A complete solution would also account for contextmenu being fired when button 0 is pressed with the ctrl key. 

You should test your solution by writing a web-platform-test in testing/web-platform/tests/webdriver/tests/actions/mouse.py, which uses testing/web-platform/tests/webdriver/tests/actions/support/test_actions_wdspec.html.

To run the test: ./mach wpt testing/web-platform/tests/webdriver/tests/actions/mouse.py

If you have any questions, feel free to need-info me on this bug or ping me (maja_zf) in on IRC in #ateam

Comment 7

[muthuraj90ec]

(In reply to Maja Frydrychowicz (:maja_zf) from comment #6)
> Comment on attachment 8890113 [details] [diff] [review]
> patch action.js in  mozilla-central/testing/marionette/action.js
> 
> Review of attachment 8890113 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks for the patch. Next time you submit a patch, please request review by
> setting the r? flag with my name so I get a notification.
> 
> This is on the right track, but I'd like to see the logic moved to
> testing/marionette/event.js.
> 
> One issue to fix: that this patch will result in
> mousedown,mouseup,(click),contextmenu, but we want to match actual Firefox
> behaviour which is mousedown,contextmenu,mouseup.
> 
> A complete solution would also account for contextmenu being fired when
> button 0 is pressed with the ctrl key. 
> 
> You should test your solution by writing a web-platform-test in
> testing/web-platform/tests/webdriver/tests/actions/mouse.py, which uses
> testing/web-platform/tests/webdriver/tests/actions/support/
> test_actions_wdspec.html.
> 
> To run the test: ./mach wpt
> testing/web-platform/tests/webdriver/tests/actions/mouse.py
> 
> If you have any questions, feel free to need-info me on this bug or ping me
> (maja_zf) in on IRC in #ateam

Thanks for the feedback, i will upload another patch when i am done with the modification.

Comment 8

[muthuraj90ec]

Attachment: contextmenuonrightclick.patch

Requesting review:

1) Moved the logic from testing/marionette/action.js to testing/marionette/event.js

2) Tested solution by writing a web-platform test in mouse.py

3) Ran ./mach wpt (all tests passed)

4) pending -> fire contextmenu when button 0 is pressed with ctrl key

=========================================================================
Clarification: I tried right click manually in the test_actions_wdspec.html

and the order of event printed in the page was:
mousedown
mouseup
contextmenu

I have modified event.js to behave in the same manner.

Let me know if I am going in the right direction, Thanks

Comment 9

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

(In reply to muthuraj90ec from comment #8)
> Clarification: I tried right click manually in the test_actions_wdspec.html
> 
> and the order of event printed in the page was:
> mousedown
> mouseup
> contextmenu

Oh interesting. What platform are you on? On macos and linux I see mousedown, contextmenu, mouseup.

Comment 10

[muthuraj90ec]

(In reply to Maja Frydrychowicz (:maja_zf) from comment #9)
> (In reply to muthuraj90ec from comment #8)
> > Clarification: I tried right click manually in the test_actions_wdspec.html
> > 
> > and the order of event printed in the page was:
> > mousedown
> > mouseup
> > contextmenu
> 
> Oh interesting. What platform are you on? On macos and linux I see
> mousedown, contextmenu, mouseup.

I am testing this on windows. Also manually doing a ctrl + button 0 on windows does not synthesize a contextmenu event, but it does on macos

Comment 11

[muthuraj90ec]

I did try right clicking manually in the ClickReporter div in test_actions_wdspec.html

on three platforms

Linux:

mousedown
cotextmenu
mouseup

Mac:

mousedown
contextmenu
mouseup

Windows:

mousedown
mouseup
contextmenu

On windows the order is different from other two oses,

Does this mean the actual behaviour is mousedown, contextmenu, mouseup and windows case is an anomaly? And the right behavior is mousedown, contextmenu, mouseup ?

Comment 12

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Thanks for checking! I found the same. It also varies across browsers. The Pointer Events spec states that the order can vary, too, so for the web-platform test, please just check that one 'contextmenu' event has fired.

As for what to do in Marionette... I didn't find any clues in the gecko source regarding why Windows is different.
However, the legacy actions implementation synthesized mousedown followed immediately by contextmenu, so let's go with that here, too.

Comment 13

[Andreas Tolfsen ❲:ato❳]

(In reply to Maja Frydrychowicz (:maja_zf) from comment #12)

> As for what to do in Marionette... I didn't find any clues in
> the gecko source regarding why Windows is different.  However,
> the legacy actions implementation synthesized mousedown followed
> immediately by contextmenu, so let's go with that here, too.

With some newfound knowledge of how Gecko interacts with widget
toolkits, in particular GTK, it would not surprise me if the
platform independence cited here has to do with platform-specific
toolkit library callbacks.

It doesn’t seem far fetched that the events may arrive in a
slightly different order because requesting a platform-specific
context menu might not be synchronous on all platforms.

Comment 14

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8891092 [details] [diff] [review]
contextmenuonrightclick.patch

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

This is on the right track.

::: testing/marionette/event.js
@@ +52,5 @@
>    altKey: 2,
>    metaKey: 3,
>  };
>  
> +event.MouseButton = {

Rather than calling the buttons "left", "right", which can be misleading, call them "primary" (or "main"), "secondary". See https://developer.mozilla.org/en-US/docs/Web/API/MouseEvent/button

@@ +71,5 @@
> +      } else {
> +        return ["mouseup"];
> +      }
> +    },
> +    unmapped: function() {

Rather than having an `unmapped` property, define mouseEventMap as an actual Map object and then use `mouseEventMap.has()` to decide what to return.

@@ +75,5 @@
> +    unmapped: function() {
> +      return [mouseEvent];
> +    }
> +  }
> +  return (mouseEventMap[mouseEvent] || 

There are a couple of style issues: please run ./mach lint testing/marionette and fix any errors next time you submit a patch for review -- thanks!

::: testing/web-platform/tests/webdriver/actions/mouse.py
@@ +118,5 @@
> +        "x": 82,
> +        "y": 187,
> +    }
> +    mouse_chain \
> +        .pointer_move(div_point["x"], div_point["y"], duration=1000) \

Omit the duration: it should be okay to have an instantaneous pointer move here, and we want the test to take as little time as possible.

@@ +124,5 @@
> +        .pointer_up(button=2) \
> +        .perform()
> +    events = get_events(session)
> +    assert len(events) == 4
> +    expected = [

Since this varies across user agents and platforms, just check that there's a contextmenu at some point after a mousedown.

::: testing/web-platform/tests/webdriver/actions/support/test_actions_wdspec.html
@@ +63,5 @@
>                `keyCode: ${event.keyCode})`);
>          }
>  
>          function recordPointerEvent(event) {
> +          if(event.type === "contextmenu") event.preventDefault();

Style nit: please use the multi-line form for the if block, with {}

Comment 15

[muthuraj90ec]

Attachment: contextmenuonrightclick.patch

Comment 16

[Andreas Tolfsen ❲:ato❳]

(In reply to Maja Frydrychowicz (:maja_zf) from comment #14)

> @@ +75,5 @@
> > +    unmapped: function() {
> > +      return [mouseEvent];
> > +    }
> > +  }
> > +  return (mouseEventMap[mouseEvent] || 
> 
> There are a couple of style issues: please run ./mach lint
> testing/marionette and fix any errors next time you submit a patch for
> review -- thanks!

Many issues, eslint is also able to fix itself:

    % ./mach lint --fix testing/marionette/action.js

Comment 17

[muthuraj90ec]

Attachment: contextmenuonrightclick.patch

Please discard the previous review,

ran

./mach lint --fix testing/marionette/event.js

Thanks

Comment 18

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8891637 [details] [diff] [review]
contextmenuonrightclick.patch

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

Great, I think we're almost there. 

There may be a bit of bit rot -- before you push up for rereview, please rebase your changes onto the latest mozilla-central. Then I'll push to the try server for you to run your changes in CI.

::: testing/marionette/event.js
@@ +55,5 @@
>  
> +event.MouseButton = {
> +  isPrimary(button) {
> +    return button === 0;
> +  }, isSecondary(button) {

Nit: Please start each function definition on a new line and order them by 0, 1, 2...

@@ +62,5 @@
> +    return button === 1;
> +  }
> +};
> +
> +event.mouseEventsToSend = function(mouseEvent, button) {

Since this is more of a helper function, we don't need to expose via the event object: remove `event.`

Also, it might be more useful to pass the whole event object as an argument instead of the type and button individually.

@@ +64,5 @@
> +};
> +
> +event.mouseEventsToSend = function(mouseEvent, button) {
> +  let mouseEventMap = new Map();
> +  mouseEventMap.set("mouseup", (button) => {

I think this would make more sense as mapping "mousedown" to "mousedown", "contextmenu".

Then you could also check for the ctrl property on the event, as well as isWin (see isMac defined in event.js) to take care of the ctrl+click case as well.

@@ +68,5 @@
> +  mouseEventMap.set("mouseup", (button) => {
> +    if (event.MouseButton.isSecondary(button)) {
> +      return ["contextmenu", "mouseup"];
> +    }
> +      return ["mouseup"]

This line is overindented. Please add a semi-colon as well.

@@ +283,5 @@
>  
>    if (("type" in opts) && opts.type) {
> +
> +    let mouseEvents = event.mouseEventsToSend(opts.type, button);
> +    for (var i = 0; i < mouseEvents.length; i++) {

let instead of var

::: testing/web-platform/tests/webdriver/actions/mouse.py
@@ +50,5 @@
>          {"type": "click", "buttons": 0},
>      ]
>      filtered_events = [filter_dict(e, expected[0]) for e in events]
>      assert expected == filtered_events[1:]
>  

Style: each top-level function should be separated by two blank lines.

@@ +68,5 @@
> +        {"type": "contextmenu", "button": 2},
> +    ]
> +    assert len(events) == 4
> +    filtered_events = [filter_dict(e, expected[0]) for e in events]
> +    filtered_events.remove({"type": "mouseup",  "button": 2})

This will fail if the mouseup isn't there. Since all we intend to check is whether a contextmenu follows a mousedown, I suggest you filter the list to only keep contextmenu and mousedown, preserving order.

::: testing/web-platform/tests/webdriver/actions/support/test_actions_wdspec.html
@@ +66,5 @@
>          function recordPointerEvent(event) {
> +          if(event.type === "contextmenu") {
> +            event.preventDefault();
> +          }
> +          

Please remove the empty line here.

Comment 19

[muthuraj90ec]

Attachment: contextmenurightclick.patch

I tried manually clicking ctrl + mouseclick in the ClickReporter div,

The contextmenu event was only triggered in MAC and not in Windows and Linux.

In mac the events triggered when ctrl + mouseclick was

mousedown,
contextmenu,
mouseup

Could you confirm this?

Also I am passing in the mouseevents object to the function mouseEventsToSend(),

so the modifier state could be checked before mapping the events, but i did not implement it as I did not have a MAC build to confirm the changes so left it as it is.

If its fine could you make the ctrl + mouseclick change and submit it along with this patch to the try server?

Comment 20

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

I see contextmenu with ctrl+click on Mac and one Linux distro (on our test machines), but not another, so I guess it varies based on OS settings in linux.

When I do see contextmenu, control + button 0 yields the following:

> mousedown(button: 2, pageX: 62, pageY: 181, button: 2, buttons: 2, ctrlKey: true, altKey: false, metaKey: false, shiftKey: false, target id: outer)
> contextmenu(button: 2, pageX: 62, pageY: 181, button: 2, buttons: 2, ctrlKey: true, altKey: false, metaKey: false, shiftKey: false, target id: outer)
> mouseup(button: 2, pageX: 62, pageY: 181, button: 2, buttons: 0, ctrlKey: true, altKey: false, metaKey: false, shiftKey: false, target id: outer)

Note that the reported button is 2! Even though we're pressing button 0. I suspect this is due to something happening at the operating system level, so let's just button 2 alone, no control-clicking.

Comment 21

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Initial try push for your patch is green (web-platform-tests only): https://treeherder.mozilla.org/#/jobs?repo=try&revision=b60a04f1b718d659060ef06e5c3c17ce92a67aa4


Bigger try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=73e426e5d064a974be06bf8ad432776bc78d35ec

Comment 22

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8892670 [details] [diff] [review]
contextmenurightclick.patch

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

Looks good. One small thing to fix and we can land it if the try pushes are happy.

Please make sure your final commit message for the patch is as follows: "Bug 1370403 - Synthesize contextmenu MouseEvent when performing webdriver actions; r=maja_zf"

If you're interested, Bug 1385476 is a direct follow-up.

::: testing/web-platform/tests/webdriver/tests/actions/mouse.py
@@ +68,5 @@
> +    assert len(events) == 4
> +    filtered_events = [filter_dict(e, expected[0]) for e in events]
> +    mousedown_contextmenu_events = [
> +        x for x in filtered_events
> +        if x["type"] == "mousedown" or x["type"] == "contextmenu"

for brevity: if x["type"] in ["mousedown", "contextmenu"]

Comment 23

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8892670 [details] [diff] [review]
contextmenurightclick.patch

Hey Andreas, I'm setting a f? on you for this in case you have something to add, since you usually review the actions work for me. 

The patch looks good to me.

Comment 24

[muthuraj90ec]

Attachment: contextmenurightclick.patch

Thanks,

I ran the following after making the fix,
$ hg ci -m "Bug 1370403 - Synthesize contextmenu MouseEvent when performing web
driver actions; r=maja_zf" -I testing/marionette/event.js  -I testing/web-platf
orm/tests/webdriver/tests/actions/mouse.py -I testing/web-platform/tests/webdri
ver/tests/actions/support/test_actions_wdspec.html

Is this the final step?

Comment 25

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8893136 [details] [diff] [review]
contextmenurightclick.patch

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

::: testing/marionette/event.js
@@ +79,5 @@
> +  });
> +  if (mouseEventMap.has(mouseEvent.type)) {
> +    return mouseEventMap.get(mouseEvent.type)(mouseEvent);
> +  }
> +  return [mouseEvent.type];

I don’t think we should be overloading synthesizeMouseAtPoint to
imply other events when a specific one is requested.  This is a
breach of the API contract.

If we look at the current code and simplify, it currently does this:

> function synthesizeMouseAtPoint(x, y, opts, win) {
>   if (opts.type) {
>     domutils.sendMouseEvent(opts.type, …);
>   } else {
>     domutils.sendMouseEvent("mousedown", …);
>     domutils.sendMouseEvent("mouseup", …);
>   }
> }

If opts.type is given emit that custom event, otherwise default to
fire a mousedown and a mouseup event.  In other words, for as long
as the caller is not specific, fall back to the sensible default of
emitting mousedown + mouseup for a normal click.

The actions module uses it in this way, where mouseEvent makes out
the opts argument from the above code sample:

> let mouseEvent = new action.Mouse("mousedown", a.button);
> mouseEvent.update(inputState);
> event.synthesizeMouseAtPoint(
>     inputState.x,
>     inputState.y,
>     mouseEvent,
>     win);

The actions module makes one invocation of
event.synthesizeMouseAtPoint per action item because this is how the
WebDriver action API is supposed to work: it gives you exact control
over when to hold down a mouse button and when to release it.  If
this function were to be called _without_ a specific type, it would
think you wanted a regular click (mousedown + mouseup).

Overloading synthesizeMouseAtPoint(x, y, {type: "mousedown"}) to
also imply the contextmenu event is wrong because the consumer
is asking for one event explicitly.  On the other hand, I would
expect synthesizeMouseAtPoint(x, y, {button: 2}) to perhaps imply
contextmenu for the same reason that synthesizeMouseAtPoint(x, y)
implies mousedown + mouseup, but not for the request of a specific
type to be overloaded with another meaning.

Now, as I said, because the actions module operates at this
granularity, I’m afraid we have to build this functionality
into the module itself, or alternatively make {button: 2}
imply that contextmenu is fired.  We are not bound to pass the
action.Mouse instance we assign to mouseEvent as options for
synthesizeMouseAtPoint, so this could work too.

My suggestion would be to make the following change to
dispatchPointerDown:

> let mousedown = new action.Mouse("mousedown", a.button);
> mousedown.update(inputState);
> event.synthesizeMouseAtPoint(…, mousedown);
> 
> if (event.MouseButton.isSecondary(a.button)) {
>   event.synthesizeMouseAtPoint(…, {type: "contextmenu"});
> }

The alternative would be to synthesise an extra contextmenu event by
calling synthesizeMouseAtPoint(…, {button: 2, …});.

@@ +296,5 @@
>    }
>  
>    if (("type" in opts) && opts.type) {
> +    let mouseEvents = mouseEventsToSend(opts, window);
> +    for (let i = 0; i < mouseEvents.length; i++) {

Use Array#forEach.

@@ +729,5 @@
>    }
>    return navigator.platform.indexOf("Mac") > -1;
>  }
>  
> +function isWin_(win = window) {

This function is unused.

::: testing/web-platform/tests/webdriver/tests/actions/support/test_actions_wdspec.html
@@ +63,5 @@
>                `keyCode: ${event.keyCode})`);
>          }
>  
>          function recordPointerEvent(event) {
> +          if(event.type === "contextmenu") {

Space after "if".

Comment 26

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8893136 [details] [diff] [review]
contextmenurightclick.patch

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

> Thanks, I ran the following after making the fix, ...
Yes, except it should be "WebDriver" not "web driver".  

I talked some more with Andreas, and I think the best thing to do at this point is to remove mouseEventsToSend and simply add the contextmenu code to action.js:

> let mousedown = new action.Mouse("mousedown", a.button);
> mousedown.update(inputState);
> event.synthesizeMouseAtPoint(…, mousedown);
> 
> if (event.MouseButton.isSecondary(a.button)) {
>   event.synthesizeMouseAtPoint(…, {type: "contextmenu"});
> }

Forgive for the back-and-forth, but it turns out this issue may require some more investigation on our part. I'll accept your proposed fix (provided you incorporate the above feedback as well as Andreas' other comments), and I'll follow-up in a separate bug with the rest if needed. Please request r?, not f? when you submit your final patch. Thanks!

Comment 27

[muthuraj90ec]

Attachment: contextmenurightclick.patch

I understand, moved the logic to action.js

Comment 28

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8894526 [details] [diff] [review]
contextmenurightclick.patch

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

Thanks. Please rebase your patch onto the latest mozilla-central so I can push to try and land.

::: testing/marionette/action.js
@@ +1216,5 @@
> +        if (event.MouseButton.isSecondary(a.button)) {
> +          event.synthesizeMouseAtPoint(
> +              inputState.x,
> +              inputState.y,
> +              {type: "contextmenu", button: a.button},

We need to pass in the other event properties we've computed, so provide a clone of mouseEvent with just the type changed: e.g. `Object.assign({}, mouseEvent, {type: "contextmenu"})`

Comment 29

[muthuraj90ec]

Attachment: contextmenurightclick.patch

Rebased changes.

Comment 30

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

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

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/44f678643644
Synthesize contextmenu MouseEvent when performing webdriver actions; r=maja_zf

Comment 32

[muthuraj90ec]

From the following link,
https://hg.mozilla.org/integration/mozilla-inbound/rev/44f678643644

I noticed that this change has a release milestone of 57.0a1

The current version of firefox is 55.0, Is it possible to have this change released as a bug fix release of firefox, for eg: 55.0.1 ? The reason for me to request this, is my current progress is blocked by this. And I am still using the old firefox driver extension.

Thanks

Comment 33

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

action.js (the new spec-conformant actions implementation), and therefore your fix, is only available via geckodriver. 

In any case, this kind of fix isn't usually approved for uplift to a dot release. Given the timing, it certainly wouldn't make it to 55.0.1.

If you're using the firefox-driver extension, your work may actually depend on a bug elsewhere in the Marionette codebase or something else altogether. You can follow-up with us by describing your issue on #ateam in irc.mozilla.org.

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/44f678643644

Comment 35

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Please uplift this test-only change to beta.

Comment 36

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/9c5b86d50b9b