Closed Bug 1447723 Opened 5 years ago Closed 5 years ago

page action commands dont work when using pattern matching

Categories

(WebExtensions :: Untriaged, defect, P1)

Product:
WebExtensions
Component:
Untriaged
Version:
58 Branch
Type:
defect

Tracking

(firefox-esr52 unaffected, firefox59 wontfix, firefox60+ fixed, firefox61 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla61
Tracking Flags:
Tracking Status
firefox-esr52 --- unaffected
firefox59 --- wontfix
firefox60 + fixed
firefox61 --- fixed

People

(Reporter: mixedpuppy, Assigned: mixedpuppy)

References

(Blocks 1 open bug)

Details

(Keywords: regression)

Attachments

(1 file)

Bug 1447723 - fix triggering pageAction when using pattern matching,
59 bytes, text/x-review-board-request
rpl
: review+
jcristau
: approval-mozilla-beta+
Details 
Page action command fails in an extension that uses page action pattern matching and a key command with _execute_page_action.

manifest:

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

This is a regression starting in fx59 from bug 1437178.
Assignee: nobody → mixedpuppy
Priority: -- → P1
Yeah, sorry, forgot this. https://searchfox.org/mozilla-central/rev/b29daa46443b30612415c35be0a3c9c13b9dc5f6/browser/components/extensions/ext-pageAction.js#232

    pageAction.getProperty(window.gBrowser.selectedTab, "show")

should now be

    pageAction.isShown(window.gBrowser.selectedTab)
Comment on attachment 8961055 [details]
Bug 1447723 - fix triggering pageAction when using pattern matching,

https://reviewboard.mozilla.org/r/229808/#review235590

::: browser/components/extensions/test/browser/browser_ext_commands_execute_page_action.js:137
(Diff revision 1)
>    await extension.awaitFinish("page-action-with-popup");
>    await extension.unload();
>  });
> +
> +add_task(async function test_execute_page_action_with_matching() {
> +  let scriptPage = url => `<html><head><meta charset="utf-8"><script src="${url}"></script></head><body>Test Popup</body></html>`;

Nit, we could just share one of these scriptPage helpers with the previous test by defining it as a let (or const) global.
Attachment #8961055 - Flags: review?(lgreco) → review+
Comment on attachment 8961055 [details]
Bug 1447723 - fix triggering pageAction when using pattern matching,

https://reviewboard.mozilla.org/r/229808/#review235596

::: browser/components/extensions/ext-pageAction.js:237
(Diff revision 1)
>     */
>    triggerAction(window) {
> +    if (this.isShown(window.gBrowser.selectedTab)) {
> -    let pageAction = pageActionMap.get(this.extension);
> +      let pageAction = pageActionMap.get(this.extension);
> -    if (pageAction.getProperty(window.gBrowser.selectedTab, "show")) {
>        pageAction.handleClick(window);

`this` and `pageAction` seem to be the same object. If they are always the same, why bother defining `pageAction`? Otherwise, shouldn't it also be `pageAction.isShown`?
Comment on attachment 8961055 [details]
Bug 1447723 - fix triggering pageAction when using pattern matching,

https://reviewboard.mozilla.org/r/229808/#review235596

> `this` and `pageAction` seem to be the same object. If they are always the same, why bother defining `pageAction`? Otherwise, shouldn't it also be `pageAction.isShown`?

I think that you are right, and looking at the hg history triggerAction was actually like this from its first version (introduced by Bug 1246035), but even then it seems that retrieving the pageAction instance from the map wasn't really necessary.
Blocks: 1447796
P1 regression in webextensions, tracking for 60.
tracking-firefox60: ? → +
Pushed by mixedpuppy@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/426e9698be8d
fix triggering pageAction when using pattern matching, r=rpl
See Also: → 1448067
https://hg.mozilla.org/mozilla-central/rev/426e9698be8d
Status: NEW → RESOLVED
Closed: 5 years ago
status-firefox61: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Comment on attachment 8961055 [details]
Bug 1447723 - fix triggering pageAction when using pattern matching,

Approval Request Comment
[Feature/Bug causing the regression]: 1437178
[User impact if declined]: key commands are unable to open page action
[Is this code covered by automated tests?]: yes, new test for key command to open page action but this change is also covered by any page action test that opens a page action.  
[Has the fix been verified in Nightly?]: by tests only
[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?]: simple change
[String changes made/needed]: none
Attachment #8961055 - Flags: approval-mozilla-beta?
Comment on attachment 8961055 [details]
Bug 1447723 - fix triggering pageAction when using pattern matching,

webextensions regression fix, beta60+
Attachment #8961055 - 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.