830064 - The new downloads view in the Places Library does not support drag and drop

Status: VERIFIED FIXED

(Firefox :: Downloads Panel, defect)

Description

[Alice0775 White]

Build Identifier:
http://hg.mozilla.org/mozilla-central/rev/1761f4a9081c
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:21.0) Gecko/20130112 Firefox/21.0 ID:20130112030947

Drag & drop links into new downloads manager to download files.

Firefox18 can do that.
But Aurora20.0a2 and Nifgtly21,0a1 cannot,

This is regression, I think.


Steps to reproduce.
1. Open any page
2. Open Downloads (Tools > Downloads Ctrl+J)
3. Drag & drop links into the new downloads manager

Actual results:
Not allowed to drop link
Cannot to save

Expected results:
Allowed to drop links and saved to folder

Comment 1

[Alice0775 White]

Err 
S/Firefox18 can do that/Firefox17 can do that/

Comment 2

[Marco Bonardo [:mak]]

I don't think this blocks the feature since you can drop links on the button that is always visible in the window.
Surely it's expected that we keep supporting that in the view at some time.

Comment 3

[Marco Bonardo [:mak]]

the view code lives in downloads component and it's easier to track this there for now.

Comment 4

[Lukas Blakk [:lsblakk] use ?needinfo]

Can we get confirmation if this is indeed a regression and then also what the user impact is here - are we breaking a popular use case?

Comment 5

[Alice0775 White]

I mean this is feature regression

Steps to reproduce:
1. Open any page
2. Open Downloads (Tools > Downloads Ctrl+J)
3. Drag & drop links into the new downloads manager

Regression window
Fails due to Bug830066:
http://hg.mozilla.org/integration/mozilla-inbound/rev/661960a9cf89
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:20.0) Gecko/20121228 Firefox/20.0 ID:20121228014553
Bad(lack of drag&drop feature):
http://hg.mozilla.org/integration/mozilla-inbound/rev/9119f282e0c0
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:20.0) Gecko/20121228 Firefox/20.0 ID:20121228032951
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=661960a9cf89&tochange=9119f282e0c0

Comment 6

[Marco Bonardo [:mak]]

it's a regression (tracked in Bug830066) for the old manager, a not-yet-implemented functionality for the new manager, I don't think it's a popular use-case, many drag&drop actions are undiscoverable per definition and it's often easier to drag the link to the downloads button, or to alt+click, or to right click and save as, than to drag it across different windows.

Comment 7

[Asaf Romano (gone)]

Attachment: port the old dm functionality

I don't like it much, but it works just as good as the old DM did.

Comment 8

[Marco Bonardo [:mak]]

Comment on attachment 702775 [details] [diff] [review]
port the old dm functionality

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

::: browser/components/downloads/content/allDownloadsViewOverlay.js
@@ +39,5 @@
>    "downloadsCmd_openReferrer", "downloadsCmd_clearDownloads"];
>  
>  const NOT_AVAILABLE = Number.MAX_VALUE;
>  
> +function DownloadURL(aURL, aFileName) {

javadoc, please.
and specify that aFileName is optional

@@ +44,5 @@
> +  // For private browsing, try to get document out of the most recent browser
> +  // window, or provide our own if there's no browser window.
> +  let browserWin = RecentWindow.getMostRecentBrowserWindow();
> +  let initiatingDoc = browserWin ? browserWin.document : document;
> +  saveURL(aURL, aFileName, null, true, true, undefined, initiatingDoc); 

trailing space

off-topic: this saveURL API is totally unreadable! we should modify it to also accept an object as second param!

@@ +699,5 @@
>    matchesSearchTerm: function DES_matchesSearchTerm(aTerm) {
>      if (!aTerm)
>        return true;
>      aTerm = aTerm.toLowerCase();
> +    return this.getDownloadMetaData().displayName.toLowerCase().indexOf(aTerm) != -1 ||

ha, this time I'm right this should use .contains()!

@@ +704,1 @@
>             this.downloadURI.toLowerCase().indexOf(aTerm) != -1;

as well as here

@@ +1491,5 @@
> +
> +  onDragStart: function DPV_onDragStart(aEvent) {
> +    let selectedItem = this._richlistbox.selectedItem;
> +    if (!selectedItem)
> +      return;

worth a comment we don't support multiple selection drags (and just fallback to the first selected item, that is what I suppose selectedItem does?)

@@ +1497,5 @@
> +    if (!("filePath" in metaData))
> +      return;
> +    let file = new FileUtils.File(metaData.filePath);
> +    if (!file.exists())
> +      return;

sigh, we need async dragstart!

@@ +1524,5 @@
> +    // redownload already downloaded file.
> +    if (dt.mozGetDataAt("application/x-moz-file", 0))
> +      return;
> +
> +    let url = dt.getData("URL");

I think you can use

let name = {};
let url = Services.droppedLinkHandler.dropLink(aEvent, name);
if (url)
  DownloadURL(url, name.value)

Comment 9

[Marco Bonardo [:mak]]

Comment on attachment 702775 [details] [diff] [review]
port the old dm functionality

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

::: browser/components/downloads/content/allDownloadsViewOverlay.js
@@ +39,5 @@
>    "downloadsCmd_openReferrer", "downloadsCmd_clearDownloads"];
>  
>  const NOT_AVAILABLE = Number.MAX_VALUE;
>  
> +function DownloadURL(aURL, aFileName) {

javadoc, please.
and specify that aFileName is optional

@@ +44,5 @@
> +  // For private browsing, try to get document out of the most recent browser
> +  // window, or provide our own if there's no browser window.
> +  let browserWin = RecentWindow.getMostRecentBrowserWindow();
> +  let initiatingDoc = browserWin ? browserWin.document : document;
> +  saveURL(aURL, aFileName, null, true, true, undefined, initiatingDoc); 

trailing space

off-topic: this saveURL API is totally unreadable! we should modify it to also accept an object as second param!

@@ +699,5 @@
>    matchesSearchTerm: function DES_matchesSearchTerm(aTerm) {
>      if (!aTerm)
>        return true;
>      aTerm = aTerm.toLowerCase();
> +    return this.getDownloadMetaData().displayName.toLowerCase().indexOf(aTerm) != -1 ||

ha, this time I'm right this should use .contains()!

@@ +704,1 @@
>             this.downloadURI.toLowerCase().indexOf(aTerm) != -1;

as well as here

@@ +1491,5 @@
> +
> +  onDragStart: function DPV_onDragStart(aEvent) {
> +    let selectedItem = this._richlistbox.selectedItem;
> +    if (!selectedItem)
> +      return;

worth a comment we don't support multiple selection drags (and just fallback to the first selected item, that is what I suppose selectedItem does?)

@@ +1497,5 @@
> +    if (!("filePath" in metaData))
> +      return;
> +    let file = new FileUtils.File(metaData.filePath);
> +    if (!file.exists())
> +      return;

sigh, we need async dragstart!

@@ +1524,5 @@
> +    // redownload already downloaded file.
> +    if (dt.mozGetDataAt("application/x-moz-file", 0))
> +      return;
> +
> +    let url = dt.getData("URL");

I think you can use

let name = {};
let url = Services.droppedLinkHandler.dropLink(aEvent, name);
if (url)
  DownloadURL(url, name.value)

Comment 10

[Asaf Romano (gone)]

Attachment: address comments

I also removed the restriction (enforced by a js exception) not to call getDownloadMetaData for inactive shells. We forgot we do that during search.

I'll check this in as soon as inbound reopens.

Comment 11

[Asaf Romano (gone)]

Attachment: add back the name variable

oops

Comment 12

[Asaf Romano (gone)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/31a9be21a0a4

Comment 13

[Lukas Blakk [:lsblakk] use ?needinfo]

Since this is for the old manager and not a popular use case, not tracking but would consider a nomination for uplift if the fix is low risk.

Comment 14

[Marco Bonardo [:mak]]

(In reply to Lukas Blakk [:lsblakk] from comment #13)
> Since this is for the old manager and not a popular use case, not tracking
> but would consider a nomination for uplift if the fix is low risk.

This is for the new manager!

Comment 15

[Asaf Romano (gone)]

Comment on attachment 702891 [details] [diff] [review]
add back the name variable

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Downloads panel feature
User impact if declined: missing UI functionality comparing to the old download manager. 
Testing completed (on m-c, etc.): tbd, but there are actually no tests for this code area yet.
Risk to taking this patch (and alternatives if risky): limited to the feature.
String or UUID changes made by this patch: none.

Comment 16

[Asaf Romano (gone)]

Resetting tracking flags.

Comment 17

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/31a9be21a0a4

Comment 18

[Marco Bonardo [:mak]]

This also fixes a regression in the Library, so approval is also required for proper functionality.

Comment 19

[Alex Keybl [:akeybl]]

We've taken the fix, and this is a regression from the old manager, so might as well track.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/6766940f3ac8

Comment 21

[Simona Badau, Desktop QA]

Verified as fixed on the latest Nightly and Aurora - the new downloads view in Library supports drag and drop.

Verified on Windows 7, Ubuntu 12.04 and Mac OS X 10.8:

Mozilla/5.0 (Windows NT 6.1; rv:21.0) Gecko/20130128 Firefox/21.0 Build ID:20130128030943
Mozilla/5.0 (Windows NT 6.1; rv:20.0) Gecko/20130128 Firefox/20.0 Build ID:20130128042018	

Mozilla/5.0 (X11; Linux i686; rv:21.0) Gecko/20130128 Firefox/21.0 Build ID:20130128030943
Mozilla/5.0 (X11; Linux i686; rv:20.0) Gecko/20130123 Firefox/20.0 Build ID:20130128042018

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:21.0) Gecko/20130128 Firefox/21.0	Build ID:20130128030943
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:20.0) Gecko/20130128 Firefox/20.0	Build ID:20130128042018