1366041 - Add "Take Screenshot" button to Page Action Menu

Status: VERIFIED FIXED

(Firefox :: Menus, enhancement, P1)

Description

[bbell]

Attachment: example.png

Screenshots are going to be a system-add-on when 57 lands. The Page Action Menu needs to activate the add-on

Comment 1

[bbell]

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"

Comment 2

[Marco Mucci [:MarcoM]]

Hi Drew, are you taking this bug bug now or later on?

Comment 3

[Drew Willcoxon :adw]

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.

Comment 4

[Ian Bicking (:ianbicking)]

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

Comment 5

[Drew Willcoxon :adw]

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.

Comment 6

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

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

Comment 7

[Drew Willcoxon :adw]

Thanks Jared.  I'll take a look.

Comment 8

[Fischer [:Fischer]]

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.

Comment 9

[Drew Willcoxon :adw]

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.

Comment 10

[Fischer [:Fischer]]

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

Comment 11

[Nicole Yee (:nicoleyee)]

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

Comment 12

[Drew Willcoxon :adw]

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?

Comment 13

[[e-mail]]

Hi Drew,

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

Comment 14

[Drew Willcoxon :adw]

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

Comment 15

[Fischer [:Fischer]]

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

Comment 16

[Fischer [:Fischer]]

(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

Comment 17

[Drew Willcoxon :adw]

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.

Comment 18

[Drew Willcoxon :adw]

Attachment: Bug 1366041 - Add "Take Screenshot" button to Page Action Menu.

Review commit: https://reviewboard.mozilla.org/r/159908/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/159908/

Comment 19

[Drew Willcoxon :adw]

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?

Comment 20

[Drew Willcoxon :adw]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/159908/diff/1-2/

Comment 21

[Drew Willcoxon :adw]

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

Comment 22

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

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.

Comment 23

[Drew Willcoxon :adw]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/159908/diff/2-3/

Comment 24

[Drew Willcoxon :adw]

Thanks Jared.  Updated to support changing the icon.  I'll send this over to GitHub once the PageActions API is r+'ed.

Comment 25

[Drew Willcoxon :adw]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/159908/diff/3-4/

Comment 26

[Drew Willcoxon :adw]

For-review patch, which I'll post to GitHub now.

Comment 27

[Drew Willcoxon :adw]

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

Hope I didn't screw it up...

Comment 28

[Drew Willcoxon :adw]

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.

Comment 29

[Drew Willcoxon :adw]

Clearing the needinfo until I address the comment in the PR.

Comment 30

[Drew Willcoxon :adw]

Commented in the PR: https://github.com/mozilla-services/screenshots/pull/3239#issuecomment-319836380

Comment 31

[Drew Willcoxon :adw]

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

Comment 32

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

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.

Comment 33

[Drew Willcoxon :adw]

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?

Comment 34

[Drew Willcoxon :adw]

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.

Comment 35

[Drew Willcoxon :adw]

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

Comment 36

[Fischer [:Fischer]]

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

Comment 37

[John Gruen [:jgruen]]

: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!

Comment 38

[:Gijs (he/him)]

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

Comment 39

[Fischer [:Fischer]]

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

Comment 40

[John Gruen [:jgruen]]

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.

Comment 41

[Fischer [:Fischer]]

(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

Comment 42

[John Gruen [:jgruen]]

Hey :fischer, responded to your email earlier today. We're QA-ing this now and will request landing once we're set there.

Comment 43

[Jean Gong :jgong]

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

Comment 44

[Drew Willcoxon :adw]

With bug 1390985 landed, this is now fixed.

Comment 45

[u554753]

Verified on Windows, Mac, and Ubuntu.