Closed Bug 1447796 Opened Last year Closed Last year

page action commands only open, do not close panels

Categories

(WebExtensions :: Untriaged, defect, P1)

58 Branch
defect

Tracking

(firefox59 wontfix, firefox60+ fixed, firefox61 fixed)

RESOLVED FIXED
mozilla61
Tracking Status
firefox59 --- wontfix
firefox60 + fixed
firefox61 --- fixed

People

(Reporter: mixedpuppy, Assigned: mixedpuppy)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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.
BTW, the bug here is that by using the key command to open the panel, you get multiple panels stacked up, never closing any.
Assignee: nobody → mixedpuppy
Priority: -- → P1
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 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 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!
Attachment #8961184 - Flags: review?(lgreco) → review+
Pushed by mixedpuppy@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/3987e4735374
fix closing page action panels using commands, r=rpl
https://hg.mozilla.org/mozilla-central/rev/3987e4735374
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
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
Attachment #8961184 - Flags: approval-mozilla-beta?
Comment on attachment 8961184 [details]
Bug 1447796 - fix closing page action panels using commands,

webextensions fix, approved for 60.0b8
Attachment #8961184 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.