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.

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.

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

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

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

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

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

(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.

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

(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.

(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

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 ?

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.

(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 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 {}

(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

Please discard the previous review, ran ./mach lint --fix testing/marionette/event.js Thanks

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.

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?

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.

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 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 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.

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 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 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!

I understand, moved the logic to action.js

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"})`

Rebased changes.

Pushed by mjzffr@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/44f678643644 Synthesize contextmenu MouseEvent when performing webdriver actions; r=maja_zf

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

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.

Please uplift this test-only change to beta.