Right/Middle-clicking on menu item will trigger the menus.onClicked callback

VERIFIED FIXED in Firefox 64

Status

defect
P1
normal
VERIFIED FIXED
6 months ago
6 months ago

People

(Reporter: kernp25, Assigned: robwu)

Tracking

({regression})

unspecified
mozilla65
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox-esr60 unaffected, firefox63 unaffected, firefox64+ verified, firefox65 verified)

Details

Attachments

(3 attachments)

Reporter

Description

6 months ago
User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:65.0) Gecko/20100101 Firefox/65.0



Actual results:

It triggers the menus.onClicked callback when you right/middle-click on the menu item.


Expected results:

It should not trigger the menu item (like with browser build in menus).
Reporter

Comment 1

6 months ago
From https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/menus#Creating_menu_items the first image.

If you right/middle-click on "Menu demo" item (not the child menus), it will trigger the menus.onClicked callback.
Left-clicking on "Menu demo" does nothing (as expected).
Flags: needinfo?(rob)
Assignee

Comment 2

6 months ago
This is a feature - bug 1469148.

Not all built-in menu items ignore the middle-click, e.g. the "View Background Image" menu item's behavior differs based on which button is clicked.

I'm marking this as WORKSFORME for now, but if you think that there is a strong case against triggering onClicked by default (i.e. require the click to be opt-in), then I'll reconsider. The feature is currently on Firefox Beta, so now would be a good time to change defaults, if any.
Status: UNCONFIRMED → RESOLVED
Last Resolved: 6 months ago
Flags: needinfo?(rob)
Resolution: --- → WORKSFORME
See Also: → 1469148
Reporter

Comment 3

6 months ago
Posted file background.zip
Reporter

Comment 4

6 months ago
This also occurs with this code:
browser.menus.onClicked.addListener((info, tab) => {
  //if (info.menuItemId == "parent-menu-item") {
    console.log("Parent item was clicked", info);
 // }
});

browser.menus.create({
  contexts: ["page"],
  id: "parent-menu-item",
  title: "Rigth-click on me"
});

browser.menus.create({
  id: "1",
  contexts: ["page"],
  title: "Click the parent menu item"
});

browser.menus.create({
  id: "2",
  contexts: ["page"],
  title: "Click the parent menu item"
});
Flags: needinfo?(rob)
Reporter

Comment 5

6 months ago
Please check the add-on, i uploaded.
Reporter

Comment 6

6 months ago
The above code should be:

browser.menus.create({
  contexts: ["page"],
  id: "1",
  title: "Click the parent menu item"
});

browser.menus.create({
  id: "2",
  contexts: ["page"],
  title: "Click the parent menu item"
});
Reporter

Comment 7

6 months ago
The built-in "Send page to device" item [1] for example. Right-clicking on the "context-sendpagetodevice" menu does nothing.
The same should also with Webextensions menus.

[1] https://searchfox.org/mozilla-central/rev/5117a4c4e29fcf80a627fecf899a62f117368abf/browser/base/content/browser-context.inc#264-271
Assignee

Comment 8

6 months ago
The issue in comment 4 / comment 6 sounds like a bug. The auto-generated top-level menu should not trigger clicks (for that matter, any menu with submenus should not trigger the onClicked event).

I'm going to fix that, but still permit right/middle-click on other extension menu items.
Does this look good to you?
Assignee: nobody → rob
Blocks: 1469148
Status: RESOLVED → REOPENED
Ever confirmed: true
Flags: needinfo?(rob)
Keywords: regression
Priority: -- → P1
Resolution: WORKSFORME → ---
See Also: 1469148
Reporter

Comment 9

6 months ago
(In reply to Rob Wu [:robwu] from comment #8)
> I'm going to fix that, but still permit right/middle-click on other
> extension menu items.
> Does this look good to you?

I do not have a problem with this :)
Reporter

Comment 10

6 months ago
The title should be:

Right/Middle-clicking on a top-level menu will trigger the menus.onClicked callback
Assignee

Updated

6 months ago
Status: REOPENED → ASSIGNED
Assignee

Comment 11

6 months ago
Bug 1469148 added support for detecting which mouse button was used,
by synthetizing "command" events when a "click" event was captured.
The implementation did not account for unclickable menu items, such
as items that act as the parent of a submenu (see bug report),
separators and disabled menu items.

This patch adds the necessary checks and regression tests for these
scenarios to make sure that such clicks are ignored, as expected.

Comment 12

6 months ago
Pushed by rob@robwu.nl:
https://hg.mozilla.org/integration/autoland/rev/164213f935e1
Ignore clicks on non-clickable menu items r=mixedpuppy
Assignee

Comment 13

6 months ago
Comment on attachment 9027931 [details]
Bug 1508144 - Ignore clicks on non-clickable menu items

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Bug 1469148

User impact if declined: A context menu will unexpectedly be closed when the user clicks on a part of the extension menu that is supposedly not clickeable (=menu items that have submenus, menu separators, and disabled menu items).

Is this code covered by automated tests?: Yes

Has the fix been verified in Nightly?: No

Needs manual test from QE?: No

If yes, steps to reproduce: 

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): Not risky; the patch adds an early return to logic that was introduced in Firefox 64. The original feature and all edge cases are covered by unit tests, and I have verified on Linux and macOS that all menu-related unit tests still pass (debug and release builds, also with test-verify).

String changes made/needed: N/A
Attachment #9027931 - Flags: approval-mozilla-beta?
Given that this hasn't landed on Nightly yet, it's unlikely this patch will be in today's gtb (b14)

Comment 15

6 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/164213f935e1
Status: ASSIGNED → RESOLVED
Last Resolved: 6 months ago6 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
Comment on attachment 9027931 [details]
Bug 1508144 - Ignore clicks on non-clickable menu items

fix ui regression with extension menus, approved for 64.0 rc1
Attachment #9027931 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment 18

6 months ago
Posted image Bug1508144.gif
I was able to reproduce this issue on Firefox 64.0b14(20181128185223) under Win 7 64-bit and  Mac OS X 10.13.6.

This issue is verified as fixed on Firefox 65.0a1(20181203214946) and Firefox 64.0rc-build 1 (20181203202653) under Win 7 64-bit and Mac OS X 10.13.6.

Please see the attached video.

Updated

6 months ago
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.