Open Bug 1295276 Opened 3 years ago Updated Last year

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

Categories

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

defect

Tracking

()

People

(Reporter: mattw, Assigned: wisniewskit, Mentored, NeedInfo)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [commands] triaged)

Attachments

(1 file, 1 obsolete file)

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.
I think the correct behavior is actually to close the pageAction popup and then execute the browserAction.
Component: WebExtensions → WebExtensions: Frontend
Mentor: mwein
Priority: -- → P3
Whiteboard: [commands] → [commands][good first bug] triaged
Keywords: good-first-bug
Priority: P3 → --
Summary: [commands] Make sure _execute_browser_action only closes the browser action popup → [commands] Handle _execute_browser_action correctly when a pageAction popup is displayed.
Whiteboard: [commands][good first bug] triaged → [commands]
Priority: -- → P3
Whiteboard: [commands] → [commands] triaged
Can i work on this as my first bug?
Thanks
Yes!  Again, ask if you get stuck.
Assignee: nobody → stevenellul
Status: NEW → ASSIGNED
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
Flags: needinfo?(mwein)
(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`).
Flags: needinfo?(mwein)
I'm usually around in #webextensions, and I'm sure others there would be happy to help out as well.
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.
Status: ASSIGNED → NEW
Assignee: stevenellul → nobody
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
Assignee: nobody → wisniewskit
Attachment #8881082 - Flags: review?(mwein)
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?
Attachment #8881082 - Flags: review?(mwein)
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?
Attachment #8881082 - Attachment is obsolete: true
Attachment #8881488 - Flags: review?(mwein)
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.
Attachment #8881488 - Flags: review?(mwein)
Attachment #8881488 - Flags: review?(mixedpuppy)
Attachment #8881488 - Flags: review+
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.
Flags: needinfo?(mwein)
Attachment #8881488 - Flags: review?(mixedpuppy)
Attachment #8881488 - Flags: review+
mattw, have you had a chance to investigate this one yet (as per comment 12)?
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.
Flags: needinfo?(matthewjwein)
>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.
Flags: needinfo?(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.
Flags: needinfo?(mixedpuppy) → needinfo?(gijskruitbosch+bugs)
>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.
(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.
Flags: needinfo?(gijskruitbosch+bugs)
>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.
Flags: needinfo?(gijskruitbosch+bugs)
(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.
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(kmaglione+bmo)
>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).
Mentor: matthewjwein → scaraveo
I should verify if this is still an issue, and whether it is a good-first-bug (given amount of comments).
Flags: needinfo?(mixedpuppy)
Depends on: 1447723
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.
Flags: needinfo?(mixedpuppy)
Keywords: good-first-bug
Priority: P3 → P2
Depends on: 1447796
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?
Flags: needinfo?(gijskruitbosch+bugs)
(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?
Flags: needinfo?(gijskruitbosch+bugs)
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.
mean to ni? for comment 26
Flags: needinfo?(gijskruitbosch+bugs)
(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).
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(abenson)
Since this is not a webext bug per my STR in comment 26, moving it to firefox/untriaged.
Component: WebExtensions: Frontend → Untriaged
Product: Toolkit → Firefox
Summary: [commands] Handle _execute_browser_action correctly when a pageAction popup is displayed. → [commands] Handle panel close correctly when a panel is displayed.
Component: Untriaged → Toolbars and Customization
Flags: needinfo?(kmaglione+bmo)
You need to log in before you can comment on or make changes to this bug.