Closed Bug 1494135 Opened 2 years ago Closed 2 years ago

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

Categories

(WebExtensions :: General, enhancement, P2)

enhancement

Tracking

(firefox64 verified, firefox65 verified)

VERIFIED FIXED
mozilla64
Tracking Status
firefox64 --- verified
firefox65 --- verified

People

(Reporter: adw, Assigned: mixedpuppy)

References

Details

(Keywords: dev-doc-complete)

Attachments

(2 files)

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
Keywords: regression
See Also: → 1483033
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.)
Flags: needinfo?(aswan)
Sorry I'm not very familiar with how page actions are implemented, I think Shane has more recent experience.
Flags: needinfo?(aswan) → needinfo?(mixedpuppy)
I think it's probably fine to add another flag like that, default to true.
Flags: needinfo?(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?
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.
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.
Flags: needinfo?(mconca)
Assignee: nobody → mixedpuppy
Status: NEW → ASSIGNED
Priority: -- → P2
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
Severity: normal → enhancement
Flags: needinfo?(mconca)
(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.
Blocks: 1495127
I filed bug 1495127 to use this new option in screenshots.
Morphing this bug according to how it's being fixed
Component: Screenshots → General
Product: Firefox → WebExtensions
Summary: The screenshots page action should not appear in the urlbar by default → Add a `pinned` page action option to allow extensions to prevent their page actions from appearing in the urlbar by default
(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.
Pushed by mixedpuppy@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/9e5ba5a3b22c
add pinned option to page_action manifest, r=aswan
Comment on attachment 9012630 [details]
Bug 1494135 add pinned option to page_action manifest, r?aswan

Andrew Swan [:aswan] has approved the revision.
Attachment #9012630 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/9e5ba5a3b22c
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
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.
Attachment #9013861 - Flags: review?(fscholz)
Blocks: 1498808
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!
Flags: needinfo?(mixedpuppy)
It probably could be manually verified with screenshots, but it is covered by automated tests.
Flags: needinfo?(mixedpuppy) → needinfo?(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
Flags: needinfo?(adw) → qe-verify+
Flags: in-testsuite+
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.
Status: RESOLVED → VERIFIED
It looks like the docs are complete for this bug; thanks Exe Boss!
You need to log in before you can comment on or make changes to this bug.