Add "Take Screenshot" button to Page Action Menu

ASSIGNED
Assigned to

Status

()

Firefox
Menus
P1
normal
ASSIGNED
3 months ago
7 days ago

People

(Reporter: bbell, Assigned: adw)

Tracking

(Depends on: 1 bug, Blocks: 3 bugs)

unspecified
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [photon-structure], URL)

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

3 months ago
Created attachment 8869143 [details]
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

3 months ago
Blocks: 1352697
Whiteboard: [photon-structure]
(Reporter)

Updated

3 months ago
Assignee: nobody → adw
(Reporter)

Comment 1

3 months 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"

Comment 2

3 months ago
Hi Drew, are you taking this bug bug now or later on?
Flags: qe-verify?
Flags: needinfo?(adw)
Priority: -- → P2
(Assignee)

Comment 3

3 months 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)

Updated

3 months ago
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

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

Updated

3 months ago
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

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

Updated

2 months ago
Blocks: 1371543

Updated

2 months ago
Iteration: 55.7 - Jun 12 → 56.1 - Jun 26
(Assignee)

Updated

2 months ago
Depends on: 1374477

Updated

2 months ago
Iteration: 56.1 - Jun 26 → 56.2 - Jul 10

Updated

2 months ago
Flags: qe-verify? → qe-verify+

Updated

2 months ago
QA Contact: gwimberly

Comment 8

2 months ago
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 months 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)

Comment 10

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

Updated

a month ago
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

a month 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)

Comment 13

a month ago
Hi Drew,

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

Comment 14

a month ago
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.
Flags: needinfo?(fliu)

Comment 15

a month ago
(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

a month ago
(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

a month 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

a month 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

a month ago
Attachment #8887165 - Attachment is obsolete: true
(Assignee)

Updated

a month ago
Flags: needinfo?(fliu)
Comment hidden (mozreview-request)
(Assignee)

Comment 21

a month 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 22

a month ago
mozreview-review
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

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

Updated

a month ago
Iteration: 56.3 - Jul 24 → 56.4 - Aug 1
Comment hidden (mozreview-request)
(Assignee)

Comment 26

26 days ago
For-review patch, which I'll post to GitHub now.
(Assignee)

Comment 27

26 days 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

26 days 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

23 days ago
Clearing the needinfo until I address the comment in the PR.
Flags: needinfo?(jhirsch)

Updated

22 days ago
Iteration: 56.4 - Aug 1 → 57.1 - Aug 15
(Assignee)

Comment 30

21 days ago
Commented in the PR: https://github.com/mozilla-services/screenshots/pull/3239#issuecomment-319836380
(Assignee)

Comment 31

20 days 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)
(Assignee)

Updated

20 days 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

20 days 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

20 days 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

19 days 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

Comment 36

16 days ago
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

16 days 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

15 days 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)

Comment 39

15 days ago
(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

15 days 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)

Comment 41

11 days ago
(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

8 days 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

7 days ago
Depends on: 1390985
You need to log in before you can comment on or make changes to this bug.