1447796 - page action commands only open, do not close panels

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect, P1)

Description

[Shane Caraveo (:mixedpuppy)]

Page action command fails to close the page action panel when run twice.  With the below, the first key command should open the panel, the second should close it.  The pageaction code uses its own panel, so the browserPageAction stuff cannot control this.  ext-pageAction simply always assumes it needs to be opened.

manifest:

  "page_action": {
    "default_popup": "page.html",
    "show_matches": ["<all_urls>"],
    "browser_style": true
  },
  "commands": {
    "_execute_page_action": {
      "suggested_key": { "default": "Alt+Shift+3" }
    }
  }

I initially thought this might be a regression from bug 1395387, but it is not, and there are no tests built around this.  It may have simply always been.

Patch is on top of bug 1447723.

Comment 1

[Shane Caraveo (:mixedpuppy)]

Attachment: Bug 1447796 - fix closing page action panels using commands,

Review commit: https://reviewboard.mozilla.org/r/229956/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/229956/

Comment 2

[Shane Caraveo (:mixedpuppy)]

BTW, the bug here is that by using the key command to open the panel, you get multiple panels stacked up, never closing any.

Comment 3

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8961184 [details]
Bug 1447796 - fix closing page action panels using commands,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/229956/diff/1-2/

Comment 4

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8961184 [details]
Bug 1447796 - fix closing page action panels using commands,

https://reviewboard.mozilla.org/r/229956/#review236224

::: browser/components/extensions/ext-pageAction.js:266
(Diff revision 2)
>    // window.
>    // If the page action has a |popup| property, a panel is opened to
>    // that URL. Otherwise, a "click" event is emitted, and dispatched to
>    // the any click listeners in the add-on.
>    async handleClick(window) {
> +    let opening = !this.popupNode || this.popupNode.panel.state === "closed";

I'm wondering if we could try to tweak this fix a bit and make it a bit nicer to read, at a first look the `opening` variable is not immediately clear (if it want to catch a panel that is opening or if it is opening something, and there could be no popup url) and it is branching this method in 3 different more places, making it a bit hard to read and probably a bit easir to break with further changes that we may apply here in the future.

How about if we check if there is a popup to close later, after we have verified that there is a popupurl (otherwise this should just emit the onClicked event as usually when there is no popup)?

If I'm not reading it wrong, it could reduce the change to something like:

```
if (popupURL) {
  if (this.popupNode && this.popupNode.panel.state !== "closed") {
    // The panel is already open, close it now.
    TelemetryStopwatch.cancel(popupOpenTimingHistogram, this);
    window.BrowserPageActions.togglePanelForAction(this.browserPageActions,
                                                   this.popupNode.panel);
  } else {
    this.popupNode = new PanelPopup(this.extension, window.document, popupURL,
                                    this.browserStyle);
    ...
    
    window.BrowserPageActions.togglePanelForAction(this.browserPageAction,
                                                   this.popupNode.panel);
    TelemetryStopwatch.finish(popupOpenTimingHistogram, this);
  }
} else {
  ...
}

```

How that sounds to you?

Comment 5

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8961184 [details]
Bug 1447796 - fix closing page action panels using commands,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/229956/diff/2-3/

Comment 6

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8961184 [details]
Bug 1447796 - fix closing page action panels using commands,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/229956/diff/3-4/

Comment 7

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8961184 [details]
Bug 1447796 - fix closing page action panels using commands,

https://reviewboard.mozilla.org/r/229956/#review237062

::: browser/components/extensions/ext-pageAction.js:280
(Diff revision 4)
>      // open a popup.
>      if (popupURL) {
> -      let popup = new PanelPopup(this.extension, window.document, popupURL,
> +      if (this.popupNode && this.popupNode.panel.state !== "closed") {
> +        // The panel is being toggled closed.
> +        TelemetryStopwatch.cancel(popupOpenTimingHistogram, this);
> +        window.BrowserPageActions.togglePanelForAction(this.browserPageActions,

`this.browserPageActions` here should actually be `this.browserPageAction` (without the final `s`)

I was wondering why it didn't make any difference for the tests, and looking at the `togglePanelForAction` method it seems that the first parameter ends up to be unused if the method is only going to close the panel:

https://searchfox.org/mozilla-central/rev/49cc27555d5b7ed2eb06baf156693dd578daa06f/browser/base/content/browser-pageActions.js#193-196

Nevertheless, the typo should be fixed before landing this change.

::: browser/components/extensions/ext-pageAction.js:288
(Diff revision 4)
> +      }
> +
> +      this.popupNode = new PanelPopup(this.extension, window.document, popupURL,
> -                                 this.browserStyle);
> +                                      this.browserStyle);
> -      await popup.contentReady;
> +      // Remove popupNode when it is closed.
> +      let panelNode = this.popupNode.panel;

Nit, panelNode is currently only used to add the popuphiding listener, we could either remove `let panelNode` and just use `this.popupNode.panel` to subscribe the event listener, or we could also pass panelNode to `togglePanelForAction` at line 281 (instead of using `this.popupNode.panel`).

::: browser/components/extensions/test/browser/browser_ext_commands_execute_page_action.js:167
(Diff revision 4)
>  
>    await extension.startup();
>    let tab = await BrowserTestUtils.openNewForegroundTab(window.gBrowser, "http://example.com/");
>    EventUtils.synthesizeKey("j", {altKey: true, shiftKey: true});
>    await extension.awaitFinish("page-action-with-popup");
> +  ok(true, "panel opened");

Nit, instead of the `ok(true, "message...");` which are not really asserting anything, we could just use `info("message...");`

But it would be probably even better if we could emit these info messages right before waiting, so that it will be immediately clear where the test got stuck if it fails for timeouts while waiting for these promises to be resolved, e.g. I propose to change it into something like:

```
...
info("Wait page action popup to open on keyboard shortcut");
EventUtils.synthesizeKey("j", {altKey: true, shiftKey: true});
await extension.awaitFinish("page-action-with-popup");

// Bug 1447796 ...
let panel = document.getElementById(`${makeWidgetId(extension.id)}-panel`);
let hidden = promisePopupHidden(panel);

info("Wait page action popup to hide on keyboard shortcut while it is open");
EventUtils.synthesizeKey("j", {altKey: true, shiftKey: true});
await hidden;
```

Comment 8

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8961184 [details]
Bug 1447796 - fix closing page action panels using commands,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/229956/diff/4-5/

Comment 9

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8961184 [details]
Bug 1447796 - fix closing page action panels using commands,

https://reviewboard.mozilla.org/r/229956/#review237140

Looks good to me, thanks Shane!

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3987e4735374
fix closing page action panels using commands, r=rpl

Comment 11

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/3987e4735374

Comment 12

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8961184 [details]
Bug 1447796 - fix closing page action panels using commands,

Approval Request Comment
[Feature/Bug causing the regression]: pageAction
[User impact if declined]: Unable to close pageAction via key commands
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: no
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: low
[Why is the change risky/not risky?]: extensive test coverage exists on opening pageAction panels, which is what could be affected by this patch.
[String changes made/needed]: none

Comment 13

[Julien Cristau [:jcristau]]

Comment on attachment 8961184 [details]
Bug 1447796 - fix closing page action panels using commands,

webextensions fix, approved for 60.0b8

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/8f10c0a7a87f