Add "Take Screenshot" button to Page Action Menu

VERIFIED FIXED

Status

()

enhancement
P1
normal
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: bbell, Assigned: adw)

Tracking

(Blocks 2 bugs)

unspecified
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox57 verified)

Details

(Whiteboard: [photon-structure], )

Attachments

(1 attachment, 2 obsolete attachments)

Reporter

Description

2 years ago
Posted image example.png
Screenshots are going to be a system-add-on when 57 lands. The Page Action Menu needs to activate the add-on
Reporter

Updated

2 years ago
Blocks: 1352697
Whiteboard: [photon-structure]
Reporter

Updated

2 years ago
Assignee: nobody → adw
Reporter

Comment 1

2 years ago
To see this feature in action you have to turn it "off" in about:config   

Look for "extensions.screenshots.system-disabled" and set it to "false"
Hi Drew, are you taking this bug bug now or later on?
Flags: qe-verify?
Flags: needinfo?(adw)
Priority: -- → P2
Assignee

Comment 3

2 years ago
I'll be working on it soon but I can't say exactly when.  I'll be working on all these page action panel bugs.
Status: NEW → ASSIGNED
Flags: needinfo?(adw)
Iteration: --- → 55.6 - May 29
Priority: P2 → P1
Note that we are currently analyzing whether Screenshots can implement its buttons using normal WebExtension methods (browserAction or pageAction), or if we need to use bootstrap.js to implement the actions and then have it lazily instantiate the WebExtension.

Ultimately we want to use just a WebExtension, and ideally if a WebExtension pageAction shows up in this menu then we can make that change, and everything might work out.  I think we're medium confident that by Firefox 56 we'll just be using a WebExtension.

There is a bootstrap.js portion of the add-on as well, and if necessary we can implement this in that location.

Note the Firefox tree is not the canonical location for that code, so ideally any changes we need to make would happen in the GitHub repo: https://github.com/mozilla-services/screenshots/tree/master/addon
Assignee

Comment 5

2 years ago
Status update:

Ultimately the plan is apparently to automatically list web extensions in the page action menu, or at least those that have page actions.  Screenshots is one such extension.  But I'd like to avoid all that for Photon's initial release and instead just hardcode a screenshots menu item that calls into the extension.

That seems pretty simple in concept, but Ian and Jared say that they're still figuring out how to load the extension, and that any changes I make will probably be broken pretty quickly.  My impression is that we may have to first load the extension before calling into it when the user clicks the menu item.

So I'll wait for that to be resolved before working on this.
Iteration: 55.6 - May 29 → 55.7 - Jun 12
Hey Drew - It looks like we're going to stick with our current approach:

- Screenshots is implemented as an embedded webextension
- Initialization is lazy: the bootstrap.js waits for 'sessionstore-windows-restored', then manually loads the webextension
- Note that the UI (context menu and toolbar button) is managed by the webextension code

Thoughts on how to move this into your page action-like menu:


## Suggested approach: expose an API (JS or XUL)

If you create a JSM that exposes an API to add a page action-like menu item, we could remove the browser_action from the Screenshots manifest, and instead call that API from the Screenshots bootstrap.js file when the webextension is ready.

Because Screenshots can be preffed on and off, this API would also need to allow items to be removed.

Note that we could also treat a new bit of XUL DOM as an API, and manually add or remove menuitems from the Screenshots code.

I think this is the cleanest approach, as it keeps all the Screenshots-related code inside the Screenshots system addon. (Modularity ftw!)


## Alternate approach: wait for a signal

If you want the menu code to control creating the menu items, there are two issues to consider: waiting for the Screenshots webextension to start, and watching the prefs that can disable the feature.

Delayed startup: I'm not sure a user *could* click into a menu just after 'sessionstore-windows-restored' has fired. If it's possible, Screenshots could fire a new nsIObserver signal when the webextension is ready. You could wait to draw the menu item, or hide/disable it, until that signal is fired.

Pref management: Two possible approaches here: Screenshots could fire nsIObserver signals when the addon is preffed on/off, or you could observe the two prefs. The prefs are `extensions.screenshots.system-disabled` and `extensions.screenshots.disabled`, and Screenshots is disabled if either one is true.


## Hacking on Screenshots code

We periodically drop code into mozilla-central, but the source actually lives in Github, so you would want to land patches over there. The bootstrap source code is at [1], docs on how to export the code to a local copy of m-c are at [2], and we'd be happy to answer any questions; just drop by #screenshots on IRC.

[1] https://github.com/mozilla-services/screenshots/blob/master/addon/bootstrap.js
[2] https://github.com/mozilla-services/screenshots/blob/master/docs/export-to-firefox.md
Flags: needinfo?(adw)
Assignee

Comment 7

2 years ago
Thanks Jared.  I'll take a look.
Flags: needinfo?(adw)

Updated

2 years ago
Blocks: 1371543
Iteration: 55.7 - Jun 12 → 56.1 - Jun 26
Assignee

Updated

2 years ago
Depends on: 1374477
Iteration: 56.1 - Jun 26 → 56.2 - Jul 10

Updated

2 years ago
Flags: qe-verify? → qe-verify+
QA Contact: gwimberly
Hi Drew and Jared,
Photon Onboarding will have a tour to introduce and highlight the screenshot feature on the page action menu so just would like to know how's the status of this bug. Thank you.
Flags: needinfo?(jhirsch)
Flags: needinfo?(adw)
Assignee

Comment 9

2 years ago
Hi Fischer, this bug has been blocked on the API bug in bug 1374477.  I'm hoping to have a patch ready for review there soon.  Basically what will happen is that screenshots's bootstrap.js will call into a PageActions module to add itself to the page action menu.  So it will be async -- screenshots won't be added to that menu immediately when the browser starts.  Do you need to highlight screenshots immediately when the browser starts?  Or when the user opens that menu?  If you need to do it immediately, then maybe the PageActions API can broadcast a notification when it adds items to the menu, and then you could listen for it to know when screenshots has been added.
Flags: needinfo?(jhirsch)
Flags: needinfo?(adw)
(In reply to Drew Willcoxon :adw from comment #9)
> Hi Fischer, this bug has been blocked on the API bug in bug 1374477.  I'm
> hoping to have a patch ready for review there soon.  Basically what will
> happen is that screenshots's bootstrap.js will call into a PageActions
> module to add itself to the page action menu.  So it will be async --
> screenshots won't be added to that menu immediately when the browser starts.
> Do you need to highlight screenshots immediately when the browser starts? 
> Or when the user opens that menu?  If you need to do it immediately, then
> maybe the PageActions API can broadcast a notification when it adds items to
> the menu, and then you could listen for it to know when screenshots has been
> added.
Hi Drew,
Thanks for reply. We would highlight the screenshot button when user browsing the onboarding tour. That is, when user enters about:newtab or about:home and sees the screenshot introduction tour so by that time, all the PageActions api works should have already be done.
Iteration: 56.2 - Jul 10 → 56.3 - Jul 24
Hey Drew!

Any status update on this? I see bug 1374477 is actively being worked on, which is great. As Fisher mentioned, the tour overlay will highlight the screenshot feature in 57. Will the above approach in comment 10 be compatible? 

Also, the code freeze deadline for feature work (set by the photon team) is aug 7th. We would appreciate if we can keep each other updated as that date approaches :)
Flags: needinfo?(adw)
Assignee

Comment 12

2 years ago
Hi Nicole, the approach in comment 10 sounds good -- the screenshots item will probably have been added by the time the user can react to an onboarding message in a tab.

All the work right now is happening in bug 1374477, so right now that's the best bug to watch to see what's happening.

You shouldn't have to wait to implement at least some of your work because I think you can add a dummy screenshots button to the panel for now.  I'm planning on using "page-action-panel-screenshots" for its ID FWIW.  Will the onboarding click the button or ask the user to click it?
Flags: needinfo?(adw)
Hi Drew,

Onboarding will use UITour to just spotlight the item, and won't do the click action for the user.
Assignee

Comment 14

2 years ago
Posted patch WIP patch (obsolete) — Splinter Review
Fischer, if it's helpful, here's a WIP patch that adds a Take Screenshot button to the page action panel.  Its ID is pageAction-panel-screenshots.  You'll first need to apply the patch/mozreview in bug 1374477.
Flags: needinfo?(fliu)
(In reply to Drew Willcoxon :adw from comment #14)
> Created attachment 8887165 [details] [diff] [review]
> WIP patch
> 
> Fischer, if it's helpful, here's a WIP patch that adds a Take Screenshot
> button to the page action panel.  Its ID is pageAction-panel-screenshots. 
> You'll first need to apply the patch/mozreview in bug 1374477.
Thank you Drew. That helps a lot.
(In reply to Drew Willcoxon :adw from comment #14)
> Created attachment 8887165 [details] [diff] [review]
> WIP patch
> 
> Fischer, if it's helpful, here's a WIP patch that adds a Take Screenshot
> button to the page action panel.  Its ID is pageAction-panel-screenshots. 
> You'll first need to apply the patch/mozreview in bug 1374477.
Hi Drew,

one questions to discuss with you:

The UITour would check the visibility of the target before highlighting the target element[1] so the case of opening menu then closing menu again because of target is not visible wouldn't happen.
We found that the buttons in the page action panel are `visibility:collapse` if the panel never opened before. So have to open the panel once to make highlighting button possible. The buttons in the app menu by default are visible while the menu is not visible. Is there some reason to hide buttons in the panel by default?

[1] https://dxr.mozilla.org/mozilla-central/rev/e0b0865639cebc1b5afa0268a4b073fcdde0e69c/browser/components/uitour/UITour.jsm#1148

Thanks
Flags: needinfo?(adw)
Assignee

Comment 17

2 years ago
The page action panel is hidden=true initially, in the browser.xul markup.  (For performance reasons supposedly.  The code that opens the panel first unhides it, and the panel remains unhidden after that.)  When I start the browser with my patches applied, open the browser console, and do:

document.getElementById("pageActionPanel").hidden=false

then

getComputedStyle(document.getElementById("pageAction-panel-screenshots")).visibility

changes from "collapse" to "visible".  So it must be because the panel is hidden initially?

I'm not sure what to do about that in your case.  You could probably just check whether the panel is hidden and if so unhide it, do your check, and then hide it again after that -- although if the whole point is to open the panel and highlight something inside it, then you would need to unhide it anyway.

Another possibility is to modify or bypass your visibility check because in this case, for the page action panel, checking visibility doesn't really mean anything because the page action panel doesn't use or impart any meaning to the visibility style.  The only meaningful thing is that a node is in the panel at all.  If the screenshots node is in the panel, then it will be visible in the panel when the panel is opened.
Flags: needinfo?(adw)
Comment hidden (mozreview-request)
Assignee

Comment 19

2 years ago
Comment on attachment 8888880 [details]
Bug 1366041 - Add "Take Screenshot" button to Page Action Menu.

This is a hopefully final, ready-for-review patch for this bug.  Once the PageActions API, which this relies on, is r+'ed in bug 1374477, I'll make a pull request to the screenshots GitHub repo.

Jared, in the meantime, would you mind glancing over this here and letting me know if anything is crazy?
Attachment #8888880 - Flags: feedback?(jhirsch)
Assignee

Updated

2 years ago
Attachment #8887165 - Attachment is obsolete: true
Assignee

Updated

2 years ago
Flags: needinfo?(fliu)
Comment hidden (mozreview-request)
Assignee

Comment 21

2 years ago
Comment on attachment 8888880 [details]
Bug 1366041 - Add "Take Screenshot" button to Page Action Menu.

Jared, please see comment 19.

(Forgot to remove the page action when the extension is unloaded, so this latest commit fixes that.)
Attachment #8888880 - Flags: feedback?(jhirsch)
Comment on attachment 8888880 [details]
Bug 1366041 - Add "Take Screenshot" button to Page Action Menu.

https://reviewboard.mozilla.org/r/159908/#review165272

Looks good, just the one comment about handling the special icon case for new users.

::: browser/extensions/screenshots/bootstrap.js:203
(Diff revision 2)
> +  // Add the page action.
> +  photonPageAction = PageActions.addAction(new PageActions.Action({
> +    id,
> +    title: title,
> +    insertAfter: PageActions.SEND_TO_DEVICE_ACTION_ID,
> +    iconURL: addonResourceURI.spec + "webextension/icons/icon-32-v2.svg",

If this pageAction replaces the toolbar button, I think we would want to keep the special case where the user is shown a starred icon if they haven't been onboarded (that is, if they have never tried screenshots before).

At startup time, you could have the webextension send over the onboarding state along with the l10n string.

The icon setup code is currently in startBackground.js and main.js in the Screenshots repo; you can grep for 'icon-starred' to see where it's conditionally inserted.
Attachment #8888880 - Flags: review+
Attachment #8888880 - Flags: review+
Attachment #8888880 - Flags: feedback?(jhirsch)
Attachment #8888880 - Flags: feedback+
Comment hidden (mozreview-request)
Assignee

Comment 24

2 years ago
Thanks Jared.  Updated to support changing the icon.  I'll send this over to GitHub once the PageActions API is r+'ed.
Iteration: 56.3 - Jul 24 → 56.4 - Aug 1
Comment hidden (mozreview-request)
Assignee

Comment 26

2 years ago
For-review patch, which I'll post to GitHub now.
Assignee

Comment 27

2 years ago
Jared, would you mind looking at the PR here: https://github.com/mozilla-services/screenshots/pull/3239

Hope I didn't screw it up...
Flags: needinfo?(jhirsch)
Assignee

Comment 28

2 years ago
Although until my changes in bug 1374477 land, and the CI is using a Firefox with those changes, the CI test is going to fail.
Assignee

Comment 29

2 years ago
Clearing the needinfo until I address the comment in the PR.
Flags: needinfo?(jhirsch)
Iteration: 56.4 - Aug 1 → 57.1 - Aug 15
Assignee

Comment 31

2 years ago
OK, my commits are passing the tests now in the PR.  Jared, would you or Ian mind taking a look?

https://github.com/mozilla-services/screenshots/pull/3239
Flags: needinfo?(jhirsch)

Updated

2 years ago
Blocks: 1387512
R+, merged. Do I need to set any flags on the Bugzilla attachment?

Do you need this landed in central by a specific date? Screenshots master branch hasn't been exported to Nightly in a while. Instead, we have been exporting a 55-specific branch that tracks small bug fixes for uplift. We don't have a specific timeline to land github master into central, but I could do the export sooner than later.
Flags: needinfo?(jhirsch) → needinfo?(adw)
Assignee

Comment 33

2 years ago
Thanks Jared.  The sooner the merge to central the better (so people can use it and test it in Nightly), but an absolute deadline is September 15, the Photon soft freeze date.  Can you update this bug when you do merge it?
Flags: needinfo?(adw)
Assignee

Comment 34

2 years ago
Comment on attachment 8888880 [details]
Bug 1366041 - Add "Take Screenshot" button to Page Action Menu.

Marking this obsolete now that the actual commits in the PR have been accepted.
Attachment #8888880 - Attachment is obsolete: true
Assignee

Comment 35

2 years ago
This caused Firefox to log an error after a screenshot is taken.  Ian says it doesn't keep anything from working though: https://github.com/mozilla-services/screenshots/issues/3280
Jared,

Would you land this into the Central this week?
The Photon Onboarding (Bug 1371538 and Bug 1371543) need this so we could introduce the Screenshot button in the Page Action Panel.
The bug (Bug 1382579) making UITour support the Page Action Panel is going to land.
That would be great that you could land this into the Central, then we will have the Screenshot onboarding tour soon.

Thanks
Flags: needinfo?(jhirsch)

Comment 37

2 years ago
:Fischer, is it 100% necessary to do so this week. We're working on last minute server fixes for our 55 launch later this month? If we could wait until next week, it would make our engineers lives considerably less stressful.

Feel free to NI me here with your response. Thanks!
Flags: needinfo?(jhirsch) → needinfo?(fliu)

Comment 38

2 years ago
(In reply to [:jgruen] from comment #37)
> :Fischer, is it 100% necessary to do so this week. We're working on last
> minute server fixes for our 55 launch later this month? If we could wait
> until next week, it would make our engineers lives considerably less
> stressful.
> 
> Feel free to NI me here with your response. Thanks!

Would it be OK for Fischer or me or someone else on the fx team to cherrypick the fixes we need in Nightly into the version of Screenshots in the tree ourselves?
Flags: needinfo?(jgruen)
(In reply to :Gijs from comment #38)
> (In reply to [:jgruen] from comment #37)
> > :Fischer, is it 100% necessary to do so this week. We're working on last
> > minute server fixes for our 55 launch later this month? If we could wait
> > until next week, it would make our engineers lives considerably less
> > stressful.
> > 
> > Feel free to NI me here with your response. Thanks!
> 
> Would it be OK for Fischer or me or someone else on the fx team to
> cherrypick the fixes we need in Nightly into the version of Screenshots in
> the tree ourselves?
Thanks. Yes.
And if this was not the case, hi :jgruen, would you plan to land this back to the Central on the next Mon?
Because the feature complete schedule for us is the next Mon(8/14) so we could quickly follow up this and complete the rest of features, thank you.
Flags: needinfo?(fliu)

Comment 40

2 years ago
Gijs, yes i think that's fine. Sorry about the timing issue. We're working of of this branch for Nightly releases.

https://github.com/mozilla-services/screenshots/tree/v10.10.0

I'll call this out in my team's standup this morning.
Flags: needinfo?(jgruen)
(In reply to [:jgruen] from comment #40)
> Gijs, yes i think that's fine. Sorry about the timing issue. We're working
> of of this branch for Nightly releases.
> 
> https://github.com/mozilla-services/screenshots/tree/v10.10.0
> 
> I'll call this out in my team's standup this morning.
Hi jgruen,
Do we have any update on this? Thank you
Flags: needinfo?(jgruen)
Iteration: 57.1 - Aug 15 → 57.2 - Aug 29

Comment 42

2 years ago
Hey :fischer, responded to your email earlier today. We're QA-ing this now and will request landing once we're set there.
Flags: needinfo?(jgruen)

Updated

2 years ago
Depends on: 1390985

Updated

2 years ago
Blocks: 1393668

Comment 43

2 years ago
(In reply to [:jgruen] from comment #42)
> Hey :fischer, responded to your email earlier today. We're QA-ing this now
> and will request landing once we're set there.

Hello :jgruen, we are still blocking on this bug.  Has it landed yet? Thanks!

Updated

2 years ago
Flags: needinfo?(jgruen)
Iteration: 57.2 - Aug 29 → 57.3 - Sep 19
Assignee

Comment 44

2 years ago
With bug 1390985 landed, this is now fixed.
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Flags: needinfo?(jgruen)
Resolution: --- → FIXED

Comment 45

2 years ago
Verified on Windows, Mac, and Ubuntu.
Status: RESOLVED → VERIFIED
Flags: qe-verify+

Updated

2 years ago
Depends on: 1396373

Updated

2 years ago
Depends on: 1400264
You need to log in before you can comment on or make changes to this bug.