Drew Willcoxon :adw Reporter
	Description • 6 years ago

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

	Drew Willcoxon :adw Reporter
	Comment 1 • 6 years ago

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.)

	Andrew Swan [:aswan]
	Comment 2 • 6 years ago

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

	Shane Caraveo (:mixedpuppy) Assignee
	Comment 3 • 6 years ago

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

	Shane Caraveo (:mixedpuppy) Assignee
	Comment 4 • 6 years ago

(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?

	Drew Willcoxon :adw Reporter
	Comment 5 • 6 years ago

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.

	Shane Caraveo (:mixedpuppy) Assignee
	Comment 6 • 6 years ago

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.

	Mike Conca [:mconca]
	Comment 8 • 6 years ago

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

	Drew Willcoxon :adw Reporter
	Comment 9 • 6 years ago

(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.

	Drew Willcoxon :adw Reporter
	Comment 10 • 6 years ago

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

	Drew Willcoxon :adw Reporter
	Comment 11 • 6 years ago

Morphing this bug according to how it's being fixed

	Shane Caraveo (:mixedpuppy) Assignee
	Comment 12 • 6 years ago

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

	Mike Conca [:mconca]
	Comment 13 • 6 years ago

(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.

	Pulsebot
	Comment 14 • 6 years ago

Pushed by mixedpuppy@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/9e5ba5a3b22c
add pinned option to page_action manifest, r=aswan

	Andrew Swan [:aswan]
	Comment 15 • 6 years ago

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

Andrew Swan [:aswan] has approved the revision.

	Daniel Varga [:dvarga]
	Comment 16 • 6 years ago
bugherder

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

	ExE Boss
	Comment 17 • 6 years ago

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

	Florian Scholz (Open Web Docs)
	Comment 18 • 6 years ago

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.

	Vlad Jiman
	Comment 19 • 6 years ago

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!

	Shane Caraveo (:mixedpuppy) Assignee
	Comment 20 • 6 years ago

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

	Drew Willcoxon :adw Reporter
	Comment 21 • 6 years ago

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

	Vlad Jiman
	Comment 22 • 6 years ago

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.

	Chris Mills [:cmills]
	Comment 23 • 6 years ago

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