Google gives us a neat "bookmarklet" that looks like this: (name) Google Search (url) javascript:q=document.getSelection();for(i=0;i<frames.length;i++){q=frames [i].document.getSelection();if(q)break;}if(!q)void(q=prompt('Enter text to search using Google. You can also highlight a word on this web page before clicking Google Search.',''));if(q)location.href='http://www.google.com/search? client=googlet&q='+escape(q) Since I want this to be in my personal toolbar, I open "manage bookmarks" and drag it to the "personal toolbar" folder. But after I do that the bookmark looks like this: (name) text to search using Google. You can also highlight a word on this web page before clicking Google Search.',''));if(q) location.href='http://www.google.com/search?client=googlet&q='+escape(q) (url) javascript:q=document.getSelection();for(i=0;i<frames.length;i++){q=frames [i].document.getSelection();if(q)break;}if(!q)void(q=prompt('Enter

I can confirm this bug. The javascript bookmark I have opens the "View Stored Cookies" modal window with: javascript:window.openDialog('chrome://communicator/content/wallet/CookieViewer.xul','','modal=yes,chrome,resizable=no', 0); There is a space before the ending "0);" and this is the text labelling the bookmark after a drag. Of course, that text is then missing in the URL.

*** Bug 54767 has been marked as a duplicate of this bug. ***

Copying correctness keyword and rtm nomination from the dup bug. Also adding the top100 keyword, because I think Google is one of the more visited sites.

Ben, see bug #54767 for Johnny Stenback's simplified testcase.

I'm poking at this a little to get familiar with bookmarks. Oddly enough, I get a different behavior when I drag the bookmark to a specific location between two bookmarks - I get the first word as the URL and the second word as the name. For example, if I drag the "Google Search" link to the "Manage Bookmarks" window in between two bookmarks, I get (name) Search (url) Google strange, eh?

I figured out what's going on - basically we're trying to encode the combination of URL and title in the transfer data, which if you ask me is really lame. So we think that the drag transfer data is in the form "<url> <title>" if url contains a space, then we interpret the string as "<url1> <url2title>" where url1 is the first part of the URL, and url2title is the second half of the URL plus the the title string. There is another problem too - the transfer data for the text/x-moz-url flavor is malformed in certain cases. Sometimes it only contains the title!

good lord, this is screwed up. What's happening is that the first time you drag something, the selection expands to include the whole <a> link element, but the code behaves as though you're only dragging a #text element. The second time you the same link, you get a different codepath because the selection has changed. I'm not entirely sure why the selection needs to expand (perhaps the drop target handler needs some node to work off of) CC'ing pinkerton just because I just KNOW he'll be excited to here about DnD foo :)

Alec, why don't you won this one?

hrm, there's also a problem given that the application/x-moz-url data flavor says that the URL and title are separated by a space. URL's with a space will cause anyone parsing that flavor to do the wrong thing. We can change the spec on the flavor to use a different character (return?), but not until after we ship.

I think I have a better way to fix this - we can escape() the URL and title before we put it into the text/x-moz-url flavor, and unescape() it on the other end. Experimenting with this now

yep, sure enough using escape()/unescape() works perfectly - I'm hunting for other text/x-moz-url consumers now to see if I need to fix them.

Ok, basically what these patches do is make sure that when the url is put into the text/x-moz-url flavor, that they are escaped. I also had to fix history to make everything happy. The only other issue is with OS-specific stuff with D&D - I've tested dragging back and forth Mozilla to Communicator 4.x - I can even still drag from the 4.x browser into the Mozilla bookmarks window, so I am confident this is the right thing. Getting a reviewer and super-reviewer now.

oh, one other comment on the navigator side of things: I had to move the space-parsing code into the if() statement for the text/x-moz-url type - this prevented us from accidentally unescaping urls that were coming in from different flavors. text/x-moz-url is the only flavor that has this "url title" format, so this is safe.

a=ben for these changes.

There seems to be a typo here: + flavourList["text/x-moz-url"] = { width: 2, data: escape(urlBarf.value) + " " + window.title }; Change urlBarf to urlBar, I think (Freudian slip?). Also, the new dump() calls should be excised. I'm going to apply the patch and test it out (and check that you've caught all "text/x-moz-url" cases, so holding of on my r= for a bit longer...

I didn't apply the patch myself (I've got other things cooking in my source tree and don't want to put it at risk). But, I looked carefully at the code. The only concern I have is with the one call to retrieveURLFromData at http://lxr.mozilla.org/seamonkey/source/xpfe/browser/resources/content/navigatorDD.js#380 What impact will it have to not unescape the URL when placing it in the URL bar? Maybe that's OK, at least for standard URLs. But I wonder about funkier ones like javascript: or those funky bookmark nickname thingies (e.g., "bug: 12354" as a shortcut to bugzilla. I'm not sure those don't undergo substantive changes when escaped. Anyway, wouldn't it be OK to unescape, regardless, in that case as well?

good call! somehow I missed that case. Anyway, attaching a slightly updated patch with this unescape.

have review, super review and patch - mark rtm+

sorry, it's been a long night with a lot of Merlot

Should the unescape-ing be limited to cases where the flavour is text/x-moz-url? I wonder what will happen to text passed via text/unicode: the onDrop: functions (in navigatorDD.js) *always* unescape, as currently written. If the data wasn't escaped when it went in (i.e., flavour is other than text/x-moz-url), then the unescaping could change it, right? Maybe the right thing to do is unescape it right in retrieveURLFromData, only in the case of text/x-moz-url. Sorry about just realizing this now.

well, on the tip I think we need to revisit all of this code and clean it up.. but the reason we need to escape stuff for x-moz-url is that it can potentially contain a title after the URL, seperated by a space... i.e. it's sort of a structured string. in the case of text/unicode or similar flavors, the transfer data is a single chunk of unstructured data, so I don't think it's as important to escape it.

well, for the tip, you should file a bug against me to use a character other than space to separate the url and title. should probably use return like we did on mac in 4.x. that way we don't have to do any of this encoding for text/x-moz-url. that's the right way to fix this bug...

PDT is a little on the fence on this, but we bet there are a ton of URLs with spaces out on the net, so [rtm++]. Please test every combination of drag/drop, copy/paste, and any other relevant thing.

Alec, I understand that we only need to escape the url for text/x-moz-url. My point was that we're now *unescaping* for the other flavours, also, but the data likely is not escaped in those cases. A concrete example: I drag the text (as flavor "text/unicode"): javascript:alert("%20") and drop it in the content area. I believe that by unescaping it on the drop, this will cause an alert box with " " in it. It should produce an alert box with "%20". This is a contrived example but it makes me believe that there could well be a convoluted URL that would produce materially different results. Even if we can get away with it, I don't think we should unescape things that ought not to be. I think the relatively simple fix I outlined can remedy this: move the unescape to the common function and only do it in the text/x-moz-url case.

you're absolutely right. I didn't mean to unescape flavors that were not x-moz-url - let me go review this once more.

ok, one more try :) This time, I made retrieveURLFromData() return the unescaped URL instead of the raw URL: if aData is a text/x-moz-url then it unescapes it to get the raw URL. This meant I was able to leave the consumers of retrieveURLFromData alone. The other thing I did was get rid of the space check in the URL bar's drop listener. The reason for this was that spaces are actually valid in URLs (as we've discovered through this bug) and without this, dropping such URLs on the url bar would fail silently. (I didn't do this in my previous patches, because those spaces would be escaped) Ok, I think I've got everything straight this time :) How does this look?

OK, I'm almost satisfied. The change to retrieveURLFromData looks good. But now I'm worried (OK, I'm a worrier; somebody has to do it :-) about removing the space check in the content onDrop handling code. If you dig into that code via CVS Blame, you find out it was added to fix bug 40911. It seems you've reopened that one (and there was at least one other bug mentioned in that one that might also be affected). I'd be more comfortable leaving that code as is (broken as it might be). But you're free to convince me otherwise. I'd ping brade@netscape.com to check on the impact vis-a-vis bug 40911.

I think it's more important to fix bug 40911, so I'll put it back. I definately want to fix both of these issues in a better way on the tip.

fix checked into the branch.. haven't checked into the trunk yet I'll do that monday, 16-Oct

are you still planning on checking this into the trunk?

as I've said before, the best way to fix this on the trunk is to change the delimeter character in text/x-moz-url to something other than space. If you want, open a new bug and assign it to me for mozilla0.9. I'm assuming \n is acceptable?

I made one run at checking this into the trunk and got conflicts. I'll file another bug on pinkerton and we can have a race to see who finishes first :) I think the combination of our fixes is actually the right idea - we should not be relying on the absence of some magic delimiter character in the data, and the only way to prevent this is to munge the data in a way that shrouds our magic delimiter (such as escaping)

ok, FINALLY checked into the tip.. it's only been, what, 6 days? :)

okay everyone pay attention. This looks to be fixed on the TRUNK (with 2000102009 linux build). BUT It is still Broken on the Branch with 2000102009 builds. The current (branch) behavior is exactly as described in the original description in this bug. Reopening

hrm, ok, trying again.

Filed bug 57548 on a regression that might have been caused by this change.

I'm attaching a patch which fixes this problem. I had inadvertently removed an isLink = true from my patch when I was cleaning it up to check it in. Please pay attention to only the top half of the patch for this bug. The bottom half fixes bug 57548 (if both bugs don't get ++'ed, I'll only check in the part of the patch that gets ++'ed)

ben/law: Can I get one more sr=/r= from each of you? thanks.

a=ben for both parts of this fix. (isLink/non-unescaping)

I'm confused. The "isLink = true" line doesn't seem to be in the current trunk revision (1.48). Claudius says (on 10/20) that the bug is fixed on the trunk but not on the branch. So I'm not sure how that line brings the branch up to par with the trunk. But maybe Claudius was wrong about it being fixed on the trunk? It seems as if the "isLink = true" is necessary, but having screwed up reviewing this once, I don't want to do it again. I'll say r=law but only if you can account for Claudius's claim that it was fixed on the trunk without this latest patch. I'm even more confused about the portion that fixes bug 57548 but I'll take that up over there.

google updated their URL, updating it now.

this is working for me again on today's branch release build, without any extra patches, and it continues to work on the tip... which is odd because the day after claudius first reported it, I also saw it not working... Claudius, care to try this again?

ok, I've really tried all sorts of methods to break this, and still the only case I've found so far is the problem that the SECOND time you drag a link it only drags the text. I finally reported this as bug 58122.

oops, sorry for more spam, I meant to re-mark this fixed since I just can't reproduce the problem today...

sadly, on Win98 with 2000102709 branch AND trunk builds this is still broken. Here are my results using the google example: (Copied from a trunk build but same results w/branch) (name) text to search using Google. You can also highlight a word on this web page before clicking Google Search.',''));if(q)location.href='http://www.google.com/search?client=googlet&q ='+escape(q) (url) javascript:q=document.getSelection();for(i=0;i<frames.length;i++){q=frames [i].document.getSelection();if(q)break;}if(!q)void(q=prompt('Enter You will notice that this is really just the URL info broken up at the first space in the sequence (between 'Enter' and 'text') with the second part being used in the (name) field and hte first part being used in the (url) field. Alec, if you can't repro this I'd be happy to come try on your machine or show you on mine. REOPENING.

ok, I've backed out this change.. moving the bug back to [rtm need info] until I can figure out what the heck is going on.

Alec, should this be minused now?

no, I have a fix where we change to using "\n" as per pink's suggestion. I need to run it by platform people (like pink) to find out if we need to change platform-specific code as well.

Can you post the new patch and get reviews in parallel?

cc'ing me selmer, you just minused two crashers with 1- and 3-line patches. I'm wondering why you're considering this.

PDT marking [rtm-]. Spaces in URLs seem like an edge case, and we're real close to N6 RTM.

ok, here's a giant fix to this problem, for the trunk. basically this: - switches to using "\n" as the text/x-moz-url seperator, which makes google URLs work correctly. - rewrites contentAreaDNDObserver::onDragStart to be much more sane and consistent about dragging images, links, and the current selection - for dragging images, it correctly constructs all the flavors (it was kinda flakey before) including the alt= attribute - uses DOM ranges to correctly determine the text value of a selection - behaves correctly when you drag the scrollbar thumb (before it would silently fail halfway through the function) - we basically short cirtuit really early so we don't do any extra processing when the thumb is dragged - backs out one of my earlier broken changes that blake accidentally moved from navigatorDD.js to contentAreaDD.js - fixes various warnings that have probably been bugging all of you!

Lots of good cleanup here. Do we need the console dumping? (esp for the selection dragging). a=ben@netscape.com

ok, fix is in. Thanks ben I left the console dumping in there for now, because I have 2-3 other bugs to fix..(and there is actually no more dumping than there was before that fix) I'll remove the dump's when I check those fixes in.

I'm guessing at the blame here but I think this fix broke DND for hyperlinks. At least, it does not work properly for me on Linux 2000-11-10-13 MTrunk. To repro: 1) load http://www.google.com/ 2) Open another window using your favorite method 3) Drag any link from google to the new window Result: nothing Expected result: link opens in new window Want to open a new bug on that?

that's bug 59799

See also bug 59799

jinx

mass-verifying claudius' Fixed bugs which haven't changed since 2001.12.31. if you think this particular bug is not fixed, please make sure of the following before reopening: a. retest with a *recent* trunk build. b. query bugzilla to see if there's an existing, open bug (new, reopened, assigned) that covers your issue. c. if this does need to be reopened, make sure there are specific steps to reproduce (unless already provided and up-to-date). thanks! [set your search string in mail to "AmbassadorKoshNaranek" to filter out these messages.]