[commands] Handle _execute_browser_action correctly when a pageAction popup is displayed.

NEW
Assigned to
(NeedInfo from)

Status

()

Toolkit
WebExtensions: Frontend
P3
normal
a year ago
23 hours ago

People

(Reporter: mattw, Assigned: Thomas Wisniewski, Mentored, NeedInfo)

Tracking

(Blocks: 2 bugs, {good-first-bug})

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [commands] triaged)

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

a year ago
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

Updated

a year ago
Mentor: mwein@mozilla.com
Priority: -- → P3
Whiteboard: [commands] → [commands][good first bug] triaged
(Reporter)

Updated

a year ago
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]
(Reporter)

Updated

a year ago
Priority: -- → P3

Updated

a year ago
Whiteboard: [commands] → [commands] triaged

Updated

11 months ago
Blocks: 1237377

Comment 2

9 months ago
Can i work on this as my first bug?
Thanks
Yes!  Again, ask if you get stuck.
Assignee: nobody → stevenellul
Status: NEW → ASSIGNED

Comment 4

9 months ago
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)
(Reporter)

Comment 5

9 months ago
(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)
(Reporter)

Comment 6

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

Comment 7

8 months ago
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

Updated

8 months ago
Assignee: stevenellul → nobody
(Assignee)

Comment 8

2 months ago
Created attachment 8881082 [details] [diff] [review]
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
Assignee: nobody → wisniewskit
Attachment #8881082 - Flags: review?(mwein)
(Reporter)

Comment 9

2 months ago
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)
(Assignee)

Comment 10

2 months ago
Created attachment 8881488 [details] [diff] [review]
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?
Attachment #8881082 - Attachment is obsolete: true
Attachment #8881488 - Flags: review?(mwein)
(Reporter)

Comment 11

2 months ago
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+
(Assignee)

Comment 13

15 days ago
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)
(Assignee)

Comment 15

6 days ago
>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)
(Assignee)

Comment 17

5 days ago
>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

4 days ago
(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)
(Assignee)

Comment 19

4 days ago
>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)

Comment 20

4 days ago
(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)
(Assignee)

Comment 21

4 days ago
>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).
You need to log in before you can comment on or make changes to this bug.