Extensions page actions are pinned to address bar again and again

VERIFIED FIXED in Firefox 58

Status

()

defect
P1
normal
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: MikkCZ, Assigned: adw)

Tracking

58 Branch
Firefox 58
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 unaffected, firefox58 fixed)

Details

(Whiteboard: [fxsearch])

Attachments

(1 attachment)

I have unpinned all installed extensions page action from address bar to keep them in the page actions menu only. But after some time (and usually after restart) they move to the address bar again, probably after they are first shown in the code.

Mozilla/5.0 (X11; Linux x86_64; rv:58.0) Gecko/20100101 Firefox/58.0 ID:20171101104430

The installed add-ons in questions are Awesome RSS 1.1.2 and Pontoon Tools 2.0.2.
Assignee

Comment 1

2 years ago
Thanks for filing this, I'll take a look.
Assignee: nobody → adw
Status: NEW → ASSIGNED
Assignee

Comment 2

2 years ago
I think I blew it with the onShutdown change in ext-pageAction.js.  We shouldn't call browserPageAction.remove() when simply quitting the app.
Priority: -- → P1
Whiteboard: [fxsearch]

Comment 3

2 years ago
Is bug 1413712 a dupe of this?
Flags: needinfo?(adw)
Comment hidden (mozreview-request)
Assignee

Comment 5

2 years ago
This fixes it.  I tested it manually.  Is there any way to write a test for this?  To send a mock webextension an APP_SHUTDOWN I guess.
Flags: needinfo?(adw)
Assignee

Comment 6

2 years ago
(In reply to :Gijs (no reviews; PTO recovery mode) from comment #3)
> Is bug 1413712 a dupe of this?

Didn't mean to clear the needinfo without answering this, but I did answer in bug 1413712.
(In reply to Drew Willcoxon :adw from comment #5)
> This fixes it.  I tested it manually.  Is there any way to write a test for
> this?  To send a mock webextension an APP_SHUTDOWN I guess.

You can use promiseStartupManager and promiseShutdownManager in an xpcshell test. I'm not so certain however if you can test against CUI from xpcshell. The extension needs to be created with {useAddonManager: "temporary"}, you'll see that in most tests under toolkit/components/extensions/test/xpcshell   test_ext_browserSettings.js is a good example of using these.
Assignee

Comment 8

2 years ago
Hmm, it may be possible to test PageActions.jsm outside of the browser, at least for this specific bug.  It might require it to be reworked a little.  I'll try.  (PageActions isn't related to CUI (unfortunately!).)  I should also take a closer look at how CUI is handling this...  Does it just never purge the placement of widgets from extensions?
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Comment 11

2 years ago
This includes a test.  I found I had to use {useAddonManager: "permanent"} to get it to work.
Comment hidden (mozreview-request)
Assignee

Comment 14

2 years ago
Let's see if this fixes the test failures... https://treeherder.mozilla.org/#/jobs?repo=try&revision=e32166c34192
Assignee

Comment 15

2 years ago
Yes, try looks good now.

Comment 16

2 years ago
mozreview-review
Comment on attachment 8924647 [details]
Bug 1413692 - Extensions page actions are pinned to address bar again and again.

https://reviewboard.mozilla.org/r/195882/#review201156

::: browser/components/extensions/test/xpcshell/test_ext_pageAction_shutdown.js:48
(Diff revision 4)
> +
> +  let extension = ExtensionTestUtils.loadExtension(extensionData);
> +  await extension.startup();
> +
> +  // Get the PageAction object and set its shownInUrlbar to false.  It's set to
> +  // true in ext-pageAction.js, when it's created.

Can we test across restart for both values (true and false)?

::: browser/modules/PageActions.jsm:394
(Diff revision 4)
>      } catch (ex) {
>        Cu.reportError(ex);
>      }
>    },
>  
> +  // For tests.

// For tests.  See Bug 1413692.
Attachment #8924647 - Flags: review?(mixedpuppy) → review+
Comment hidden (mozreview-request)
Assignee

Comment 18

2 years ago
Thanks Shane, this addresses your comments.  One more try push and then I'll land this.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b7e1b8930ef5

Comment 19

2 years ago
mozreview-review
Comment on attachment 8924647 [details]
Bug 1413692 - Extensions page actions are pinned to address bar again and again.

https://reviewboard.mozilla.org/r/195882/#review202082

::: browser/components/extensions/test/xpcshell/test_ext_pageAction_shutdown.js:48
(Diff revision 5)
> +
> +  let extension = ExtensionTestUtils.loadExtension(extensionData);
> +  await extension.startup();
> +
> +  // Get the PageAction.Action object.  Its shownInUrlbar should have been
> +  // initialized to true in ext-pageAction.js, when it's created.

If we fix bug 1413574 will this still be true?  Typically we would run browser.pageAction.show|hide in the addon, with the default that it is not shown until show() is called.
Assignee

Comment 21

2 years ago
(In reply to Shane Caraveo (:mixedpuppy) from comment #19)
> If we fix bug 1413574 will this still be true?  Typically we would run
> browser.pageAction.show|hide in the addon, with the default that it is not
> shown until show() is called.

Bug 1413574 renames shownInUrlbar to pinnedInUrlbar, and it maps the webextension show()/hide() methods to the PageActions disabled state.  Pinning is controlled by the user, and the disabled state is controlled by the extension.  An action is shown in the urlbar only if it's both pinned and not disabled.  All webextension actions start off pinned but disabled, so it's up to each extension to call show() as appropriate.

Comment 22

2 years ago
Pushed by dwillcoxon@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/71a2c8b33ae0
Extensions page actions are pinned to address bar again and again. r=mixedpuppy
Duplicate of this bug: 1415218

Comment 24

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/71a2c8b33ae0
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
The patch works for me in Mozilla/5.0 (X11; Linux x86_64; rv:58.0) Gecko/20100101 Firefox/58.0 ID:20171108110838. Unpinned icons stay in the menu and pinned ones in the address bar too.
Status: RESOLVED → VERIFIED
Assignee

Updated

2 years ago
Duplicate of this bug: 1415484
You need to log in before you can comment on or make changes to this bug.