1295276 - [commands] Handle panel close correctly when a panel is displayed.

Status: NEW

(Firefox :: Toolbars and Customization, defect, P3)

Description

[Matthew Wein [:mattw]]

As pointed out by Gijs in https://bugzilla.mozilla.org/show_bug.cgi?id=1246034#c9, it may be possible for _execute_browser_action to close the PageAction popup if it is showing.  We should make sure that this won't happen and add in the necessary tests to confirm it.

Comment 1

[Kris Maglione [:kmag]]

I think the correct behavior is actually to close the pageAction popup and then execute the browserAction.

Comment 2

[Steven Ellul]

Can i work on this as my first bug?
Thanks

Comment 3

[Tomislav Jovanovic :zombie]

Yes!  Again, ask if you get stuck.

Comment 4

[Steven Ellul]

Not really sure how to start here.

To me it looks like the first step is to try to get a browser action to close the pageAction popup before trying to account for that.

How do I recreate this possible issue?

Thanks

Comment 5

[Matthew Wein [:mattw]]

(In reply to Steven Ellul from comment #4)
> Not really sure how to start here.
> 
> To me it looks like the first step is to try to get a browser action to
> close the pageAction popup before trying to account for that.

I think this sounds like the right way to start.

The first step would be to add a test to http://searchfox.org/mozilla-central/source/browser/components/extensions/test/browser/browser_ext_commands_execute_browser_action.js which opens a page action popup and then uses the shortcut to execute the browser action. The test should check to see if the browser action popup is shown and since this isn't fixed yet the test should fail.

The second step would be to fix the issue by closing the page action popup before executing the browser action. I think the code to close the page action popup would go before this line here: https://dxr.mozilla.org/mozilla-central/source/browser/components/extensions/ext-commands.js#133.

The last step would be to confirm that the test passes now. If so, you're all done :)

Here's some more information on how to write the test:
 - An example test to follow: http://searchfox.org/mozilla-central/source/browser/components/extensions/test/browser/browser_ext_commands_execute_browser_action.js#106

 - How to handle options passed into the test: http://searchfox.org/mozilla-central/source/browser/components/extensions/test/browser/browser_ext_commands_execute_browser_action.js#21

 - To open the page action popup you'll first need to show the page action - http://searchfox.org/mozilla-central/source/browser/components/extensions/test/browser/browser_ext_commands_execute_page_action.js#101, and then you can open it using https://dxr.mozilla.org/mozilla-central/source/browser/components/extensions/test/browser/browser_ext_pageAction_popup.js#146.

Writing the test for this might be a little tricky, so please ping me if you would like any more help (my irc nick is `k9`).

Comment 6

[Matthew Wein [:mattw]]

I'm usually around in #webextensions, and I'm sure others there would be happy to help out as well.

Comment 7

[Steven Ellul]

Unassigned myself.

Sorry to have prevented others working on this, but I was a bit silly asking to be assigned to this bug because it is outside of my skill set.

Comment 8

[Thomas Wisniewski]

Attachment: 1295276-close-the-page-action-popup-just-before-handling-_execute_browser_action.diff

Here's a patch to close the page action popup before _execute_browser_action, as per comment 5.

A try run seems fine, there just appear to be unrelated intermittents: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f021dda0162f6390352b194de6006f0cf190ad38

Comment 9

[Matthew Wein [:mattw]]

Comment on attachment 8881082 [details] [diff] [review]
1295276-close-the-page-action-popup-just-before-handling-_execute_browser_action.diff

Review of attachment 8881082 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for working on this! I'll be happy to r+ after the additional test is added.

::: browser/components/extensions/test/browser/browser_ext_commands_execute_browser_action.js
@@ +118,5 @@
> +  if (options.withPageActionPopup) {
> +    await awaitExtensionPanel(extension);
> +    EventUtils.synthesizeKey("j", {altKey: true, shiftKey: true});
> +    await extension.awaitFinish("execute-browser-action-popup-opened");
> +    SimpleTest.is(getPageActionPopup(extension), null, "Page action popup was hidden");

Could you add a test to confirm that `getPageActionPopup(extension)` is not null before opening the browser action popup?

Comment 10

[Thomas Wisniewski]

Attachment: 1295276-close-the-page-action-popup-just-before-handling-_execute_browser_action.diff

Sure, here's a new copy of the patch, just with this line added:

>SimpleTest.isnot(getPageActionPopup(extension), null, "Page action popup was shown");

I take it that's all you meant, or was there another test that I might be missing?

Comment 11

[Matthew Wein [:mattw]]

Comment on attachment 8881488 [details] [diff] [review]
1295276-close-the-page-action-popup-just-before-handling-_execute_browser_action.diff

Review of attachment 8881488 [details] [diff] [review]:
-----------------------------------------------------------------

Yeah, this looks good to me. Passing on to mixedpuppy for the stamp of approval.

Comment 12

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8881488 [details] [diff] [review]
1295276-close-the-page-action-popup-just-before-handling-_execute_browser_action.diff

I think this fix is too narrow.  We should probably check if *any* panel is open, and close it.  We should also do that for _execute_page_action.  mattw is going to investigate a bit more.

Comment 13

[Thomas Wisniewski]

mattw, have you had a chance to investigate this one yet (as per comment 12)?

Comment 14

[Shane Caraveo (:mixedpuppy)]

mattw is gone, he may pop in, but lets not depend on that.

At a minimum lets do the same for browserAction, but really I'd prefer a general fix, we shouldn't ever have two panels open without being explicit about wanting that.

Comment 15

[Thomas Wisniewski]

>At a minimum lets do the same for browserAction

I'm not quite sure how to do this. While the browser action popup is open, no page event commands seem to be being sent to the handler in ext-commands.js. I have to close the browser action popup first. Oddly, browser action commands *are* still being sent, just not page action ones.

I'm basically just doing this in a testcase:

>  // first open browser action popup
>  await EventUtils.synthesizeKey("3", {altKey: true, shiftKey: true});
>  let browser = await awaitExtensionPanel(extension);
>  let panel = getPanelForNode(browser);
> 
>  // now open the page action popup
>  await EventUtils.synthesizeKey("j", {altKey: true, shiftKey: true});
>  await awaitExtensionPanel(extension);

With logging, I can see that the first call to synthesize key ends up sending a command event and the browser action popup opens. But the second call to synthesizeKey never triggers a command event, and so we're stuck waiting forever in the last line above (unless I manually dismiss the browser action popup by clicking the mouse, then press the page action key myself). And if I have the second synthesizeKey send "3" instead, a second command is sent.

Comment 16

[Shane Caraveo (:mixedpuppy)]

Is the content in the browserAction getting that event instead?  What if you are explicit about the window the event happens on? (looks like that should be the default though).  I also wonder if CUI may be doing something here.  Maybe Gijs would know.

Comment 17

[Thomas Wisniewski]

>Is the content in the browserAction getting that event instead?

No, the though the content script does get the first synthesized keypress which opens the browser action popup (but not the second one which is meant to trigger the page action popup).

>What if you are explicit about the window the event happens on?

No dice, I'm afraid. I tried the following:

>let windowId = await extension.awaitMessage("windowId"); // returns the id of the window returned by browser.windows.getCurrent()
>let {Management: {global: {windowTracker}}} = Cu.import("resource://gre/modules/Extension.jsm", {});
>let win = windowTracker.getWindow(windowId);
>await EventUtils.synthesizeKey("3", {altKey: true, shiftKey: true}, win);

I guess I'll just wait for Gijs' feedback.

Comment 18

[:Gijs (he/him)]

(In reply to Thomas Wisniewski from comment #17)
> >Is the content in the browserAction getting that event instead?
> 
> No, the though the content script does get the first synthesized keypress
> which opens the browser action popup (but not the second one which is meant
> to trigger the page action popup).
> 
> >What if you are explicit about the window the event happens on?
> 
> No dice, I'm afraid. I tried the following:
> 
> >let windowId = await extension.awaitMessage("windowId"); // returns the id of the window returned by browser.windows.getCurrent()
> >let {Management: {global: {windowTracker}}} = Cu.import("resource://gre/modules/Extension.jsm", {});
> >let win = windowTracker.getWindow(windowId);
> >await EventUtils.synthesizeKey("3", {altKey: true, shiftKey: true}, win);
> 
> I guess I'll just wait for Gijs' feedback.

CUI won't be doing anything, but it sounds like focus is in the document in the iframe of the browser action popup that's open? Without moving focus (and doing that without closing the popup might be tricky).

I would like to help more than that, but I can't tell what this bug is about from the summary, comment #0 and the description of the (2-months-old) patch on the bug, because they conflict (well, the summary is just nondescript rather than conclicting - what's "correct"?).

What are we actually trying to do? If we want to close all other panels, I suspect you want something like:

let panels = document.querySelectorAll("panel");
for (let panel of panels) {
  if (panel.state != "closed")
    panel.hidePopup();
}

note that this doesn't close menupopups or <popup> nodes. I don't know if you want those closed, too, though if you did you could just expand the selector (and maybe rename the variables).

There isn't a sane API, that I know of, to close all popups. Note also that some things (like UITour) explicitly keep some popups permanently open (noautohide). Closing their popups may or may not mess with them, you'd want to test that.

Comment 19

[Thomas Wisniewski]

>What are we actually trying to do?

For now, just to ensure that the addon's page action panel is closed when the hotkey/command to open the browser action panel is sent, and vice versa (so only one is open at a given time). This would be done specifically when the relevant XUL command event is fired (in the same handler which already opens the panels/popups upon keypress).

However, in the specific case while the browser action panel is currently open (CUI-based), the page action hotkey command is either never fired, or at least doesn't end up being directed to that handler. Oddly, the command event for the currently open panel's own hotkey *is* being sent, just not the command event for the other one (the page action popup, which I don't think is CUI based). I also know that the command events are being triggered as expected while the browser action panel isn't open.

In other words, in my test-case, I'm simply sending two synthesized key events, one to open the browser action panel (which works) followed by one to open the page action popup (which is never sent, even when I press the key myself as the test runs, so it seems unrelated to whether it's a synthesized key press or real one).

I suppose it's possible that only one of the keys is being registered on the frame for the CUI panel, but I'm not sure how those key events are handled by its frame. The addon code in ext-commands.js seems to be registering both hotkeys with the top-level window, at least.

Comment 20

[:Gijs (he/him)]

(In reply to Thomas Wisniewski from comment #19)
> However, in the specific case while the browser action panel is currently
> open (CUI-based), the page action hotkey command is either never fired, or
> at least doesn't end up being directed to that handler.

Just in the automated test, or also when testing completely manually?

> Oddly, the command
> event for the currently open panel's own hotkey *is* being sent, just not
> the command event for the other one (the page action popup, which I don't
> think is CUI based)

How sure are you about this? Last I checked page actions were basically implemented the same way. Maybe that's changed. But either way, the hotkeys are completely unrelated to CUI, right?

Does your code work if you manually _execute_browser_action in the test?

> . I also know that the command events are being triggered
> as expected while the browser action panel isn't open.
> 
> In other words, in my test-case, I'm simply sending two synthesized key
> events, one to open the browser action panel (which works) followed by one
> to open the page action popup (which is never sent, even when I press the
> key myself as the test runs, so it seems unrelated to whether it's a
> synthesized key press or real one).

I don't really know anything about the internals of webextension popups and why they would steal command keys. Maybe Kris knows more.

> I suppose it's possible that only one of the keys is being registered on the
> frame for the CUI panel, but I'm not sure how those key events are handled
> by its frame. The addon code in ext-commands.js seems to be registering both
> hotkeys with the top-level window, at least.

Comment 21

[Thomas Wisniewski]

>Just in the automated test, or also when testing completely manually?

Good call, I just ran a test manually rather than through the test-suite, but the results are the same: while the browser action panel is open, the XUL command event for the page action menu is never received at the top of this function in ext-commands.js: https://dxr.mozilla.org/mozilla-central/source/browser/components/extensions/ext-commands.js#129
(Verified by adding a console.log(event) to the top of that function).

Note that both hotkey events are logged while the browser action panel *isn't* active (and their popups pop up), but while the browser action panel is active only its own hotkey works/is logged (the page action one doesn't get logged).

For the record, this is the interesting manifest.js section of the addon I tested with:
>  "commands": {
>    "_execute_page_action": {
>      "suggested_key": { "default": "Alt+Shift+J" }
>    },
>    "_execute_browser_action": {
>      "suggested_key": { "default": "Alt+Shift+3" }
>    }
>  },
>  "browser_action": {
>    "default_popup": "popup.html",
>  },
>  "page_action": {
>    "default_popup": "popup.html",
>  },
>  "permissions": ["<all_urls>"],
>  "background": {
>    "scripts": ["background.js"]
>  }

Where background.js just enables the page action:
>  browser.tabs.onUpdated.addListener(tabId => {
>    browser.pageAction.show(tabId);
>  });


And popup.html is trivial:
>  <html>POPUP</html>


>Last I checked page actions were basically implemented the same way. Maybe that's changed. But either way, the hotkeys are completely unrelated to CUI, right?

I'm actually not sure about it, so you may be right (I'm quite new to this code and CUI in general).


>Does your code work if you manually _execute_browser_action in the test?

No, while the browser action popup is open, it doesn't matter whether I press the key for the page action menu manually or with synthesizeKey, it doesn't come up either way (tested with console.log as above).

Comment 22

[Shane Caraveo (:mixedpuppy)]

I should verify if this is still an issue, and whether it is a good-first-bug (given amount of comments).

Comment 23

[Shane Caraveo (:mixedpuppy)]

I've verified this is still an issue.  Bug 1447723 will need to land before others can reproduce this.

also: Repeatedly using the keys to open the page action in fact opens multiple panels on top of each other, rather than open/close as the browser action does.

Comment 24

[Shane Caraveo (:mixedpuppy)]

Should be able to close any non-extension page action by:

if (window.BrowserPageActions.activatedActionPanelNode) {
  window.BrowserPageActions.togglePanelForAction();
}

however it might be best to add a BrowserPageActions.closeActivatedPanel method.

I also wonder if we should set activatedActionPanelNode to panelNode if one is provided.  That would ensure any action could be closed.

/me looking into cui toolbar buttons and browser action buttons for same functionality.

The ideal situation would be if there were a window.activatedPanelNode available and we could just call window.activatedPanelNode.hidePopup();  All panels would hide the prior panel and set activatedPanelNode.

Gijs, thoughts on this?

Comment 25

[:Gijs (he/him)]

(In reply to Shane Caraveo (:mixedpuppy) from comment #24)
> Gijs, thoughts on this?

I'm not really sure what the question is. Off-hand keeping track of it like you suggest seems OK, though you might already be able to do what you want just by using querySelector and an attribute set on the panel when it opens, esp. if it's using panelmultiview.

I'm less sure that storing the active panel as a global on the window would be the best idea, though OTOH I don't have a much better idea. If we do do that, perhaps instead of having JS keep track of it, we could implement it in C++/webidl from the popup manager code.

Unfortunately, PanelMultiView panels need to be closed by using the relevant methods from there, not by just calling popup.hidePopup() directly.

Does that help?

Comment 26

[Shane Caraveo (:mixedpuppy)]

I guess the question is, should opening a page action or toolbar panel force all other panels closed?  When mouse clicking this is handled already (e.g. noautohide), but using key commands it is not.  

This can be replicated without webextensions using the download and bookmark buttons.  Open the downloads panel, then (on osx) cmd+D to open the bookmark page action.

Comment 27

[Shane Caraveo (:mixedpuppy)]

mean to ni? for comment 26

Comment 28

[:Gijs (he/him)]

(In reply to Shane Caraveo (:mixedpuppy) from comment #26)
> I guess the question is, should opening a page action or toolbar panel force
> all other panels closed?  When mouse clicking this is handled already (e.g.
> noautohide), but using key commands it is not.  

This is a UX question. I think the answer is likely "yes", but I'm not sure. Maybe Aaron can give a definitive answer. (Aaron: see also next quoted section on how to see current behaviour that may be thought of as problematic.)

> This can be replicated without webextensions using the download and bookmark
> buttons.  Open the downloads panel, then (on osx) cmd+D to open the bookmark
> page action.

Note that implementing this thoroughly will likely be tricky. For instance, submenus in "old-style" menupopup elements are themselves menupopups, and of course in that case multiple popups can be open at any one time. Similarly, <select> is implemented with a panel (I think?) as are form validation and date/time picker things. Of course those should ideally work "inside" webextension popups (for instance).

Comment 29

[Shane Caraveo (:mixedpuppy)]

Since this is not a webext bug per my STR in comment 26, moving it to firefox/untriaged.

Comment 30

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months and this bug has priority 'P2'.
:Gijs, could you have a look please?
For more information, please visit auto_nag documentation.