Treat Screenshots as a native pageAction once we remove bootstrapping

VERIFIED FIXED in Firefox 64

Status

()

enhancement
P2
normal
VERIFIED FIXED
8 months ago
5 months ago

People

(Reporter: jgruen, Assigned: mstriemer)

Tracking

(Depends on 1 bug)

63 Branch
Firefox 64
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox64+ verified)

Details

Attachments

(2 attachments)

(Reporter)

Description

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

Updated

8 months ago
Depends on: 1483572, 1483085, 1483033
Version: unspecified → 63 Branch

Updated

8 months ago
Blocks: 1422437
- 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.
Flags: needinfo?(aswan)

Updated

8 months ago
Duplicate of this bug: 1483088
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.
Component: Screenshots → Frontend
Flags: needinfo?(aswan) → needinfo?(ddurst)
Priority: -- → P2
Product: Firefox → WebExtensions
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?)
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.
No longer depends on: 1483620
Flags: needinfo?(gijskruitbosch+bugs)
Flags: needinfo?(ddurst)
Flags: needinfo?(adw)

Updated

8 months ago
Duplicate of this bug: 1482701
(Assignee)

Comment 7

8 months ago
I will take a look at this.
Assignee: nobody → mstriemer
Flags: needinfo?(gijskruitbosch+bugs)
Flags: needinfo?(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

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

Thanks.
[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.

Updated

7 months ago
Depends on: 1483088
(Assignee)

Comment 12

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

Comment 13

7 months ago
Here's a screenshot of where the page action will go after this patch. Is that a good spot?
Flags: needinfo?(jgruen)
(Reporter)

Comment 14

7 months ago
Looks good
(Reporter)

Updated

7 months ago
Flags: needinfo?(jgruen)
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.
Attachment #9010446 - Flags: review+

Comment 16

7 months ago
Pushed by mstriemer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/13b93b43ad5e
Treat built-in webextension page actions as built-in r=adw

Comment 17

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/13b93b43ad5e
Status: NEW → RESOLVED
Last Resolved: 7 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Moving this bug to Firefox since it ended up being about the browser front end and not webextensions
Component: Frontend → Toolbars and Customization
Product: WebExtensions → Firefox
Target Milestone: mozilla64 → ---

Updated

7 months ago
Target Milestone: --- → Firefox 64
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
QA Whiteboard: [testday-20181123]
Thanks Mohammad.

Marking this accordingly.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.