It turns out one of the most used features (at least for designers) in devtools is the screenshot tool. It is available both from the toolbar button and from the markup-view context menu (to screenshot a single node). These save screenshots as files on the disk for now. We would like to be able to copy to the clipboard too. The gcli command already knows how to save screenshots to the clipboard, so this part isn't hard. What's harder is find a UI to do this. So far, the idea we discussed about is a long-press on the screenshot button in the toolbar would show a popup menu that allows to switch the default action to clipboard (similar to how you can access extra tools in Photoshop by long-pressing on buttons). Ryan, I know you started to look at this, do you want to take this on?

If Ryan is too busy with other stuff, I would happy to take this on; I'm working on screenshot for RDM, and I think this feature should be implemented there too anyway.

I did start to work on this in Toronto, but I did not get too far because I switched to investigating browser toolbox things. I think it's fine for anyone to take it. We should discuss the relative priority of this vs. other RDM items during the standup today.

I've been trying to think through what the UX for this would be. This is what I'm going to propose: 1. User clicks the screenshot icon. 2. Screenshot is downloaded to the Downloads folder. (See Bug 1243525 for my UX notes.) 3. Screenshot is also automatically copied to clipboard. 4. 'Copied to clipboard' label appears by cursor. Thoughts?

(In reply to Helen V. Holmes (:helenvholmes) (:✨)(pls ni?)) from comment #3) > I've been trying to think through what the UX for this would be. This is > what I'm going to propose: > > 1. User clicks the screenshot icon. > 2. Screenshot is downloaded to the Downloads folder. (See Bug 1243525 for my > UX notes.) > 3. Screenshot is also automatically copied to clipboard. > 4. 'Copied to clipboard' label appears by cursor. > > Thoughts? It'd definitely a tricky UI, especially since we don't want to add a new button. I don't know about doing both by default though - I think these are two different actions. If you are just wanting the store to clipboard then the side effect of a new file could cause clutter. And if you just want a file then changing clipboard contents could cause you to lose whatever was on the clipboard before. My 2 cents: I still think the 'easiest' thing to start with (for implementation and UI) would be a change in behavior when 'alt' is pressed. So by default it saves a file and if you hold alt it copies to the clipboard instead (and also changes the tooltip text to indicate that). It's not discoverable but we maybe could change the normal tooltip text to say something like ('Save a full page screenshot. Hold alt to copy to clipboard'). Alternatively, are there any other applications that have this sort of behavior that we could get ideas from?

See bug 1259062 for an alternative proposal (which involves showing a preview popup of the screenshot that was just taken and providing options to either save to disk or store to the clipboard). My 2 cents: the clipboard thing should be intentional and obvious, overriding whatever is already in the user's clipboard might make people angry at us :)

For the full page screenshot, another option could be to have a dropdown in the devtools preferences, below the "Take a fullpage screenshot" option (cf. attachment). Depends if users actually want to switch from one behavior to the other regularly. In my workflow I always prefer using the clipboard, so setting it once in the preferences makes sense "for me". (In reply to Patrick Brosset [:pbro] from comment #0) > It turns out one of the most used features (at least for designers) in > devtools is the screenshot tool. > It is available both from the toolbar button and from the markup-view > context menu (to screenshot a single node). Do we know if one is more used than the other (between the fullpage screenshot and the node screenshot) ?

(In reply to Julian Descottes [:jdescottes] from comment #6) > Do we know if one is more used than the other (between the fullpage > screenshot and the node screenshot) ? Unfortunately I believe we have no data on this. I suspect that the fullpage option is used more because it's a button in the toolbar as opposed to a context menu item in the inspector. The designers we've talked to used the fullpage option only (but also the macosX native screenshot tool which allows to drag an area of the screen you want to capture).

I think we need to move ahead with some decisions. We don't need the perfect route but we need to try something so we can iterate on it later to prevent this from languishing. We don't have any good data to go off of right now so lets take a stand somewhere and measure our decision to determine if we were right. Here my default recommendations: 1. Full page screenshot by default. This isn't our current default but more is better here. Designers can easily crop what isn't needed compared to stitching together a full page from viewport captures. 2. Copy to Clipboard by default. We may override a users clipboard but if we don't turn this on we'll need to teach people how to turn it on. Those who lose their clipboard will turn it off once and hopefully not get burned again. 3. Save to file by default. Both clipboard and file probably aren't exactly what people want but unless we have a good intro step I worry people will feel it is broken without the file save. Those who don't want a file anymore can turn it off. Lets limit the discussion until the end of this week, Friday April 1st.... ugh. For UX I'd propose something like a split button dropdown with these options very close to the action UI. ( http://getbootstrap.com/components/#btn-dropdowns-split ) Here's a terrible looking example just to be clearer: https://codepen.io/clarkbw/pen/Vazpjq And for feedback I like the full screen hud type thing where you could possibly say "Copied to Clipboard, use ⌘+v to paste". (click the screenshot button in the codepen to see demo)

I agree we don't need a perfect solution at first, and thanks for the example snippet. I'll add that the sort of split dropdown button ui isn't anywhere else in the tools AFAIK right now and is a different behavior than the other 'dropdown buttons' that we have in the top toolbar (i.e. the frame picker). Not saying it's the wrong ui, but it is different so we'll be having multiple close-but-different button types next to each other. Note that Bug 1260225 is about maybe moving the screenshot button into the inspector, so that might change things a bit. We would also consider if we want the popup in the button to be a 'native' (XUL) popup or some kind of fake popup in HTML. And if fake, then is there some self-contained dependency that we could pull in to handle the fun positioning issues we'll run into with that?

(In reply to Bryan Clark (DevTools PM) [@clarkbw] from comment #8) > Here my default recommendations: > 1. Full page screenshot by default. This isn't our current default but more > is better here. Designers can easily crop what isn't needed compared to > stitching together a full page from viewport captures. > 2. Copy to Clipboard by default. We may override a users clipboard but if > we don't turn this on we'll need to teach people how to turn it on. Those > who lose their clipboard will turn it off once and hopefully not get burned > again. > 3. Save to file by default. Both clipboard and file probably aren't exactly > what people want but unless we have a good intro step I worry people will > feel it is broken without the file save. Those who don't want a file > anymore can turn it off. I support this. Let's make a big deal out of it and announce it as intentional (as it is) but also to alert people that it's happening. (It's a big enough change done for intentional reasons to make things easier that I think we should let people know it's happening. > For UX I'd propose something like a split button dropdown with these options > very close to the action UI. ( > http://getbootstrap.com/components/#btn-dropdowns-split ) Here's a terrible > looking example just to be clearer: https://codepen.io/clarkbw/pen/Vazpjq > > And for feedback I like the full screen hud type thing where you could > possibly say "Copied to Clipboard, use ⌘+v to paste". (click the screenshot > button in the codepen to see demo) I can work on making as attractive as we can, perhaps? To address what Julian said: there are a lot of screenshotting quirks that would be nice to be able to control... probably buried within settings. (You could turn off copy to clipboard, for example, in Settings—I think that would be a good thing to ship along with this bug so users can turn off the change if they really don't like it.) Some other nice enhancements we should address/debate with follow-up, backlogged bugs: - export screenshots @2x - screenshotting above the fold/entire page by default - whatever other ~~cool stuff~~ the gcli currently does.

Discussed at team meeting, not ready for development yet.

Jaideep is currently working on bug 1243525. In comment #6 we have a proposal for how to handle copying to clipboard with the same camera icon.

The only thing left in this patch from the spec which Helen gave me is the ability to simulate camera flashing effect on remote pages. Any pointers on how to proceed? Helen: the rest is according to our discussion. for the screen flashing feature, you could try to screenshot new tab or rdm tool which are non-remote pages.

(In reply to Jaideep Bhoosreddy [:jbhoosreddy] from comment #14) > Created attachment 8763943 [details] [diff] [review] > 1257913.patch > > The only thing left in this patch from the spec which Helen gave me is the > ability to simulate camera flashing effect on remote pages. Any pointers on > how to proceed? You can add a white overlay over the page, and make trigger an opacity animation on it. Like this: https://jsfiddle.net/ntim/py860wsw/

Comment on attachment 8763972 [details] [diff] [review] 1257913.patch I'm noticing a few odd discrepancies: The flash is looking great in RDM, but it doesn't seem like the sound is triggering properly, even when I have "Enable Audio Feedback" checked. (I also wonder if we could reword this to something less obscure—"Play camera click", perhaps.) There also seems to be an error when screenshotting using the toolbar: "Security wrapper denied access to property Symbol.iterator on privileged Javascript object. Support for exposing privileged objects to untrusted content via __exposedProps__ is being gradually removed - use WebIDL bindings or Components.utils.cloneInto instead. Note that only the first denied property access from a given global object will be reported." Oddly, it worked the first couple of times I used the tool (with sound as well), but failed subsequent times with the above error.

Comment on attachment 8763972 [details] [diff] [review] 1257913.patch Review of attachment 8763972 [details] [diff] [review]: ----------------------------------------------------------------- Hopefully we can unify this a bit more. It would be great to actually have only one code path between RDM and GCLI, but if that's too much for now, let's at least make them flow the same with the same operations. Not sure how to best improve the command button portion, hopefully :bgrins has some thoughts. ::: devtools/client/framework/toolbox-options.js @@ +320,5 @@ > + newToolbarSpec[5] = "screenshot --clipboard"; > + } else { > + newToolbarSpec[5] = "screenshot --fullpage"; > + } > + setPrefAndEmit(toolbarPref, JSON.stringify(newToolbarSpec)); This feels a bit hairy, so I'm curious to see :bgrins impression of it. ::: devtools/client/framework/toolbox-options.xhtml @@ +105,5 @@ > + class="command-button command-button-invertable" style="vertical-align: sub;"/> > + </legend> > + <label title="&options.screenshotClipboard.tooltip;"> > + <input type="checkbox" > + data-pref="devtools.screenshot-clipboard.enabled"/> I would consider "screenshot" as a "group" of options, so use "." as a separator after it, so: devtools.screenshot.clipboard.enabled devtools.screenshot.audio.enabled ::: devtools/client/framework/toolbox.js @@ +1054,5 @@ > > /** > + * Rebuild toolbox buttons again, if needed to reload from Spec. > + */ > + _rebuildButtons: function () { This one seems to not be called...? ::: devtools/client/locales/en-US/toolbox.dtd @@ +185,5 @@ > +<!ENTITY options.screenshotClipboard.tooltip "Saves to the screenshot directly to the clipboard"> > + > +<!-- LOCALIZATION NOTE (options.stylesheetAutocompletion.label): This is the > + - label for the checkbox that toggles autocompletion of css in the Style Editor --> > +<!ENTITY options.screenshotAudio.label "Enable Audio Feedback"> The options pane is mess as far as capitalization, but I'd suggest sentence case for this label to match more of the others. ::: devtools/client/responsive.html/actions/screenshot.js @@ +15,5 @@ > const { getToplevelWindow } = require("sdk/window/utils"); > const { Task: { spawn } } = require("devtools/shared/task"); > const e10s = require("../utils/e10s"); > > +const BASE_URL = "resource://devtools/client/themes"; I am not sure BASE_URL makes sense anymore when it points outside RDM. Probably just remove it and embed the value in the string below. ::: devtools/shared/gcli/commands/screenshot.js @@ +229,5 @@ > params: [ filenameParam, standardParams ], > + exec: function (args, context) { > + const selector = args.selector ? args.selector : "html"; > + const node = context.environment.document.querySelector(selector); > + node.animate({ opacity: [ 0, 1 ] }, 500); The security error is because of this frames object we created in a high-privileged context (this script) that we're passing to `animate` from a low-privileged context (content). To work around this, do: let document = context.environment.document; let frames = Cu.cloneInto({ opacity: [ 0, 1 ] }, document.defaultView); document.documentElement.animate(frames, 500); This creates the object in the right context. You'll need to import Cu in the require("chrome") line at the top. @@ +234,1 @@ > return captureScreenshot(args, context.environment.document); For consistency, let's keep the order of operations the same as RDM: 1. Capture screenshot 2. Play sound 3. Flash screen Since you need to be on the remote document to flash the screen, change this to a generator so you can yield on the result and add the effects: exec: function* () { let result = yield captureScreenshot(...); simulateCameraEffects(context.environment.document); }

Comment on attachment 8763972 [details] [diff] [review] 1257913.patch Review of attachment 8763972 [details] [diff] [review]: ----------------------------------------------------------------- ::: devtools/client/framework/toolbox-options.js @@ +316,5 @@ > + const toolbarPref = "devtools.toolbox.toolbarSpec"; > + const toolbarSpec = JSON.parse(GetPref(toolbarPref)); > + const newToolbarSpec = Object.assign([], toolbarSpec); > + if (checkbox.checked) { > + newToolbarSpec[5] = "screenshot --clipboard"; Do we really want this to remove the --fullpage arg in the case of clipboard? @@ +320,5 @@ > + newToolbarSpec[5] = "screenshot --clipboard"; > + } else { > + newToolbarSpec[5] = "screenshot --fullpage"; > + } > + setPrefAndEmit(toolbarPref, JSON.stringify(newToolbarSpec)); I'd rather not modify devtools.toolbox.toolbarSpec here. I'd let the change to devtools.screenshot-clipboard.enabled happen automatically, then catch it over in toolbox.js. Then, instead of changing the toolbarSpec preference, let's make the _buildButtons function in the toolbox smarter, and find the screenshot value in the spec then modify it depending on the value of the clipboard pref (leaving the toolbarSpec pref alone the whole time). Similar to what screenshotNode is doing in inspector-panel. ::: devtools/client/framework/toolbox.js @@ +1066,5 @@ > + /** > + * Rebuild toolbox button again > + * Currently, only for building screenshot button > + */ > + _rebuildButton: function (id) { Prefer to modify _buildButtons to be able to be called multiple times as opposed to having copy/pasted code here. Then when the 'devtools.screenshot-clipboard.enabled' pref changes, call _buildButtons again.

(In reply to Brian Grinstead [:bgrins] from comment #19) > Comment on attachment 8763972 [details] [diff] [review] > 1257913.patch > > Review of attachment 8763972 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: devtools/client/framework/toolbox-options.js > @@ +316,5 @@ > > + const toolbarPref = "devtools.toolbox.toolbarSpec"; > > + const toolbarSpec = JSON.parse(GetPref(toolbarPref)); > > + const newToolbarSpec = Object.assign([], toolbarSpec); > > + if (checkbox.checked) { > > + newToolbarSpec[5] = "screenshot --clipboard"; > > Do we really want this to remove the --fullpage arg in the case of clipboard? > I guess you are right, I'll leave the --fullpage flag as is. > @@ +320,5 @@ > > + newToolbarSpec[5] = "screenshot --clipboard"; > > + } else { > > + newToolbarSpec[5] = "screenshot --fullpage"; > > + } > > + setPrefAndEmit(toolbarPref, JSON.stringify(newToolbarSpec)); > > I'd rather not modify devtools.toolbox.toolbarSpec here. I'd let the change > to devtools.screenshot-clipboard.enabled happen automatically, then catch it > over in toolbox.js. > > Then, instead of changing the toolbarSpec preference, let's make the > _buildButtons function in the toolbox smarter, and find the screenshot value > in the spec then modify it depending on the value of the clipboard pref > (leaving the toolbarSpec pref alone the whole time). Similar to what > screenshotNode is doing in inspector-panel. > > ::: devtools/client/framework/toolbox.js > @@ +1066,5 @@ > > + /** > > + * Rebuild toolbox button again > > + * Currently, only for building screenshot button > > + */ > > + _rebuildButton: function (id) { > > Prefer to modify _buildButtons to be able to be called multiple times as > opposed to having copy/pasted code here. Then when the > 'devtools.screenshot-clipboard.enabled' pref changes, call _buildButtons > again. The reason I moved from _rebuildButtons to _rebuildButton(id) was that there is that the "command-button-frames" button is destroyed but never rebuilt by _buildButtons. So I wrote it such that it only rebuilt the screenshot button. Also, when I rebuilt the buttons, there's a whole range of errors I got in the terminal. And rebuilding only the screenshot button prevented that.

(In reply to Jaideep Bhoosreddy [:jbhoosreddy] from comment #20) > > ::: devtools/client/framework/toolbox.js > > @@ +1066,5 @@ > > > + /** > > > + * Rebuild toolbox button again > > > + * Currently, only for building screenshot button > > > + */ > > > + _rebuildButton: function (id) { > > > > Prefer to modify _buildButtons to be able to be called multiple times as > > opposed to having copy/pasted code here. Then when the > > 'devtools.screenshot-clipboard.enabled' pref changes, call _buildButtons > > again. > The reason I moved from _rebuildButtons to _rebuildButton(id) was that there > is that the "command-button-frames" button is destroyed but never rebuilt by > _buildButtons. So I wrote it such that it only rebuilt the screenshot > button. Also, when I rebuilt the buttons, there's a whole range of errors I > got in the terminal. And rebuilding only the screenshot button prevented > that. How about we move the CommandUtils bits out of buildButtons and into a buildCommandButtons or similar, then only rebuild those ones when the pref changes so we don't affect the picker button, frames button, etc.

I've incorporated all the feedback in this patch. I don't see any errors anymore, but I don't think I was able to replicate one of the errors Helen talked about. But also, I'm not sure how to proceed with writing tests for this feature. Thanks to Ryan's help, I was able to add page flashing effect in the same patch. Another issue is that I've not modified RDM screenshot to clipboard. We might want to do that also. But I'd like to do submit this for now, with tests and create a follow-up bug later.

Comment on attachment 8764701 [details] [diff] [review] 1257913.patch [1.0] Review of attachment 8764701 [details] [diff] [review]: ----------------------------------------------------------------- Looking good so far ::: devtools/client/framework/toolbox.js @@ +1048,3 @@ > return CommandUtils.createButtons(spec, this.target, this.doc, > requisition).then(buttons => { > + let container = this.doc Would it be possible to not take in 'id' or 'next' here, and instead for each button check to see if there's a button with the same id already in the DOM as the one returned from CommandUtils. Then if so, remove the existing one before appending the next one? Then the _rebuildButtons function could go away and you could call _buildButtons directly when the clipboard pref changes. ::: devtools/client/inspector/inspector-panel.js @@ +1310,5 @@ > /** > * Initiate gcli screenshot command on selected node > */ > screenshotNode: function () { > + const pref = "devtools.screenshot-clipboard.enabled"; I would slightly reorganize this as something like: const command = Services.prefs.getBoolPref("devtools.screenshot-clipboard.enabled") ? "screenshot --selector --clipboard" : "screenshot --selector"; then later: requisition.updateExec(command + " '" + this.selectionCssSelector + "'");

Based on previews comments, I have made the appropriate changes and removed _rebuildButtons method. Working on writing tests, I notice that many aspects of the feature are already tested in existing tests. The screenshot feature is especially extensively tested. Currently, I'm writing tests for the following: 1. checking if the preference has changed. 2. number of tool buttons is always 1. (button is always rebuilt after being removed and not duplicated)

I'm noticing some discussion about the --fullpage flag on this. How do users currently change that setting? Is that something where having it in the Settings along with the options Jaideep has added would be useful?

(In reply to Helen V. Holmes (:helenvholmes) (:✨)(pls ni?) from comment #25) > I'm noticing some discussion about the --fullpage flag on this. > > How do users currently change that setting? Is that something where having > it in the Settings along with the options Jaideep has added would be useful? That settings could only be changed by the power users, as it is hardcoded into the toolbar spec. I could probably add a new option under screenshot behaviour to allow them to choose, depending on what you think, if we want users to have that option in the UX.

(In reply to Jaideep Bhoosreddy [:jbhoosreddy] from comment #26) > (In reply to Helen V. Holmes (:helenvholmes) (:✨)(pls ni?) from comment #25) > > I'm noticing some discussion about the --fullpage flag on this. > > > > How do users currently change that setting? Is that something where having > > it in the Settings along with the options Jaideep has added would be useful? > > That settings could only be changed by the power users, as it is hardcoded > into the toolbar spec. I could probably add a new option under screenshot > behaviour to allow them to choose, depending on what you think, if we want > users to have that option in the UX. I think it would definitely be a nice-to-have. I think it'd be safe to open a new bug for it.

Comment on attachment 8765354 [details] [diff] [review] 1257913.patch [2.0] Review of attachment 8765354 [details] [diff] [review]: ----------------------------------------------------------------- Going to forward the request for feedback on the gcli/commands/screenshot.js and responsive.html/actions/screenshot.js to jryans. Ryan, I'm not sure if you've already reviewed that part so feel free to clear if so, just want to make sure it's not missed ::: devtools/client/framework/test/browser_toolbox_screenshot.js @@ +7,5 @@ > +"use strict"; > + > +var doc = null, toolbox = null, panelWin = null, modifiedPrefs = []; > + > +function test() { Please base this on the newer 'add_task' test format. I know this is based on existing options tests, but maybe they can be up for refactor next :). See something like browser_toolbox_options.js or browser_toolbox_split_console.js for an example. ::: devtools/client/framework/toolbox.js @@ +928,5 @@ > > /** > * Add buttons to the UI as specified in the devtools.toolbox.toolbarSpec pref > */ > + _buildButtons: function (id) { I still don't think you need to pass the 'id' here. Inside of the buttons.forEach loop you could look at button.id and see if it's in the DOM in the first place. Something like this (untested): return CommandUtils.createButtons(...).then(buttons => let container = this.doc.getElementById("toolbox-buttons"); buttons.forEach(button => { let currentButton = this.doc.getElementById(id); if (currentButton) { container.replaceChild(button, currentButton); } else { container.appendChild(button); } } }); @@ +956,5 @@ > + const clipboardEnabled = Services.prefs.getBoolPref("devtools.screenshot-clipboard.enabled"); > + if (clipboardEnabled) { > + spec[5] = spec[5] + " --clipboard"; > + } > + Nit: trailing whitespace

I actually moved those changes to a new bug and requested Ryan to look into it separately, so that this patch becomes more manageable and also more focused on initial objectives. Will remove those changes in the next submission.

Based on the previous feedback for from Brian. I'll submit tests in a different patch.

Comment on attachment 8766538 [details] [diff] [review] 1257913.patch [3.0] Review of attachment 8766538 [details] [diff] [review]: ----------------------------------------------------------------- ::: devtools/client/framework/toolbox.js @@ +954,5 @@ > + let spec = CommandUtils.getCommandbarSpec("devtools.toolbox.toolbarSpec"); > + // Special case for screenshot command button to check for clipboard preference > + const clipboardEnabled = Services.prefs.getBoolPref("devtools.screenshot-clipboard.enabled"); > + if (clipboardEnabled) { > + spec[5] = spec[5] + " --clipboard"; This shouldn't rely on the ordering of the values in this pref. Please make a new function called getToolbarSpec() that maps CommandUtils.getCommandbarSpec("devtools.toolbox.toolbarSpec") and only selectively changes the correct item

I'm uploading a new patch based on Brian's review.

Re-uploading; Uploaded the wrong patch by mistake.

Comment on attachment 8766597 [details] [diff] [review] 1257913.patch [4.0] Please fix up the pref names as I suggested in comment 18.

The current patch for this bug will save to clipboard _or_ to a file. In comment 8 and comment 10, it feels like Helen and Bryan agreed we should actually do _both_ by default. Helen, do you still think we should do both by default? There is bug 1282466 that discusses some kind of notification after the screenshot is taken, but it seems somewhat focused on the save to file case. Do we want some kind of message (perhaps in product, not desktop notification) when we save to _clipboard_ so it's clear the clipboard has changed? Helen, it seems you thought we should have something like this back in comment 10. Do we still want that for the clipboard path? If so, should it be part of the work here?

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #35) > The current patch for this bug will save to clipboard _or_ to a file. In > comment 8 and comment 10, it feels like Helen and Bryan agreed we should > actually do _both_ by default. Helen, do you still think we should do both > by default? Yep, I believe so! I don't think we should be swapping, based off of how other screenshotting tools seem to work. > There is bug 1282466 that discusses some kind of notification after the > screenshot is taken, but it seems somewhat focused on the save to file case. > Do we want some kind of message (perhaps in product, not desktop > notification) when we save to _clipboard_ so it's clear the clipboard has > changed? Helen, it seems you thought we should have something like this > back in comment 10. Do we still want that for the clipboard path? If so, > should it be part of the work here? It would be nice to have a small in-context notification displaying, "Copied to clipboard!". We could display a small label with that message by the mouse when that happens (perhaps the default label we use over Debugger buttons?). Open to other suggestions on this, but I think this will work well. I think it would be great to have that work either done in this bug or in a follow-up bug done immediately after this one—the work really does belong conceptually together.

Comment on attachment 8766597 [details] [diff] [review] 1257913.patch [4.0] Review of attachment 8766597 [details] [diff] [review]: ----------------------------------------------------------------- This generally works for me, but please see jryans' comment

Based on the previous comments by Ryan and Helen.

Comment on attachment 8769881 [details] [diff] [review] 1257913.patch [5.0] Review of attachment 8769881 [details] [diff] [review]: ----------------------------------------------------------------- In comment 36, Helen agreed we need some kind of notification in product when the clipboard changes. Do you intend to do that in this bug or in a follow up? If it's a follow up, we might not want to turn on save to clipboard default until it's ready, to avoid surprise. So let's either: A. add some notification of clipboard change in this bug B. disable clipboard by default for now, and re-enable once we have the notification Other than that, it's working well! ::: devtools/shared/locales/en-US/gclicommands.properties @@ +102,5 @@ > > +# LOCALIZATION NOTE (fileDesc) A very short string to describe > +# the 'file' parameter to the 'screenshot' command, which is displayed in > +# a dialog when the user is using this command. > +fileDesc=Save to file? (true/false) These string IDs are too generic in a file for all GCLI commands. Start them with "screenshotFile*".

I added the screenshot notification to clipboard. And made other changes based on Ryan's previous comment. Tests to follow.

Based on Brian's comment regarding tests, I've written new tests based on existing tests. this tests to check if the screenshot toolbar button is rebuilt upon changing of pref values for screenshot options.

Comment on attachment 8772266 [details] [diff] [review] 1257913.patch [6.0] Review of attachment 8772266 [details] [diff] [review]: ----------------------------------------------------------------- I think we need take another look at the approach for the notification, see comments inline. ::: devtools/client/shared/developer-toolbar.js @@ +108,5 @@ > button.setAttribute("title", command.description); > } > > button.addEventListener("click", () => { > + if (typed.indexOf("screenshot") != -1) { It's pretty unfortunate to keep lacing all these "if (screenshot)" checks in generic code for all buttons like this. Could you investigate the GCLI tooltip mechanism that's used with the real GCLI command line (not buttons) and see how that works? Maybe there's a way to use it with command buttons too. It would be great to have something that can be configured as part of the button's setup or the command itself when used as a button, instead this check here. @@ +188,5 @@ > + createTooltip: function (output, target, document) { > + if (output.args.clipboard && output.data != null) { > + const toolbox = gDevTools.getToolbox(target); > + if (!this._tooltip) { > + this._tooltip = new HTMLTooltip(toolbox, { type: "normal", useXulWrapper: true }); This doesn't seem to give the best appearance at the moment. I'm guessing it needs some CSS first. For example, there's no padding inside the tooltip. In the dark theme, I could barely even see that the tooltip is there. We might also want the "arrow" mode? Helen may know of a good reference example, which should help a lot. Also is there a reason you need `useXulWrapper` here? We may also be able to use GCLI's own tooltip facility, see above.

Comment on attachment 8772289 [details] [diff] [review] 1257913.tests.patch Review of attachment 8772289 [details] [diff] [review]: ----------------------------------------------------------------- ::: devtools/client/framework/test/browser_toolbox_options.js @@ +155,5 @@ > // We use executeSoon here to ensure that the element is in view and > // clickable. > executeSoon(function () { > info("Click event synthesized for pref " + pref); > + // EventUtils.synthesizeMouseAtCenter(node, {}, panelWin); This is fine with me - you can delete the commented out line ::: devtools/client/framework/test/browser_toolbox_screenshot.js @@ +8,5 @@ > + > +// Tests that changing preferences in the options panel updates the prefs > +// and toggles appropriate things in the toolbox. > + > +var doc = null, toolbox = null, panelWin = null, tool = null; Please remove any variables here that aren't used (doc can be replaced with panelWin.document anywhere, strings is unused, etc) @@ +16,5 @@ > + > +add_task(function* () { > + const URL = "data:text/html;charset=utf8,test for screenshot " + > + "to clipboard tool"; > + let tab = yield addTab(URL); For all this setup, please switch to the 'openToolboxForTab' helper function in shared-head.js which will remove the need for a lot of these lines (and the testSelectTool function) @@ +42,5 @@ > + "#enabled-toolbox-buttons-box input[type=checkbox]:not([data-unsupported])"); > + let screenshotNode = [...toolNodes].filter(node => node.id === "command-button-screenshot")[0]; > + if(!screenshotNode.checked) { > + toggledTool = true; > + is(getScreenshotButton() === null, true, "Screenshot button is not created or hidden"); you can use is(getScreenshotButton(), null, ..) instead @@ +75,5 @@ > + return screenshotButton[0] || null; > + > +} > +function* cleanup() { > + yield toolbox.destroy(); This line and the next are unnecessary due to the cleanup already happening in shared-head.js

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #42) > Comment on attachment 8772266 [details] [diff] [review] > > We may also be able to use GCLI's own tooltip facility, see above. Just a warning: GCLI tooltips (using XUL <panel> and <tooltip> elements) have explicitly been left out of the devtools-html migration because they are only used by the developer toolbar. If we start using them in other components, we might have to reconsider? On a sidenote, I would prefer if new tooltips could rely on the new API to avoid fragmentation. We are currently migrating <panel> and Tooltip to use the HTMLTooltip, would be super helpful if new implementations could rely on it as well. Also if we want to have a consistent style across tooltips (especially for the arrow style), focusing on one implementation would make it easier. Looks like notifications will be handled in a separate bug, so we'll discuss it in more details there.

(In reply to Julian Descottes [:jdescottes] from comment #44) > (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #42) > > Comment on attachment 8772266 [details] [diff] [review] > > > > We may also be able to use GCLI's own tooltip facility, see above. > > Just a warning: GCLI tooltips (using XUL <panel> and <tooltip> elements) > have explicitly been left out of the devtools-html migration because they > are only used by the developer toolbar. If we start using them in other > components, we might have to reconsider? Note: GCLI tooltips were designed without thought to reuse, also we plan to merge GCLI into the console at some point in the future, so if there is code here that's useful, I suggest you treat it like you would code that you found on SO. i.e. rip it off and do your thing with it.

I am submitting this patch with some minor modifications. The clipboard preference is turned off by default. I'll work on the notifications in a follow-up bug next so that it can be turned back up.

Comment on attachment 8773680 [details] [diff] [review] 1257913.patch [7.0] Review of attachment 8773680 [details] [diff] [review]: ----------------------------------------------------------------- Thanks, this version looks good to me. Please file the follow up for notification and mark it as depending on this one.

Hello Brian. I followed up on your feedback for the tests. I made all the modifications you suggested except using openToolboxForTab(tab, "options"). Every time I tried to use it, I was unable to select the screenshot node (returns as undefined). Maybe you have some pointers for that. But the test still works.

Comment on attachment 8773989 [details] [diff] [review] 1257913.tests.patch [1.0] Review of attachment 8773989 [details] [diff] [review]: ----------------------------------------------------------------- ::: devtools/client/framework/test/browser_toolbox_screenshot.js @@ +36,5 @@ > + info("Testing screenshot options"); > + let tool = toolbox.getPanel("options"); > + panelWin = tool.panelWin; > + let toolNodes = panelWin.document.querySelectorAll( > + "#enabled-toolbox-buttons-box input[type=checkbox]:not([data-unsupported])"); I don't see why you are fetching toolNodes in this way - you could directly to `let screenshotNode = panelWin.document.querySelector("#command-button-screenshot") instead @@ +38,5 @@ > + panelWin = tool.panelWin; > + let toolNodes = panelWin.document.querySelectorAll( > + "#enabled-toolbox-buttons-box input[type=checkbox]:not([data-unsupported])"); > + let screenshotNode = [...toolNodes] > + .filter(node => node.id === "command-button-screenshot")[0]; So apparently if the options panel is opened initially, it builds the available command buttons based on the toolbox DOM (via toolbox.toolboxButtons) and this is racing with actually adding buttons to the DOM (_buildButtons / CommandUtils.createButtons). This is a pre-existing but but I guess we haven't bumped into it because tests tend to not open the options panel immediately. We could either open the toolbox normally then switch to the options panel (immediate solution), or fix this race and make the options panel behave better (for instance, make toolbox buttons a resolve with buttons once they are ready, or build the options based directly on toolbarSpec). For now I think it's OK to do the former. Here's a pastebin with that change and some other cleanup: https://pastebin.mozilla.org/8887097. The rest of the comments are assuming you import those changes to start with @@ +45,5 @@ > + is(getScreenshotButton(), null, "Screenshot button is not created or hidden"); > + info("Enabling screenshot toolbar button"); > + screenshotNode.scrollIntoView(); > + screenshotNode.click(); > + is(!!getScreenshotButton(), true, "Screenshot button is created"); any time you are doing is(!!something(), true) you could do ok(something())

Thanks Brian! openNewTabAndToolbox is what I needed. I didn't know my problem was an existing bug, but your comment saved a lot of time! I followed up on the other things you pointed at.

Comment on attachment 8774959 [details] [diff] [review] 1257913.tests.patch [2.0] Review of attachment 8774959 [details] [diff] [review]: ----------------------------------------------------------------- ::: devtools/client/framework/test/browser_toolbox_screenshot.js @@ +21,5 @@ > + let toggledTool = false; > + let tool = toolbox.getPanel("options"); > + let panelWin = tool.panelWin; > + let screenshotNode = panelWin.document.querySelector("#command-button-screenshot"); > + if (!screenshotNode.checked) { This toggledTool condition seems unnecessary to me. We should be able to assume a clean slate as far as which command buttons are on by default so we can drop all references to this and just assume that screenshotNode won't be checked (so removing this if statement and just running the code directly). If it turns out to be a problem due to another test enabling the button, then we should reset the devtools.toolbox.toolbarSpec pref in between tests (in a cleanup function in shared-head.js) @@ +44,5 @@ > + "Screenshot button is rebuilt after toggling: " + node.id); > + } > + } > + // Setting screenshot node preference to default value > + if (toggledTool) { This logic can be removed and replaced with this code (outside of any other function): registerCleanupFunction(() => { Services.prefs.clearUserPref("devtools.toolbox.toolbarSpec"); }); Then it'll run regardless of whether this test passes or fails

Made changes according to previous review.

Comment on attachment 8773680 [details] [diff] [review] 1257913.patch [7.0] Review of attachment 8773680 [details] [diff] [review]: ----------------------------------------------------------------- ::: devtools/client/framework/toolbox.js @@ +1022,5 @@ > + getToolbarSpec: function () { > + let spec = CommandUtils.getCommandbarSpec("devtools.toolbox.toolbarSpec"); > + // Special case for screenshot command button to check for clipboard preference > + const clipboardEnabled = Services.prefs > + .getBoolPref("devtools.screenshot.clipboard.enabled");1 There's an extra "1" at the end of this line

Comment on attachment 8776343 [details] [diff] [review] 1257913.tests.patch [3.0] Review of attachment 8776343 [details] [diff] [review]: ----------------------------------------------------------------- What's there right now looks good. The only thing is I'm not sure if this is covering enough of the code introduced in the last patch. This is checking if the button gets rebuilt, but it's not checking that the options to the screenshot command are right, or that the screenshot command handles those options properly. The latter might be a difficult thing to test, but from the toolbox level I'd expect that we are checking at least that getToolbarSpec() is returning the right thing as these options get checked. ::: devtools/client/framework/test/browser_toolbox_screenshot.js @@ +30,5 @@ > + let toolNodes = panelWin.document.querySelectorAll( > + "#screenshot-options input[type=checkbox]:not([data-unsupported])"); > + > + // Executing test twice to return to default state > + for (let i = 0; i < 2; i++) { Instead of looping here and making sure the button is still there, I think we could just grab the two buttons we actually care about ("#screenshot-options #devtools-screenshot-audio" and "#screenshot-options #devtools-screenshot-clipboard") and assign them to two different variables instead of using toolNodes. Then click one and check that toolbox.getToolbarSpec() returns the right thing. Then click it again to reset. Then click the other and check that toolbox.getToolbarSpec() returns the right thing. Then click again to reset. That will at least cover a bit of the toolbox functionality more than just if the button exists.

This was very helpful. Thanks for your detailed comments. I did not know if I could access that data in the test, at the time. These changes make it more comprehensive. Let me know if you think I can make any further changes to the tests.

Comment on attachment 8778972 [details] [diff] [review] 1257913.tests.patch [4.0] Review of attachment 8778972 [details] [diff] [review]: ----------------------------------------------------------------- ::: devtools/client/framework/test/browser_toolbox_screenshot.js @@ +36,5 @@ > + info("Enabling clipboard option: " + clipboardNode.getAttribute("id")); > + if (!clipboardNode.checked) { > + clipboardNode.click(); > + } > + ok(GetPref("devtools.screenshot.clipboard.enabled"), "Clipboard preference is enabled"); Instead of bringing in this helper function, you can directly use Services.prefs.get*Pref (I think in this case both prefs are bools so Services.prefs.getBoolPref) @@ +39,5 @@ > + } > + ok(GetPref("devtools.screenshot.clipboard.enabled"), "Clipboard preference is enabled"); > + ok(toolbarSpecHas(toolbox, "--clipboard"), > + "Screenshot tool created with the right spec"); > + clipboardNode.click(); No need to do the second click here if you do the cleanup function as suggested below @@ +47,5 @@ > + if (!audioNode.checked) { > + audioNode.click(); > + } > + ok(GetPref("devtools.screenshot.audio.enabled"), "Audio preference is enabled"); > + audioNode.click(); Ditto @@ +56,5 @@ > + return button && !button.hidden ? button : null; > +} > + > +registerCleanupFunction(() => { > + Services.prefs.clearUserPref("devtools.toolbox.toolbarSpec"); Please also clear prefs here for: Services.prefs.clearUserPref("devtools.screenshot.clipboard.enabled"); Services.prefs.clearUserPref("devtools.screenshot.audio.enabled");

Jaideep, I know your internship is done now, will you have time to continue working on this bug? A lot of work was done already, and it'd be really great to land this.

I've tested the latest patch quickly, it's really cool! I love the flash effect and the shutter sound! 2 comments (that might have already been made earlier): - the screenshot node context menu in the inspector doesn't work anymore - the screenshot in RDM doesn't save to the clipboard even if the option is checked in the options panel. once these are fixed, we should totally land this.

(In reply to Patrick Brosset <:pbro> from comment #61) > I've tested the latest patch quickly, it's really cool! I love the flash > effect and the shutter sound! > 2 comments (that might have already been made earlier): > - the screenshot node context menu in the inspector doesn't work anymore > - the screenshot in RDM doesn't save to the clipboard even if the option is > checked in the options panel. > once these are fixed, we should totally land this. This bug has been somewhat of a long-term struggle for me, but I do intend to see this through. Currently, I am trying to make the tests pass. I got some great pointers from :gl and :bgrins. Thank you for pointing the issues, I'll make a note of them. There's a follow-up bug for the screenshot tool in the RDM. It uses a separate mechanism for screenshotting, which doesn't use the GCLI. After this patch lands, I'll work on porting that mechanism to the GCLI command.

That's great to hear. Thanks Jaideep.

Comment on attachment 8784194 [details] [diff] [review] 1257913.patch [9.0] Review of attachment 8784194 [details] [diff] [review]: ----------------------------------------------------------------- ::: devtools/client/framework/options-panel.css @@ +107,5 @@ > } > + > +#screenshot-icon::before { > + background-image: url(chrome://devtools/skin/images/command-screenshot.svg); > + margin-left: 5px; nit: margin-inline-start for RTL support ::: devtools/client/framework/toolbox-options.xhtml @@ +99,5 @@ > </fieldset> > + > + <fieldset id="screenshot-options" class="options-groupbox"> > + <legend>&options.screenshot.label; > + <span id="screenshot-icon" class="command-button command-button-invertable devtools-button"></span> nit: remove command-button and command-button-invertable, those classes have no effect (especially since the command-button css is in toolbox.css) ::: devtools/client/inspector/inspector-panel.js @@ +1557,5 @@ > * Initiate gcli screenshot command on selected node > */ > screenshotNode: function () { > + const command = Services.prefs.getBoolPref("devtools.screenshot-clipboard.enabled") ? > + "screenshot --clipboard --selector " : "screenshot --selector "; nit: align this line with the one above, or use 2 space indentation ::: devtools/shared/gcli/commands/screenshot.js @@ +255,5 @@ > * Save the captured screenshot to one of several destinations. > */ > function saveScreenshot(args, context, reply) { > const fileNeeded = args.filename != FILENAME_DEFAULT_VALUE || > + (!args.imgur && !args.clipboard) || args.file; nit: would be nice to align this with the previous line

Based on the comments by :ntim, and also fixed the first issue pointed out by :pbro in [1]. The second issue will be resolved in a follow-up bug. [1]: https://bugzilla.mozilla.org/show_bug.cgi?id=1257913#c61

Since Jaideep's internship has ended, I am going to assume he may not be working on this anymore. If that's incorrect, feel free to pick it up again!

Pushed by ntim.bugs@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/e54021050e77 Add copy screenshot to clipboard feature; r=jryans

I think bug 418833 has fixed the failure, because I can't reproduce them locally.

I was able to reproduce at least on try as recently as Nov. 16: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bc439d2c3199799a906c2838e65819757494a7a2 but that was before bug 418833 landed, so let's hope you're right!

A couple of questions on the command line strings https://hg.mozilla.org/mozilla-central/rev/e54021050e77#l8.26 > screenshotFileDesc=Save to file? (true/false) Are "true/false" literal values that should not be localized? If that's the case, it should be explained in the localization comment. > screenshotFileManual=True if the screenshot should save the file even when other options are enabled (eg. clipboard). eg. -> e.g.

Thanks :ntim for resolving this issue! I've been trying for so long, and never figured out why it was failing on try! But now that this has landed, I can probably (finally, sigh!) resolve a couple follow-up bugs for this! Yay! Thanks :jryans for helping me out with this too. Also, I could make the changes in Comment 77 in the follow-up patch too.

(In reply to Francesco Lodolo [:flod] from comment #77) > A couple of questions on the command line strings > https://hg.mozilla.org/mozilla-central/rev/e54021050e77#l8.26 > > > screenshotFileDesc=Save to file? (true/false) > Are "true/false" literal values that should not be localized? If that's the > case, it should be explained in the localization comment. > > > screenshotFileManual=True if the screenshot should save the file even when other options are enabled (eg. clipboard). > eg. -> e.g. Agreed on both issues: filed bug 1257913. (In reply to Jaideep Bhoosreddy [:jbhoosreddy] from comment #78) > Thanks :ntim for resolving this issue! I really didn't apply any fix :) Bug 418833 seemed to fix the issue. Thanks for working on this feature! > Also, I could make the changes in Comment 77 in the follow-up patch too. Can you take a look ? I've filed bug 1322179 for this issue.

This bug was about to " to copy screenshot to the clipboard when click camera icon on the Developer Tools toolbar to take a screenshot " I have seen the feature being implemented with latest Nightly on Windows 10 , 64 Bit ! This bug's fix is now verified in Latest Nightly . Build ID : 20170103030204 User Agent : Mozilla/5.0 (Windows NT 10.0; WOW64; rv:53.0) Gecko/20100101 Firefox/53.0 [bugday-20170104]

This bug was about to "Introduce a "copy to clipboard" mode to the screen-shot tool". I have seen the feature being implemented with latest Nightly on Ubuntu 16.04 LTS ! This bug's fix is now verified in Latest Nightly Build ID : 20170111030235 User Agent : Mozilla/5.0 (X11; Linux x86_64; rv:53.0) Gecko/20100101 Firefox/53.0 [bugday-20170111]

Dear DevTools team, please don't forget to add the dev-doc-needed keyword to feature bugs like this one, so the MDN team knows what to document. Sebastian

We apparently don't have a page on the screenshot tool (at least I couldn't find one) so I wrote one: https://developer.mozilla.org/en-US/docs/Tools/Screenshot_tool I also updated https://developer.mozilla.org/en-US/docs/Tools/Settings to cover this new setting. Please let me know if we need anything else.

Thanks Will! Looks great! It might be worth noting that the button is also available in the Responsive Design Mode.

Thanks Bryan. I updated the RDM page to mention "copy to clipboard".

Docs look good to me overall! Maybe it's useful to mention as related functionality that the Inspector allows capturing screenshots of certain nodes (right click on a node in inspector, screenshot node)? Then we'd have a single page with all the ways to screenshot things in DevTools.

Ryan, that makes a lot of sense. I've recast the page as "Taking screenshots" and covered both in there, and linked across to this page from the Inspector docs. What do you think? https://developer.mozilla.org/en-US/docs/Tools/Taking_screenshots

(In reply to Will Bamberg [:wbamberg] from comment #88) > Ryan, that makes a lot of sense. I've recast the page as "Taking > screenshots" and covered both in there, and linked across to this page from > the Inspector docs. What do you think? > > https://developer.mozilla.org/en-US/docs/Tools/Taking_screenshots Great, it looks good to me! :)

There's also a GCLI command allowing to take screenshots. Sebastian