Any approximate ETA for this? Thanks.

Hi Marat, as a P5 this isn't currently being worked on by staff (and it's unlikely to be assigned in the near future).

However, we would accept patches for this! Rob has outlined some implementation possibilities in comment #7. More information about uplifting a patch can be found at https://webextensions-experiments.readthedocs.io/en/latest/index.html.

Marat, sorry, I sent you the wrong link! It looks like you don't need to go through the uplift process for this enhancement; you can submit a patch. We've got instructions for how to do so here: https://wiki.mozilla.org/WebExtensions/Contribution_Onramp

Before this change, browserActions and pageActions did not trigger
onClick events when middle-clicked, and no information on the button or
any modifiers were passed in the onClick event. With this change, middle
clicking triggers an event, and a clickData object is passed in the
onClick event, with the button and a list of modifiers.

I think I'd like to take a crack at this. I don't want to show up with a headache-inducing, on-the-wrong-track patch, but here's a patch to show that I've investigated a little. Let's first make sure I have the correct basic approach.

Things I haven't done yet but have a good chance in researching:

• This change doesn't yet support browserActions that are left-clicked (command event) from a subview.
• This might not respect the enabled/disabled state of a browserAction (if such a state exists) when middle clicked (similar to Bug 1508144)
• I have completely ignored the request in comment 6 thus far.

Things I need help with:

• If a browserAction or pageAction with a popupURL is middle-clicked, do we want it to emit the click event to the extension, or do we want to display the popup and not emit the event, the way that it is done with the left click?
• What amount of plumbing through other components are we expecting to do here? For example, MultiPanelView throws away the modifier information we use when handling left clicks, so I modified it here. Maybe there are some better ways of obtaining this information that don't require changing non-webextension parts of Firefox.
• Currently, I'm running these tests as I go along:
  • browser/modules/test/
  • toolkit/components/extensions/test/
  • browser/components/extensions/test/
  • browser/components/customizableui/test/

Which others should I be running?

• What options do we have for breaking up this bug? Am I supposed to be trying to check in one big patch that meets all the criteria we have above us, or do we maybe want to break this bug up along the browserAction/pageAction line and check them in separately?

I think I'd like to take a crack at this. I don't want to show up with a headache-inducing, on-the-wrong-track patch, but here's a patch to show that I've investigated a little. Let's first make sure I have the correct basic approach.

Thanks for the patch and your comment. It really shows that you've put a lot of thought in it!

• I have completely ignored the request in comment 6 thus far.

You've implemented key modifiers, which is good enough. I was not expecting "a native way to make an extension button behaving like a true link" - that is outside the scope of this bug.

• If a browserAction or pageAction with a popupURL is middle-clicked, do we want it to emit the click event to the extension, or do we want to display the popup and not emit the event, the way that it is done with the left click?

Currently, middle-clicking the button does not have any effect.
To keep things simple, only trigger onClicked, regardless of the presence of the popup when the middle-mouse button is clicked. This allows extensions to respond differently to primary click vs secondary click. If extensions want to show the popup in response to middle-click, then they could use browserAction.openPopup or pageAction.showPopup (it might be a good idea to add a unit test for calling those methods from a middle-mouse click).

And do not trigger (browser|page)Action.onClicked when the right-mouse button is clicked. When right-click is used, a context menu is shown (and the behavior cannot be canceled). Extensions can already use the menus.onShown event, to detect the modifiers and button that were used to open the menu.

• What amount of plumbing through other components are we expecting to do here? For example, MultiPanelView throws away the modifier information we use when handling left clicks, so I modified it here. Maybe there are some better ways of obtaining this information that don't require changing non-webextension parts of Firefox.

The custom event notifications in CustomizableUI (BrowserAction and PageActions) are currently independent of the actual key/mouse events that trigger the notification. This keeps the API simple. I believe that it is possible to implement the requested functionality without any changes to CustomizableUI, by registering a click listener on the page/browser action button element, and then saving the state of the button.

This approach was also taken in bug 1469148, where support for middle-click was added to menu items: https://searchfox.org/mozilla-central/rev/ec806131cb7bcd1c26c254d25cd5ab8a61b2aeb6/browser/components/extensions/parent/ext-menus.js#462,486-497
Note: In menus, the menu element disappears upon clicking a menu item. browserAction/pageAction buttons are not removed on click, so you make sure that the button and modifiers are correctly (re)set when the button is clicked.

• Currently, I'm running these tests as I go along

During development you probably need to repeatedly run tests to check if everything works, and you need to find the balance between coverage (too few and you might miss important test cases) and time (running them all takes too much time).
If you follow my suggestion (and not change anything in customizableui), then you need to run at least the following tests:

./mach test browser/components/extensions/test/browser/browser_ext_browserAction*.js browser/components/extensions/test/browser/browser_ext_pageAction*.js

To catch any other potential issues, run ./mach mochitest browser/components/extensions/test/ (note: using mach mochitest instead of mach test because you're modifying UI code, so there is likely no need to run xpcshell tests as well).

• What options do we have for breaking up this bug? Am I supposed to be trying to check in one big patch that meets all the criteria we have above us, or do we maybe want to break this bug up along the browserAction/pageAction line and check them in separately?

If the implementations are very similar, one patch for everything would be fine.
If there are significant differences between pageAction and browserActions, then it makes more sense to have two separate patches, one for each UI type.

Thanks for the feedback, Rob; it's been really helpful and your approach simplified the problem a lot. I'm checking in on the status of this bug since I've been silent for an entire week, but I also don't want to spam the 16 people on the watch list, so maybe some of this discussion should be going in phabricator. Main time consuming thing this week was learning to write the tests that wait for the right components to pop up. The approach is pretty much the same for both so I think that one patch works.

The big implementation snag I've hit is that it seems like the synthesizeMouseAtCenter() is respecting the disabled state of pageAction panel buttons when manually clicking doesn't. I'm not yet sure why this is occurring. I also still plan to add a test with openPopup/showPopup.

(In reply to graham.mcknight2 from comment #15)

Thanks for the feedback, Rob; it's been really helpful and your approach simplified the problem a lot. I'm checking in on the status of this bug since I've been silent for an entire week, but I also don't want to spam the 16 people on the watch list, so maybe some of this discussion should be going in phabricator. Main time consuming thing this week was learning to write the tests that wait for the right components to pop up. The approach is pretty much the same for both so I think that one patch works.

Indeed, Phabricator is a good place to discuss implementation details. If you prefer real-time communication, you could also ping me on IRC. I'm robwu_nl at irc.mozilla.org

The big implementation snag I've hit is that it seems like the synthesizeMouseAtCenter() is respecting the disabled state of pageAction panel buttons when manually clicking doesn't. I'm not yet sure why this is occurring. I also still plan to add a test with openPopup/showPopup.

synthesizeMouseAtCenter emulates a click on whatever element you pass to it. If manually clicking doesn't trigger the event, double-check whether the event target is the same as what you use in tests. If it helps, you could use the Browser Toolbox to inspect the Firefox UI.

browser_ext_browserAction_popup_resize_bottom.js has the same failures on my machine that I've had since before working on this patch - the panel's position being off by a part of a pixel. Everything else seems fine when running with the --verify flag. I compared to a workspace without this patch just to be sure and the failure is the same there; I'm fairly certain that this is unrelated.

I'm going to add the checkin-needed tag at this point. Because this is a change to UI, I think that QE will also need to verify this; I will add the qe-verify+ tag after landing. I will attach extensions and STR here. I will make extensions for:

• Click info on browserActions and pageActions
• Opening popups from browserActions and pageActions using middle clicks
• Disabled/hidden browserActions/pageActions

In the process of making this patch, we found that closing the panel quickly leaves the transitioning="true" attribute. I've filed Bug 1580949 for this.

Rob, thanks for your patience with me on this patch, and for the quick reviews.

Pushed by ccoroiu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/74f522fc33ae
Add middle-click and click modifiers to browserAction r=robwu

Backed out changeset 74f522fc33ae (bug 1405031) for browser-chrome failures at browser/components/extensions/test/browser/browser_ext_browserAction_click_types.js

Backout: https://hg.mozilla.org/integration/autoland/rev/9b65c42a873628bc2c25ad220b5832fc48a2bae5

Failure push: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=74f522fc33ae919d152eca6d2a0fde14339bfdad

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=266451479&repo=autoland&lineNumber=1278

[task 2019-09-13T00:37:00.919Z] 00:37:00 INFO - TEST-PASS | browser/components/extensions/test/browser/browser_ext_browserAction_click_types.js | Correct button in nav-bar click -
[task 2019-09-13T00:37:00.920Z] 00:37:00 INFO - Buffered messages finished
[task 2019-09-13T00:37:00.920Z] 00:37:00 INFO - TEST-UNEXPECTED-FAIL | browser/components/extensions/test/browser/browser_ext_browserAction_click_types.js | No unnecessary modifiers for exactly one key on event - Got 2, expected 1
[task 2019-09-13T00:37:00.920Z] 00:37:00 INFO - Stack trace:
[task 2019-09-13T00:37:00.920Z] 00:37:00 INFO - chrome://mochikit/content/browser-test.js:test_is:1595
[task 2019-09-13T00:37:00.920Z] 00:37:00 INFO - chrome://mochitests/content/browser/browser/components/extensions/test/browser/browser_ext_browserAction_click_types.js:assertSingleModifier:43
[task 2019-09-13T00:37:00.921Z] 00:37:00 INFO - chrome://mochitests/content/browser/browser/components/extensions/test/browser/browser_ext_browserAction_click_types.js:test_clickData:68
[task 2019-09-13T00:37:00.921Z] 00:37:00 INFO - chrome://mochikit/content/browser-test.js:Tester_execTest/<:1350
[task 2019-09-13T00:37:00.921Z] 00:37:00 INFO - chrome://mochikit/content/browser-test.js:Tester_execTest:1385
[task 2019-09-13T00:37:00.921Z] 00:37:00 INFO - chrome://mochikit/content/browser-test.js:nextTest/<:1213
[task 2019-09-13T00:37:00.921Z] 00:37:00 INFO - chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<:805
[task 2019-09-13T00:37:00.921Z] 00:37:00 INFO - TEST-PASS | browser/components/extensions/test/browser/browser_ext_browserAction_click_types.js | Correct modifier on nav-bar click -
[task 2019-09-13T00:37:00.923Z] 00:37:00 INFO - TEST-PASS | browser/components/extensions/test/browser/browser_ext_browserAction_click_types.js | Expect widget not to be overflowed -
[task 2019-09-13T00:37:00.923Z] 00:37:00 INFO - TEST-PASS | browser/components/extensions/test/browser/browser_ext_browserAction_click_types.js | Correct button in nav-bar click -
[task 2019-09-13T00:37:00.923Z] 00:37:00 INFO - Not taking screenshot here: see the one that was previously logged

I was using the key instead of the value when determining whether to check for the MacCtrl modifier. My bad; this was easily testable.

1. MacOS failure: addressed
2. Linux TV failure: bad workaround for Bug 1580949, which is now fixed

Pushed by btara@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6c464eede88a
Add middle-click and click modifiers to browserAction r=robwu

Backed out changeset 6c464eede88a (Bug 1405031) for browser_ext_pageAction_click_types.js failures

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&searchStr=os%2Cx%2C10.14%2Cdebug%2Cmochitests%2Ctest-macosx1014-64%2Fdebug-mochitest-browser-chrome-e10s-12%2Cm%28bc12%29&fromchange=847db8b778d81d6f94deece9c1c151c8d3158cc5&tochange=27235c08afbbb130570035bec3d792526253f11e&selectedJob=267511361

Backout link: https://hg.mozilla.org/integration/autoland/rev/27235c08afbbb130570035bec3d792526253f11e

Failure link: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=267511361&repo=autoland&lineNumber=15322

[task 2019-09-19T21:40:22.793Z] 21:40:22 INFO - TEST-START | browser/components/extensions/test/browser/browser_ext_pageAction_click_types.js
[task 2019-09-19T21:40:22.838Z] 21:40:22 INFO - GECKO(1653) | [Parent 1653, Main Thread] WARNING: Need BrowserChild to get the nativeWindow from!: file /builds/worker/workspace/build/src/widget/PuppetWidget.cpp, line 1090
[task 2019-09-19T21:40:22.838Z] 21:40:22 INFO - GECKO(1653) | ++DOCSHELL 0x11b455800 == 6 [pid = 1655] [id = {2ec23f86-9a4e-4e4a-801a-e2f4553eee44}]
....
[task 2019-09-19T21:40:35.450Z] 21:40:35 INFO - GECKO(1653) | --DOMWINDOW == 12 (0x15d0c9000) [pid = 1653] [serial = 699] [outer = 0x0] [url = chrome://mozapps/content/extensions/aboutaddons.html]
[task 2019-09-19T21:41:52.915Z] 21:41:52 INFO - TEST-INFO | started process screencapture
[task 2019-09-19T21:41:53.094Z] 21:41:53 INFO - TEST-INFO | screencapture: exit 0
[task 2019-09-19T21:41:53.095Z] 21:41:53 INFO - Buffered messages logged at 21:40:22
[task 2019-09-19T21:41:53.095Z] 21:41:53 INFO - Entering test bound test_clickData
[task 2019-09-19T21:41:53.095Z] 21:41:53 INFO - Extension loaded
[task 2019-09-19T21:41:53.095Z] 21:41:53 INFO - Console message: Warning: attempting to write 17270 bytes to preference extensions.webextensions.uuids. This is bad for general performance and memory usage. Such an amount of data should rather be written to an external file. This preference will not be sent to any content processes.
[task 2019-09-19T21:41:53.095Z] 21:41:53 INFO - TEST-PASS | browser/components/extensions/test/browser/browser_ext_pageAction_click_types.js | Correct button on click event -
[task 2019-09-19T21:41:53.095Z] 21:41:53 INFO - TEST-PASS | browser/components/extensions/test/browser/browser_ext_pageAction_click_types.js | No unnecessary modifiers for exactly one key on event -
[task 2019-09-19T21:41:53.096Z] 21:41:53 INFO - TEST-PASS | browser/components/extensions/test/browser/browser_ext_pageAction_click_types.js | Correct modifier on click event -
[task 2019-09-19T21:41:53.096Z] 21:41:53 INFO - Buffered messages logged at 21:40:23
[task 2019-09-19T21:41:53.096Z] 21:41:53 INFO - TEST-PASS | browser/components/extensions/test/browser/browser_ext_pageAction_click_types.js | Correct button on click event -
[task 2019-09-19T21:41:53.096Z] 21:41:53 INFO - TEST-PASS | browser/components/extensions/test/browser/browser_ext_pageAction_click_types.js | No unnecessary modifiers for exactly one key on event -
[task 2019-09-19T21:41:53.096Z] 21:41:53 INFO - TEST-PASS | browser/components/extensions/test/browser/browser_ext_pageAction_click_types.js | Correct modifier on click event -
[task 2019-09-19T21:41:53.097Z] 21:41:53 INFO - TEST-PASS | browser/components/extensions/test/browser/browser_ext_pageAction_click_types.js | Key command emulates left click -
[task 2019-09-19T21:41:53.097Z] 21:41:53 INFO - TEST-PASS | browser/components/extensions/test/browser/browser_ext_pageAction_click_types.js | No unnecessary modifiers for exactly one key on event -
[task 2019-09-19T21:41:53.101Z] 21:41:53 INFO - TEST-PASS | browser/components/extensions/test/browser/browser_ext_pageAction_click_types.js | Correct modifier on click event -
[task 2019-09-19T21:41:53.102Z] 21:41:53 INFO - TEST-PASS | browser/components/extensions/test/browser/browser_ext_pageAction_click_types.js | Correct button on click event -
[task 2019-09-19T21:41:53.102Z] 21:41:53 INFO - TEST-PASS | browser/components/extensions/test/browser/browser_ext_pageAction_click_types.js | No unnecessary modifiers for exactly one key on event -
[task 2019-09-19T21:41:53.102Z] 21:41:53 INFO - TEST-PASS | browser/components/extensions/test/browser/browser_ext_pageAction_click_types.js | Correct modifier on click event -
[task 2019-09-19T21:41:53.103Z] 21:41:53 INFO - TEST-PASS | browser/components/extensions/test/browser/browser_ext_pageAction_click_types.js | Correct button on click event -
[task 2019-09-19T21:41:53.103Z] 21:41:53 INFO - TEST-PASS | browser/components/extensions/test/browser/browser_ext_pageAction_click_types.js | No unnecessary modifiers for exactly one key on event -
[task 2019-09-19T21:41:53.103Z] 21:41:53 INFO - TEST-PASS | browser/components/extensions/test/browser/browser_ext_pageAction_click_types.js | Correct modifier on click event -
[task 2019-09-19T21:41:53.103Z] 21:41:53 INFO - TEST-PASS | browser/components/extensions/test/browser/browser_ext_pageAction_click_types.js | Key command emulates left click -
[task 2019-09-19T21:41:53.103Z] 21:41:53 INFO - TEST-PASS | browser/components/extensions/test/browser/browser_ext_pageAction_click_types.js | No unnecessary modifiers for exactly one key on event -
[task 2019-09-19T21:41:53.103Z] 21:41:53 INFO - TEST-PASS | browser/components/extensions/test/browser/browser_ext_pageAction_click_types.js | Correct modifier on click event -
[task 2019-09-19T21:41:53.103Z] 21:41:53 INFO - TEST-PASS | browser/components/extensions/test/browser/browser_ext_pageAction_click_types.js | Correct button on click event -
[task 2019-09-19T21:41:53.104Z] 21:41:53 INFO - TEST-PASS | browser/components/extensions/test/browser/browser_ext_pageAction_click_types.js | No unnecessary modifiers for exactly one key on event -
[task 2019-09-19T21:41:53.104Z] 21:41:53 INFO - TEST-PASS | browser/components/extensions/test/browser/browser_ext_pageAction_click_types.js | Correct modifier on click event -
[task 2019-09-19T21:41:53.104Z] 21:41:53 INFO - TEST-PASS | browser/components/extensions/test/browser/browser_ext_pageAction_click_types.js | Correct button on click event -
[task 2019-09-19T21:41:53.104Z] 21:41:53 INFO - TEST-PASS | browser/components/extensions/test/browser/browser_ext_pageAction_click_types.js | No unnecessary modifiers for exactly one key on event -
[task 2019-09-19T21:41:53.104Z] 21:41:53 INFO - TEST-PASS | browser/components/extensions/test/browser/browser_ext_pageAction_click_types.js | Correct modifier on click event -
[task 2019-09-19T21:41:53.110Z] 21:41:53 INFO - TEST-PASS | browser/components/extensions/test/browser/browser_ext_pageAction_click_types.js | Key command emulates left click -
[task 2019-09-19T21:41:53.111Z] 21:41:53 INFO - TEST-PASS | browser/components/extensions/test/browser/browser_ext_pageAction_click_types.js | No unnecessary modifiers for exactly one key on event -
[task 2019-09-19T21:41:53.111Z] 21:41:53 INFO - TEST-PASS | browser/components/extensions/test/browser/browser_ext_pageAction_click_types.js | Correct modifier on click event -
[task 2019-09-19T21:41:53.111Z] 21:41:53 INFO - TEST-PASS | browser/components/extensions/test/browser/browser_ext_pageAction_click_types.js | Correct button on click event -
[task 2019-09-19T21:41:53.112Z] 21:41:53 INFO - TEST-PASS | browser/components/extensions/test/browser/browser_ext_pageAction_click_types.js | MacCtrl modifier with control click on Mac -
[task 2019-09-19T21:41:53.112Z] 21:41:53 INFO - TEST-PASS | browser/components/extensions/test/browser/browser_ext_pageAction_click_types.js | MacCtrl modifier with control click on Mac -
[task 2019-09-19T21:41:53.112Z] 21:41:53 INFO - TEST-PASS | browser/components/extensions/test/browser/browser_ext_pageAction_click_types.js | Correct modifier on click event -
[task 2019-09-19T21:41:53.112Z] 21:41:53 INFO - TEST-PASS | browser/components/extensions/test/browser/browser_ext_pageAction_click_types.js | Correct button on click event -
[task 2019-09-19T21:41:53.112Z] 21:41:53 INFO - TEST-PASS | browser/components/extensions/test/browser/browser_ext_pageAction_click_types.js | MacCtrl modifier with control click on Mac -
[task 2019-09-19T21:41:53.112Z] 21:41:53 INFO - TEST-PASS | browser/components/extensions/test/browser/browser_ext_pageAction_click_types.js | MacCtrl modifier with control click on Mac -
[task 2019-09-19T21:41:53.112Z] 21:41:53 INFO - TEST-PASS | browser/components/extensions/test/browser/browser_ext_pageAction_click_types.js | Correct modifier on click event -
[task 2019-09-19T21:41:53.113Z] 21:41:53 INFO - TEST-PASS | browser/components/extensions/test/browser/browser_ext_pageAction_click_types.js | Key command emulates left click -
[task 2019-09-19T21:41:53.113Z] 21:41:53 INFO - TEST-PASS | browser/components/extensions/test/browser/browser_ext_pageAction_click_types.js | MacCtrl modifier with control click on Mac -
[task 2019-09-19T21:41:53.113Z] 21:41:53 INFO - TEST-PASS | browser/components/extensions/test/browser/browser_ext_pageAction_click_types.js | MacCtrl modifier with control click on Mac -
[task 2019-09-19T21:41:53.113Z] 21:41:53 INFO - TEST-PASS | browser/components/extensions/test/browser/browser_ext_pageAction_click_types.js | Correct modifier on click event -
[task 2019-09-19T21:41:53.113Z] 21:41:53 INFO - TEST-PASS | browser/components/extensions/test/browser/browser_ext_pageAction_click_types.js | Correct button on click event -
[task 2019-09-19T21:41:53.114Z] 21:41:53 INFO - TEST-PASS | browser/components/extensions/test/browser/browser_ext_pageAction_click_types.js | No unnecessary modifiers for exactly one key on event -
[task 2019-09-19T21:41:53.114Z] 21:41:53 INFO - TEST-PASS | browser/components/extensions/test/browser/browser_ext_pageAction_click_types.js | Correct modifier on click event -
[task 2019-09-19T21:41:53.114Z] 21:41:53 INFO - TEST-PASS | browser/components/extensions/test/browser/browser_ext_pageAction_click_types.js | Correct button on click event -
[task 2019-09-19T21:41:53.114Z] 21:41:53 INFO - TEST-PASS | browser/components/extensions/test/browser/browser_ext_pageAction_click_types.js | No unnecessary modifiers for exactly one key on event -
[task 2019-09-19T21:41:53.115Z] 21:41:53 INFO - TEST-PASS | browser/components/extensions/test/browser/browser_ext_pageAction_click_types.js | Correct modifier on click event -
[task 2019-09-19T21:41:53.115Z] 21:41:53 INFO - Buffered messages finished
[task 2019-09-19T21:41:53.115Z] 21:41:53 INFO - TEST-UNEXPECTED-FAIL | browser/components/extensions/test/browser/browser_ext_pageAction_click_types.js | Test timed out -
[task 2019-09-19T21:41:53.115Z] 21:41:53 INFO - Not taking screenshot here: see the one that was previously logged
[task 2019-09-19T21:41:53.115Z] 21:41:53 INFO - TEST-UNEXPECTED-FAIL | browser/components/extensions/test/browser/browser_ext_pageAction_click_types.js | Extension left running at test shutdown -
[task 2019-09-19T21:41:53.115Z] 21:41:53 INFO - Stack trace:
[task 2019-09-19T21:41:53.115Z] 21:41:53 INFO - chrome://mochikit/content/browser-test.js:test_ok:1580
[task 2019-09-19T21:41:53.115Z] 21:41:53 INFO - chrome://mochikit/content/tests/SimpleTest/ExtensionTestUtils.js:ExtensionTestUtils.loadExtension/<:109
[task 2019-09-19T21:41:53.115Z] 21:41:53 INFO - chrome://mochikit/content/browser-test.js:nextTest:860
[task 2019-09-19T21:41:53.116Z] 21:41:53 INFO - chrome://mochikit/content/browser-test.js:timeoutFn:1471
[task 2019-09-19T21:41:53.116Z] 21:41:53 INFO - setTimeout handlerchrome://mochikit/content/browser-test.js:Tester_execTest:1418
[task 2019-09-19T21:41:53.116Z] 21:41:53 INFO - chrome://mochikit/content/browser-test.js:nextTest/<:1213
[task 2019-09-19T21:41:53.116Z] 21:41:53 INFO - chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<:805
[task 2019-09-19T21:41:53.116Z] 21:41:53 INFO - GECKO(1653) | MEMORY STAT | vsize 8052MB | residentFast 640MB | heapAllocated 150MB
[task 2019-09-19T21:41:53.116Z] 21:41:53 INFO - TEST-OK | browser/components/extensions/test/browser/browser_ext_pageAction_click_types.js | took 90235ms
[task 2019-09-19T21:41:53.116Z] 21:41:53 INFO - GECKO(1653) | ++DOCSHELL 0x108448800 == 1 [pid = 1657] [id = {fd1222d7-182e-5048-95a4-c3b79cbfa28c}]
[task 2019-09-19T21:41:53.116Z] 21:41:53 INFO - GECKO(1653) | ++DOMWINDOW == 1 (0x10f2c4020) [pid = 1657] [serial = 307] [outer = 0x0]
[task 2019-09-19T21:41:53.116Z] 21:41:53 INFO - GECKO(1653) | ++DOMWINDOW == 2 (0x108498000) [pid = 1657] [serial = 308] [outer = 0x10f2c4020]
[task 2019-09-19T21:41:53.117Z] 21:41:53 INFO - GECKO(1653) | ++DOMWINDOW == 3 (0x10f0dcc00) [pid = 1657] [serial = 309] [outer = 0x10f2c4020]
[task 2019-09-19T21:41:53.129Z] 21:41:53 INFO - checking window state
[task 2019-09-19T21:41:53.137Z] 21:41:53 INFO - Not taking screenshot here: see the one that was previously logged
[task 2019-09-19T21:41:53.138Z] 21:41:53 INFO - TEST-UNEXPECTED-FAIL | browser/components/extensions/test/browser/browser_ext_pageAction_click_types.js | Uncaught exception received from previously timed out test - at chrome://mochitests/content/browser/browser/components/extensions/test/browser/head.js:765 - TypeError: SimpleTest.executeSoon is not a function
[task 2019-09-19T21:41:53.138Z] 21:41:53 INFO - Stack trace:
[task 2019-09-19T21:41:53.138Z] 21:41:53 INFO - triggerPageActionWithKeyboardInPanel@chrome://mochitests/content/browser/browser/components/extensions/test/browser/head.js:765:10
[task 2019-09-19T21:41:53.138Z] 21:41:53 INFO - asynctestClickPageAction@chrome://mochitests/content/browser/browser/components/extensions/test/browser/browser_ext_pageAction_click_types.js:73:13
[task 2019-09-19T21:41:53.138Z] 21:41:53 INFO - asynctest_clickData@chrome://mochitests/content/browser/browser/components/extensions/test/browser/browser_ext_pageAction_click_types.js:85:9
[task 2019-09-19T21:41:53.138Z] 21:41:53 INFO - AsyncTester_execTest/<@chrome://mochikit/content/browser-test.js:1350:34
[task 2019-09-19T21:41:53.138Z] 21:41:53 INFO - Tester_execTest@chrome://mochikit/content/browser-test.js:1385:11
[task 2019-09-19T21:41:53.138Z] 21:41:53 INFO - nextTest/<@chrome://mochikit/content/browser-test.js:1213:14
[task 2019-09-19T21:41:53.138Z] 21:41:53 INFO - SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:805:67

The cause of the Mac failure is that the keypress tests were skipping panel handling code by not navigating through overflow/pageActions panels.

Gijs, would you be willing to provide some guidance on potentially modifiying CustomizableUI/PageAction code in order to support this feature more cleanly?

To summarize the approach so far, whenever a click or key event occurs that will activate a page or browser action (even if not directly), we save the button and modifier information on the extension's browserAction/pageAction class itself. One reason for this is that some of the events we use when activating the buttons. A concrete example of this is that browerActions are opened in onViewShowing():
https://searchfox.org/mozilla-central/rev/7cc0f0e89cb40e43bf5c96906f13d44705401042/browser/components/extensions/parent/ext-browserAction.js#207
where the "ViewShowing" event passed to this method doesn't have modifier information from the original click event. This also results in a lot of the implementation being conditional on the which place (panel vs urlbar/navbar) the given browser/page action's node lives.

Here are some things that I would like to start by asking:

• One thing that really makes the implementation sketchy is that PanelMultiView captures keydown events on the document, and then dispatches a command event with none of the modifiers of the keydown event:
  https://searchfox.org/mozilla-central/rev/7cc0f0e89cb40e43bf5c96906f13d44705401042/browser/components/customizableui/PanelMultiView.jsm#1833
  How do you feel about dispatching a command event that has these modifiers instead (and potentially attaching this information to the mousedown and click events)? Does your answer change if it it can only be done conditionally because some button/element that can be in the PanelMultiView relies on the doCommand() method itself being called, instead of the command event we expect to dispatch?

• What is your opinion of taking a more broad approach, such as saving modifier information in the customizableUI code? For instance, the widgets could track the relevant information from the last click/key event that occurred on any of their nodes, and if any component captures these events, that component could be responsible for ensuring the widget updates its modifier and click information accordingly.

Thanks!

(In reply to graham.mcknight2 from comment #25)

Gijs, would you be willing to provide some guidance on potentially modifiying CustomizableUI/PageAction code in order to support this feature more cleanly?

Sure. I haven't followed the patch here though so I'm a little lost as to whether you're asking me about a completely new plan or about the patch as-is, and how it relates to the backouts of the patch so far...

To summarize the approach so far, whenever a click or key event occurs that will activate a page or browser action (even if not directly), we save the button and modifier information on the extension's browserAction/pageAction class itself.

I understand this bit.

One reason for this is that some of the events we use when activating the buttons. A concrete example of this is that browerActions are opened in onViewShowing():
https://searchfox.org/mozilla-central/rev/7cc0f0e89cb40e43bf5c96906f13d44705401042/browser/components/extensions/parent/ext-browserAction.js#207
where the "ViewShowing" event passed to this method doesn't have modifier information from the original click event. This also results in a lot of the implementation being conditional on the which place (panel vs urlbar/navbar) the given browser/page action's node lives.

But I'm confused by this bit because I don't see conditions about the placement of the button in the link you gave (though I didn't look in too much detail) - only checks that have to deal with not knowing if the browser/page action has a popupURL, which is a webextension issue (ie the webextension can change that at runtime, IIRC - but that's nothing to do with where the button itself is).

Here are some things that I would like to start by asking:

• One thing that really makes the implementation sketchy is that PanelMultiView captures keydown events on the document, and then dispatches a command event with none of the modifiers of the keydown event:
  https://searchfox.org/mozilla-central/rev/7cc0f0e89cb40e43bf5c96906f13d44705401042/browser/components/customizableui/PanelMultiView.jsm#1833
  How do you feel about dispatching a command event that has these modifiers instead (and potentially attaching this information to the mousedown and click events)?

A few notes here:

• I don't think this code should be running when focus is in an extension subview - they have HTML frames and focus should Just Work without us micromanaging it. In fact, I thought we already excluded those. If not, IIRC it's easy to do so - there's an attribute we check for on subviews, ISTR. (Asking because review comments on phab indicate you're worried about non-XUL items that don't have a doCommand() method - that shouldn't happen, I think?)
• Have you checked what happens for "real" modified-clicks? That is, for builtin toolbar buttons (on the toolbar, not in a panel), if you modifier-click them, does a command event fire, and what modifiers does it contain? We should probably do the same thing here...
• ditto for keyboard events; for the toolbar those are handled in https://searchfox.org/mozilla-central/source/browser/base/content/browser-toolbarKeyNav.js#319-347 - it doesn't look like your patch touches that code right now, which I'm finding a little confusing.

Does your answer change if it it can only be done conditionally because some button/element that can be in the PanelMultiView relies on the doCommand() method itself being called, instead of the command event we expect to dispatch?

I'm afraid I don't understand this question.

• What is your opinion of taking a more broad approach, such as saving modifier information in the customizableUI code? For instance, the widgets could track the relevant information from the last click/key event that occurred on any of their nodes, and if any component captures these events, that component could be responsible for ensuring the widget updates its modifier and click information accordingly.

I think generally trying to keep "state" like this around is likely a losing battle because we have to remember to listen for all possible events everywhere, and things get very messy when people call preventDefault() / stopPropagation() in various parts of the affected subtree.

Why can't we get this information off the event? E.g. in your patch in the browserAction's onClicked event manager, why can't we read things off of event which is in scope there?

This is not so related to the backouts as it is to the general code quality of the current approach.

I guess I'm asking about both the patch as-is and a new plan. I mean, I'm trying to take the many listeners in this patch and reduce them to something more consistent. So, I'm trying to assess whether I can accomplish this by modifying customizableUI code. That could mean rewriting the patch, but it's also possible (probable, actually) that I have just overlooked something that does what I want:

When looking into one of your questions, I found that the onBeforeCommand handler already fires right before the widget is activated. In 3/4 cases, an event with the info I want is passed in. As long as the PanelMultiView propagates event modifiers in key navigation, I should, in theory have all the needed information going to a single handler (and one whose behavior I think is unlikely to change).

Why can't we get this information off the event?

That indeed works. It's just that different listeners are sometimes needed depending on where the widget, and it's potentially a bad idea to rely on the particular location's implementation in extension page/browser action code.

I'm confused by this bit because I don't see conditions about the placement of the button in the link you gave

That was maybe a poor choice of what to link. What I wanted to demonstrate is that is that the event passed in doesn't have the needed modifiers, so that info has to be obtained with an event listener on the earlier click event. The comment on the placement of the node is not so much related to the link as it is to the fact that we have to picks events in which to save the modifiers, and that the same event doesn't necessarily work in both the toolbar and panel.

I'm afraid I don't understand this question.

This was the concern of non-XUL elements being allowed as buttons in some PanelMultiView.

ditto for keyboard events; for the toolbar those are handled in https://searchfox.org/mozilla-central/source/browser/base/content/browser-toolbarKeyNav.js#319-347

I didn't realize that the toolbar's key navigation was attempting to emulate a click as well. For the extension browser actions, I think the widget handling code gets the event first and prevents this from occurring: https://searchfox.org/mozilla-central/rev/05a22d864814cb1e4352faa4004e1f975c7d2eb9/browser/components/customizableui/CustomizableUI.jsm#1951-1958

Have you checked what happens for "real" modified-clicks? That is, for builtin toolbar buttons (on the toolbar, not in a panel), if you modifier-click them, does a command event fire, and what modifiers does it contain? We should probably do the same thing here...

• Browser action: command event fires with the same modifiers as the click, fired here: https://searchfox.org/mozilla-central/rev/05a22d864814cb1e4352faa4004e1f975c7d2eb9/layout/xul/nsButtonBoxFrame.cpp#205-209. One interesting distinction is that for clicks, the click event occurs first.
• Page action: command event doesn't fire because it's not a XUL element. In the panel however, a command event is fired and the page action implementation relies on this: https://searchfox.org/mozilla-central/rev/05a22d864814cb1e4352faa4004e1f975c7d2eb9/browser/base/content/browser-pageActions.js#251-253.

Looks like I didn't answer the question about toolbarbuttons when keypressed; those also fire command events with modifiers in the nav-bar.

Focusing on the PanelMultiView do I have the right takeaways here?

1. Since the toolbarbuttons in the navbar dispatch command events which have the modifiers, it should be alright to make the event carry the modifiers in the PanelMultiView.
2. I'm most likely able to get away with the assumption that the elements which are navigable in the panel are XULElements, and replace doCommand() by dispatching the event it would have fired, but with modifiers (but should maybe proceed with caution when doing this?).

Sorry for the slow response here, been distracted hither and thither all day.

(In reply to graham.mcknight2 from comment #28)

Looks like I didn't answer the question about toolbarbuttons when keypressed; those also fire command events with modifiers in the nav-bar.

Focusing on the PanelMultiView do I have the right takeaways here?

1. Since the toolbarbuttons in the navbar dispatch command events which have the modifiers, it should be alright to make the event carry the modifiers in the PanelMultiView.
2. I'm most likely able to get away with the assumption that the elements which are navigable in the panel are XULElements, and replace doCommand() by dispatching the event it would have fired, but with modifiers (but should maybe proceed with caution when doing this?).

Yes, I think both of these are correct. Based on the other places you linked in comment #27, it seems we should test a variety of cases here to ensure existing behaviour is preserved. A test matrix of some description might be helpful, for combinations of:

• enter/space activation
• click activation
• page actions in the url bar vs. page action menu
• browser actions in the navbar vs. overflow menu
• browser/page actions that open subview/popup vs do not open anything
• builtin (non-addon) page action items in both locations
• builtin (non-addon) browser buttons in both locations, that open popups vs. don't open things
• specialcases for non-panelmultiview things like the pocket and bookmark star items

Unfortunately per your earlier links all of this seems... hairy. :-(

It would be nice if we could simplify this rather than adding more cases where we manually dispatch and/or ignore events based on unclear preconditions, but I'm not sure how easily that is possible (if at all).

Does that help?

Yes; thanks -- that's very helpful!

Beating the toolbar navigator to these key events while also ensuring the handling of everything that would get to the events before the toolbar navigation is a hard problem. I'm not quite what to do about it in general, but I am a little more adamant that dispatching a command event with the modifiers from the key event is a favorable behavior for the PanelMultiView.

It sounds like I should catalog the current behaviors for the permutations (w/r/t the command event), indicate which are affected by the change I'm proposing, and verify their behavior.

Last question: do we want to turn some of the affected combinations into browser tests? Not sure if this would help refactors down the road or if it will just be redundant as some of the existing tests do check the behavior of keyboard activation inside a PanelMultiView already (for instance browser_panel_keyboard_navigation). If so, it might be best to track the change I'm suggesting in another bug/patch to avoid trying to do too much more in this one. Right now, this patch has tests for keyboard activation for addon page/browser actions in both locations.

Thanks for the help here!

(In reply to graham.mcknight2 from comment #30)

Last question: do we want to turn some of the affected combinations into browser tests? Not sure if this would help refactors down the road or if it will just be redundant as some of the existing tests do check the behavior of keyboard activation inside a PanelMultiView already (for instance browser_panel_keyboard_navigation). If so, it might be best to track the change I'm suggesting in another bug/patch to avoid trying to do too much more in this one. Right now, this patch has tests for keyboard activation for addon page/browser actions in both locations.

I think at a minimum we should have tests for the cases we're changing here, though it might not hurt to have a generic test that checks the thing we're interested in (modified keyboard/mouse behaviour) on a number of different widgets - it'll make creating fixes here and being confident in them working correctly across platforms easier.

https://hg.mozilla.org/mozilla-central/rev/317bf7c3f39c

I think this falls into the category of bugs that need QE verification. It looks like I'm unable to set the qeverify+ flag as the dropdown isn't clickable for me. Rob, are you able to set the flag? Also, are there any documentation needs for this or do the changes to browser/components/extensions/schemas/page_action.json and browser/components/extensions/schemas/browser_action.json satisfy this?

There are a lot of things going on here. Here are some notes that might help with verification:

The way that I did manual testing for this by modifying the open-my-page-button extension. The extensions I attached are also modifications meant to help test the specific parts:

• Click info extension: logs the click data (button, modifiers) passed in when the browser action or page action are clicked.
• Disabled buttons extension: the browser action and page action are disabled/not shown.
• Always open popup extension: page action and browser action each have a popup. When middle clicked, they call openPopup() to show this popup, which is not the same flow as left clicking where the popup is shown immediately.

Unfortunately, because the implementation of handling the events is slightly different based on the container (navbar/urlbar vs panel), we have a lot of different concerns here:

• Click modifiers (shift/crtl/alt/meta) are propagated when page actions and browser actions are left clicked
• Click modifiers are propagated when page actions and browser actions are middle clicked and the button is 1 in this case
• Click modifiers are propagated when triggering page actions and browser actions with keyboard
• All of the above work with the page action in the page actions panel (the triple dot in the urlbar opens this panel), and when the browser action is moved to the overflow panel via customization (Hamburger menu -> Customize)
• Middle clicks have no effect when the browser action is disabled or the page action is not shown (still visible in the page actions panel).
• When an extension calls openPopup() due to a middle click, it works as expected (ie. works as if left clicked, doesn't close immediately due to the click event)

To verify these it takes me about ~20 minutes on one platform. Every combination of click modifiers for every situation listed above isn't necessary; if a single modifier is carried to the extension, then all are. However, I found that alternating the modifier I used was needed to reveal some errors because we save the previous click modifiers (ie. using control to left click a page action, then shift when triggering it with the keyboard). Not to mention, there are combinations that wouldn't result in an event in the first place, such as meta + space on Mac which opens spotlight instead.

I've tested this mainly on Windows, tested the on Mac a few times, but tested on Linux only once or twice. Generally, there isn't a whole lot of platform-specific stuff going on here as long as the key events get intercepted by the panel code. The only differences that I know of are on Mac:

• The control key adds both "Ctrl" and "MacCtrl" modifiers for control-clicks.
• XULElements treat the enter key differently than they treat the spacebar. It's the panel code that makes up the difference here.

Thanks for providing the test cases and expectations! I have added qe-verify+ so that QE can take a look and verify on Linux, macOS and Windows.

This feature should also be documented in MDN, by updating the articles of:

• https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/pageAction/onClicked
• https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/browserAction/onClicked

It may also be useful to add a new row to the compat table of those pages, which can be done by adding an entry after the following locations in the BCD (browser compat data) repository:

• https://github.com/mdn/browser-compat-data/blob/ea8e1eefdda1d86ea0d62cfd31a3c0ff11f095dc/webextensions/api/browserAction.json#L341-L360
• https://github.com/mdn/browser-compat-data/blob/ea8e1eefdda1d86ea0d62cfd31a3c0ff11f095dc/webextensions/api/pageAction.json#L118-L137

(by adding dev-doc-needed, eventually someone from the documentation team will take a look and document this new feature. If you want to, you could also update the docs yourself, but this is not required at all)

Verified this enhancement on Windows Pro 10 64-bit, Ubuntu 18.04.3 LTS and macOS Catalina 10.15 on FF 72.0a1 (20191031194708)
To start with I’ve checked that after loading the "Disabled buttons" extensions on each of those OSes the browser action button is displayed in the browser but unresponsive and that the page Action is hidden in the page action panel and no actions are logged in the console when trying to trigger it.
For the "Always open popup" extension the tests performed were verifying that when left clicking/middle clicking/ pressing Enter on any of the locations where the pageAction button (pageaction panel, URL bar) and browserAction button (navigation bar. Overflow menu), the “It’s my page!” pop-up shows up.
In the case of the "Click info extensions" the tests were done to see if any of the left, or middle clicks, by themselves or in combinations with the ALT,SHIFT, CTRL keys produce the appropriate console logs on Windows and Ubuntu.
On macOS I’ve also tried it in combination with the command key.
One little thing that is not behaving in an appropriate manner (but I considered it out of scope for this ticket) is that when opening a new page (which has an URL address) the pageAction icons are not displayed in the navigation bar or the address bar until moving and returning from a previously opened tab.
On another note, the UsableHomeButton extension was a good real life test to verify that the new tabs can be opened using middle click or Enter.

Verified this enhancement on Windows Pro 10 64-bit, Ubuntu 18.04.3 LTS and macOS Catalina 10.15 on FF 72.0a1 (20191031194708)

Thanks for verifying this bug!

One little thing that is not behaving in an appropriate manner (but I considered it out of scope for this ticket) is that when opening a new page (which has an URL address) the pageAction icons are not displayed in the navigation bar or the address bar until moving and returning from a previously opened tab.

This is a problem with the demo extensions that I uploaded; they always make the pageAction show on updates to the current tab, which is misses the case where the extension is first loaded.

The schema documentation for buttonstates that it is "An integer value of button by which menu item was clicked.” From my reading of the bug/spec I think that this is recording either the activation of the left or middle mouse buttons. Correct? If so, what are the integers associated with each of the 2 mouse buttons?

The standard values for the MouseEvent interface are used @ https://developer.mozilla.org/en-US/docs/Web/API/MouseEvent/button
Currently, only the following values are supported (see the link on MDN for more details):

• 0 - left click (or a click not associated with a mouse, such as using the keyboard)
• 1 - middle/wheel click

Other buttons (e.g. 2 - right click) are not supported.

Details of OnClickData have been added to browserAction.onClicked and pageAction.onClicked. Updated compatibility data has been submitted in PR #5285. Please let me know if any changes are needed or I can mark this is dev-doc-complete. Thanks!

Thanks!