Closed Bug 1421323 Opened 6 years ago Closed 6 years ago

WebDriver:PerformActions: Control + click should synthesize a contextmenu event

Categories

(Remote Protocol :: Marionette, defect, P2)

defect

Tracking

(firefox62 fixed)

RESOLVED FIXED
mozilla62
Tracking Status
firefox62 --- fixed

People

(Reporter: impossibus, Assigned: impossibus)

References

(Blocks 2 open bugs)

Details

Attachments

(1 file)

Currently, Marionette synthesizes a mousedown, mouseup, click when ctrl is true, but it should be mousedown, contextmenu, mouseup.
I'm going to have to rebase this after Bug 1385476 lands, since it takes care of removing some broken tests.
Comment on attachment 8932554 [details]
Bug 1421323 - Ctrl+click should synthesize a contextmenu event;

https://reviewboard.mozilla.org/r/203604/#review209060

::: commit-message-cad9c:1
(Diff revision 1)
> +Bug 1421323 - Ctrl+click should synthesize a contextmenu event; r?ato

Is this true on Windows and Linux as well?

::: testing/marionette/action.js:1212
(Diff revision 1)
> +        if (mouseEvent.ctrlKey === true) {
> +          mouseEvent.button = 2;
> +        }

So holding down Control always overrides the mouse button of the
generated event?  In other words, if I press button 1 on my mouse
whilst holding down Control, the MouseEvent will show button 2?
Attachment #8932554 - Flags: review?(ato) → review-
Comment on attachment 8932554 [details]
Bug 1421323 - Ctrl+click should synthesize a contextmenu event;

https://reviewboard.mozilla.org/r/203604/#review209060

> Is this true on Windows and Linux as well?

True on Linux, Mac. But, yay, I got ahead of myself, not on Windows. Windows just treats it as a regular click.

> So holding down Control always overrides the mouse button of the
> generated event?  In other words, if I press button 1 on my mouse
> whilst holding down Control, the MouseEvent will show button 2?

Yes, exactly. (Except, on Windows.)
[Mass update] Setting P1 as currently being worked on.
Priority: -- → P1
Do you have time to fix up your patch?
Flags: needinfo?(mjzffr)
Not at the moment. Will set aside some time for it next week.
Flags: needinfo?(mjzffr)
Reducing priority since it is not actively worked on.
Blocks: webdriver
OS: Unspecified → All
Priority: P1 → P2
Hardware: Unspecified → All
Version: Version 3 → Trunk
Summary: [actions] Ctrl+click should synthesize a contextmenu event → WebDriver:PerformActions: Control + click should synthesize a contextmenu event
Comment on attachment 8932554 [details]
Bug 1421323 - Ctrl+click should synthesize a contextmenu event;

https://reviewboard.mozilla.org/r/203604/#review257372


Code analysis found 2 defects in this patch:
 - 2 defects found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: testing/marionette/event.js:934
(Diff revision 2)
> + * @param {string} name
> + *     Lowercase platform name substring, e.g. "mac", "win"
> + * @param {Window=} window
> + *     Window object.  Defaults to the current window.
> + */
> +event.isPlatform = function (name, win = window) {

Error: Unexpected space before function parentheses. [eslint: space-before-function-paren]

::: testing/marionette/event.js:941
(Diff revision 2)
> +    try {
> +      return win.navigator.platform.toLowerCase().indexOf(name) > -1;
> +    } catch (ex) {}
> +  }
> +  return navigator.platform.toLowerCase().indexOf(name) > -1;
> +}

Error: Missing semicolon. [eslint: semi]
Comment on attachment 8932554 [details]
Bug 1421323 - Ctrl+click should synthesize a contextmenu event;

https://reviewboard.mozilla.org/r/203604/#review257418

This is excellent!  Please feel free to land this after you’ve
resolved/dismissed my issues.  I have no fundamental problems with
this patch.

::: testing/marionette/action.js:1219
(Diff revision 3)
>  
>      switch (inputState.subtype) {
>        case action.PointerType.Mouse:
>          let mouseEvent = new action.Mouse("mousedown", a.button);
>          mouseEvent.update(inputState);
> -        if (event.DoubleClickTracker.isClicked()) {
> +        if (mouseEvent.ctrlKey === true) {

If this is a boolean, is the comparison to true necessary?  Wouldn’t
just "if (mouseEvent.ctrlKey)" be sufficient?

::: testing/marionette/event.js:926
(Diff revision 3)
>  /**
> + * Test whether the platform matches the given name.
> + *
> + * @param {string} name
> + *     Lowercase platform name substring, e.g. "mac", "win"
> + * @param {Window=} window
> + *     Window object.  Defaults to the current window.
> + */
> +event.isPlatform = function(name, win = window) {
> +  if (win) {
> +    try {
> +      return win.navigator.platform.toLowerCase().indexOf(name) > -1;
> +    } catch (ex) {}
> +  }
> +  return navigator.platform.toLowerCase().indexOf(name) > -1;
> +};

You could replace this entire function with Services.appinfo.OS, e.g.:

> Services.appinfo.OS == "Darwin" ? "Meta" : "Control"

::: testing/web-platform/tests/webdriver/tests/actions/modifier_click.py:51
(Diff revision 3)
> +    # The 800 pause avoids a dblclick
>      key_chain \
>          .pause(0) \
> -        .key_down(Keys.CONTROL) \
> +        .key_down(Keys.ALT) \
>          .key_down(Keys.SHIFT) \
> -        .pause(0) \
> +        .pause(800) \

Can we please put this in a global variable with a descriptive name?

I have an aversion against magical hard-coded numbers.  This would
also save you the comment.
Attachment #8932554 - Flags: review?(ato) → review+
Pushed by mjzffr@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/d62ce3e3ee8a
Ctrl+click should synthesize a contextmenu event; r=ato
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/11532 for changes under testing/web-platform/tests
https://hg.mozilla.org/mozilla-central/rev/d62ce3e3ee8a
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
One question folks. Maja who reported this is using MAC, isn't she? TBH, this is a MACos specific behavior, not something one would expect in other systems like Linux and Windows, where holding CTRL + left clicking shouldn't fire the context menu. Would it be possible to revert this change?
(In reply to tplevko from comment #20)
> One question folks. Maja who reported this is using MAC, isn't she? TBH,
> this is a MACos specific behavior, not something one would expect in other
> systems like Linux and Windows, where holding CTRL + left clicking shouldn't
> fire the context menu. Would it be possible to revert this change?

As I understand it, it’s macOS and Linux specific behaviour.  See
the != "WINNT" conditions in the patch:
https://hg.mozilla.org/mozilla-central/rev/d62ce3e3ee8a
I'm sorry, but I'm a Linux user for almost 5 years now and I'm pretty sure, it's not how it should work.
Didn't actually try this on Windows, but it is causing us serious problems in our linux based test environment.
If anyone else runs into this issue, we solved the problem (i.e. being unable to control-click custom elements to perform a multiselect in Firefox>=62) by changing our code to use META instead of CONTROL. Not exactly sure about the logic behind this, but it works in both Firefox and Chrome on linux.
Thanks for your feedback.  I think we should follow up on these
findings in a new bug so we can have it properly investigated.
Regressions: 1566644
Product: Testing → Remote Protocol
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: