1319212 - Dragging and dropping a url from the desktop onto the browser no longer works

Status: RESOLVED FIXED

(SeaMonkey :: OS Integration, defect)

Description

[Frank-Rainer Grahl (:frg)]

Dragging and dropping a url into an open tab no longer works. I see an error in the Error console at the same time:

Timestamp: 11/21/2016 10:55:06 PM
Error: Error: At least keyword or url must be provided
Source File: resource://gre/modules/PlacesUtils.jsm
Line: 2160

Might be related to bug 1150678 but unsure at the current time.

Comment 1

[Rainer Bielefeld]

Reproducible with Server-Installation of  unofficial (by FRG) en-US SeaMonkey 2.50a1 (NT 6.1; WOW64; rv:53.0) Gecko/20100101 Firefox/53.0 Build 20161121115504  (Default Classic Theme) on German WIN7 64bit:

0. Preparation:ˋSelect URL of this page in Location Bar → <control+c> for "copy"
   → Rightclick Desktop → New → link connection → paste URL → [Next] → [Finish]ˊ
1. In Browser: Menu ˋFile  → New → Browser TABˊ
   » New TAB (empty or with default "Home" page) appears
2. Click icon for newly created link on Desktop → Drag and drop it to TAB 
   heading from step (1)
   Expected: This page appears in TAB
   Actual: As expected

BUT

11. In Browser: Menu ˋFile  → New → Browser TABˊ
   » New TAB (empty or with default "Home" page) appears
12. Click icon for newly created link on Desktop → Drag and drop it to TAB
    contents area from step (11)
   Expected: This page appears in TAB
   Actual: nothing happens

Additional informationen
a) Step 12 already broken in 
   unofficial en-US SeaMonkey 2.49a1  (NT 6.1; WOW64; rv:52.0) Gecko/20100101
   Firefox/52.0 Build 20160924050758  (Default Classic Theme)
   on German WIN7 64bit
b) Step 12 still worked fine with
   unofficial en-US SeaMonkey 2.48a1  (NT 6.1; X64; rv:51.0) Gecko/20100101 
   Firefox/51.0 Build 20160905222145  (Default Classic Theme) 
   on German WIN7 64bit
   So Version 2.49 for now
c) I did tests for a,b with newly created user profile

Comment 3

[Frank-Rainer Grahl (:frg)]

Have it almost fixed so taking the bug.

Comment 4

[Frank-Rainer Grahl (:frg)]

Attachment: 1319212-DragAndDrop.patch

Basically 

> return PlacesUtils.keywords.fetch(keyword).then(entry => { 

didn't work any longer. I looked at the corresponding FF code and found that it was just did what was needed. As a bonus you can now drop several items at once into the tabbrowser.
With the current patch there is a change however. Dropped urls will honor the pref for background loading. I think this is ok and I like it but it might be undesired. In this case I would probably suggest if one item is dropped to load it in the current tab as before and if several items are dropped loading them as in the patch now. Just let me know. 

I picked up changes from FF for middleMousePaste too. There is at leat one imho security relevant change in there which should be kept. Can do a separate patch but works as is and is tested. Not sure if the whole code for middle button pasting shouldn't be removed. Its behind two prefs which are flipped by default and seems to be a very non standard way to paste with the middle button.

Comment 5

[Ian Neal]

Comment on attachment 8819883 [details] [diff] [review]
1319212-DragAndDrop.patch

>+++ b/suite/browser/navigator.js
> function BrowserOpenWindow()

>       case "4": // private
>         openNewPrivateWith(params.url);
Just wondered why this wasn't url to start with and should it be data.url now?

>         // Open a new window with the URL
>-        var newWin = openDialog(getBrowserURL(), "_blank", "all,dialog=no", url,
>-            null, null, postData, true, isUTF8);
>+        var newWin = openDialog(getBrowserURL(), "_blank", "all,dialog=no", data.url,
>+            null, null, data.postData, true, isUTF8);
Worth indenting so it lines up with the bracket?

Should the documentation of the new function be included? https://dxr.mozilla.org/comm-central/source/mozilla/browser/base/content/browser.js#2124

>+function handleDroppedLink(event, urlOrLinks, name)

>+  let lastLocationChange = gBrowser.selectedBrowser.lastLocationChange;
>+
>+  // Usually blank for SeaMonkey
Nit: end comment with full stop.

>+++ b/suite/common/contentAreaClick.js

>+  function middleMousePaste(event) {
>+
>+    let clipboard = readFromClipboard();
>+
>+    if (!clipboard)
>       return;

>-        // fix up our pasted URI in case it is malformed.
>-        const nsIURIFixup = Components.interfaces.nsIURIFixup;
>-        if (!gURIFixup)
>-          gURIFixup = Components.classes["@mozilla.org/docshell/urifixup;1"]
>-                                .getService(nsIURIFixup);
> 
>-        url = gURIFixup.createFixupURI(url, nsIURIFixup.FIXUP_FLAGS_MAKE_ALTERNATE_URI).spec;
Should we still be looking to fix up pasted URIs?

>+    // Strip embedded newlines and surrounding whitespace, to match the URL
>+    // bar's behavior (stripsurroundingwhitespace).
>+    clipboard = clipboard.replace(/\s*\n\s*/g, "");
> 
>-        if (openNewTabOrWindow(event, url, null))
>-          event.stopPropagation();
>+    clipboard = stripUnsafeProtocolOnPaste(clipboard);
>+
>+    // if it's not the current tab, we don't need to do anything because the
>+    // browser doesn't exist.
Nit: start the comment with a capital if ending it with a full stop.

>+    let where = whereToOpenLink(event, true, false);
SeaMonkey's function takes 4 arguments. Need to check whether the function needs updating too and/or this call is correct.
The behaviour of middleclick paste now gives the save option when using shift middleclick whereas I don't think it did anything previously (it also stops responding to middleclick until you copy another url even though there is still something in the clipboard).

f+ for the moment, getting there.

Comment 6

[Frank-Rainer Grahl (:frg)]

Attachment: 1319212-DragAndDrop-V2.patch

> Just wondered why this wasn't url to start with and should it be data.url now?

Good question. Left it in for now and need to test it with data.url. Maybe Ratty has an idea.

> Should we still be looking to fix up pasted URIs?

I think not neccesary any more. makeURI will take care of it. Firefox doesn't do it so its either more restrictive or works. Will check it again.

> >+    let where = whereToOpenLink(event, true, false);
> SeaMonkey's function takes 4 arguments. Need to check whether the function 
> needs updating too and/or this call is correct.

Fourth is optional and only used if browser.tabs.opentabfor.middleclick is true.

My best guess would be that ignorebackground should not be set here.

Comment 7

[Frank-Rainer Grahl (:frg)]

Attachment: 1319212-DragAndDrop-V3.patch

My friend the tabbrowser needed an addition patch for this to fully work. Dropped urls now replace the first tab again and the next ones are loaded in the background. Method loadtabs previously didn't recognize a parameter object. I picked the ones from browser I thought are the most needed or additional code would have been needed.

Comment 8

[kevink9876543]

Comment on attachment 8833457 [details] [diff] [review]
1319212-DragAndDrop-V3.patch

Works well in 2.49, thanks! :)

Comment 9

[Ian Neal]

Comment on attachment 8833457 [details] [diff] [review]
1319212-DragAndDrop-V3.patch

Should the bugs we're porting be mentioned? e.g. bug 92737
r/a=me

Comment 10

[Ian Neal]

Comment on attachment 8833457 [details] [diff] [review]
1319212-DragAndDrop-V3.patch

[Triage Comment]
a=me for c-b and c-a too

Comment 11

[Frank-Rainer Grahl (:frg)]

Modified the comment to include the important mozilla bugs. I noticed that not all cases in Bug g 92737 are fixed with this one. If you drop more than one link into the tab bar only one will open. Will open a followup bug. This one needed to go in to unbreak drag and drop generally. 

https://hg.mozilla.org/comm-central/rev/a23c9cbe5de187e5fdcdc2c2bb7822ccf916b7ec
https://hg.mozilla.org/releases/comm-aurora/rev/46cbc0349275224b59f37a22a7a121f9a8003916
https://hg.mozilla.org/releases/comm-beta/rev/a2d1c2016c8fb143f2f6b466f4d9554067e57c26