773983 - Switch to new drag and drop api for urlbar proxyIcon

Status: RESOLVED FIXED

(SeaMonkey :: Location Bar, defect)

Description

[Ian Neal]

Attachment: Switch proxyIconDNDObserver

This is a fairly simple port from the equivalent Firefox one.
Should it be:
var value = window.content.location.href;
rather than:
var value = document.getElementById("urlbar").value;

Comment 1

[[e-mail]]

(In reply to Ian Neal from comment #0)
> This is a fairly simple port from the equivalent Firefox one.
> Should it be:
> var value = window.content.location.href;
> rather than:
> var value = document.getElementById("urlbar").value;
I guess the value may have been "decoded" so that the href would be better.

Comment 2

[[e-mail]]

Comment on attachment 642254 [details] [diff] [review]
Switch proxyIconDNDObserver

>           <deck id="page-proxy-deck"
>                 class="urlbar-icons"
>                 onclick="handlePageProxyClick(event);">
>             <image id="page-proxy-button"
>-                   ondraggesture="PageProxyDragGesture(event);"
>+                   ondraggesture="proxyIconDNDObserver.onDragStart(event);"
>                    tooltiptext="&proxyIcon.tooltip;"/>
>             <image id="page-proxy-favicon" validate="never"
>-                   ondraggesture="PageProxyDragGesture(event);"
>+                   ondraggesture="proxyIconDNDObserver.onDragStart(event);"
>                    onload="this.parentNode.selectedIndex = 1;
>                            event.stopPropagation();"
>                    onerror="gBrowser.addToMissedIconCache(this.src);"
>                    tooltiptext="&proxyIcon.tooltip;"/>
>           </deck>
Why aren't we using ondragstart?
[Could move the event handler to the <deck> while you're there?]

>+    var value = document.getElementById("urlbar").value;
[Presumably when you switch to href you'll rename the variable too?]

>+    // XXX - do we want to allow the user to set a blank page to their homepage?
>+    //       if so then we want to modify this a little to set about:blank as
>+    //       the homepage in the event of an empty urlbar.
>+    if (!value)
>+      return;
[Won't apply if you use href of course]

>+    var htmlString = "<a href=\"" + value + "\">" + value + "</a>";
Ah, we really need to use href, because dragging URLs containing %22 is broken... actually dragging URLs containing &s is still broken.

Comment 3

[Ian Neal]

Attachment: Using location.href

Perhaps the .replace code should be a helper as it might be needed for the homeButtonObserver?

Comment 4

[[e-mail]]

Comment on attachment 642280 [details] [diff] [review]
Using location.href

>+    var link = href.replace(/&/g, "&")
>+                   .replace(/>/g, ">")
>+                   .replace(/</g, "&lt;")
>+                   .replace(/"/g, "&quot;")
>+                   .replace(/'/g, "&apos;");
[It's not strictly necessary to encode these all; only & and ' are allowed in an href, the others will already have been %-escaped. But see below about possibly encoding the value or title as well.]

>-      var urlString = urlBar.value + "\n" + window.content.document.title;
>-      var htmlString = "<a href=\"" + urlBar.value + "\">" + urlBar.value + "</a>";
>+    var urlString = href + "\n" + window.content.document.title;
>+    var htmlString = "<a href=\"" + link + "\">" + href + "</a>";
I just noticed that the htmlString actually uses the href twice. So it needs to use the HTML encoded version both times. r=me with that fixed.

I did wonder why it uses the href twice, but I guess that was to avoid thinking about the HTML encoding.
There are a couple of other choices for the text of the link:
* Use the urlBar value, after HTML encoding
* Use the document title, after HTML encoding (this would match what a bookmark would do, for instance.)

>-      aXferData.data.addDataForFlavour("text/unicode", urlBar.value);
>-      aXferData.data.addDataForFlavour("text/html", htmlString);
>+    dt.setData("text/plain", href);
>+    dt.setData("text/html", htmlString);
This means that Composer will prefer to drop the URL rather than a link element. If we switch these around, it would then prefer to drop the link. (If someone wants the URL, they could always drop it in to the source view.)

Comment 5

[Ian Neal]

Attachment: With helpers

Changes since last patch:
* Created a helper for encoding the html
* Used document title for the link text
* Created a helper for use by page proxy icon and, in the future, home button
* Switched order of text/plain and text/html

Comment 6

[[e-mail]]

Comment on attachment 642293 [details] [diff] [review]
With helpers

>+var browserDragAndDrop = {
>+  onDragStart: function (aEvent, aHref, aTitle)
Nit: I don't quite see the point of having this as a separate object, a function called something like BeginDragLink would have worked just as well.

Comment 7

[Ian Neal]

Attachment: With renamed helper [Checked in: Comment 8]

Helper name changed as per review suggestion.

Comment 8

[Ian Neal]

Comment on attachment 642586 [details] [diff] [review]
With renamed helper [Checked in: Comment 8]

http://hg.mozilla.org/comm-central/rev/1e7b4868e82b