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

Status: VERIFIED FIXED

(WebExtensions :: General, defect, P1)

Description

[kernp25]

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

Comment 1

[kernp25]

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

Comment 2

[Rob Wu [:robwu]]

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.

Comment 3

[kernp25]

Attachment: background.zip

Comment 4

[kernp25]

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

Comment 5

[kernp25]

Please check the add-on, i uploaded.

Comment 6

[kernp25]

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

Comment 7

[kernp25]

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

Comment 8

[Rob Wu [:robwu]]

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?

Comment 9

[kernp25]

(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 :)

Comment 10

[kernp25]

The title should be:

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

Comment 11

[Rob Wu [:robwu]]

Attachment: Bug 1508144 - Ignore clicks on non-clickable menu items

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

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/164213f935e1
Ignore clicks on non-clickable menu items r=mixedpuppy

Comment 13

[Rob Wu [:robwu]]

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

Comment 14

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Given that this hasn't landed on Nightly yet, it's unlikely this patch will be in today's gtb (b14)

Comment 15

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/164213f935e1

Comment 16

[Julien Cristau [:jcristau]]

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

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/3be25c15b7e9

Comment 18

[CosminB]

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