Closed Bug 991045 Opened 11 years ago Closed 11 years ago

Screenshot button in the toolbar UI

Categories

(DevTools :: Inspector, defect, P2)

defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 32

People

(Reporter: pbro, Assigned: lviknesh)

References

Details

(Keywords: dev-doc-complete, Whiteboard: [good first bug][lang=js][mentor=pbrosset])

Attachments

(1 file, 5 obsolete files)

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.
Whiteboard: [good first bug][lang=js][mentor=pbrosset]
Priority: -- → P2
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.
That's a good point, it should definitely work with remote targets.
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
Can i get this bug assigned and can you guide me on how do i fix it. Thank you :)
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.
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 :)
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!
Also, ping pbrosset/patrick on irc if you need help.
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.
Attached patch screenshot.patch (obsolete) — Splinter Review
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
Attachment #8418218 - Flags: review?(pbrosset)
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.
Attachment #8418218 - Flags: review?(pbrosset) → feedback?(pbrosset)
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.
Attachment #8418218 - Flags: feedback?(pbrosset)
Attached patch screenshot-toolbox.patch (obsolete) — Splinter Review
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 :)
Flags: needinfo?(pbrosset)
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.
Flags: needinfo?(pbrosset) → needinfo?(dhenein)
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.
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.
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.
Attached patch screenshot-button.patch (obsolete) — Splinter Review
Hope , it works . And i have used paintflashing PNG for screenshot :)
Attachment #8418218 - Attachment is obsolete: true
Attachment #8421917 - Attachment is obsolete: true
Flags: needinfo?(pbrosset)
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
Attachment #8422683 - Flags: review?(pbrosset)
Flags: needinfo?(pbrosset)
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 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.
Attachment #8422683 - Flags: review?(pbrosset)
Attached patch burron-screenshot.patch (obsolete) — Splinter Review
Attachment #8422683 - Attachment is obsolete: true
Attachment #8423044 - Flags: review?(pbrosset)
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.
Attachment #8423044 - Flags: review?(pbrosset) → review+
Assignee: nobody → lviknesh
Status: NEW → ASSIGNED
Ongoing try build for this patch: https://tbpl.mozilla.org/?tree=Try&rev=3f1604354f53
Attached patch screenshot-toolbox-button.patch (obsolete) — Splinter Review
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.
Attachment #8423044 - Attachment is obsolete: true
Attachment #8423084 - Flags: review+
Try build is green
Made a better looking image and rebased. Since try is green, I will check this in.
Attachment #8423084 - Attachment is obsolete: true
Attachment #8431437 - Flags: review+
Whiteboard: [good first bug][lang=js][mentor=pbrosset] → [good first bug][lang=js][mentor=pbrosset][fixed-in-fx-team]
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [good first bug][lang=js][mentor=pbrosset][fixed-in-fx-team] → [good first bug][lang=js][mentor=pbrosset]
Target Milestone: --- → Firefox 32
Depends on: 1018716
Will, for 32 there was a command button added that wraps up the gcli screenshot command to take a full page screenshot
Flags: needinfo?(wbamberg)
Keywords: dev-doc-needed
Clearing needinfo from Darrin for the icon
Flags: needinfo?(dhenein)
(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.
Flags: needinfo?(wbamberg)
Blocks: 936380
No longer blocks: 936380
See Also: → 936380
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: