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

VERIFIED FIXED in Firefox 64

Status

enhancement
P2
normal
VERIFIED FIXED
7 months ago
5 months ago

People

(Reporter: adw, Assigned: mixedpuppy)

Tracking

({dev-doc-complete})

Trunk
mozilla64
Dependency tree / graph
Bug Flags:
in-testsuite +
qe-verify +

Firefox Tracking Flags

(firefox64 verified, firefox65 verified)

Details

Attachments

(2 attachments)

(Reporter)

Description

7 months 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
(Reporter)

Updated

7 months ago
Keywords: regression
See Also: → 1483033
(Reporter)

Comment 1

7 months 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.)
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)
(Assignee)

Comment 3

7 months ago
I think it's probably fine to add another flag like that, default to true.
Flags: needinfo?(mixedpuppy)
(Assignee)

Comment 4

7 months 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?
(Reporter)

Comment 5

7 months 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.
(Assignee)

Comment 6

7 months 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.
Flags: needinfo?(mconca)
(Assignee)

Updated

7 months ago
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)
(Reporter)

Comment 9

7 months 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.
(Reporter)

Updated

7 months ago
Blocks: 1495127
(Reporter)

Comment 10

7 months ago
I filed bug 1495127 to use this new option in screenshots.
(Reporter)

Comment 11

7 months ago
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.

Comment 14

7 months ago
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+

Comment 16

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/9e5ba5a3b22c
Status: ASSIGNED → RESOLVED
Last Resolved: 7 months 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)

Updated

6 months ago
Blocks: 1498808

Comment 19

6 months 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!
Flags: needinfo?(mixedpuppy)
(Assignee)

Comment 20

6 months ago
It probably could be manually verified with screenshots, but it is covered by automated tests.
Flags: needinfo?(mixedpuppy) → needinfo?(adw)
(Reporter)

Comment 21

6 months 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
Flags: needinfo?(adw) → qe-verify+
(Reporter)

Updated

6 months ago
Flags: in-testsuite+

Comment 22

6 months 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.
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.