Web Extension menus should be able to react to middle click

VERIFIED FIXED in Firefox 64

Status

enhancement
P5
normal
VERIFIED FIXED
11 months ago
6 months ago

People

(Reporter: Kwan, Assigned: arshadkazmi42, Mentored)

Tracking

({dev-doc-needed, good-first-bug})

unspecified
mozilla64

Firefox Tracking Flags

(firefox64 verified)

Details

(Whiteboard: [design-decision-approved])

Attachments

(3 attachments)

(Reporter)

Description

11 months ago
If you right-click on an image in a web-page in Firefox one of the menu entries listed is "View Image".  Left-clicking this item navigates the current tab to the image.  This item also has the interesting and useful behaviour that if you instead middle-click it, it will open a new tab navigated to the image.  It would be useful to be able to replicate such behaviour with web extensions.

While the UI Events spec has an "auxclick" event[1] for non-primary clicks such as the middle button which could be used as a name, that also covers the right-button, so we might need to use "middleclick" instead to avoid confusingly different behaviour from the web.

Menuitems, both native and web extension, currently react to right-clicks as regular clicks (at least on Linux), which is why I think right-click should be left out.

Rough draft of what the actual API changes would look like:

New event:
browser.menus.on(Aux|Middle)clicked

New createProperty:
browser.menus.create({ on(aux|middle)click: function });


A possible alternative would be to fire onclicked on middle-button click, and provide a new 'button(s?)' property to OnClickedData to distinguish, though I wonder if that'd be safely backwards-compatible with extensions that aren't aware of said button property and suddenly start treating middle-clicks as left-clicks.  A new event has the advantage of extension authors having to opt-in.


Relatedly, native menuitems react to Ctrl+click as if middle clicked (possibly platform-dependant).  onClicked already provides modifiers in OnClickData which can be used to follow this behaviour.

[1]: https://w3c.github.io/uievents/#event-type-auxclick

Updated

11 months ago
Product: Toolkit → WebExtensions

Comment 1

11 months ago
I can confirm the missing behavior here on Windows 10 x64 and Ubuntu 18.10x64 with Firefox 62.0a1 (06-19), setting component accordingly.
Component: Untriaged → General

Updated

11 months ago
Component: General → Frontend
Whiteboard: [design-decision-needed]
Hi Ian, this bug has been added to the agenda for the WebExtensions APIs triage on July 10. Would you be able to join us? 

Here’s a quick overview of what to expect at the triage: 

* We normally spend 5 minutes per bug
* The more information in the bug, the better
* The goal of the triage is to give a general thumbs up or thumbs down on a proposal; we won't be going deep into implementation details

Relevant Links: 

* Wiki for the meeting: https://wiki.mozilla.org/WebExtensions/Triage#Next_Meeting
* Meeting agenda: https://docs.google.com/document/d/1Y_oYPldTT_kQOOouyJbC-8y3ASIizScLKFRhQfsDQWI/edit#heading=h.mr8julfcusgv
* Vision doc for WebExtensions: https://wiki.mozilla.org/WebExtensions/Vision
Flags: needinfo?(rob)
Priority: -- → P5
Whiteboard: [design-decision-needed] → [design-decision-approved]

Comment 3

10 months ago
This feature request has been approved, but as a new property instead of a new event.

Currently middle-clicking has no effect on extension menus on Linux, macOS and Windows.
The risk of unexpectedly triggering "onClicked" upon middle-click is likely negligible.
Flags: needinfo?(rob)

Comment 4

9 months ago
Bug 1405031 has some similarities to this bug. Both bugs request the ability to detect the used mouse key.
The difference is that this bug requests that property for the menus.onClicked event, whereas the other one requests the functionality for the pageAction.onClicked and browserAction.onClicked event.
Mentor: rob
See Also: → 1405031
(Assignee)

Comment 5

9 months ago
Hey Rob
I would like to work on this. Can you guide me where can I start with for this issue?
Flags: needinfo?(rob)

Comment 6

9 months ago
When a menu item is clicked with the PRIMARY mouse button, the following "command" event handler is triggered:
https://searchfox.org/mozilla-central/rev/55da592d85c2baf8d8818010c41d9738c97013d2/browser/components/extensions/parent/ext-menus.js#275

That handler creates an "info" object with the details of the event, and dispatches the onClicked event of the menus API:
https://searchfox.org/mozilla-central/rev/55da592d85c2baf8d8818010c41d9738c97013d2/browser/components/extensions/parent/ext-menus.js#297-317
https://searchfox.org/mozilla-central/rev/55da592d85c2baf8d8818010c41d9738c97013d2/browser/components/extensions/parent/ext-menus.js#926-937

The "command" event is only triggered when the left/right button is clicked, or even through non-mouse actions such as a keypress on the keyboard.
The event object passed to the "command" event handler does not contain any information about the mouse button.

So, you need to capture this information by other means, for example via the event.button property of the "auxclick" event:
https://w3c.github.io/uievents/#event-type-auxclick
https://developer.mozilla.org/en-US/docs/Web/API/MouseEvent/button



You could move the command handler to a separate function, and use that function for both the auxclick and command handler.
Make sure that you call event.preventDefault() to avoid dispatching the menus.onClicked event twice (e.g. when the context menu is clicked).

The easiest way to use this button information is to just forward this event.button value.

Aside from the above implementation, you need two other changes, as usual:
- Expand the schema: Declare the "button" property (of the integer type) (e.g. after "modifiers") at
  https://searchfox.org/mozilla-central/source/browser/components/extensions/schemas/menus.json

- Add unit tests to verify that when the middle mouse button is used, that the info.button property is set to 1.

For inspiration on writing menu/click tests, see the existing examples:
https://searchfox.org/mozilla-central/search?q=onClicked&case=false&regexp=false&path=extensions%2Ftest%2Fbrowser%2F*menu*
These tests simulate a click using the closeExtensionContextMenu utility function, defined here:
https://searchfox.org/mozilla-central/rev/2fe43133dbc774dda668d4597174d73e3969181a/browser/components/extensions/test/browser/head.js#344-356
I will leave it as an exercise to you for how you can simulate the middle mouse button using closeExtensionContextMenu. Hint: look at the implementation of synthesizeMouseAtCenter in https://searchfox.org/mozilla-central/source/testing/mochitest/tests/SimpleTest/EventUtils.js
Assignee: nobody → arshadkazmi42
Status: NEW → ASSIGNED
Flags: needinfo?(rob)
Keywords: good-first-bug
(Assignee)

Comment 7

9 months ago
Thanks Rob, for the details explanation.

I will look into it later today.
(Assignee)

Comment 8

9 months ago
Rob, So I was going through the information you provided.
So I have to change this piece of code 

https://searchfox.org/mozilla-central/rev/55da592d85c2baf8d8818010c41d9738c97013d2/browser/components/extensions/parent/ext-menus.js#275

Mode everything from this command event handler to seperate function and call that function on this event,
And then I have to add an auxClick event which will also call the same function

But I didn't get this, where I have to update information about mouse button?

Let me know if my I am on the right track.

Below is a piece of code what I am trying to do

    element.addEventListener("command", event => { // eslint-disable-line mozilla/balanced-listeners
      this.eventCommandHandler(event, item, contextData)
    });

    element.addEventListener("auxclick", function(e) {
      this.eventCommandHandler(event, item, contextData)
    }); 

And this is the function I have created, which contains the handler

eventCommandHandler(event, item, contextData) {
    if (event.target !== event.currentTarget) {
      return;
    }
    const wasChecked = item.checked;
    if (item.type == "checkbox") {
      item.checked = !item.checked;
    } else if (item.type == "radio") {
      // Deselect all radio items in the current radio group.
      for (let child of item.parent.children) {
        if (child.type == "radio" && child.groupName == item.groupName) {
          child.checked = false;
        }
      }
      // Select the clicked radio item.
      item.checked = true;
    }

    if (contextData.tab) {
      item.tabManager.addActiveTabPermission(contextData.tab);
    }

    let info = item.getClickInfo(contextData, wasChecked);

    const map = {shiftKey: "Shift", altKey: "Alt", metaKey: "Command", ctrlKey: "Ctrl"};
    info.modifiers = Object.keys(map).filter(key => event[key]).map(key => map[key]);
    if (event.ctrlKey && AppConstants.platform === "macosx") {
      info.modifiers.push("MacCtrl");
    }

    // Allow menus to open various actions supported in webext prior
    // to notifying onclicked.
    let actionFor = {
      _execute_page_action: global.pageActionFor,
      _execute_browser_action: global.browserActionFor,
      _execute_sidebar_action: global.sidebarActionFor,
    }[item.command];
    if (actionFor) {
      let win = event.target.ownerGlobal;
      actionFor(item.extension).triggerAction(win);
    }

    item.extension.emit("webext-menu-menuitem-click", info, contextData.tab);
  },
Flags: needinfo?(rob)

Comment 9

9 months ago
> But I didn't get this, where I have to update information about mouse button?

"info" will hold the following information: https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/menus/OnClickData

I suggest that you use the Browser Toolbox to debug your ext-menus.js (https://developer.mozilla.org/en-US/docs/Tools/Browser_Toolbox). Then you can put a breakpoint at a specific line and inspect all relevant variables, which makes it much easier to understand what the code is doing.
Flags: needinfo?(rob)
(Assignee)

Comment 10

9 months ago
Hi Rob,
Can you give me some steps to get to the event function?
I tried clicking on addons but it does not logs anything from that function neigther in browser console nor in console
Flags: needinfo?(rob)

Comment 11

9 months ago
1. Load an extension that has adds a menu item via the browser.menus or browser.contextMenus API (e.g. via about:debugging).
2. Open the Browser Toolbox: https://developer.mozilla.org/en-US/docs/Tools/Browser_Toolbox#Opening_the_Browser_Toolbox
3. Open ext-menus.js (Ctrl - P, "ext-menus.js")
4. Scroll down to the "command" handler, and click on the line number inside the "command" handler function, to set a breakpoint.
5. Open the context menu and click on the extension menu item (from the extension from step 1). Now the breakpoint will be triggered and you can use the debugger and/or console to inspect variables.
Flags: needinfo?(rob)
(Assignee)

Comment 12

9 months ago
Hi Rob,
I was a bit busy last few days. had a chance to look into it today.
So i added these functions and add a extension which users browser.menus api.

element.addEventListener("contextmenu", (e) => {
      // This call makes sure no context menu is shown
      // to interfere with page receiving the events.
      e.preventDefault();
    });
    element.addEventListener("auxclick", (e) => {
      if (e.button === 2) {
        console.log(e)
      }
    }); 


And added Breakpoints to these two functions. But on click the menu item of extension both from right click menu and top "Tools" menu. Its not going to any of these events.
Its always going to this event

element.addEventListener("command", event => {


So as per my understanding, I have to use auxclick event and capture middle button click and add it in the info using the same functions as command event.

But the auxclick or contextmenu event is not getting triggered. Am I doing something wrong here?
Flags: needinfo?(rob)

Comment 13

9 months ago
I don't understand how the code snippet that you've showed relates to "an extension which uses the browser.menus API". There is no "browser.menus" API call in your snippet, and "element" is not defined either.

In the steps from comment 11, you should be debugging the command/auxclick event in parent/ext-menus.js (the main variable of interest is the first "event" parameter).

For step 1, a minimal test extension is an extension with the "menus" permission in manifest.json and the following snippet in the background page:
browser.menus.create({title: "Some menu item"});
Flags: needinfo?(rob)
(Assignee)

Comment 14

9 months ago
I am using this addon

https://github.com/mdn/webextensions-examples/tree/master/menu-demo

the code in Comment 12 is the code I have added in /parent/ext-menus.js

So, I will get the click event in "command" event? 
Or Do I have to add another auxclick listener?

As per your comment in Comment 6,

``So, you need to capture this information by other means, for example via the event.button property of the "auxclick" event:
https://w3c.github.io/uievents/#event-type-auxclick
https://developer.mozilla.org/en-US/docs/Web/API/MouseEvent/button``

I added the auxclick listener and on clicking the context menu its going to command listener and in that 
"event.button" is undefined
Flags: needinfo?(rob)
(Assignee)

Comment 15

9 months ago
Got it working.

Writing test script for testing it
Flags: needinfo?(rob)
(Assignee)

Comment 16

9 months ago
I was writing the test case.
But cannot figure out how to trigger secondary mouse click on menu.

I checked other scripts, all have click events but normal click events.

At present I have this test script

const PAGE = "http://mochi.test:8888/browser/browser/components/extensions/test/browser/context.html";

add_task(async function test_onclick_frameid() {
  const manifest = {
    permissions: ["menus"],
  };

  function background() {
    function onclick(info) {
      browser.test.sendMessage("click", info);
    }
    browser.menus.create({contexts: ["frame", "page"], title: "modify", onclick}, () => {
      browser.test.sendMessage("ready");
    });
  }

  const extension = ExtensionTestUtils.loadExtension({manifest, background});
  const tab = await BrowserTestUtils.openNewForegroundTab(gBrowser, PAGE);

  await extension.startup();
  await extension.awaitMessage("ready");

  async function click(selector) {
    const func = selector === "body" ? openContextMenu : openContextMenuInFrame;
    const menu = await func(selector);
    const items = menu.getElementsByAttribute("label", "modify");
    await closeExtensionContextMenu(items[0]);
    return extension.awaitMessage("click");
  }

  let info = await click("body");
  is(info.modifiers.length, 0, "left click should have no modifiers");
  

  BrowserTestUtils.removeTab(tab);
  await extension.unload();
});


But how can I fire the secondary mouse click event in this?
Can you help me in that?
Flags: needinfo?(rob)

Comment 17

9 months ago
See the hint at the end of comment 6.
Flags: needinfo?(rob)
(Assignee)

Comment 18

9 months ago
I was trying to simulate the mouse middle click, I have checked the function here

https://searchfox.org/mozilla-central/rev/2fe43133dbc774dda668d4597174d73e3969181a/browser/components/extensions/test/browser/head.js#344-356

Since it takes modifiers as parameter, I am passing MouseMiddleClick as modifier to that function, is that correct way? 
As I don't feel it verifies the changes done

As the changes done is checks for the button event and then updates the modifiers.

Am I on the right path? Passing as modifier to that function. Also when I tried doing that, it does not fires the click event automatically.
Flags: needinfo?(rob)

Comment 19

9 months ago
You're on the right track in the sense that you should indeed pass a property via the modifiers paramter.
However, there is no need to invent a new property key "MouseMiddleClick" as the EventUtils.jsm.

Another hint: the parameter called "modifiers" in https://searchfox.org/mozilla-central/rev/2fe43133dbc774dda668d4597174d73e3969181a/browser/components/extensions/test/browser/head.js#344-356 is called "aEvent" in https://searchfox.org/mozilla-central/source/testing/mochitest/tests/SimpleTest/EventUtils.js
Flags: needinfo?(rob)
(Assignee)

Comment 20

9 months ago
Ok. One more doubt
So this is how I have handled it in ext-menus.js


    `if (event.button === 1) {
      info.modifiers.push("MouseMiddleClick")
    }`

Added check for button, and if its MouseMiddleClick I am adding this "MouseMiddleClick" as a modifier.
This name is fine? Or I have to use someother predefined name? Or do I have to directly push the "event.button" value in the modifier?
Flags: needinfo?(rob)

Comment 21

9 months ago
You could add a new property "button".

event.button is a bitmask, so you should be using "if (event.button & 1) {". Otherwise if someone presses two mouse buttons at the same time, the "===1" check would not recognize the click.
Flags: needinfo?(rob)
(Assignee)

Comment 22

9 months ago
Ok. you mean new button property in "info"?
And will it be like string? or should I directly store the event.button value in it?
Flags: needinfo?(rob)

Comment 23

9 months ago
Actually, I stand corrected: You can use "=== 1". I confused event.button for event.buttons. The first always has the value of the clicked button, the latter is a bitmask:
https://developer.mozilla.org/en-US/docs/Web/API/MouseEvent/button
https://developer.mozilla.org/en-US/docs/Web/API/MouseEvent/buttons

So you can use "event.button === 1".

(In reply to Arshad Kazmi from comment #22)
> Ok. you mean new button property in "info"?

Yes.

> And will it be like string? or should I directly store the event.button
> value in it?

The event.button value, i.e. an integer.
Flags: needinfo?(rob)
(Assignee)

Comment 24

9 months ago
Done with the changes ext-menus.js
And back to the testing script.

So now I have added this 

`await closeExtensionContextMenu(items[0], {button: 1});`

to fire event and pass button value as event.

But automatic click event is not working on adding {button: 1}
when I am removing it then click is getting triggered.

I have checked the functions in EventUtils.js function takes modifiers as input, but passing any value is disabling the auto click.

Can you guide me through what am I missing here
Flags: needinfo?(rob)
(Assignee)

Comment 25

9 months ago
I feel like I am stuck at this point. Tried alot of things but can't seem to figure it out on how to trigger the secondary mouse click

Comment 26

9 months ago
Try using 2 for now (=simulates right-mouse button).
On macOS, I just tried closeExtensionContextMenu(..., {button: 2}), and that triggered the auxclick event as expected.
When I use 1, that did not work, but I think that it is a macOS-only issue, since wheel-click seems to work on Windows and Linux.
Flags: needinfo?(rob)
(Assignee)

Comment 27

9 months ago
Yes. That was the issue. I have been trying that for hours. I just missed trying with button: 2.

It worked. Almost done with the test case. 

Pushing code for review in coupon of minutes

Thanks Rob.
(Assignee)

Comment 28

9 months ago
Hey Rob.

I added code in Phabricator and got lint erros.
Checked at my end also.
Getting this error

Error: No corresponding 'removeEventListener(auxclick)' was found. [eslint: mozilla/balanced-listeners]

Where I need to add removeEventListener? As I don't see removeEventListener for 'command' anywhere
Flags: needinfo?(rob)

Comment 29

9 months ago
At "command" there was an eslint comment.
eslint is commonly used in JavaScript, so I suggest that you familiarize yourself with how eslint rules can be configured:
https://eslint.org/docs/user-guide/configuring#disabling-rules-with-inline-comments
Flags: needinfo?(rob)
(Assignee)

Comment 30

9 months ago
So, for auxclick also we need to disable eslint?

Or Should I add removeListener at the end of the function?

I tried adding removeEventListener("auxclick") at the end of the function before return. And the EsLint error is gone.

So which is preferred using here? 
Disabled Eslint or removeEventListener at end of the function?
Flags: needinfo?(rob)

Comment 31

9 months ago
The event listener is necessary, so it cannot be removed.
That leaves us with one option, disabling the specific eslint rule for that line, as done here: https://searchfox.org/mozilla-central/rev/c3fef66a5b211ea8038c1c132706d02db408093a/browser/components/extensions/parent/ext-menus.js#275

I highly recommend that you read the eslint documentation to understand what that comment does, so you will be able to apply this knowledge if you come across a similar situation in the future.
Flags: needinfo?(rob)
(Assignee)

Comment 32

9 months ago
more tests added, test script name changed
(Assignee)

Comment 33

9 months ago
I am going through the eslint document as I made this comment.

Added the code to phabricator for review.
https://phabricator.services.mozilla.com/D4960

Added you as reviewer

Comment 34

9 months ago
Comment on attachment 9006315 [details]
Add button info to click event of contextMenus API

Rob Wu [:robwu] has approved the revision.
Attachment #9006315 - Flags: review+
Comment on attachment 9006315 [details]
Add button info to click event of contextMenus API

Shane Caraveo (:mixedpuppy) has approved the revision.
Attachment #9006315 - Flags: review+
(Assignee)

Comment 36

9 months ago
Thank you Rob & Shane for the review

Thank you Rob for the help throughout the work.

@Rob
Do I need to add checkin-needed flag now?
Flags: needinfo?(rob)

Comment 37

9 months ago
I asked for two very small changes to the code: https://phabricator.services.mozilla.com/D4960#110537

Please make those changes, ensure that all tests pass and then add the checkin-needed keyword.
Flags: needinfo?(rob)
(Assignee)

Comment 38

9 months ago
Totally missed those last comments of yours. Making those changes now.
(Assignee)

Updated

9 months ago
Keywords: checkin-needed
Attachment #9006315 - Attachment description: Secondary click on contextMenu, capturing → Add button info to click event of contextMenus API

Comment 39

9 months ago
Pushed by apavel@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0adb1474cc23
Add button info to click event of contextMenus API r=robwu,mixedpuppy
Keywords: checkin-needed
Backed out for failing bc at browser/components/extensions/test/browser/test-oop-extensions/browser_ext_menus_capture_secondary_click.js 

Push that started the failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=0adb1474cc23ca5dc12363a7755e0bdb2b833709

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=197671488&repo=autoland&lineNumber=23459

Backout: https://hg.mozilla.org/integration/autoland/rev/1eef890debe96a14d09676c6208164160e94c96a
Flags: needinfo?(arshadkazmi42)

Comment 41

9 months ago
Hi Arshad, your patch has been backed out because of test failures.
To reland, follow the following steps:
1. Go to Phabricator, scroll to the bottom and use the "Reopen revision" action in the dropdown, then submit.
2. Fix the issue that caused the back-out.
3. Ensure that the tests pass and wait for review.
4. Add checkin-needed label.

The failure is a time-out, with the last message in the log as follows:
https://treeherder.mozilla.org/logviewer.html#?job_id=197671488&repo=autoland&lineNumber=23459

TEST-PASS | browser/components/extensions/test/browser/test-oop-extensions/browser_ext_menus_capture_secondary_click.js | Button value should be 0 -
TEST-PASS | browser/components/extensions/test/browser/test-oop-extensions/browser_ext_menus_capture_secondary_click.js | Button value should be 1 -
TEST-UNEXPECTED-FAIL | browser/components/extensions/test/browser/test-oop-extensions/browser_ext_menus_capture_secondary_click.js | Test timed out -
TEST-UNEXPECTED-FAIL | browser/components/extensions/test/browser/test-oop-extensions/browser_ext_menus_capture_secondary_click.js | message queue is empty - Got ["click"], expected []

The first two messages shows that the "click" event was fired as expected.
The last two messages show that the test timed out because it was waiting for a "click" message (of the test with event.button=2).

Based on these messages, I think somehow the menu from the button 1 test was not closed, or that something went wrong with the button 2 test.

I tried to reproduce the bug locally on macOS (even by passing the --verify flag to the "mach test" command), but was not able to.
Therefore I created a new patch with extra logging, and sent it (together with your patch) to the try server.
Results will be posted here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=31a50e0917e1adeb025049a0d77c56899df4fabd
(Assignee)

Comment 42

9 months ago
AS per the log here. 

https://treeherder.mozilla.org/testview.html?repo=try&revision=31a50e0917e1adeb025049a0d77c56899df4fabd

Looks like its failing in Windows.

Is there a way to test my patch in windows? I am also not able to reproduce it on my macOS
Flags: needinfo?(arshadkazmi42) → needinfo?(rob)
(Assignee)

Comment 43

9 months ago
Hey Rob,
Do you this this could be the problem??

`BrowserTestUtils.removeTab(tab);
 await extension.unload();
`

First I am removing tab and then unloading the extension.
Is it right? Or it should first unload extension & then remove the tab

Comment 44

9 months ago
That can't be the problem, because (as the logs show) that part of the code is not reached yet.
The execution of the test gets stuck at the `await extension.awaitMessage("click");` call (see comment 41 for the relevant part of the logs).

I created a patch with extra logging: https://hg.mozilla.org/try/rev/7602eec02fe5830ef402dadef7f8971da5ea45d5
and started the tests on Windows machines via the try server:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9ba29e55f3d690ed44db052c04a3e028c20968ae
Results:
https://taskcluster-artifacts.net/CoWt1FsUQFCjU2rjYUplHA/0/public/logs/live_backing.log
09:22:43     INFO - TEST-START | browser/components/extensions/test/browser/test-oop-extensions/browser_ext_menus_capture_secondary_click.js
09:22:43     INFO - GECKO(6656) | Will openContextMenu 0
09:22:43     INFO - GECKO(6656) | Will closeExtensionContextMenu 0
09:22:43     INFO - GECKO(6656) | element_click: 0 for 335 state=hiding
09:22:43     INFO - GECKO(6656) | element_command: 0 for 335 state=hiding
09:22:43     INFO - GECKO(6656) | element_popuphiding: 0 for 335
09:22:43     INFO - GECKO(6656) | element_popuphidden: 0 for 335
09:22:43     INFO - GECKO(6656) | Will wait for click0
09:22:43     INFO - GECKO(6656) | Will openContextMenu 1
09:22:43     INFO - GECKO(6656) | Will closeExtensionContextMenu 1
09:22:43     INFO - GECKO(6656) | element_click: 1 for 415 state=open
09:22:43     INFO - GECKO(6656) | element_click will doCommand: 1 for 415
09:22:43     INFO - GECKO(6656) | element_command: 1 for 415 state=open
09:22:43     INFO - GECKO(6656) | element_click will hidePopup: 1 for 415
09:22:43     INFO - GECKO(6656) | element_popuphiding: 1 for 415
09:22:43     INFO - GECKO(6656) | element_popuphidden: 1 for 415
09:22:43     INFO - GECKO(6656) | Will wait for click1
09:22:43     INFO - GECKO(6656) | Will openContextMenu 2
09:22:43     INFO - GECKO(6656) | Will closeExtensionContextMenu 2
09:22:43     INFO - GECKO(6656) | element_click: 2 for 453 state=open
09:22:43     INFO - GECKO(6656) | element_click will doCommand: 2 for 453
09:22:43     INFO - GECKO(6656) | element_command: 2 for 453 state=open
...
09:23:28     INFO - TEST-UNEXPECTED-FAIL | browser/components/extensions/test/browser/test-oop-extensions/browser_ext_menus_capture_secondary_click.js | Test timed out -

From these logs, it is apparent that the menu with button 2 is not closed.
So I modified your patch to call hidePopup when event.button is set ( https://hg.mozilla.org/try/rev/f46f498f1a96be54bb3a34cc3522f136defb8013 ) and the tests seem to pass now:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=752e3469daa60ccd9cf574daeed733415a67bafc

So to fix the failure, remove "if (event.button !== 0 && event.button !== 2) {".
Flags: needinfo?(rob)
(Assignee)

Comment 45

9 months ago
Rob, thank you so much for debugging that. I don't have windows, was thinking what to do to test this whole day. Thanks alot.


So seems like for windows event button 2 is not closing the context menu.

Is it fine to remove this "if (event.button !== 0 && event.button !== 2) {". condition?
As you told during code review process, there can be buttons like back/forward which can trigger this.


````
I think that you should use: button !== 0 && button !== 2 (and add a comment that by default the menu will be hidden when either of these buttons are clicked).

That is in case someone uses another mouse button (e.g. the "back"/"forward" button on their mouse) - https://developer.mozilla.org/en-US/docs/Web/API/MouseEvent/button

````

So should I remove the condition? Or including event the "button !== 1" is better option?
Flags: needinfo?(rob)

Comment 46

9 months ago
> So should I remove the condition?

Please see the last line of my previous comment (comment 44).

> Or including event the "button !== 1" is better option?

No, hidePopup should be called when button === 1.
And apparently also when button === 2.
And probably also when button > 2 (i.e. 3 or 4).
Therefore the condition should be that button is non-zero, and that is already handled by `if (event.button) {` a few lines back.
Flags: needinfo?(rob)
(Assignee)

Comment 47

9 months ago
Ok. Thanks for clarification.
I will make that change and push the code.

Should I add checkin-needed flag pushing this change?
Flags: needinfo?(rob)

Comment 48

9 months ago
See the steps at the top of from comment 41: I'll review your change after you update the revision, and then you can add the checkin-needed label.
Flags: needinfo?(rob)
(Assignee)

Comment 49

9 months ago
Done with the change. Tested in my local machine and pushed for review in phabricator
Flags: needinfo?(rob)

Comment 50

9 months ago
Looks good.
Flags: needinfo?(rob)
(Assignee)

Updated

9 months ago
Keywords: checkin-needed

Comment 51

9 months ago
Pushed by nbeleuzu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b2a53a75ba3d
Add button info to click event of contextMenus API r=robwu,mixedpuppy
Keywords: checkin-needed

Comment 52

9 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b2a53a75ba3d
Status: ASSIGNED → RESOLVED
Last Resolved: 9 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64

Comment 53

9 months ago
Will manual testing be required on this bug? if yes please provide a valid extension example for testing as I assume extensions created before this feature do not include the middle click functionality. If manual testing is not required please set the “qe-verify -“ flag.

Thanks!
Flags: needinfo?(arshadkazmi42)
(Assignee)

Comment 54

9 months ago
I am not sure, whether it needs manual testing also.
I have already written a test script, which has passed.

I am not sure, how the testing workflow works here.

@Rob, Any suggestions? Do we need manual testing here?
Flags: needinfo?(arshadkazmi42) → needinfo?(rob)

Comment 55

9 months ago
Yes, manual testing is needed here. The tests simulate a wheel click, but manual testing will tell whether it works as expected on all platforms.

Do you think that you can create a test extension and steps that prints the value of event.button to the console upon click?
Flags: needinfo?(rob)
(Assignee)

Comment 56

9 months ago
I will be able to create a test extension. 
Give me sometime. 
I will create a test extension and share it here.

Comment 57

9 months ago
Thanks guys, will validate this once I have the extension.
(Assignee)

Comment 58

9 months ago
I have created the extension. 
But not sure how to package it to xpi.

Can you should me some guide.

I tried making a zip but it shows package corrupted on installing
Flags: needinfo?(rob)
(Assignee)

Comment 59

9 months ago
Got it working.

You can get the addon here

https://www.dropbox.com/s/mdo7wluu150o92c/addon.xpi.zip?dl=0


I was not sure how to attach file here. So added it in my dropbox

I was getting error while installing addon, that addon is not verified.
You can follow these steps to install if you get the same error

"
type about:config into the address bar, press Enter, accept the warning, scroll down to xpinstall.signatures.required and double click on it to change 'Value' from True to False
"
Flags: needinfo?(rob) → needinfo?(vlad.jiman)

Comment 60

9 months ago
To attach a file, click on "Attach file" near the top of this page and select the file.

To load the unsigned extension, visit about:debugging and select the xpi or zip file.

When you attach a file for QA, make sure to include the steps to reproduce and the expected result.
(Assignee)

Comment 61

9 months ago
str
Posted file addon.xpi
Test Addon
(Assignee)

Comment 62

9 months ago
str
To Use Addon

These are the steps to follow

1. Install Addon
2. Open Developer Console CTRL + Shift + I (For MAC: CMD + OPT + I)
3. Right click on any page, click on "modify" option using any button using mouse
4. You will be able to see button details, like for left click you will see "Button Clicked: 0" and log of the "info" object.
5. Same for all other mouse clicks.


These are the button details

Left Mouse Button: 0
Middle Mouse Button: 1
Right Mouse Button: 2

Comment 63

8 months ago
Thank you for the addon, it worked great and I managed to validate the bug fix using it with debug mode:

Validated the fix using FF 62.0, 63.0a and 64.0a1 - On earlier versions than 64 the addon did not manage to log anything for using the middle mouse button, this was expected, on FF Nightly 64.0a1 the addon logged each mouse button interaction as described in Comment #62, attaching log. Tests were run using Windows 10 x64
Flags: needinfo?(vlad.jiman)

Updated

8 months ago
Status: RESOLVED → VERIFIED

Comment 64

8 months ago
Posted image Verified log
(Assignee)

Comment 65

8 months ago
Thank you for the validation Vlad :)
Hi Rob,

Can you give me a one sentence description of what has been added here, and to where? I found this bug really hard to follow.

Thanks!
Flags: needinfo?(rob)

Comment 67

6 months ago
Extensions can now detect which mouse button was used when a menu item was clicked.

(dev-doc-needed note:)
The new menus.OnClickedData.button property has the same semantics as https://developer.mozilla.org/en-US/docs/Web/API/MouseEvent/button
Flags: needinfo?(rob)
(In reply to Rob Wu [:robwu] from comment #67)
> Extensions can now detect which mouse button was used when a menu item was
> clicked.
> 
> (dev-doc-needed note:)
> The new menus.OnClickedData.button property has the same semantics as
> https://developer.mozilla.org/en-US/docs/Web/API/MouseEvent/button

Perfect, thanks Rob! Note now added to https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/64#API_changes

Updated

6 months ago
See Also: → 1508144

Updated

6 months ago
Depends on: 1508144
See Also: 1508144
You need to log in before you can comment on or make changes to this bug.