1494135 - Add a `pinned` page action option to allow extensions to prevent their page actions from appearing in the urlbar by default

Status: VERIFIED FIXED

(WebExtensions :: General, enhancement, P2)

Description

[Drew Willcoxon :adw]

After the conversion of the screenshots page action to a webextension page action, it appears in the urlbar by default.  It didn't before that.  jgruen says that's an accident -- it still should not appear in the urlbar by default -- in bug 1483033 comment 51.

The reason it does appear is that all webextension page actions start out pinned to the urlbar by default [1], and screenshots specifies `"show_matches": ["<all_urls>"]` [2], so it's enabled for every page.  Both those things means it shows up by default.

Maybe the simplest way to fix this would be to have PageActions.jsm specifically check for screenshots and set its pinnedToUrlbar to false initially.  That seems like a little bit of a hack to me, but it's probably OK.  I guess ideally the webextension could specify pinnedToUrlbar in its manifest and then that info would be passed on to the action class in ext-pageAction.js.

I'm marking this as blocking bug 1466575 because that seems like the bug that most likely regressed this, but I'm not sure.

[1] https://dxr.mozilla.org/mozilla-central/rev/ebeba937ca2a01df86d98089f2368bb975182dcc/browser/components/extensions/parent/ext-pageAction.js#90
[2] https://dxr.mozilla.org/mozilla-central/rev/ebeba937ca2a01df86d98089f2368bb975182dcc/browser/extensions/screenshots/webextension/manifest.json#39

Comment 1

[Drew Willcoxon :adw]

Andrew, do you think we could add an option like pinned_to_urlbar to the page_action manifest options, at least for Mozilla extensions?  This is separate from an enabled state.  It determines whether the action should appear in the urlbar when it's enabled.

(Sorry if you're not the right person to ask about this.  I was going to flag Andy since he answered a similar question in bug 1419842, but it looks like he's no longer here.)

Comment 2

[Andrew Swan [:aswan]]

Sorry I'm not very familiar with how page actions are implemented, I think Shane has more recent experience.

Comment 3

[Shane Caraveo (:mixedpuppy)]

I think it's probably fine to add another flag like that, default to true.

Comment 4

[Shane Caraveo (:mixedpuppy)]

(In reply to Shane Caraveo (:mixedpuppy) from comment #3)
> I think it's probably fine to add another flag like that, default to true.

Though it may need some definition beyond that.  Is this just an initial install issue? if so, no default (undefined can maintain current behavior).  If pinned_to_urlbar is true, do we always pin it, or is there any reason we wouldn't want to?

Comment 5

[Drew Willcoxon :adw]

Yes, it's an initial install issue.  After install, the user can unpin or pin the action using the context menu on the action (the "Remove from Address Bar" and "Add to Address Bar" menu items).  We should default it to true because the behavior we want for random page actions the user installs is to show them in the urlbar by default when they are enabled.  We just want to override that for the screenshots action, and potentially other Mozilla actions in the future that we don't want to appear in the urlbar by default.

Comment 6

[Shane Caraveo (:mixedpuppy)]

Basically, you want enabled: always + pinned: false so the icon is in the ... menu unless the user moves it.  

Lets just call it "pinned", default true to match current behavior, and make it available to all extensions.

FYI mconca.

Comment 7

[Shane Caraveo (:mixedpuppy)]

Attachment: Bug 1494135 add pinned option to page_action manifest, r?aswan

Comment 8

[Mike Conca [:mconca]]

I like this addition to the page action.  I'll note that browser actions have a 'default_area' property that specifies where the button should appear upon installation.  It would be nice for consistency if we could use the same property name for page actions, perhaps with the following possible values:

  'urlbar' - pinned next to the URL bar (default when not specified)
  'pagemenu' - listed as an item in the page action menu of the URL bar

Comment 9

[Drew Willcoxon :adw]

(In reply to Mike Conca [:mconca] from comment #8)
> I like this addition to the page action.  I'll note that browser actions
> have a 'default_area' property that specifies where the button should appear
> upon installation.  It would be nice for consistency if we could use the
> same property name for page actions, perhaps with the following possible
> values:
> 
>   'urlbar' - pinned next to the URL bar (default when not specified)
>   'pagemenu' - listed as an item in the page action menu of the URL bar

Actions always appear in the panel, even when they're disabled.  That's not something that actions can toggle.  So a `default_area` option would be a little misleading at best I think, because if you specify 'urlbar', your action would still appear in the panel.

Comment 10

[Drew Willcoxon :adw]

I filed bug 1495127 to use this new option in screenshots.

Comment 11

[Drew Willcoxon :adw]

Morphing this bug according to how it's being fixed

Comment 12

[Shane Caraveo (:mixedpuppy)]

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

Comment 13

[Mike Conca [:mconca]]

(In reply to Drew Willcoxon :adw from comment #9)
> (In reply to Mike Conca [:mconca] from comment #8)
> > I like this addition to the page action.  I'll note that browser actions
> > have a 'default_area' property that specifies where the button should appear
> > upon installation.  It would be nice for consistency if we could use the
> > same property name for page actions, perhaps with the following possible
> > values:
> > 
> >   'urlbar' - pinned next to the URL bar (default when not specified)
> >   'pagemenu' - listed as an item in the page action menu of the URL bar
> 
> Actions always appear in the panel, even when they're disabled.  That's not
> something that actions can toggle.  So a `default_area` option would be a
> little misleading at best I think, because if you specify 'urlbar', your
> action would still appear in the panel.

I didn't realize until this comment that the page action appears in both places when pinned. Thanks for pointing that out. A 'pinned' property might make more sense.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9e5ba5a3b22c
add pinned option to page_action manifest, r=aswan

Comment 15

[Andrew Swan [:aswan]]

Comment on attachment 9012630 [details]
Bug 1494135 add pinned option to page_action manifest, r?aswan

Andrew Swan [:aswan] has approved the revision.

Comment 16

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/9e5ba5a3b22c

Comment 17

[ExE Boss]

Attachment: MDN: Browser compatibility table update

I have now documented this on:
- https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/64#Changes_for_add-on_developers
- https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/manifest.json/page_action#Syntax

Comment 18

[Florian Scholz (Open Web Docs)]

Comment on attachment 9013861 [details] [review]
MDN: Browser compatibility table update

We don't review GitHub PRs on Bugzilla. Also, this has been reviewed by Will in the original PR.

Comment 19

[Vlad Jiman]

Hello, will this bug require manual validation? please provide a test extension and steps to correctly validate if yes, otherwise please set the qe-validate- flag, thanks!

Comment 20

[Shane Caraveo (:mixedpuppy)]

It probably could be manually verified with screenshots, but it is covered by automated tests.

Comment 21

[Drew Willcoxon :adw]

Yeah, with the way this bug ended up, it doesn't really need verification because this new manifest option isn't a user-facing thing.  But it would probably be good to verify the original problem.

STR:

1. Start Firefox with a new profile
2. Click the "..." button in the urlbar to open the page action menu
3. Verify that "Take a Screenshot" appears in the menu
4. Verify that the only buttons in the urlbar after the "..." button are Pocket and the bookmark star.  The screenshots button should not appear

Comment 22

[Vlad Jiman]

Thank you for the replies, I've verified using the steps from comment #21 that by default the screenshots button does not appear on several FF versions: 65.0a1, 64.0b3, 63.0b5 - running on windows 10 x64 bits.

Comment 23

[Chris Mills [:cmills]]

It looks like the docs are complete for this bug; thanks Exe Boss!