1018619 - No feedback about the saved screenshot via the screenshot command button

Status: VERIFIED FIXED

(DevTools Graveyard :: Graphic Commandline and Toolbar, defect, P2)

Description

[Girish Sharma [:Optimizer]]

We should show a message saying that screenshot was successfully taken and stored at this location. Just like what we show for the similar gcli command.

We can have a arrow panel appear on top of the command button in the toolbar.

Maybe add the screenshot there too :)

Comment 1

[Tim Nguyen :ntim]

It should at least show up in the downloads panel (with the downloads animation happening).

Comment 2

[Joe Walker [:jwalker]]

I think whatever notification we do should not require user action to dismiss it.
We've already got an animation to highlight the download button when a download starts, maybe we could reuse that?

Comment 3

[Tim Nguyen :ntim]

(In reply to Joe Walker [:jwalker] from comment #2)
> I think whatever notification we do should not require user action to
> dismiss it.
> We've already got an animation to highlight the download button when a
> download starts, maybe we could reuse that?

Yes, and it should also appear in the downloads panel too. Currently it does absolutely nothing. You might even think it doesn't work.

Comment 4

[Mahdi Dibaiee [:mahdi][:mdibaiee]]

The gcli screenshot command has it's own notification and doesn't make use of downloads panel, maybe we should use the same? Having debug screenshots in downloads panel doesn't seem right to me.

Comment 5

[Tim Nguyen :ntim]

(In reply to Mahdi Dibaiee [:mahdi] from comment #4)
> The gcli screenshot command has it's own notification and doesn't make use
> of downloads panel, maybe we should use the same? Having debug screenshots
> in downloads panel doesn't seem right to me.

THe responsive mode screenshot button makes use of the downloads panel, and I think that makes more sense, since you might want to reupload the screenshot later without taking the hassle of looking in your downloads directory. If it was in the downloads panel, you would just need to drag and drop the screenshot into the file upload input.

Comment 6

[Mahdi Dibaiee [:mahdi][:mdibaiee]]

(In reply to Tim Nguyen [:ntim] from comment #5) 
> THe responsive mode screenshot button makes use of the downloads panel, and
> I think that makes more sense, since you might want to reupload the
> screenshot later without taking the hassle of looking in your downloads
> directory. If it was in the downloads panel, you would just need to drag and
> drop the screenshot into the file upload input.

I guess you're right, accessibility+.

Do you think we need to open a bug for gcli to use downloads panel?

Comment 7

[sjw]

If it uses the same as responsive mode screenshot button you are also able to change the download folder or set, that you will be asked where it would be saved over the global download settings.
These would be a huge improvement.

Comment 8

[Brian Grinstead [:bgrins]]

I agree about using the same UI that responsive mode does for saving screenshots.  This would be a big improvement.

> Do you think we need to open a bug for gcli to use downloads panel?

I think this bug is a fine place to make this change.

Comment 10

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

Attachment: MozReview Request: bz://1018619/jryans

/r/6649 - Bug 1018619 - Feedback for screenshot button. r=bgrins

Pull down this commit:

hg pull -r 796ba0f324ec590bb683d4a07e030ce4ef60120c https://reviewboard-hg.mozilla.org/gecko/

Comment 11

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

I've reused the path that Responsive Design takes.  Basically, I just removed most of the code GCLI was using and it worked. ;)

Also, I fixed a latent bug with the "selector" option not being passed when a delay is used.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c068e9e7d541

Comment 12

[Brian Grinstead [:bgrins]]

Comment on attachment 8588410 [details]
MozReview Request: bz://1018619/jryans

Joe, how should we proceed with this change to the screenshot command with regards to Bug 1128988?  Wait for remote support to land and rebase this patch on top of that, or vice versa?

Comment 13

[Joe Walker [:jwalker]]

(In reply to Brian Grinstead [:bgrins] from comment #12)
> Comment on attachment 8588410 [details]
> MozReview Request: bz://1018619/jryans
> 
> Joe, how should we proceed with this change to the screenshot command with
> regards to Bug 1128988?  Wait for remote support to land and rebase this
> patch on top of that, or vice versa?

Here's what the screenshot command looks like now:
  https://github.com/joewalker/gecko-dev/blob/pr10-tip/toolkit/devtools/gcli/commands/screenshot.js

There are 1 or 2 changes that we're going to need to make on top of that:

* Split the command into 2, adding a hidden screenshot_chrome command that uses the chromeWindow, and has runAt:client (where runAt:server is used for the normal 'screenshot' command) and have 'screenshot --chrome' call the 'screenshot_chrome' command.
* (Maybe) make the converter smarter so it handles things like --clipboard, but this depends a bit on what we can do from the content process.

Opening a finder window is the job of the converter which really should be done in the chrome process, so maybe the best solution is to make it easy to use the converter from the screenshot button. Can we land bug 1128988 and then think again?

Comment 14

[Brian Grinstead [:bgrins]]

Comment on attachment 8588410 [details]
MozReview Request: bz://1018619/jryans

Sorry, but going to have to wait for Bug 1128988 before proceeding with this

Comment 15

[Jeff Griffiths (:canuckistani) (:⚡︎)]

Setting devedition-40 bugs to P2, filter on C17C996C-A0BE-4153-8057-FAB642D0746D

Comment 16

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

Comment on attachment 8588410 [details]
MozReview Request: bz://1018619/jryans

/r/6649 - Bug 1018619 - Feedback for screenshot button. r=bgrins

Pull down this commit:

hg pull -r 39ba75e331b8baa5182d5e3540e80f36c2ca9061 https://reviewboard-hg.mozilla.org/gecko/

Comment 17

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

This version works with e10s using the new runAt support.  We now get feedback for chrome and content, e10s on or off.

A few notes: clipboard and imgur options don't seem to work for me, but they also don't work before my patch, so I don't think I broke them.

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=134a1da2e6e4

Comment 18

[Brian Grinstead [:bgrins]]

Comment on attachment 8588410 [details]
MozReview Request: bz://1018619/jryans

https://reviewboard.mozilla.org/r/6647/#review6505

Nice!

Comment 19

[Brian Grinstead [:bgrins]]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #17)
> A few notes: clipboard and imgur options don't seem to work for me, but they
> also don't work before my patch, so I don't think I broke them.

I tested and the --clipboard option does work for me with the patch applied.  I wonder if it'd work for you in a clean profile.

The imgur option is intentionally hidden and isn't working right now.

Comment 20

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

(In reply to Brian Grinstead [:bgrins] from comment #19)
> (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #17)
> > A few notes: clipboard and imgur options don't seem to work for me, but they
> > also don't work before my patch, so I don't think I broke them.
> 
> I tested and the --clipboard option does work for me with the patch applied.
> I wonder if it'd work for you in a clean profile.

Oh, I guess it does work for me too.  I was expecting the data URI to go there, but it's actually the image data.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

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

Comment 22

[Ryan VanderMeulen [:RyanVM]]

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

Comment 24

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

Attachment: MozReview Request: Bug 1018619 - Feedback for screenshot button. r=bgrins

Comment 26

[Nawaf Chowdhury Akash]

I have reproduced this bug with nightly 32.0a1 (2014-05-31) on windows 10, 32 bit.

The bug’s fix is now verified on Latest Nightly 50.0a1 and Latest Aurora 49.0a2.

Build ID:20160709030233.
User Agent:Mozilla/5.0 (Windows NT 10.0; rv:50.0) Gecko/20100101 Firefox/50.0.

Build ID:20160710004020.
User Agent:Mozilla/5.0 (Windows NT 10.0; rv:49.0) Gecko/20100101 Firefox/49.0.