No feedback about the saved screenshot via the screenshot command button

VERIFIED FIXED in Firefox 40

Status

defect
P2
normal
VERIFIED FIXED
5 years ago
9 months ago

People

(Reporter: Optimizer, Assigned: jryans)

Tracking

unspecified
Firefox 40
Dependency tree / graph

Firefox Tracking Flags

(firefox40 verified)

Details

(Whiteboard: [polish-backlog][difficulty=easy])

Attachments

(1 attachment, 1 obsolete attachment)

Reporter

Description

5 years ago
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 :)
Reporter

Updated

5 years ago
Depends on: 991045

Comment 1

5 years ago
It should at least show up in the downloads panel (with the downloads animation happening).
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

5 years ago
(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.
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

5 years ago
(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.
(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

5 years ago
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.
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.
Component: Developer Tools → Developer Tools: Graphic Commandline and Toolbar
OS: Windows 7 → All
Hardware: x86_64 → All

Updated

5 years ago
Duplicate of this bug: 1097120
Assignee

Updated

4 years ago
Whiteboard: [devedition-40]
Assignee

Updated

4 years ago
Assignee: nobody → jryans
Status: NEW → ASSIGNED
Assignee

Comment 10

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

Pull down this commit:

hg pull -r 796ba0f324ec590bb683d4a07e030ce4ef60120c https://reviewboard-hg.mozilla.org/gecko/
Attachment #8588410 - Flags: review?(bgrinstead)
Assignee

Comment 11

4 years ago
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 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?
Attachment #8588410 - Flags: feedback?(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 on attachment 8588410 [details]
MozReview Request: bz://1018619/jryans

Sorry, but going to have to wait for Bug 1128988 before proceeding with this
Attachment #8588410 - Flags: review?(bgrinstead)
Attachment #8588410 - Flags: feedback?(jwalker)
Depends on: 1128988
Assignee

Updated

4 years ago
Whiteboard: [devedition-40] → [devedition-40][difficulty=easy]
Setting devedition-40 bugs to P2, filter on C17C996C-A0BE-4153-8057-FAB642D0746D
Priority: -- → P2
Assignee

Updated

4 years ago
Attachment #8588410 - Flags: review?(bgrinstead)
Assignee

Comment 16

4 years ago
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/
Assignee

Comment 17

4 years ago
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 on attachment 8588410 [details]
MozReview Request: bz://1018619/jryans

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

Nice!
Attachment #8588410 - Flags: review?(bgrinstead) → review+
(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.
Assignee

Comment 20

4 years ago
(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.
Assignee

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/ab89cbfcd3e6
Keywords: checkin-needed
Whiteboard: [devedition-40][difficulty=easy] → [devedition-40][difficulty=easy][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/ab89cbfcd3e6
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Whiteboard: [devedition-40][difficulty=easy][fixed-in-fx-team] → [devedition-40][difficulty=easy]
Target Milestone: --- → Firefox 40
Assignee

Comment 23

4 years ago
Attachment #8588410 - Attachment is obsolete: true
Attachment #8618167 - Flags: review+
Whiteboard: [devedition-40][difficulty=easy] → [polish-backlog][difficulty=easy]
Assignee

Updated

4 years ago
Blocks: 1018722

Updated

4 years ago
Depends on: 1220341

Updated

4 years ago
No longer blocks: 1018722
Duplicate of this bug: 1018722
Assignee

Updated

3 years ago
See Also: → 1243525
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.

Updated

3 years ago
Status: RESOLVED → VERIFIED

Updated

Last year
Product: Firefox → DevTools

Updated

9 months ago
Product: DevTools → DevTools Graveyard
You need to log in before you can comment on or make changes to this bug.