1483591 - Treat Screenshots as a native pageAction once we remove bootstrapping

Status: VERIFIED FIXED

(Firefox :: Toolbars and Customization, enhancement, P2)

Description

[John Gruen [:jgruen]]

Even though Screenshots is becoming a webExtenison, it should appear in the pageAction as though it is a native Firefox tool, since users do not install it themselves and cannot uninstall it.

We need to make sure of the following:
- in the meatball menu, Screenshots is placed in alphabetical order among other native tools such as `Copy Link` and `Share` rather than being placed with other user-installed webExtensions icons
- When users context click on the Screenshots pageAction, the context click options should be `Remove from Address Bar` or `Add to Address Bar` only, and not the webExtensions options of `Don't show in address bar` and `Manage Extension`

Comment 1

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

- How much of this should the addons team own, vs the screenshots team? This mostly seems like core addons stuff to me, since these same issues will recur with future system addons.

- Is this doable in the 63 timeframe? If not, I think we'll want to revert bug 1466575 until 63 branches.

Comment 3

[Mike Conca [:mconca]]

Per conversations in IRC, moving this over to the WebExtensions team.  We will need someone to verify that all requested changes are possible, and in what timeframe.

Comment 4

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Note that the webextension page action is now disabled on about:reader pages, but Screenshots is supposed to work in reader mode. (Do I need a separate bug for this?)

Comment 5

[David Durst [:ddurst]]

I'm going to proactively NI adw or gijs to speak to "is this doable in the 63 timeframe?" question, as the implementation is largely in frontend.

Comment 7

[Mark Striemer [:mstriemer]]

I will take a look at this.

Comment 8

[Drew Willcoxon :adw]

I'm not sure what all the consequences are of making it a real webextension -- the webextensions team probably knows better than me when it comes to things like permissions (like the fact that the action is now disabled on about:reader is news to me).  But I can comment on some of it:

(In reply to [:jgruen] from comment #0)
> - in the meatball menu, Screenshots is placed in alphabetical order among
> other native tools such as `Copy Link` and `Share` rather than being placed
> with other user-installed webExtensions icons

Bug 1483088 covers this, and I agree that it shouldn't be marked as a dupe.  All the work here is in webextensions land.

Or, there's another way we could do this, which is to whitelist screenshots directly in PageActions.jsm, in which case that's on the front-end team (like me).  But IMO that's not as good a fix.

(BTW the ordering of the built-in actions isn't entirely alphabetical.  It's more ad hoc.)

> - When users context click on the Screenshots pageAction, the context click
> options should be `Remove from Address Bar` or `Add to Address Bar` only,
> and not the webExtensions options of `Don't show in address bar` and `Manage
> Extension`

There are two parts to the context menu.  One part is choosing the labeling of the add/remove menu items -- we say "add" and "remove" for built-in actions and "show" and "don't show" for extensions.  Barry's patch in bug 1483085 will fix that.

The other part is not adding the "Manage Extension" menu item.  That item is added by webextensions code.

Comment 9

[Mehmet]

Maybe one of the devs here can also take a look at this issue, which is probably related: Bug 1482697

Thanks.

Comment 10

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

[Tracking Requested - why for this release]: Firefox Screenshots is migrating to a webextension in the 64 release. This is one of two UI bugs related to this change that we'd like to fix for the 64 release.

Comment 11

[Mark Striemer [:mstriemer]]

Attachment: Bug 1483591 - Treat built-in webextension page actions as built-in r?adw

Comment 12

[Mark Striemer [:mstriemer]]

Here's a simple patch to treat it as a built-in action. It was looking through the gBuiltInActions array before, but there's code to check that along with our built-in add-on ids on the action that makes it work.

Comment 13

[Mark Striemer [:mstriemer]]

Attachment: Screenshot 2018-09-19 16.27.20.png

Here's a screenshot of where the page action will go after this patch. Is that a good spot?

Comment 14

[John Gruen [:jgruen]]

Looks good

Comment 15

[Drew Willcoxon :adw]

Comment on attachment 9010446 [details]
Bug 1483591 - Treat built-in webextension page actions as built-in r?adw

Drew Willcoxon :adw has approved the revision.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/13b93b43ad5e
Treat built-in webextension page actions as built-in r=adw

Comment 17

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/13b93b43ad5e

Comment 18

[Drew Willcoxon :adw]

Moving this bug to Firefox since it ended up being about the browser front end and not webextensions

Comment 19

[Mohammad Maruf Rahman]

I have reproduced this bug with Nightly 63.0a1 (2018-08-15) on Windows 7, 64 Bit!
This bug's fix is verified with latest Beta!

Build ID 	20181122182000
User Agent 	Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:64.0) Gecko/20100101 Firefox/64.0

Comment 20

[Emil Ghitta, QA [:emilghitta]]

Thanks Mohammad.

Marking this accordingly.