Closed Bug 842553 Opened 7 years ago Closed 7 years ago

Double clicking an in-progress download in the Library opens its containing folder

Categories

(Firefox :: Downloads Panel, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 22
Tracking Status
firefox19 --- unaffected
firefox20 --- verified
firefox21 --- verified

People

(Reporter: simona.marcu, Assigned: mano)

References

Details

Attachments

(1 file, 2 obsolete files)

Mozilla/5.0 (Windows NT 6.1; rv:21.0) Gecko/20130218 Firefox/21.0
Build ID: 20130218031106

The behavior when double clicking on a paused/in progress download is different in the New Downloads Manager compared with the Old Downloads Manager. 

Steps to reproduce: 
1. Launch Firefox
2. Download a large file (for e.g Ubuntu cd installer: http://www.ubuntu.com/download/desktop/thank-you?release=latest&bits=32&distro=desktop&status=zeroc)
3. Open the panel -> pause the download -> click on Show All Downloads
4. While in the Downloads View -> double click on the paused download started in step 2.
5. Repeat step 4.

Expected results:
- in step 4: double clicking on a paused download -> resumes the download.
- in step 5: double clicking on a download in progress -> pauses the download.

Actual results:
- in step 5: double clicking on a download in progress -> opens the containing folder. 

Note:
- reproducible on the latest Nightly
- reproducible on the latest Aurora:
Mozilla/5.0 (Windows NT 6.1; rv:20.0) Gecko/20130218 Firefox/20.0 (20130218042018)
- the behavior is reproducible ever since February 6th, before this - double clicking on a paused/in progress download in the Downloads View (in Library) - did nothing.

Last good nightly: 2013-01-05
First bad nightly: 2013-01-06

Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=d8ca3e1c469e&tochange=20d1a5916ef6
Paolo the default action for "downloading" status seems to be "show", was this done on purpose?
Looks like we just copied that in the Library:
http://mxr.mozilla.org/mozilla-central/source/browser/components/downloads/content/allDownloadsViewOverlay.js#727
Flags: needinfo?(paolo.mozmail)
Summary: Double clicking in the Downloads Manager → Double clicking an in-progress download in the Library opens its containing folder
Regression window(cached m-i)
Good:
http://hg.mozilla.org/integration/mozilla-inbound/rev/07fa18b4c450
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:20.0) Gecko/20130105 Firefox/20.0 ID:20130105061001
Bad:
http://hg.mozilla.org/integration/mozilla-inbound/rev/04557efa1fd9
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:20.0) Gecko/20130105 Firefox/20.0 ID:20130105070201
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=07fa18b4c450&tochange=04557efa1fd9

Suspected: 
 8f9693d92791	Asaf Romano — Bug 826425 - double-click on finished downloads in Library view doesn't open them. r=mak.
Blocks: 826425
I'll implement whatever ux/paolo says.
Assignee: nobody → mano
Status: NEW → ASSIGNED
(In reply to Marco Bonardo [:mak] (Away Feb 22) from comment #1)
> Paolo the default action for "downloading" status seems to be "show", was
> this done on purpose?

The reason is that we didn't want pause and resume to be primary interactions,
when the panel still supported double click.

We didn't revise the list of default actions when we changed the design to have
the single button and no double click interaction. The reason why double-clicking
a paused download resumes it, instead of opening the containing folder, is that
there used to be a "resume" button in that case, that is no longer there.

I'm not sure if we should preserve the old behavior here - we may want to reserve
double click exclusively for item activation. The "pause" function can already be
accessed using the space bar or the context menu. If opening the containing folder
is confusing, we may simply do no action.

Stephen, what do you think?
Flags: needinfo?(paolo.mozmail) → needinfo?(shorlander)
yes, my options where "pause" or "no action"
the old DM uses "pause"
to be noticed that double clicking a paused download resumes it
My 2 cents (and well after some hyper-inflation): There's almost no use case for opening the downloads folder for an in-progress download: all one could do is damage the downloaded file by moving it too early etc.
I'm not suggesting to to disable the command, of course, but I cannot see the reason to make it the default command.

Thus I'm for reversing to the old DM behavior.
I suppose the old DM behavior is also good for users that we are migrating to a new UI and are used to the old behavior.
May you create a patch that does that for now? Could probably be a oneliner for both downloads.js and allDownloadsOverlay.js, but likely in downloads.js we may also want to check isCommandEnabled before calling doCommand
and if we change our mind will be just matter of removing that single line.
As a side note, Seamonkey on double-click has a no-op, but it has a separate button for pause/resume.
[17:09]	mak	shorlander: the old DM was pausing the download. though another valid option could be "nothing"
[17:10]	shorlander	mak: Yeah I think nothing

let's go for "nothing".
Flags: needinfo?(shorlander)
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → WONTFIX
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
Status: REOPENED → ASSIGNED
Mano, do you have an ETA for the patch?
Flags: needinfo?(mano)
Comment on attachment 718892 [details] [diff] [review]
option 2: only change the behavior for double click

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

::: browser/components/downloads/content/allDownloadsViewOverlay.js
@@ +710,5 @@
>    },
>  
>    // Handles return kepress on the element (the keypress listener is
>    // set in the DownloadsPlacesView object).
> +  doDefaultCommand: function DES_doDefaultCommand(aIsDoubleClick = false) {

I don't think this works if you don't pass true in the double click handler :)

Btw, I think we'll take the other patch that fixes both.
Attachment #718892 - Flags: review?(mak77) → review-
Comment on attachment 718891 [details] [diff] [review]
option 1: make it so both double-click and enter don't open the containing folder for in-progress downloads

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

please also fix the panel view
http://mxr.mozilla.org/mozilla-central/source/browser/components/downloads/content/downloads.js#1461
while there I think would make sense to null check the command... and likely DVC_doCommand should check commandenabled in the first if...
Attachment #718891 - Flags: review?(mak77) → feedback+
Attached patch option 1Splinter Review
(It's the caller job to check if the command is enabled)
Attachment #718891 - Attachment is obsolete: true
Attachment #720636 - Flags: review?(mak77)
Comment on attachment 720636 [details] [diff] [review]
option 1

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

Thanks, please land this asap so we can ask approval sooner.

::: browser/components/downloads/content/downloads.js
@@ +1472,2 @@
>        }.apply(this);
>        // Invoke the command.

nit: this comment looks pointless.
Attachment #720636 - Flags: review?(mak77) → review+
Attachment #718892 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/80c2a7a350fe
Status: ASSIGNED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 22
Comment on attachment 720636 [details] [diff] [review]
option 1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): downloads panel feature, this has been exposed by adding double-click handler in bug 826425, it's not a regression but a mis-handling of the operation
User impact if declined: double clicking on in progress downloads shows the download in its containing folder, paving the way to possible download corruption if the user interacts with the file at this wrong time
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): the patch removes the command when double clicking on an in progress download, it's a trivial change
String or UUID changes made by this patch: none
Attachment #720636 - Flags: approval-mozilla-beta?
Attachment #720636 - Flags: approval-mozilla-aurora?
Keywords: verifyme
QA Contact: simona.marcu
Attachment #720636 - Flags: approval-mozilla-beta?
Attachment #720636 - Flags: approval-mozilla-beta+
Attachment #720636 - Flags: approval-mozilla-aurora?
Attachment #720636 - Flags: approval-mozilla-aurora+
Verified as fixed on Firefox 20 beta 5 - double clicking an in-progress download in the Library does nothing.

Verified on Windows 7 64-bit, Ubuntu 12.10 and Mac OS X 10.8:
Build ID: 20130313170052
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:20.0) Gecko/20100101 Firefox/20.0
Mozilla/5.0 (X11; Linux i686; rv:20.0) Gecko/20100101 Firefox/20.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:20.0) Gecko/20100101 Firefox/20.0
Verified as fixed on Firefox 21 Beta1(Build ID: 20130401192816)
- double clicking an in-progress download in the Library does nothing.
- double clicking on a paused download in the Library resumes the download.

Mozilla/5.0 (X11; Linux i686; rv:21.0) Gecko/20100101 Firefox/21.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:21.0) Gecko/20100101 Firefox/21.0
Mozilla/5.0 (Windows NT 6.2; rv:21.0) Gecko/20100101 Firefox/21.0
Based on Comment 24, setting the status for Firefox 21 to verified.
mass remove verifyme requests greater than 4 months old
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.