Closed Bug 1319212 Opened 7 years ago Closed 7 years ago

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

Categories

(SeaMonkey :: OS Integration, defect)

SeaMonkey 2.49 Branch
Unspecified
Windows
defect
Not set
normal

Tracking

(seamonkey2.48 unaffected, seamonkey2.49esr fixed, seamonkey2.50 fixed, seamonkey2.51 fixed)

RESOLVED FIXED
seamonkey2.51
Tracking Status
seamonkey2.48 --- unaffected
seamonkey2.49esr --- fixed
seamonkey2.50 --- fixed
seamonkey2.51 --- fixed

People

(Reporter: frg, Assigned: frg)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

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.
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
Version: unspecified → SeaMonkey 2.49 Branch
Have it almost fixed so taking the bug.
Assignee: nobody → frgrahl
Status: NEW → ASSIGNED
Attached patch 1319212-DragAndDrop.patch (obsolete) — Splinter Review
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.
Attachment #8819883 - Flags: feedback?(philip.chee)
Attachment #8819883 - Flags: feedback?(iann_bugzilla)
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.
Attachment #8819883 - Flags: feedback?(iann_bugzilla) → feedback+
Attached patch 1319212-DragAndDrop-V2.patch (obsolete) — Splinter Review
> 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.
Attachment #8819883 - Attachment is obsolete: true
Attachment #8819883 - Flags: feedback?(philip.chee)
Attachment #8827620 - Flags: feedback?(philip.chee)
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.
Attachment #8827620 - Attachment is obsolete: true
Attachment #8827620 - Flags: feedback?(philip.chee)
Attachment #8833457 - Flags: review?(philip.chee)
Attachment #8833457 - Flags: review?(iann_bugzilla)
Attachment #8833457 - Flags: feedback?(kevink9876543)
Comment on attachment 8833457 [details] [diff] [review]
1319212-DragAndDrop-V3.patch

Works well in 2.49, thanks! :)
Attachment #8833457 - Flags: feedback?(kevink9876543) → feedback+
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
Attachment #8833457 - Flags: review?(iann_bugzilla) → review+
Comment on attachment 8833457 [details] [diff] [review]
1319212-DragAndDrop-V3.patch

[Triage Comment]
a=me for c-b and c-a too
Attachment #8833457 - Flags: approval-comm-beta+
Attachment #8833457 - Flags: approval-comm-aurora+
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
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.51
Blocks: 1342654
Attachment #8833457 - Flags: review?(philip.chee)
You need to log in before you can comment on or make changes to this bug.