991045 - Screenshot button in the toolbar UI

Status: RESOLVED FIXED

(DevTools :: Inspector, defect, P2)

Description

[Patrick Brosset <:pbro>]

There are 2 ways of taking screenshots in the devtools:
- using the gcli command (which by the way can take full page screenshots)
- from a button in the responsive mode

The goal here would be to create a new toolbar button for it (toolbar buttons are the things you see on the top right corner of the toolbox: pick an element, split console mode, paint flashing, scratchpad, ...).

We have many buttons already, but there is now a way of choosing which one you see in the toolbox from the options panel. So this button could probably be hidden by default and people who need it often could turn it on from the options panel.

Toolbar buttons can already call gcli commands (in fact that's the way they normally work), so this is a pretty easy bug. I will flag it as such.

Comment 1

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

I don't know how the gcli command does it, but if possible I would prefer this happening the same way the app manager does it so it works with remote targets.

Comment 2

[Patrick Brosset <:pbro>]

That's a good point, it should definitely work with remote targets.

Comment 3

[Patrick Brosset <:pbro>]

From IRC, about the screenshot size:

05:58 < flaki> pbrosset cheilmann what should the toolbar screenshot button do? take a viewport screenshot or a full page one?
05:59 < cheilmann> that is a good question
05:59 < flaki> wouldn't exposing fullpage screenshots (apart from the command line screenshot command) be a good thing here?
05:59 < cheilmann> could it be a multi-button?
05:59 < cheilmann> by default the screen but allowing you to select “full page"
06:00 < flaki> I'm thinking ctrl/shift + click to take fullpage, but that might be not much less obscure than the current method... :/
06:00 < pbrosset> flaki: might be difficult to discover
06:00 < pbrosset> I would have said full page by default
06:00 < pbrosset> cause you can always crop the image later if you need to
06:00 < pbrosset> and it's not like it's the only way to take screenshots either
06:01 < pbrosset> so I don't think it should be configurable
06:02 < flaki> pbrosset yeah, problem with that is that currently all of the actual screenshot-buttons work by taking a viewport-wide 
               screenshot, and if this tool used maybe the same icon but behaved differently out of the box that might be confusing 
               (well I'm not UX expert, but I would imagine this happening)
06:03 < cheilmann> pbrosset: good call. give more, allow for people to crop

Comment 4

[vikneshwar]

Can i get this bug assigned and can you guide me on how do i fix it. Thank you :)

Comment 5

[Mike Hoye [:mhoye]]

Hi, Vikneshwar - You certainly can! Start by setting up your development environment, as per:

https://wiki.mozilla.org/Mobile/Get_Involved
https://developer.mozilla.org/docs/Introduction

... and then you'll be ready to get started.

Comment 6

[vikneshwar]

Hai mike , i had already have mozilla build in my system and also fixed two devtools bugs 

https://bugzilla.mozilla.org/show_bug.cgi?id=957072
https://bugzilla.mozilla.org/show_bug.cgi?id=994307

Can you guide me on what should i do . Thank you :)

Comment 7

[Gabriel Luong [:gl] (ΦωΦ)]

Hi vikneshwar, you can find the gcli screenshot command at
https://mxr.mozilla.org/mozilla-central/source/browser/devtools/commandline/BuiltinCommands.jsm#1651

Based on the previous comments, I think we are aiming for full page screenshot. I would inspect existing toolbar buttons and find one that interacts with the gcli commands and use that as a reference.

Hope that is enough to get your started!

Comment 8

[Gabriel Luong [:gl] (ΦωΦ)]

Also, ping pbrosset/patrick on irc if you need help.

Comment 9

[Patrick Brosset <:pbro>]

Most buttons in the toolbox toolbar are based on GCLI commands already, and the screenshot is already a GCLI command, so this should really be easy by looking at the existing code.
I suggest looking at the eyedropper command, or the paintflashing command.

Here is some more information:

Any GCLI command that defines the necessary information can be a toolbar button easily.

For instance, look at paintflashing.js:
http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/gcli/commands/paintflashing.js

This command defines "buttonId" and "buttonClass":
http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/gcli/commands/paintflashing.js#98
these are used by the toolbox to register buttons to be displayed in the toolbar.

See:
http://mxr.mozilla.org/mozilla-central/source/browser/devtools/framework/toolbox.js#529
which uses `CommandUtils.createButtons()` to create the buttons according to the commands that have specified the buttonId/buttonClass properties.

So, from this, it looks like the first step is to simply define these properties in the screenshot command (which is here http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/gcli/commands/screenshot.js).

After that, in order to register the command as being part of the toolbar, you'll need to modify the devtools.toolbox.toolbarSpec preference in firefox.js.

This should be enough to get started.

Comment 10

[vikneshwar]

Attachment: screenshot.patch

There are some errors , please provide some suggestion for the fix . And i cant find the exact place where to define the properties in screenshot.js

Comment 11

[Patrick Brosset <:pbro>]

Comment on attachment 8418218 [details] [diff] [review]
screenshot.patch

As discussed on IRC, please use a feedback or needinfo request to get more information, if the code doesn't work yet, it's not the time for a review.

Comment 12

[Patrick Brosset <:pbro>]

Comment on attachment 8418218 [details] [diff] [review]
screenshot.patch

Review of attachment 8418218 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/app/profile/firefox.js
@@ +1227,5 @@
>  pref("devtools.toolbox.footer.height", 250);
>  pref("devtools.toolbox.sidebar.width", 500);
>  pref("devtools.toolbox.host", "bottom");
>  pref("devtools.toolbox.selectedTool", "webconsole");
> +pref("devtools.toolbox.toolbarSpec", '["splitconsole", "paintflashing toggle","tilt toggle","scratchpad","resize toggle","eyedropper","screenshot toggle"]');

There's no command named `screenshot toggle`.
If you open the developer toolbar (shift-F2), you type `help`, you'll see the list of commands, then type `help screenshot` to see how the screenshot command works.
You'll see there's no `screenshot toggle`.

The way the screenshot command works is that it takes optional parameters: --fullpage (which is the one we want), or --chrome, --clipboard, ...

::: browser/locales/en-US/chrome/browser/devtools/gclicommands.properties
@@ +1387,5 @@
>  mediaResetDesc=Stop emulating a CSS media type
>  mediaResetManual=Stop emulating a CSS media type
> +screenshotTooltip=Take screenshot of the page
> +screenshotToggleDesc=Toggle screenshot
> +screenshotManual=Take screenshot of the entire page and save it in PNG format

Several things here:

- There's already a section dedicated to screenshot l10n resources in this file (around line 40, just search for "screenshot" in the file, you'll find it), so it's better to group your new properties with those.
- There are already "desc" and "manual" resources, so no need for these. There only one you do need is the "screenshotTooltip" entry that will be used as a tooltip when hovering over the icon in the toolbox.
- Last, don't forget to add documentation for each new resource you add here. If you look at other properties in this file, you'll see that they each have a LOCALIZATION NOTE which is quite important for the translation to be done.

::: toolkit/devtools/gcli/commands/screenshot.js
@@ +35,5 @@
>          description: gcli.lookup("screenshotFilenameDesc"),
>          manual: gcli.lookup("screenshotFilenameManual")
>        },
>        {
> +

nit: No need to add an empty line here

@@ +74,5 @@
>          ]
>        }
>      ],
> +    {
> +        name: "screenshot toggle",

Why are you adding a new "screenshot toggle" command here?
If you look at the very top of the file (around line 25), you'll see that's where the "screenshot" command is defined and that's the one you need to use.
What was your reasoning behind adding a toggle command? And the isChecked function? I don't see how a screenshot command could have an on/off toggle state.
Also, the test for the localTab is to be avoided, we need everything to work remotely. To be honest, this looks like copy/pasted code from another command.

So you can remove this new command entry, and instead, move the buttonId and buttonClass and tooltipText into the "screenshot" command part.

Comment 13

[vikneshwar]

Attachment: screenshot-toolbox.patch

The patch works for visible window in browser . I need suggestion on how do i make it to work for full screen , and how do i add option to enable/disable in settings 

Since there is no PNG for screenshot , i have used paint-flashing PNG . Thank you :)

Comment 14

[Patrick Brosset <:pbro>]

Hi Darrin, for this bug, it'd be great to have a screenshot icon to be used in the toolbar (next to the other toolbar icons like eyedropper, pick element, scratchpad, ...).

Hi Viknesh, I'll be reviewing this patch in a minute. Thanks for submitting it.

Comment 15

[Patrick Brosset <:pbro>]

Comment on attachment 8421917 [details] [diff] [review]
screenshot-toolbox.patch

Review of attachment 8421917 [details] [diff] [review]:
-----------------------------------------------------------------

Nice. A lot simpler than the first patch. See my comments in the code.

A couple of extra things:

- There's no checkbox in the devtools options panel to toggle it: for this, you need to add an entry in 'get toolboxButtons' in Toolbox.prototype (browser/devtools/framework/toolbox.js). This should correspond to your button ID.
- The button is in the toolbar is visible by default. For this kind of features the button should be hidden by default. For this, you need to add a new pref in firefox.js. In this file, search for "// Toolbox Button preferences", you'll see where all preferences for this are defined. Just add one to false for your button.
- Finally, for your question about taking fullpage screenshot, the answer is pretty easy: in firefox.js, where you added the "screenshot" command to the list of commands in "devtools.toolbox.toolbarSpec", just add "screenshot --fullpage" instead.

Other than this, we should be good to go.

::: browser/locales/en-US/chrome/browser/devtools/gclicommands.properties
@@ +124,5 @@
>  # LOCALIZATION NOTE (screenshotCopied) Text displayed to user when the
>  # screenshot is successfully copied to the clipboard.
>  screenshotCopied=Copied to clipboard.
>  
> +#LOCALIZTION NOTE (screenshotTooltip) Text displayed as tooltip for screenshoot button in devtools ToolBox.

s/LOCALIZTION/LOCALIZATION
s/screenshoot/screenshot

@@ +125,5 @@
>  # screenshot is successfully copied to the clipboard.
>  screenshotCopied=Copied to clipboard.
>  
> +#LOCALIZTION NOTE (screenshotTooltip) Text displayed as tooltip for screenshoot button in devtools ToolBox.
> +screenshotTooltip=Take fullpage Screenshot.

"Take *a* fullpage screenshot" sounds better to me. Also, "Screenshot" should be all lowercase.

::: toolkit/devtools/gcli/commands/screenshot.js
@@ +26,5 @@
>      name: "screenshot",
>      description: gcli.lookup("screenshotDesc"),
>      manual: gcli.lookup("screenshotManual"),
>      returnType: "dom",
> +    buttonId: "command-button-paintflashing",

Even if we don't have the right icon to start with, we need to use a unique ID here. So please define a new ID like "command-button-screenshot", and then define the required selectors in 'browser/themes/shared/devtools/toolbars.inc.css' to assign an image to the button (pay attention in this file, we have a media-query for high-density displays).

In the CSS, you can always reuse an existing png until we have something suited for this new command.

@@ +29,5 @@
>      returnType: "dom",
> +    buttonId: "command-button-paintflashing",
> +    buttonClass: "command-button command-button-invertable",
> +    tooltipText: gcli.lookup("screenshotTooltip"),
> +    hidden: "true",

Why did you decide to hide the command?
Adding 'hidden:true' will remove it from the gcli command line help/autocompletion, which isn't what we want. 
You should just remove this line.

Comment 16

[Patrick Brosset <:pbro>]

Also, when you upload a new patch to bugzilla, please mark all other patches it replaces as obsolete. You can do this on the upload page. Otherwise it's harder to know which is the right patch to be applied.

Comment 17

[Patrick Brosset <:pbro>]

Darrin, I just realized we had an icon already for screenshot: browser/themes/shared/devtools/responsiveui-screenshot.png, only it's just not the same size/arrangement as the other toolbox button icons we have.

Comment 18

[vikneshwar]

Attachment: screenshot-button.patch

Hope , it works . And i have used paintflashing PNG for screenshot :)

Comment 19

[Patrick Brosset <:pbro>]

Comment on attachment 8422683 [details] [diff] [review]
screenshot-button.patch

Viknesh, at this stage, since the patch works and I've made a couple of reviews, you need to ask for R? (I've set that flag already).
Please go through this doc and make sure you know the process as needinfo, feedback and review all have different meanings: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch

Comment 20

[Patrick Brosset <:pbro>]

Comment on attachment 8422683 [details] [diff] [review]
screenshot-button.patch

Review of attachment 8422683 [details] [diff] [review]:
-----------------------------------------------------------------

Nearly there!
You just missed one thing in the CSS file. Then we need a new icon, and we're good to go.

::: browser/locales/en-US/chrome/browser/devtools/gclicommands.properties
@@ +124,5 @@
>  # LOCALIZATION NOTE (screenshotCopied) Text displayed to user when the
>  # screenshot is successfully copied to the clipboard.
>  screenshotCopied=Copied to clipboard.
>  
> +#LOCALIZATION NOTE (screenshotTooltip) Text displayed as tooltip for screenshot button in devtools ToolBox.

nit: please add a space between # and LOCALIZATION. This is very minor but we need to make sure the formatting is consistent within a file. This help make it easier to read.

::: browser/themes/shared/devtools/toolbars.inc.css
@@ +560,5 @@
>  }
>  
> +#command-button-screenshot > image {
> +  background-image: url("chrome://browser/skin/devtools/command-paintflashing.png");
> +}

As I said in my previous review, we have a media query in this file to target higher density displays: '@media (min-resolution: 2dppx)'.

Look in this media query, you'll see that the background-images are defined in there too, with a different image.

Please add a new selector in there for your button.

Comment 21

[Patrick Brosset <:pbro>]

Comment on attachment 8422683 [details] [diff] [review]
screenshot-button.patch

Resetting R? flag since there is a last change to do.
Please ask again for R? when ready.

Comment 22

[vikneshwar]

Attachment: burron-screenshot.patch

Comment 23

[Patrick Brosset <:pbro>]

Comment on attachment 8423044 [details] [diff] [review]
burron-screenshot.patch

Review of attachment 8423044 [details] [diff] [review]:
-----------------------------------------------------------------

Ok, looks good to me!
Only thing missing now is the icon. We can't ship this with the paintflashing icon.

Comment 24

[Patrick Brosset <:pbro>]

Ongoing try build for this patch: https://tbpl.mozilla.org/?tree=Try&rev=3f1604354f53

Comment 25

[Patrick Brosset <:pbro>]

Attachment: screenshot-toolbox-button.patch

Minor change to your patch just to get the image ready.
I've added a new image (referenced in jar.mn and in the css file).
Now we just need to actually get a nice image. This one was put together in 2 minutes.

Comment 26

[Patrick Brosset <:pbro>]

Try build is green

Comment 27

[Patrick Brosset <:pbro>]

Attachment: bug991045-screenshot-toolbar-button.patch

Made a better looking image and rebased.
Since try is green, I will check this in.

Comment 28

[Patrick Brosset <:pbro>]

https://hg.mozilla.org/integration/fx-team/rev/517a797c2533

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/517a797c2533

Comment 30

[Brian Grinstead [:bgrins]]

Will, for 32 there was a command button added that wraps up the gcli screenshot command to take a full page screenshot

Comment 31

[Brian Grinstead [:bgrins]]

Clearing needinfo from Darrin for the icon

Comment 32

[Will Bamberg [:wbamberg]]

(In reply to Brian Grinstead [:bgrins] from comment #30)
> Will, for 32 there was a command button added that wraps up the gcli
> screenshot command to take a full page screenshot

Thanks Brian, I updated the Toolbox page for that: https://developer.mozilla.org/en-US/docs/Tools/Tools_Toolbox#Toolbar. The other tools in there (Eyedropper, Scrtachpad, &c) each get a whole page to themselves, but I didn't think this needed enough explanation for that to be needed. So it's a bit inconsistent with the other toolbox-button-tools.