Closed Bug 1469148 Opened 6 years ago Closed 6 years ago

Web Extension menus should be able to react to middle click

Categories

(WebExtensions :: Frontend, enhancement, P5)

enhancement

Tracking

(firefox64 verified)

VERIFIED FIXED
mozilla64
Tracking Status
firefox64 --- verified

People

(Reporter: Kwan, Assigned: arshadkazmi42, Mentored)

References

Details

(Keywords: dev-doc-complete, good-first-bug, Whiteboard: [design-decision-approved])

Attachments

(3 files)

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
Product: Toolkit → WebExtensions
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
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]
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)
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
Hey Rob
I would like to work on this. Can you guide me where can I start with for this issue?
Flags: needinfo?(rob)
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
Thanks Rob, for the details explanation.

I will look into it later today.
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)
> 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)
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)
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)
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)
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)
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)
Got it working.

Writing test script for testing it
Flags: needinfo?(rob)
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)
See the hint at the end of comment 6.
Flags: needinfo?(rob)
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)
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)
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)
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)
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)
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)
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)
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
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)
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.
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)
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)
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)
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)
more tests added, test script name changed
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 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+
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)
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)
Totally missed those last comments of yours. Making those changes now.
Keywords: checkin-needed
Attachment #9006315 - Attachment description: Secondary click on contextMenu, capturing → Add button info to click event of contextMenus API
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)
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
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)
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
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)
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)
> 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)
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)
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)
Done with the change. Tested in my local machine and pushed for review in phabricator
Flags: needinfo?(rob)
Looks good.
Flags: needinfo?(rob)
Keywords: checkin-needed
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
Keywords: dev-doc-needed
https://hg.mozilla.org/mozilla-central/rev/b2a53a75ba3d
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
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)
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)
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)
I will be able to create a test extension. 
Give me sometime. 
I will create a test extension and share it here.
Thanks guys, will validate this once I have the extension.
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)
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)
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.
Attached file addon.xpi
Test Addon
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
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)
Status: RESOLVED → VERIFIED
Attached image Verified log
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)
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
See Also: → 1508144
Depends on: 1508144
See Also: 1508144
See Also: → 1704883
You need to log in before you can comment on or make changes to this bug.