Closed Bug 1690585 Opened 3 months ago Closed 1 month ago

Replace and reorder screenshots content context menu item

Categories

(Firefox :: Screenshots, enhancement, P1)

Desktop
All
enhancement

Tracking

()

VERIFIED FIXED
88 Branch
Tracking Status
firefox87 --- wontfix
firefox88 --- verified

People

(Reporter: Gijs, Assigned: emalysz)

References

(Blocks 1 open bug)

Details

(Whiteboard: [proton-context-menus])

Attachments

(1 file)

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).

Assignee: nobody → emalysz
Status: NEW → ASSIGNED
Duplicate of this bug: 1692278
Duplicate of this bug: 1595387

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

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

Priority: -- → P1

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).

(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?

Flags: needinfo?(emalysz)

(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.

Flags: needinfo?(emalysz)

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

Flags: needinfo?(gijskruitbosch+bugs)

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

Flags: needinfo?(gijskruitbosch+bugs)
Blocks: 1696574

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)

(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.

Pushed by emalysz@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/dcb828d28573
replace screenshot context menu item r=Gijs,fluent-reviewers,robwu

(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.

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

(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!).

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.

Status: ASSIGNED → RESOLVED
Closed: 1 month ago
Resolution: --- → FIXED
Target Milestone: --- → 88 Branch

(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.

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).

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