900514 - Some minor tweaks to Drag and Drop in Downloads UI

Status: RESOLVED FIXED

(SeaMonkey :: Download & File Handling, defect)

Description

[Philip Chee]

Relevant Firefox Bugs:
Bug 567377 - Drag&drop of download items to desktop/external applications doesn't work on Linux
Bug 615515 - Dragged out items from Download manager to a FF tab have a blurry thumbnail
Bug 830066 - Use proper document for dropping links on toolkit download UI

Comment 1

[Philip Chee]

Attachment: Patch v1.0 Tweaks.

> -    dt.setData("text/uri-list", url + "\r\n");
> -    dt.setData("text/plain", url + "\n");
> +    dt.setData("text/uri-list", url);
> +    dt.setData("text/plain", url);

From: https://bugzilla.mozilla.org/show_bug.cgi?id=567377#c3

> I think the spec refers to CRLF in the sense of "where line termination is 
> needed it should be a CRLF", though that's quite unclear. Also HTML5 dnd specs 
> uses "separated by CRLF", not "terminated by CRLF", there's nothing to 
> separate here.

> What I can say for sure is we never added newline terminators, and our 
> documentation does the same, so if it works regardless I'd left them out.
> Same comment is valid for the other changes, so could you please verify if it 
> works without the trailing terminators?

> +    dt.setDragImage(gDownloadStatus, 16, 16);

Bug 615515 Comment 5 suggests that:
> Using addElement instead of setDragImage would be more correct here.

However I couldn't get this to work. Probably because we use a generated tree and the individual columns in a row are not accessible. I worked around this by pointing the drag image to the statusbar text so you get the file name as the drag image.

I also tried the following with mixed results (the drag image worked but was sort of blurry).

>     var dragIcon = document.createElement("image");
>     var src = "moz-icon://" + file.leafName + "?size=16";
>     dragIcon.setAttribute("src", src);
>     dt.setDragImage(dragIcon, 0, 0);

>      var dt = aEvent.dataTransfer;
> +    // If dragged item is from our source, do not try to
> +    // redownload already downloaded file.
> +    if (dt.mozGetDataAt("application/x-moz-file", 0))
> +      return;

From Bug 567377 Comment 5:

> When you start to drag from download manager and you drop also on download 
> manager file is "downloaded" from file://... again because Mozilla accepts 
> drops of text/uri-list or text/plain

> -    if (url)
> -      saveURL(url, name, null, true, true, null, document);
> +    if (url) {
> +      let sourceDoc = dt.mozSourceNode ? dt.mozSourceNode.ownerDocument : document;
> +      saveURL(url, name || url, null, true, true, null, sourceDoc);

Use proper document for dropping links on toolkit download UI.
See Bug 830066 Comment 0:

> Error: TypeError: aInitiatingDocument is undefined
> Source file: chrome://global/content/contentAreaUtils.js
> Line: 340

Comment 2

[Ian Neal]

Comment on attachment 784414 [details] [diff] [review]
Patch v1.0 Tweaks.

This seems to work okay, but I'd be happier if Neil had a quick look at it too.

Comment 3

[Philip Chee]

I've done a bit more polish. New patch coming.

Comment 4

[Philip Chee]

Attachment: Patch v1.1 More tweaks. Has r=IanN

> This seems to work okay, but I'd be happier if Neil had a quick look at it too.
OK.

Changes since the last patch:

> -    dt.setData("text/uri-list", url + "\r\n");
> -    dt.setData("text/plain", url + "\n");
> +    dt.setData("text/uri-list", url);
> +    dt.setData("text/plain", url);

In bug 192728 comment 10, Jens said:
> <http://www.rfc-editor.org/rfc/rfc2483.txt>
> (Notes that CRLF is required. Something that ROX will happily tell you, too.)"

So I reverted this change. However the Toolkit reviewer in Bug 567377 comment 3 Read the RFCs differently and decided that newlines weren't required.

I Iconified the statusbaritem to use as the drag image:

> -    gDownloadStatus.label = GetFileFromString(selItemData.file).path;
> +    var file = GetFileFromString(selItemData.file);
> +    gDownloadStatus.label = file.path;
> +    gDownloadStatus.src = "moz-icon://" + file.leafName + "?size=16";
>    } else {
>      gDownloadStatus.label = "";
> +    gDownloadStatus.src = "";
.....
> +    dt.setDragImage(gDownloadStatus, 16, 16);
.....
> -    <statusbarpanel id="statusbar-display" flex="1"/>
> +    <statusbarpanel id="statusbar-display" class="statusbarpanel-iconic-text"
> +                    flex="1" pack="start"/>


> -    if (types.contains("text/uri-list") ||
> -        types.contains("text/x-moz-url") ||
.....
> +    if (types.contains("text/x-moz-url") ||
> +        types.contains("text/uri-list") ||

From: https://developer.mozilla.org/en-US/docs/DragDrop/Recommended_Drag_Types

> You may also see data using the text/x-moz-url type which is a Mozilla 
> specific type. If it appears, it should be used before the text/uri-list type. 
> It holds the URL of the link followed by the title of the link, separated by a 
> linebreak. For example:

> +    // This is a local file, Don't allow drop on the download manager.
> +    if (dt.mozGetDataAt("application/x-moz-file", 0))
> +      return;

This changes the drop effect to the slash-circle cursor when the mouse is over the DM.

> +    var sourceNode = dt.mozSourceNode;
> +    var sourceDoc = sourceNode ? sourceNode.ownerDocument : document;
> +    // Disallow droping a download manager node back on the manager.
> +    if (sourceDoc == aEvent.originalTarget.ownerDocument)
> +      return;
Which is more correct? target or originalTarget? Also I'm not sure if this is still needed given the previous paragraph disallows droping on the DM.

Comment 5

[Philip Chee]

> In bug 192728 comment 10, Jens said:
>> <http://www.rfc-editor.org/rfc/rfc2483.txt>
>> (Notes that CRLF is required. Something that ROX will happily tell you, too.)"
> So I reverted this change. However the Toolkit reviewer in Bug 567377 comment 3
> Read the RFCs differently and decided that newlines weren't required.

CC Jens. For some reason I can't run any tests locally. Can you check that this patch doesn't break any of the Downloads/DnD tests? Thanks muchly.

Comment 6

[[e-mail]]

Comment on attachment 786273 [details] [diff] [review]
Patch v1.1 More tweaks. Has r=IanN

>+    gDownloadStatus.src = "moz-icon://" + file.leafName + "?size=16";
This doesn't work for e.g. applications. You need the original selItemData.file instead.

>+    dt.setDragImage(gDownloadStatus, 16, 16);
This fails badly for multiple selection.

>+    var sourceNode = dt.mozSourceNode;
>+    var sourceDoc = sourceNode ? sourceNode.ownerDocument : document;
>+    // Disallow droping a download manager node back on the manager.
>+    if (sourceDoc == aEvent.originalTarget.ownerDocument)
>+      return;
As you noticed you always drag a file from the download manager, so this check isn't strictly necessary, but if it was, you would need to put it in the onDragOver method too. Anyway, an easier check would be to see whether the source node is the target. (I think your current check prevents external drops on the download manager. Also you misspelled dropping.)

>+    <statusbarpanel id="statusbar-display" class="statusbarpanel-iconic-text"
>+                    flex="1" pack="start"/>
The problem here is that this binding's label doesn't flex, which means that if the path is very long it just pushes the online icon out of the window instead of cropping.

Comment 7

[Jens Hatlak (:InvisibleSmiley)]

(In reply to Philip Chee from comment #5)
> For some reason I can't run any tests locally. Can you check that
> this patch doesn't break any of the Downloads/DnD tests?

Unfortunately compilation currently fails for me with

e:/mozilla-src/comm-central/mailnews/base/src/nsMsgContentPolicy.cpp(606) : error C2039: 'HasAttribute' : is not a member of 'nsIDOMHTMLImageElement'

(couldn't find a bug for that yet but currently lack the time to do any analysis).

Once I'm able to build again I'll try to perform before/after test runs.

Apart from that I'm not sure the changes proposed here are a good idea. I was the one who introduced the current behavior to SM in bug 192728, doing quite some amount of testing especially on Linux to make sure all sorts of use cases are covered (cf. bug 192728 comment 10). At the same time I filed bug 567377 for FF. Then much later (about 2.5 years AFAICS) the FF devs looked into that bug but introduced a differing behavior, seemingly based on guesses rather than the real-world tests I had described in bug 567377 comment 0. Now this is what you're trying to back-port to SM. If I were you I'd apply "if it ain't broken, don't fix it" here.

Comment 8

[Jens Hatlak (:InvisibleSmiley)]

(In reply to Jens Hatlak (:InvisibleSmiley) from comment #7)
> Unfortunately compilation currently fails for me with
> 
> e:/mozilla-src/comm-central/mailnews/base/src/nsMsgContentPolicy.cpp(606) :
> error C2039: 'HasAttribute' : is not a member of 'nsIDOMHTMLImageElement'
> 
> (couldn't find a bug for that yet but currently lack the time to do any
> analysis).

Just saw that standard8 landed a "Fix bustage from bug 901060", but I'm out of time for today. Will try again within the next few days.

Comment 9

[Philip Chee]

> Apart from that I'm not sure the changes proposed here are a good idea.

I've already backed out this bit:
> -    dt.setData("text/uri-list", url + "\r\n");
> -    dt.setData("text/plain", url + "\n");
> +    dt.setData("text/uri-list", url);
> +    dt.setData("text/plain", url);

Also less is more, I'm going to trim this down more.

Comment 10

[Jens Hatlak (:InvisibleSmiley)]

Turns out I cannot run tests right now either, see bug 853720 comment 14.

Comment 11

[Philip Chee]

Attachment: Patch v1.2 minimalist approach.

Changing to a more minimalist approach.

>>+    gDownloadStatus.src = "moz-icon://" + file.leafName + "?size=16";
> This doesn't work for e.g. applications. You need the original 
> selItemData.file instead.
I'm not using any icons now.

>>+    dt.setDragImage(gDownloadStatus, 16, 16);
> This fails badly for multiple selection.
I've changed this so that the drag image is only set if the selection count is 1.
For multiple selection, if I don't set a drag image or set it to null then the drag image is all the rows in the selection. Should I set it to some dummy element? If so which?

>>+    var sourceNode = dt.mozSourceNode;
>>+    var sourceDoc = sourceNode ? sourceNode.ownerDocument : document;
>>+    // Disallow droping a download manager node back on the manager.
>>+    if (sourceDoc == aEvent.originalTarget.ownerDocument)
>>+      return;
> As you noticed you always drag a file from the download manager, so this check 
> isn't strictly necessary, but if it was, you would need to put it in the 
> onDragOver method too.
I've abstracted out a helper function and now call it from both onDragOver and onDrop.

> Anyway, an easier check would be to see whether the 
> source node is the target.
It turns out that internal Download Manager nodes are all "application/x-moz-file" so checking the source node isn't needed, instead I can just check for nsIFile. See next comment.

> (I think your current check prevents external drops 
> on the download manager.
Changed so that external drops are only prevented if they are local files.

> Also you misspelled dropping.)
Fixed.

>>+    <statusbarpanel id="statusbar-display" class="statusbarpanel-iconic-text"
>>+                    flex="1" pack="start"/>
> The problem here is that this binding's label doesn't flex, which means that 
> if the path is very long it just pushes the online icon out of the window 
> instead of cropping.
I've abandoned this change.

Comment 12

[[e-mail]]

Comment on attachment 798529 [details] [diff] [review]
Patch v1.2 minimalist approach.

>+  if (file && file instanceof Components.interfaces.nsIFile)
>+    return true;
>+  return false;
Nit: just return the value of the expression rather than hiding it in an if/true/false.

Comment 13

[Philip Chee]

Pushed to comm-central
http://hg.mozilla.org/comm-central/rev/1f3bf5d837dd