1239461 - Screenshot button for taking a screenshot of the current viewport

Status: VERIFIED FIXED

(DevTools :: Responsive Design Mode, defect, P1)

Comment 1

[Helen V. Holmes (:helenvholmes) (:✨)]

Attachment: screenshot.svg

Comment 2

[Tim Nguyen :ntim]

Comment on attachment 8712353 [details]
screenshot.svg

Nice !

Comment 3

[Helen V. Holmes (:helenvholmes) (:✨)]

Attachment: camera-click.mp3

Comment 4

[Helen V. Holmes (:helenvholmes) (:✨)]

Attachment: screenshot-viewport-ux-notes.png

Copying ux notes here from the attached screenshot:

- Screenshot button sits in the global tabbar.
- Is fill gray (#fcfcfc) normally, and #468ee5 when :active.
- Visual feedback for the screenshot action could be better, so I think we should include two things:

1. A screenshot noise (attached a camera-click.mp3 from freesounds.org)
2. Flash white in the viewport (many screenshot tools do this).

Comment 5

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

This is great, I really like the sound effect and white flash ideas.

People have complained in the past that the screenshots don't have enough feedback to know something happened, so this should help.

The current RDM tool also "pulses" the downloads icon in the toolbar when done, should we keep that behavior?

Comment 6

[Tim Nguyen :ntim]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #5)
> The current RDM tool also "pulses" the downloads icon in the toolbar when
> done, should we keep that behavior?
My take would be yes, since it's consistent with other downloads, but Helen might have a different opinion :)

Comment 7

[Helen V. Holmes (:helenvholmes) (:✨)]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #5)
> The current RDM tool also "pulses" the downloads icon in the toolbar when
> done, should we keep that behavior?

I'm also going to say yes. I know that the larger UX team is going to be thinking about screenshotting more holistically in the future, but I don't know what's on their minds yet (I don't think they've thought much about implementation details yet). 

We should probably open a bug to get the command button screenshotting in sync with the UX of the responsive design mode screenshot button.

Comment 8

[Matteo Ferretti [:zer0] [:matteo]]

Attachment: Screenshot button for taking a screenshot of the current viewport

MozReview-Commit-ID: 8BBw61yn538

Comment 9

[Matteo Ferretti [:zer0] [:matteo]]

Comment on attachment 8725666 [details] [diff] [review]
Screenshot button for taking a screenshot of the current viewport

That's a WIP, please don't consider empty new lines and similar, I'm adding, removing, adding, removing… :)

I want to have a general opinion about the approach I'm using here and how to make it more "react-like"; for example: I was thinking to make the screenshot button "active" during the snapshot - and non clickable until is finished – and play the sound once the screenshot is saved. For that, I'm dispatching TAKE_SCREENSHOT_START and TAKE_SCREENSHOT_END – that are not used currently.

Comment 10

[Tim Nguyen :ntim]

(In reply to Matteo Ferretti [:matteo][:zer0] from comment #9)
> Comment on attachment 8725666 [details] [diff] [review]
> Screenshot button for taking a screenshot of the current viewport
> 
Note that <iframe mozbrowser> has an API to take screenshots directly, so the canvas stuff you're doing will probably not be needed soon.

Comment 11

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8725666 [details] [diff] [review]
Screenshot button for taking a screenshot of the current viewport

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

Hmm, this doesn't seem like the full patch?  It modifies other WIP screenshot stuff you probably have locally and / or doesn't seem to contain everything?

Comment 12

[Matteo Ferretti [:zer0] [:matteo]]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #11)
> Comment on attachment 8725666 [details] [diff] [review]
> Screenshot button for taking a screenshot of the current viewport
> 
> Review of attachment 8725666 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Hmm, this doesn't seem like the full patch?  It modifies other WIP
> screenshot stuff you probably have locally and / or doesn't seem to contain
> everything?

Hmm, it should be. It has to be applied on top of the previous patch – Close Button.
Can you try to do so now that the bug 1239464 has landed? If it's still missing some pieces, let me know!

Comment 13

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

The patch here contains, for example, *modifications* to devtools/client/responsive.html/actions/screenshot.js, but this file does not exist in the tree or in bug 1239464.  So, there must be something more on your machine that is not in the current patch.

Comment 14

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

During Monday's standup, we discussed that this bug should aim to apply all the pieces of the UX from bug 1243525 with this button in RDM:

> 1. The command button should have a blue :active state.
> 2. A screenshot noise should play (attaching a camera-click.mp3 from freesounds.org in a moment)
> 3. The screen should flash white in the viewport (many screenshot tools do this).

Comment 15

[Matteo Ferretti [:zer0] [:matteo]]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #14)

> During Monday's standup, we discussed that this bug should aim to apply all
> the pieces of the UX from bug 1243525 with this button in RDM:

What I said is that this bug already includes most of them – point 1 & 2 – but that probably the flashy thing – point 3 – might be added as follow up bug, since it needs more UX & UI clarification, and because it wasn't in the original scope of this bug, it would be better add as additional feature (or, nice to have) without making the UX as bottleneck.

Comment 16

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Matteo Ferretti [:zer0] [:matteo] from comment #15)
> (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #14)
> 
> > During Monday's standup, we discussed that this bug should aim to apply all
> > the pieces of the UX from bug 1243525 with this button in RDM:
> 
> What I said is that this bug already includes most of them – point 1 & 2 –
> but that probably the flashy thing – point 3 – might be added as follow up
> bug, since it needs more UX & UI clarification, and because it wasn't in the
> original scope of this bug, it would be better add as additional feature
> (or, nice to have) without making the UX as bottleneck.

Looking at the history, it seems like it is mentioned in this bug, see comment 4.

Anyway, please file a follow up if you think it should be done separately.

Comment 17

[Matteo Ferretti [:zer0] [:matteo]]

Attachment: MozReview Request: Bug 1239461 - Screenshot button for taking a screenshot of the current viewport; r=jryans

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

Comment 18

[Matteo Ferretti [:zer0] [:matteo]]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #16)

> Looking at the history, it seems like it is mentioned in this bug, see
> comment 4.

Sorry, I totally missed that! I will try to have something working tomorrow, but I will file a separate bug due the UX feedback / discussion, if needed, to avoid blocking the current patch.

About the patch I submit for review: at the end, I ended up with a test method that probably doesn't need to specify a filename for test purpose, as you will see. But I decided to submit the patch as is anyway, so in case you think it's better keep it it would be easier; otherwise I can simply remove the additional code.

Comment 19

[Marco Mucci [:MarcoM]]

(In reply to Matteo Ferretti [:zer0] [:matteo] from comment #18)
> (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #16)
> 
> > Looking at the history, it seems like it is mentioned in this bug, see
> > comment 4.
> 
> Sorry, I totally missed that! I will try to have something working tomorrow,
> but I will file a separate bug due the UX feedback / discussion, if needed,
> to avoid blocking the current patch.


Hi Matteo, please submit the new follow-up bug for triage first so we can prioritize accordingly for the RDM project.  Here's the list of available work marked as 'qe-verify+' you can pick up:  http://mzl.la/1RLCfTM

Comment 20

[Matteo Ferretti [:zer0] [:matteo]]

Comment on attachment 8736458 [details]
MozReview Request: Bug 1239461 - Screenshot button for taking a screenshot of the current viewport; r=jryans

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

Comment 21

[Matteo Ferretti [:zer0] [:matteo]]

(In reply to Marco Mucci [:MarcoM] from comment #19)

> > Sorry, I totally missed that! I will try to have something working tomorrow,
> > but I will file a separate bug due the UX feedback / discussion, if needed,
> > to avoid blocking the current patch.
> 
> 
> Hi Matteo, please submit the new follow-up bug for triage first so we can
> prioritize accordingly for the RDM project.  Here's the list of available
> work marked as 'qe-verify+' you can pick up:  http://mzl.la/1RLCfTM

I just added a really quick flashing effect to this bug; I think that if the results is OK as is, we don't need another bug. If we need more complex things, and further discussion with UX, then I filed a follow up bug. Sounds good?

Comment 22

[Marco Mucci [:MarcoM]]

(In reply to Matteo Ferretti [:zer0] [:matteo] from comment #21)
> (In reply to Marco Mucci [:MarcoM] from comment #19)
> 
> > > Sorry, I totally missed that! I will try to have something working tomorrow,
> > > but I will file a separate bug due the UX feedback / discussion, if needed,
> > > to avoid blocking the current patch.
> > 
> > 
> > Hi Matteo, please submit the new follow-up bug for triage first so we can
> > prioritize accordingly for the RDM project.  Here's the list of available
> > work marked as 'qe-verify+' you can pick up:  http://mzl.la/1RLCfTM
> 
> I just added a really quick flashing effect to this bug; I think that if the
> results is OK as is, we don't need another bug. If we need more complex
> things, and further discussion with UX, then I filed a follow up bug. Sounds
> good?

Sounds good, thanks Matteo.

Comment 23

[Matteo Ferretti [:zer0] [:matteo]]

Helen, you can check the current effect using this build I made:

http://archive.mozilla.org/pub/firefox/try-builds/[e-mail]-fe2d15b573e91ed1360fa09dc7c8adb42e2373f7/try-macosx64/

Comment 24

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8736458 [details]
MozReview Request: Bug 1239461 - Screenshot button for taking a screenshot of the current viewport; r=jryans

https://reviewboard.mozilla.org/r/43297/#review40049

The behavior seems to work quite nicely overall!  Lots of little nits here, so I'd like to see another round first.

::: devtools/client/locales/en-US/responsive.properties:26
(Diff revision 2)
>  # LOCALIZATION NOTE (responsive.noDeviceSelected): placeholder text for the
>  # device selector
>  responsive.noDeviceSelected=no device selected
> +
> +# LOCALIZATION NOTE  (responsive.screenshot): tooltip of the screenshot button.
> +responsive.screenshot=Screenshot

I think should be a phrase the describes an action to be performed.  The toolbox command button for screenshots uses "Take a full page screenshot".  Perhaps here we can use "Take a screenshot of the viewport"?

::: devtools/client/responsive.html/actions/index.js:41
(Diff revision 2)
> +  "SET_SCREENSHOT_NAME",
> +
> +  // Take a screenshot of the viewport
> +  "TAKE_SCREENSHOT_START",
> +
> +  // Take a screenshot of the viewport

Nit: should say something else here

::: devtools/client/responsive.html/actions/index.js:43
(Diff revision 2)
> +  // Take a screenshot of the viewport
> +  "TAKE_SCREENSHOT_START",
> +
> +  // Take a screenshot of the viewport
> +  "TAKE_SCREENSHOT_END",
>  ], module.exports);

Nit: add blank line at the end since it begins with a blank line

::: devtools/client/responsive.html/actions/screenshot.js:58
(Diff revision 2)
> +
> +    // append .png extension to filename if it doesn't exist
> +    filename = filename.replace(/\.png$|$/i, ".png");
> +
> +    chromeWindow.saveURL(data, filename, null,
> +                   true, true,

Nit: unusual indentation

::: devtools/client/responsive.html/actions/screenshot.js:75
(Diff revision 2)
> +  takeScreenshot(filename = getFileName()) {
> +    return function* (dispatch, getState) {
> +      dispatch({type: TAKE_SCREENSHOT_START});
> +
> +      let iframe = document.querySelector("iframe");
> +      let data = createScreenshotFor(iframe);

The picture currently includes the corner resize gripper at the bottom right since it's within view of the iframe (I guess?).

We don't want it in the image, so maybe we need to temporarily hide the gripper while capturing the screenshot.

::: devtools/client/responsive.html/actions/screenshot.js:78
(Diff revision 2)
> +
> +      let iframe = document.querySelector("iframe");
> +      let data = createScreenshotFor(iframe);
> +      yield saveToFile(data, filename);
> +
> +      photoshoot(iframe);

I would say let's trigger the UI effects as soon after clicking as we can, so before any yields.

Let's call the function `simulateCameraEffects` or something.

::: devtools/client/responsive.html/app.js:39
(Diff revision 2)
>    onChangeViewportDevice(id, device) {
>      this.props.dispatch(changeDevice(id, device));
>    },
>  
> +  onExit() {
> +    window.postMessage({type: "exit"}, "*");

We've mostly used `{ type: "exit" }` style in RDM, so let's keep going with it here.

::: devtools/client/responsive.html/app.js:77
(Diff revision 2)
>          id: "app",
>        },
>        GlobalToolbar({
>          onExit,
> +        onScreenshot,
> +        screenshot,

Nit: We appear to be ordering all `on*` things at the end in other places, so move screenshot to the top of this list.

::: devtools/client/responsive.html/components/global-toolbar.js:20
(Diff revision 2)
>      onExit: PropTypes.func.isRequired,
> +    onScreenshot: PropTypes.func.isRequired,
> +    screenshot: PropTypes.shape(Types.screenshot).isRequired,
>    },
>  
> +  displayName: "GlobalToolbar",

We should keep the `displayName` at the top.  See the discussion in bug 1258353.  It sounds like `displayName`, `propTypes`, then `mixins` would meet the suggested ordering from that bug.

::: devtools/client/responsive.html/index.css:110
(Diff revision 2)
>  #global-exit-button::before {
>    background-image: url("./images/close.svg");
>    margin: -6px 0 0 -6px;
>  }
>  
> +#global-screenshot-button:disabled {

I agree it's correct to set @disabled while capturing to avoid double capturing, but I don't think you need these styles.  My understanding is the button should highlight blue when **active** which should already happen with the existing button styles.

::: devtools/client/responsive.html/reducers/screenshot.js:16
(Diff revision 2)
> +} = require("../actions/index");
> +
> +let reducers = {
> +
> +  [SET_SCREENSHOT_NAME](screenshot, action) {
> +    return Object.assign({}, screenshot, {name: action.name});

Nit: "{ " style

::: devtools/client/responsive.html/reducers/screenshot.js:28
(Diff revision 2)
> +  [TAKE_SCREENSHOT_START](screenshot, action) {
> +    return Object.assign({}, screenshot, {progress: true});
> +  },
> +};
> +
> +module.exports = function(screenshot = {progress: false}, action) {

I think it's nice to have the initial state defined separately near the top of the file, like we do in other reducer files.

::: devtools/client/responsive.html/types.js:74
(Diff revision 2)
>   * The location of the document displayed in the viewport(s).
>   */
>  exports.location = PropTypes.string;
>  
>  /**
> + * The screenshot progress

This overall structure is more than just the progress.

::: devtools/client/responsive.html/types.js:77
(Diff revision 2)
>  
>  /**
> + * The screenshot progress
> + */
> +exports.screenshot = {
> +  progress: PropTypes.bool.isRequired,

I think something like `isCapturing` seems easier to understand than `progress` since it's just a boolean.

::: devtools/client/responsive.html/types.js:81
(Diff revision 2)
> +exports.screenshot = {
> +  progress: PropTypes.bool.isRequired,
> +
> +  // The name of the current screenshot
> +  name: PropTypes.string,
> +};

Nit: blank line before and after to match other objects

::: devtools/client/responsive.html/utils/moz.build:8
(Diff revision 2)
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +DevToolsModules(
> +    'l10n.js',

It seems like your patch is missing the removal of l10n.js from the previous location.

Comment 25

[Matteo Ferretti [:zer0] [:matteo]]

https://reviewboard.mozilla.org/r/43297/#review40049

> I think should be a phrase the describes an action to be performed.  The toolbox command button for screenshots uses "Take a full page screenshot".  Perhaps here we can use "Take a screenshot of the viewport"?

For the properties I just reuse the current text we have in RDM – so that, ideally, we don't have additional work for translation in other languages.
If you still think we should rephrase, I don't mind to use the one string you suggested.

Comment 26

[Patrick Brosset <:pbro>]

Looking at that patch, it seems like we are (at least partially) duplicating the code from the screenshot gcli command. It's a bit sad because we'll be duplicating bugs too.
Why not extract the screenshot code from the gcli command and use it in both places instead?
This would also have the added advantage of having the sound/flashing effect also work with the command (and therefore toolbox button).

Comment 27

[Matteo Ferretti [:zer0] [:matteo]]

https://reviewboard.mozilla.org/r/43297/#review40049

> The picture currently includes the corner resize gripper at the bottom right since it's within view of the iframe (I guess?).
> 
> We don't want it in the image, so maybe we need to temporarily hide the gripper while capturing the screenshot.

I fixed that, however I'm not sure if it's the proper way, maybe there is a better approach. You'll see in the next patch.

Comment 28

[Matteo Ferretti [:zer0] [:matteo]]

https://reviewboard.mozilla.org/r/43297/#review40049

> I agree it's correct to set @disabled while capturing to avoid double capturing, but I don't think you need these styles.  My understanding is the button should highlight blue when **active** which should already happen with the existing button styles.

I interpreted the point 1 of bug 1243525 in this sense: until the screenshot is over, the button is blue. That also consistent with similar UI I saw in other apps. Without that, the button will be blue only when is clicked, and immediately get gray – in our case, a slightly lightly gray, since is disabled. Usually the screenshot operation is almost immediate, so you don't see that. But, for some reason, saving on file system or generating the data takes a while, it can be perceived.

Let's double check with Helen, I'm fine either way.

Comment 29

[Matteo Ferretti [:zer0] [:matteo]]

https://reviewboard.mozilla.org/r/43297/#review40049

> This overall structure is more than just the progress.

Any suggestion how to rephrase that? Actually in the app only the progress currently count, the `name` is only for testing purpose – and as I mentioned, we could probably get rid of.

Comment 30

[Matteo Ferretti [:zer0] [:matteo]]

https://reviewboard.mozilla.org/r/43297/#review40049

> Nit: blank line before and after to match other objects

Since it seems differs from other guidelines in our codebase, do we have a place where those guidelines are described? For future reference.

Comment 31

[Matteo Ferretti [:zer0] [:matteo]]

Sorry for the sequels of comments; I didn't realize reviewboard was publishing every single one. :)

ryan: as said in comment 16, do you think that, with this test I wrote, we could remove the `name` from the `screenshot` object? Or do you think it's better in this way? You didn't comment the unit test.

(In reply to Patrick Brosset [:pbro] from comment #26)
> Looking at that patch, it seems like we are (at least partially) duplicating
> the code from the screenshot gcli command.

It's really, really partially duplicating. The command's screenshot is more complex – and not only in number of functionalities, but for the same functionality –, where this one is tailored on the RDM and it simplify a lot.

I agree that we could have a 3rd module, that aggregates the common parts, used by both RDM and GCLI, but I would say it's not in the scope of this bug; especially because it potentially leads to regression on GCLI – plus, if we wants to add the sound and flashing effect generally: the current approach takes advantages of the structure of RDM, where it can't be done as is in an unknown page.

I would suggest this sort of work to be part of bug 1243525 or bug 1257913 (depends by the scope of the first one). The screenshot functionality in RDM will also need a refactoring once we'll introduce the multiple viewports – currently it ignores that fact.

Comment 32

[Matteo Ferretti [:zer0] [:matteo]]

(In reply to Matteo Ferretti [:zer0] [:matteo] from comment #31)
> (In reply to Patrick Brosset [:pbro] from comment #26)
> > Looking at that patch, it seems like we are (at least partially) duplicating
> > the code from the screenshot gcli command.
[…]
> I would suggest this sort of work to be part of bug 1243525 or bug 1257913
> (depends by the scope of the first one). The screenshot functionality in RDM
> will also need a refactoring once we'll introduce the multiple viewports –
> currently it ignores that fact.

Almost forgot: we'll probably remove the duplicating code anyway – with such refactoring or earlier –, since this is a temporary workaround until we'll be able to ask to the content to generate the screenshot data itself – as is in the current RDM.

Comment 33

[Helen V. Holmes (:helenvholmes) (:✨)]

Mateo, seems like this doesn't apply cleanly to the latest fx-team. Would you mind rebasing so I can ui-review it?

Comment 34

[Matteo Ferretti [:zer0] [:matteo]]

Helen, you can use directly the build I linked in comment 23! I'm going to rebase with the new patch that also address the comment by ryan.

If you need the patch instead the build, let me know, I'll push a wip of the current work.

Comment 35

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Matteo Ferretti [:zer0] [:matteo] from comment #25)
> https://reviewboard.mozilla.org/r/43297/#review40049
> 
> > I think should be a phrase the describes an action to be performed.  The toolbox command button for screenshots uses "Take a full page screenshot".  Perhaps here we can use "Take a screenshot of the viewport"?
> 
> For the properties I just reuse the current text we have in RDM – so that,
> ideally, we don't have additional work for translation in other languages.
> If you still think we should rephrase, I don't mind to use the one string
> you suggested.

As discussed on IRC, let's use the new text I suggested.  It seems to match other buttons in DevTools more closely.

Comment 36

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

https://reviewboard.mozilla.org/r/43297/#review40049

> Any suggestion how to rephrase that? Actually in the app only the progress currently count, the `name` is only for testing purpose – and as I mentioned, we could probably get rid of.

We agreed on IRC the name is not needed for testing purposes, so we'll remove it.

I don't think I care _too_ much about the exact comment, I just meant that it says "progress" and it happened to contain another field.  How about "The screenshot of a viewport"?

> Since it seems differs from other guidelines in our codebase, do we have a place where those guidelines are described? For future reference.

I don't think I've seen any clear guidelines on this specific point of style, and I don't think we have any ESLint rule one way or the other.  Different files seems to use different spacing approaches for objects.  In this case I am bringing it up so that it matches the rest of the file.

Comment 37

[Helen V. Holmes (:helenvholmes) (:✨)]

Comment on attachment 8736458 [details]
MozReview Request: Bug 1239461 - Screenshot button for taking a screenshot of the current viewport; r=jryans

This is awesome!

For the screenshot button (and the rotate-phone/close buttons), can we apply cursor: pointer;? I believe that's customary for buttons even though *technically* they're not links.

Comment 38

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Matteo Ferretti [:zer0] [:matteo] from comment #31)
> (In reply to Patrick Brosset [:pbro] from comment #26)
> > Looking at that patch, it seems like we are (at least partially) duplicating
> > the code from the screenshot gcli command.
> 
> It's really, really partially duplicating. The command's screenshot is more
> complex – and not only in number of functionalities, but for the same
> functionality –, where this one is tailored on the RDM and it simplify a lot.
> 
> I agree that we could have a 3rd module, that aggregates the common parts,
> used by both RDM and GCLI, but I would say it's not in the scope of this
> bug; especially because it potentially leads to regression on GCLI – plus,
> if we wants to add the sound and flashing effect generally: the current
> approach takes advantages of the structure of RDM, where it can't be done as
> is in an unknown page.
> 
> I would suggest this sort of work to be part of bug 1243525 or bug 1257913
> (depends by the scope of the first one). The screenshot functionality in RDM
> will also need a refactoring once we'll introduce the multiple viewports –
> currently it ignores that fact.

I think for the moment, I am okay with duplicating the code like it is in the patch for this bug.  Like :zer0 says in comment 32, we'll have to change approaches here a bit once bug 1240896 lands in RDM.  Once we're using an <iframe mozbrowser> we have to use a frame script or the browser API to get the screenshot data, so we'll need to change to something like the legacy RDM approach.  Who ends up changing to the new approach might depend on whether this bug lands first or bug 1240896 does. :)

Overall I agree we want the same UX applied for the other ways to take screenshots.  The UX of flashing and making a sound are pretty self-contained.  Bug 1243525 seems like a good bug to use for applying them throughout the code base.

Comment 39

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

:helenvholmes, can you take a look at the button color discussion in comment 28?

Comment 40

[Matteo Ferretti [:zer0] [:matteo]]

https://reviewboard.mozilla.org/r/43297/#review40049

> We agreed on IRC the name is not needed for testing purposes, so we'll remove it.
> 
> I don't think I care _too_ much about the exact comment, I just meant that it says "progress" and it happened to contain another field.  How about "The screenshot of a viewport"?

I thought that at the beginning, the point is that is not a representation of a screenshot of the viewport – it doesn't contains any data, like the image – it's more like an indication of its state. I think if it's fine to you, since we're going to remove the `name` anyway, to keep the comment about the capturing action.

Comment 41

[Helen V. Holmes (:helenvholmes) (:✨)]

(In reply to Matteo Ferretti [:zer0] [:matteo] from comment #28)
> https://reviewboard.mozilla.org/r/43297/#review40049
> 
> > I agree it's correct to set @disabled while capturing to avoid double capturing, but I don't think you need these styles.  My understanding is the button should highlight blue when **active** which should already happen with the existing button styles.
> 
> I interpreted the point 1 of bug 1243525 in this sense: until the screenshot
> is over, the button is blue. That also consistent with similar UI I saw in
> other apps. Without that, the button will be blue only when is clicked, and
> immediately get gray – in our case, a slightly lightly gray, since is
> disabled. Usually the screenshot operation is almost immediate, so you don't
> see that. But, for some reason, saving on file system or generating the data
> takes a while, it can be perceived.
> 
> Let's double check with Helen, I'm fine either way.

I think I agree with Mateo here—keeping the active, blue state until the screenshot is complete I think is a nice addition.

Comment 42

[Helen V. Holmes (:helenvholmes) (:✨)]

So, a clarification on screenshotting UX:

- a user takes a snapshot of a large viewport.
- button turns active (blue) on press, and remains blue until screenshotting process is complete.
- also on press, white flash and camera noise play.
- when file saves to disc, blue animation to downloads folder happens.
- button becomes gray and is no longer disabled.

Hope that clears that up!

Comment 43

[Matteo Ferretti [:zer0] [:matteo]]

Comment on attachment 8736458 [details]
MozReview Request: Bug 1239461 - Screenshot button for taking a screenshot of the current viewport; r=jryans

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

Comment 44

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8736458 [details]
MozReview Request: Bug 1239461 - Screenshot button for taking a screenshot of the current viewport; r=jryans

https://reviewboard.mozilla.org/r/43297/#review40941

Seems quite close, the main thing I was unsure of was the use of rAF here.  Let me know why it's there!  Also, it may change given Helen's comment 42 about the ordering...?  We'll see what you think.

::: devtools/client/responsive.html/actions/screenshot.js:72
(Diff revisions 2 - 3)
>  
>  module.exports = {
>  
> -  takeScreenshot(filename = getFileName()) {
> +  takeScreenshot() {
>      return function* (dispatch, getState) {
> -      dispatch({type: TAKE_SCREENSHOT_START});
> +      yield dispatch({type: TAKE_SCREENSHOT_START});

Nit: { type

::: devtools/client/responsive.html/actions/screenshot.js:74
(Diff revisions 2 - 3)
>  
> -  takeScreenshot(filename = getFileName()) {
> +  takeScreenshot() {
>      return function* (dispatch, getState) {
> -      dispatch({type: TAKE_SCREENSHOT_START});
> +      yield dispatch({type: TAKE_SCREENSHOT_START});
>  
> +      window.requestAnimationFrame(async(function* () {

Why do you need rAF here?

::: devtools/client/responsive.html/actions/screenshot.js:78
(Diff revisions 2 - 3)
> +      window.requestAnimationFrame(async(function* () {
> -      let iframe = document.querySelector("iframe");
> +        let iframe = document.querySelector("iframe");
> -      let data = createScreenshotFor(iframe);
> +        let data = createScreenshotFor(iframe);
> -      yield saveToFile(data, filename);
>  
> -      photoshoot(iframe);
> +        simulateCameraEffects(iframe);

Given Helen's comment 42 (read carefully, it's different from what was discussed at the standup), it seems `simulatedCameraEffects` should happen pretty early (Helen says "on press"), so maybe just after:

  yield dispatch({type: TAKE_SCREENSHOT_START});

::: devtools/client/responsive.html/actions/screenshot.js:82
(Diff revisions 2 - 3)
>  
> -      photoshoot(iframe);
> +        simulateCameraEffects(iframe);
> +
> +        yield saveToFile(data, getFileName());
>  
> -      dispatch({type: TAKE_SCREENSHOT_END});
> +        dispatch({type: TAKE_SCREENSHOT_END});

Nit: { type

::: devtools/client/responsive.html/index.css:57
(Diff revisions 2 - 3)
>  .toolbar {
>    background-color: var(--theme-toolbar-background);
>    border: 1px solid var(--theme-splitter-color);
>  }
>  
>  .toolbar-button {

In Helen's comment 37, she requested `cursor: pointer` for all buttons.

::: devtools/client/responsive.html/reducers/screenshot.js:17
(Diff revisions 2 - 3)
>  
> +const INITIAL_SCREENSHOT = { isCapturing: false };
> +
>  let reducers = {
>  
>    [SET_SCREENSHOT_NAME](screenshot, action) {

We can remove the `SET_SCREENSHOT_NAME` action, right?

::: devtools/client/responsive.html/index.css:112
(Diff revision 3)
>    margin: -6px 0 0 -6px;
>  }
>  
> +#global-screenshot-button:disabled {
> +  filter: url("chrome://devtools/skin/images/filters.svg#checked-icon-state");
> +  opacity: 1!important;

Nit: DevTools style is typically `1 !important` (with space)

Comment 45

[Matteo Ferretti [:zer0] [:matteo]]

https://reviewboard.mozilla.org/r/43297/#review40941

About the rAF: is there in order to ensure that react components have time to render themselves. Specifically, as you indicates in your previous review we have to hide the handle that resize the viewport. Since the handler itself become invisible if `screenshot.isCapturing` is `true`, we need to wait the next repaint.
That's the only way I thought it could be done in a "react way", but if you know a better approach let me know! As alternative, we can always doing it old-school (setting an attribute on the document.body for example, or to the viewport's node, inside the action).

About Helen's comment: So, yeah, when I saw the comment I was confused, because it seems to me that was the opposite of what she said during the stand up, so I contacted her and she confirmed she changed her mind in the meantime. :) I'll explain better below.

> Why do you need rAF here?

As explained above, is there in order to ensure that react components have time to render themselves; otherwise the handle won't be hidden when the snapshot is taken.

> Given Helen's comment 42 (read carefully, it's different from what was discussed at the standup), it seems `simulatedCameraEffects` should happen pretty early (Helen says "on press"), so maybe just after:
> 
>   yield dispatch({type: TAKE_SCREENSHOT_START});

Yes, it's different from what she said before. I think it's safe having the `simulatedCameraEffects` here; I put there because everything is used in the same scope, otherwise I have to do something like:
```js
    return function* (dispatch, getState) {
      yield dispatch({ type: TAKE_SCREENSHOT_START });
      
      let iframe = document.querySelector("iframe");
      simulateCameraEffects(iframe);

      window.requestAnimationFrame(async(function* () {
        let data = createScreenshotFor(iframe);

        yield saveToFile(data, getFileName());

        dispatch({ type: TAKE_SCREENSHOT_END });
      }));
    };

```
I think is cleaner and readable as is, but if you prefer the code above I don't have strong opinion about that. Let me know!

> In Helen's comment 37, she requested `cursor: pointer` for all buttons.

She asked if it's possible; and yes, it's possible but I didn't change that because I think we should discuss this topic, despite this bug.
I'm saying that because currently I'm not aware of any of our UI that uses `pointer` for buttons in devtools or Firefox, so we won't be consistent with our UI, that I think is important. Maybe there is a bug about changing them to `pointer` for all our buttons?

Comment 46

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

https://reviewboard.mozilla.org/r/43297/#review40941

> As explained above, is there in order to ensure that react components have time to render themselves; otherwise the handle won't be hidden when the snapshot is taken.

Okay, I get it now.  It think it's fine this way, but add a comment about why it is here.

> Yes, it's different from what she said before. I think it's safe having the `simulatedCameraEffects` here; I put there because everything is used in the same scope, otherwise I have to do something like:
> ```js
>     return function* (dispatch, getState) {
>       yield dispatch({ type: TAKE_SCREENSHOT_START });
>       
>       let iframe = document.querySelector("iframe");
>       simulateCameraEffects(iframe);
> 
>       window.requestAnimationFrame(async(function* () {
>         let data = createScreenshotFor(iframe);
> 
>         yield saveToFile(data, getFileName());
> 
>         dispatch({ type: TAKE_SCREENSHOT_END });
>       }));
>     };
> 
> ```
> I think is cleaner and readable as is, but if you prefer the code above I don't have strong opinion about that. Let me know!

I think it is fine to keep `simulateCameraEffects` inside the rAF scope.  Mainly I just thought it should come before the call to `createScreenshotFor`, so you could do that either inside or outside the rAF.

> She asked if it's possible; and yes, it's possible but I didn't change that because I think we should discuss this topic, despite this bug.
> I'm saying that because currently I'm not aware of any of our UI that uses `pointer` for buttons in devtools or Firefox, so we won't be consistent with our UI, that I think is important. Maybe there is a bug about changing them to `pointer` for all our buttons?

Okay, I agree consistency is important, so we should discuss this separately from this bug.  It seems more like a general UX task for all of DevTools.

Comment 47

[Matteo Ferretti [:zer0] [:matteo]]

Comment on attachment 8736458 [details]
MozReview Request: Bug 1239461 - Screenshot button for taking a screenshot of the current viewport; r=jryans

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

Comment 48

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8736458 [details]
MozReview Request: Bug 1239461 - Screenshot button for taking a screenshot of the current viewport; r=jryans

https://reviewboard.mozilla.org/r/43297/#review41013

Looks good overall, but still need to tweak `simulateCameraEffects` ordering.

::: devtools/client/responsive.html/actions/screenshot.js:80
(Diff revision 4)
> +      // can be properly render after the action dispatched above
> +      window.requestAnimationFrame(async(function* () {
> +        let iframe = document.querySelector("iframe");
> +        let data = createScreenshotFor(iframe);
> +
> +        simulateCameraEffects(iframe);

See my previous comment about this:

I think it is fine to keep `simulateCameraEffects` inside the rAF scope.  Mainly I just thought it should come before the call to `createScreenshotFor`, so you could do that either inside or outside the rAF.

Comment 49

[Matteo Ferretti [:zer0] [:matteo]]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #48)

> Looks good overall, but still need to tweak `simulateCameraEffects` ordering.

We sorting that out on IRC – changing the ordering of `simulateCameraEffects` will makes the screenshot having the effect applied, resulting in a white image.

Comment 50

[Matteo Ferretti [:zer0] [:matteo]]

Comment on attachment 8736458 [details]
MozReview Request: Bug 1239461 - Screenshot button for taking a screenshot of the current viewport; r=jryans

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

Comment 51

[Matteo Ferretti [:zer0] [:matteo]]

The last update was about a left over in the test, I fixed that.

Here the try build: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7d3389952d15&selectedJob=19035000

Comment 52

[Pulsebot]

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

Comment 53

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e003ad2de364

Comment 54

[Mihai Boldan, Desktop QA [:mboldan]]

I managed to test the screenshot button and I confirm that the actions mentioned below are working correctly:
- a user takes a snapshot of a large viewport.
- button turns active (blue) on press, and remains blue until screenshotting process is complete.
- also on press, white flash and camera noise play.
- when file saves to disc, blue animation to downloads folder happens.
- button becomes gray and is no longer disabled.

During tests I encountered a crash on Ubuntu OS - Bug 1263601.
Also another issue is related to white screenshots - Bug 1262594 

Since the found issues related to this bug were logged separately, I am marking this bug Verified - Fixed.

The tests were performed with FF 48.0a1 (2016-04-10) on Windows 7 x64, Ubuntu 14.04 x86 and on Mac OS X 10.10.4.