1690585 - Replace and reorder screenshots content context menu item

Status: VERIFIED FIXED

(Firefox :: Screenshots, enhancement, P1)

Description

[:Gijs (he/him)]

There should be a "Take Screenshot" context menu item, replacing the context menu from the extension, that does the same thing. It should not have an icon. It should appear with a separator above it, and above the separator for the "Inspect" item(s) (see also: bug 1686929).

Comment 3

[:Gijs (he/him)]

Additional note from https://docs.google.com/presentation/d/1Ufd0A3HsziyXXAjXYNH84P2bu7RdTxARsA0xpKmwIWs/edit?ts=6011b061#slide=id.gb7754f26c5_10_182 : don't show this item if the context menu is opened on a selection in an <input> or <textarea>.

Comment 4

[Blake Winton (:bwinton) (:☕️)]

As per guidance from Vicky, for tracking, we're marking all the bugs that people are working on as P1.

Comment 5

[Emma Malysz]

Attachment: Bug 1690585, replace screenshot context menu item

Comment 6

[Rob Wu [:robwu]]

The patch completely moves away from the contextMenus extension API in favor of logic elsewhere. I guess that it is due to the lack of ability to specify a specific insertion point.

There has been a feature request at bug 1325758 to allow extensions to specify a specific insertion point. It was closed because we didn't feel like the feature was safe enough to expose to all extensions. I'm not opposed to introducing a new property (maybe initially restricted to the screenshots add-on, or generally to privileged add-ons) if it allows you to implement this feature without the need to put Screenshots-specific logic/registrations across the codebase.

Another difference between the requirement expressed in comment 0 and the extension API is that extension menus always have an icon, enforced via forceManifestIcons at https://searchfox.org/mozilla-central/rev/c24ecdc6f5025ea7e60d0691673de030bd5bf411/browser/components/extensions/parent/ext-menus.js#249 . Again, I'm not opposed to hardcode a temporary exception there for screenshots (better alternatives are welcome).

Comment 7

[:Gijs (he/him)]

(In reply to Rob Wu [:robwu] from comment #6)

The patch completely moves away from the contextMenus extension API in favor of logic elsewhere. I guess that it is due to the lack of ability to specify a specific insertion point.

I don't think it's just that, I was under the impression that we also want to move towards having screenshots just be part of the browser instead of a system add-on, as it is pretty much never updated out of band, so it's added complexity to have it work as an extension rather than a normal browser component. Emma, is that right?

Comment 8

[Emma Malysz]

(In reply to :Gijs (he/him) from comment #7)

I was under the impression that we also want to move towards having screenshots just be part of the browser instead of a system add-on, as it is pretty much never updated out of band, so it's added complexity to have it work as an extension rather than a normal browser component. Emma, is that right?

Yes, that's the ultimate goal for screenshots, though timing-wise I'm not sure when that's going to happen. The first part of that was moving screenshots away from the git repo, and we're now maintaining the code in tree.

I had a working patch that specified the insertion point to be above "View Page Source" if we'd ultimately prefer to go in that direction, but I'm going to look into exposing this event to the extension first.

Comment 9

[Emma Malysz]

Gijs, did product/UX mention anything about an access key?

Comment 10

[:Gijs (he/him)]

(In reply to Emma Malysz from comment #9)

Gijs, did product/UX mention anything about an access key?

I don't think they've considered access keys, the expectation is normally that engineering figures that part out... 😅

What I tend to do is look at https://searchfox.org/mozilla-central/source/browser/locales/en-US/browser/browserContext.ftl and find-in-page/editor/whatever for accesskey = [letter] and it looks like T for Take Screenshot is actually free (well, it's used for links and frames, but the screenshot item is hidden in link context menus (and the frame one is in a submenu) so that's OK.)

Comment 11

[Mike Hommey [:glandium]]

Am I right that the patch attached to this bug will make the menu item more consistently present in the context menu? (I noticed it doesn't appear in some cases currently, and it's quite confusing as to why)

Comment 12

[Rob Wu [:robwu]]

(In reply to Mike Hommey [:glandium] from comment #11)

Am I right that the patch attached to this bug will make the menu item more consistently present in the context menu?

In the sense of the menu item being in a consistent order, rather than a seemingly random order when you have multiple extensions with menu items? Yes.

(I noticed it doesn't appear in some cases currently, and it's quite confusing as to why)

If you describe the circumstances where the menu item isn't showing up as expected (possibly in a new bug), then I can check whether it's expected or a bug.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/dcb828d28573
replace screenshot context menu item r=Gijs,fluent-reviewers,robwu

Comment 14

[Mike Hommey [:glandium]]

(In reply to Rob Wu [:robwu] from comment #12)

(I noticed it doesn't appear in some cases currently, and it's quite confusing as to why)

If you describe the circumstances where the menu item isn't showing up as expected (possibly in a new bug), then I can check whether it's expected or a bug.

That's what I meant.

Comment 15

[Emma Malysz]

Note that most about: pages are not possible to screenshot. About:reader is the exception (Bug 1620992)

Comment 16

[:Gijs (he/him)]

(In reply to Mike Hommey [:glandium] from comment #14)

(In reply to Rob Wu [:robwu] from comment #12)

(I noticed it doesn't appear in some cases currently, and it's quite confusing as to why)

If you describe the circumstances where the menu item isn't showing up as expected (possibly in a new bug), then I can check whether it's expected or a bug.

That's what I meant.

We're not changing (well, not aiming to change) which pages / situations we offer screenshots in the context menu. If you're seeing issues with that not captured by Emma's comment 15, please file a separate, orthogonal bug (which, to be fair, may be easier to fix after the change from this bug!).

Comment 17

[Mike Hommey [:glandium]]

To be clearer: in the same page, there are places where the item shows up and places where it doesn't. I'll check whether it keeps happening after this lands in a nightly.

Comment 18

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/dcb828d28573

Comment 19

[Mike Hommey [:glandium]]

(In reply to Mike Hommey [:glandium] from comment #17)

I'll check whether it keeps happening after this lands in a nightly.

It seems to be gone. I'll watch out if it happens again in case I just happened not to trigger it, since I'm not sure under what conditions it was happening in the first place.

Comment 20

[Bogdan Maris, Desktop QA]

Verified that the changes from comment 0 are correctly seen and Take Screenshot functionality is working as expected in latest Nightly 88.0a1 across platforms (Windows 10, macOS 11.3 and Ubuntu 18.04).