User-Agent: Mozilla/5.0 (Windows; U; Windows NT 6.0; nl; rv:1.9.0.6) Gecko/2009011913 Firefox/3.0.6 Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.0; nl; rv:1.9.0.6) Gecko/2009011913 Firefox/3.0.6 When you drag a tab, link or bookmark to the desktop, firefox creates a shortcut to that website. When you drag the current url to the desktop, it doesn't create a shortcut. This is said to be working on OS X, however it seems it has never worked on win xp or win vista. Reproducible: Always Steps to Reproduce: 1. Select url in Navigation Toolbar 2. Drag the url to the desktop 3. Create shortcut Actual Results: 3. No shortcut!

Dorus, do you have the bug number off-hand where I mentioned that? Thanks.

(In reply to comment #1) > Dorus, do you have the bug number off-hand where I mentioned that? Thanks. bug 471955 comment 11

Can you please check if the regression range which is mentioned in bug 388522 comment 0 is the same for you? That would be really helpful.

As I mentioned before, there is no regresion range, this has NEVER worked on windows as far as i can tell. I tried it and it's not working on: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9a6pre) Gecko/20070630 Minefield/3.0a6pre Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9a8pre) Gecko/2007082705 Minefield/3.0a8pre I also tried various 1.5, 2.0 and 3.0 beta/alpha/trunk builds. Any other builds i could have more luck on trying?

Ok, so we have no-one to blame for. But I wonder why such a bug isn't filed yet. I wasn't able to find one. Natch, are you aware of one?

I don't think this is a Core::Drag and Drop problem, it just isn't getting the right flavors set afaict. Not sure why this works on OSX (maybe that's because the os recognizes text drops and tries to convert them to urls?). Unless there's some MAC specific code to make this work (I highly suspect not). Anyway, this probably belongs in Location Bar and Autocomplete, IMHO. Henrik, can you try dragging a text url from another place (e.g. a webpage) that isn't linkified to the desktop and see if it works? (In reply to comment #5) > Ok, so we have no-one to blame for. But I wonder why such a bug isn't filed > yet. I wasn't able to find one. Natch, are you aware of one? No I am unaware of any bugs filed for this specifically.

(In reply to comment #6) > Anyway, this probably belongs in Location Bar and Autocomplete, IMHO. Henrik, > can you try dragging a text url from another place (e.g. a webpage) that isn't > linkified to the desktop and see if it works? No, that doesn't work but this problem is bug 475045. No idea if a patch on bug 475045 could fix this one too.

taking. A fix for this should be relatively easy, and unlike the case in bug 475045, here this should obviously work and is a targeted case.

Not sure why this doesn't work... The handler is called but no shortcut is made...

Neil, is the text-dragging handled by something else that would ruin this type of fix?

this works instead, and uses the new apis. i think your issue is the preventDefault

Comment on attachment 370867 [details] [diff] [review] patch that works :) yeah this is probably a better choice, assuming this works on Vista (I'll try it out later today if you haven't). I had tried that before but at that time I was using dragstart because draggesture was deprecated. Then I realized dragstart wasn't being called but by then I already had been using addEventListener. Does the same thing not work with addEventListener? Oh, you should probably check if event.target == this.inputField, otherwise this will enable dragging the entire bar (I think, at least in my tries it had). If this isn't true, I'll r+ (fwiw).

Comment on attachment 370867 [details] [diff] [review] patch that works :) Dragging the bar does now create a shortcut, other then that this seems great (tested it on Vista). /me is not a peer though...

So my current thoughts are: Something else is handling the dnd which is stomping on the values of either .action or the flavors. When a <handler> is used, it gets called last and has the final say of what to do with the drag. That's why the addEventListener doesn't work, while <handler> does....

i don't think we can automatically test a drop to the desktop, unless you have any idea about how to do that.

Please do not add any new code which uses draggesture. Use dragstart instead.

(In reply to comment #15) > i don't think we can automatically test a drop to the desktop, unless you have > any idea about how to do that. But we can test 1) that the session has the correct flavors and 2) that the whole urlbar isn't draggable, switch to in-litmus? if you think these aren't important... (In reply to comment #16) > Please do not add any new code which uses draggesture. Use dragstart instead. dragstart isn't getting called here (unless I'm doing something very wrong) see comment 12.

(In reply to comment #17) > dragstart isn't getting called here (unless I'm doing something very wrong) see > comment 12. OK, I didn't realize that this was the urlfield. This is the case because of bug 455215.

Comment on attachment 371236 [details] [diff] [review] patch v1.0 >diff --git a/browser/base/content/urlbarBindings.xml b/browser/base/content/urlbarBindings.xml >+ // Drag only if the gesture starts from the input field. >+ if (event.originalTarget != this.inputField) >+ return; Does this really work? In my tests it hadn't (i.e. the condition was never met). That's why I switched to using addEventListener directly on this.inputField (although for the reason stated in comment 14 that didn't work), I'll retest to confirm.

worked fine here, while event.target points to the urlbar, event.originalTarget points to inputField.

(In reply to comment #20) > worked fine here, while event.target points to the urlbar, event.originalTarget > points to inputField. Yes, just tested the patch and it works fine.

Natch, atm i don't have enough time to create a test, but i see we could partly test this as you point out in comment 17. so if you have time for that and want to do it, would be great (and maybe allow to take this for 3.5). We'll also probably need to move review to another toolkit peer, Gavin has too many.

Comment on attachment 371236 [details] [diff] [review] patch v1.0 Actually this isn't a toolkit patch, it's in browser/base/content which, per the review docs, Dao can review :) I've been trying to pull together a test but it isn't as simple as I thought it to be. Synthesizing a drag takes a lot of time and can easily timeout due to the 30 second limit. I also can't get the test to work, but I'll see what I can do...

Comment on attachment 371236 [details] [diff] [review] patch v1.0 You should do this only if the whole value is selected, and probably set effectAllowed only when you're committed to do it.

(In reply to comment #24) > (From update of attachment 371236 [details] [diff] [review]) > You should do this only if the whole value is selected I'm not sure this is true. What i mean is that the user could want to only drag part of the url (for example for an url with a very long query string maybe i only want to select and drag the page address, leaving out the query string), or we could be in an url like http://special_url?my_url (bookmarking services, anon services, anti-leech services...). I generated an uri object exactly to ensure that what we have has the syntax of an url, and handle all of those cases. Do you think that is an excessive edge use-case?

Added some comment. Notice i've not fixed the "full selection" comment, see my comment above, btw if we want to only handle such a case i'll do. I've tried to build a test but unfortunately here we still use draggesture, and synthesizeDrag traps dragstart, so i can't do such a test. I'll flag in-litmus.

(In reply to comment #25) > > You should do this only if the whole value is selected > > I'm not sure this is true. What i mean is that the user could want to only drag > part of the url (for example for an url with a very long query string maybe i > only want to select and drag the page address, leaving out the query string), Or maybe the user erroneously selected part of the very long url. The resulted link will likely be useless. > or we could be in an url like http://special_url?my_url (bookmarking services, > anon services, anti-leech services...). The page title will also be missing, then. It seems that you'd be better off if you edit the address in the shortcut afterwards. > I generated an uri object exactly to ensure that what we have has the syntax of > an url, and handle all of those cases. > Do you think that is an excessive edge use-case? http://moz, p://m and Foo:Bar are all valid URIs.

(In reply to comment #27) > Or maybe the user erroneously selected part of the very long url. The resulted > link will likely be useless. I usually think it's less likely that a user makes the wrong thing when he is able to see what he is doing. The selection is a quite visible feedback. > > or we could be in an url like http://special_url?my_url (bookmarking services, > > anon services, anti-leech services...). > > The page title will also be missing, then. If page title is missing we use the url, that's not a problem, it is expected that in some situation it could be missing. > http://moz, p://m and Foo:Bar are all valid URIs. this is true. Well since we should be more oriented toward non-technical users i'll fix that to only drag the full selection.

(In reply to comment #28) > (In reply to comment #27) > > Or maybe the user erroneously selected part of the very long url. The resulted > > link will likely be useless. > > I usually think it's less likely that a user makes the wrong thing when he is > able to see what he is doing. The selection is a quite visible feedback. If it's a very long url, though, part of it could be hidden. > > The page title will also be missing, then. > > If page title is missing we use the url, that's not a problem, it is expected > that in some situation it could be missing. Certainly not a huge problem, but suboptimal nevertheless. I think you should also check that the pageproxystate attribute's value is "valid", and then use content.document.title instead of PlacesUtils.

And you should use content.location.href instead of urlString, as the latter is decoded. Last but not least, note that proxyIconDNDObserver doesn't use the page title in the HTML string, presumably because it might contain characters that could be problematic in HTML.

I wouldn't see why you only want to be able to drag out the entire URL. I often want to make a shortcut to the main site, for example https://bugzilla.mozilla.org/ and not https://bugzilla.mozilla.org/show_bug.cgi?id=478070. The biggest difference between dragging the url and dragging the favicon is that with the url you got control over what part of the url you want to make a shortcut. That is also how dragging the url from the Navigation Toolbar to the bookmark bar currently works. Or do you want to 'break' that too to keep things consistent?

(In reply to comment #31) > The biggest difference > between dragging the url and dragging the favicon is that with the url you got > control over what part of the url you want to make a shortcut. This is a good point, since the favicon is quite near and you can use it, we would end up duplicating something we already have. I see god points from both views, so i think it would be better to involve someone from UX. > That is also how dragging the url from the Navigation Toolbar to the bookmark > bar currently works. This does not work here, i cannot drag url from the location to the bookmarks toolbar... regression? Argh, yes :\ filing the regression bug.

(In reply to comment #32) > (In reply to comment #31) > > The biggest difference > > between dragging the url and dragging the favicon is that with the url you got > > control over what part of the url you want to make a shortcut. > > This is a good point, since the favicon is quite near and you can use it, we > would end up duplicating something we already have. Redundancy is not a problem here, and not a reason to make this different from favicon drags.

(In reply to comment #33) > Redundancy is not a problem here, and not a reason to make this different from > favicon drags. filed bug 495211. Actually reduncancy is not my first thought, seeing how 3.0.x was behaving, we are making this different, it allowed to drag part of the address to create a bookmark, in the same way it would allow dragging part of the address to create a shortcut.

fixing this bug will also fix bug 475045 (bug 495211 was really a dupe of that). So i think we should preserve Firefox 3.0 behavior. I will directly ask Faaborg or Beltzner as soon as they appear.

There's a good chance that this wasn't implemented intentionally for the bookmarks toolbar, and it just happened to work. Technically this is different, as you're explicitly creating html and uri data flavors.

So, after a brief talk with Faaborg, we see the potential of the accidental 3.0 behavior (dragging part of an url), but at the same time the risk of users dragging not working uris is high, and we prefer "safety proof things". Yeah, probably was implemented accidentally in 3.0 because bookmarks toolbar accepts also text flavors. To create a bookmark you can still copy part of an url and paste it to the toolbar.

(In reply to comment #37) > To create a bookmark you can still copy part of an url and paste it to the > toolbar. I mean "to create a bookmark from only part of an url"

(In reply to comment #30) > Last but not least, note that proxyIconDNDObserver doesn't use the > page title in the HTML string, presumably because it might contain characters > that could be problematic in HTML. sounds a bit limiting avoiding using page title because in some rare case it could contain bad html chars, if that's the case is the page that should be fixed. Personally i would directly use title in both places, unless we want a special encoding function, but sounds not worth doing that for edge cases.

(In reply to comment #39) > (In reply to comment #30) > > Last but not least, note that proxyIconDNDObserver doesn't use the > > page title in the HTML string, presumably because it might contain characters > > that could be problematic in HTML. > > sounds a bit limiting avoiding using page title because in some rare case it > could contain bad html chars, if that's the case is the page that should be > fixed. What that page does is perfectly valid: data:text/html,<title>&lt;/body&gt;</title>

(In reply to comment #40) > What that page does is perfectly valid: > data:text/html,<title>&lt;/body&gt;</title> How many pages do you expect to have such a behavior? I'm not sure that not using page title is a better fix that using a bad page title. Nor i think we have a good html entities encoder.

(In reply to comment #41) > (In reply to comment #40) > > What that page does is perfectly valid: > > data:text/html,<title>&lt;/body&gt;</title> > > How many pages do you expect to have such a behavior? I'm not sure that not > using page title is a better fix that using a bad page title. It's consistent with the favicon, at least. I suggest you file a new bug for using the page title in the html link (which is irrelevant for drags to the desktop, afaik).

(In reply to comment #42) > (In reply to comment #41) > It's consistent with the favicon, at least. I suggest you file a new bug for > using the page title in the html link (which is irrelevant for drags to the > desktop, afaik). it is not, we use the title provided in the flavor to name the link on the desktop. So when you drag an x-moz-url that provides url\ntitle we create a shortcut named title pointing to url.

I'm referring to the html flavor. The pageproxydrag thingy doesn't use the page title there.

this should fix comments and adds a browser-chrome test. Due to the particular way this drag works it listen on dragend. The test probably still needs some cleanup.

This would be fine, apart the fact that the test hangs till i move mouse on browser window, then it works correctly and passes. Can't see a reason for that, it is waiting on a mousemove event just after i send the second mousemove (that causes the drag). sounds like some odd behavior in events handling.

Comment on attachment 381764 [details] [diff] [review] patch v1.3 >+ is(dataTransfer.mozItemCount, aExpectedDragData.length, >+ "Number of dragged items should be the same"); Are you sure that the test should fail if we add more flavors? Shouldn't it just check that those added in this bug are there? >+ var isFullSelection = this.selectionStart == 0 && >+ this.selectionEnd == this.value.length; this.textLength

(In reply to comment #47) > (From update of attachment 381764 [details] [diff] [review]) > >+ is(dataTransfer.mozItemCount, aExpectedDragData.length, > >+ "Number of dragged items should be the same"); > > Are you sure that the test should fail if we add more flavors? Shouldn't it > just check that those added in this bug are there? I quoted the wrong code lines, but the question remains the same...

(In reply to comment #48) > (In reply to comment #47) > > Are you sure that the test should fail if we add more flavors? Shouldn't it > > just check that those added in this bug are there? i think then we should update the test, since it checks what we are dragging, potentially having more flavors could even be an error. btw i'm not sure we can take that test if i can't find a way to stop it from hanging.

(In reply to comment #49) > i think then we should update the test, Well, yes, ideally the test would always be updated. But I also think it's nicer to make that a "can" rather than a "must". A test that just looks for a regression is obviously valuable, but if at some point random legacy tests start to fail whenever you touch code, not because you broke something but because they make too narrow assumptions, then these tests start to reduce productivity. > since it checks what we are dragging, > potentially having more flavors could even be an error Are there certain flavors we're concerned about? In that case, should there be a blacklist?

this comes without a test, i tried several ways to get a working test: - using the new API and dt - using the old API and dt - using the old API and transferable I tried asking Neil, he suggested to preventDefault draggesture and stopPropagation dragstart. Unfortunatly looks like preventDefault is not well handled by current input fields code, and dragstart is not even fired. Unless someone has a solution, i'd flag back in-testsuite- and in-litmus? i have no further ideas.

I filed bug 499008 on moving the editor over to the new api.

(In reply to comment #52) > I filed bug 499008 on moving the editor over to the new api. Marco, it might be useful to add that bug number to the comment.

i'll do, thanks.

In relation to bug 475045 Comment #5, and a question about this on the dutch support forum (i'm talking about the "drag a url from the bar to the bookmark toolbar" use case here): Are there any plans to fix this in FF 3.5 also? I see the wanted flag, but no other activity on this. Or would that be more suited for bug 475045 and/or a new bug?

Bug 471955 has just been marked a duplicate of this, however from bug 471955 comment 10 Dorus states it is useful to be able to "select a part of the url (for example "https://bugzilla.mozilla.org/" instead of "https://bugzilla.mozilla.org/show_bug.cgi?id=471955" and make a bookmark/shortcut of only that." I have just tried dragging part of a url to the desktop and I get the 'no entry' cursor and no shortcut is create (no action happens). This is on latest trunk Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2a2pre) Gecko/20090809 Minefield/3.6a2pre. Should a new bug be created for this with clear description & STR or reopen either this or bug 471955?

even if you would file a new bug, according to the above comments that behavior has been dropped (read the discussion above in this bug, since we talked about that) and will probably end up being wontfixed. Nor reopening one of the two would help. Dragging only part of an url is too much error-prone. An extension could do that though.

added https://litmus.mozilla.org/show_test.cgi?id=9467 to Litmus.

Verified fixed with builds on Windows, Linux and OS X like Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a2pre) Gecko/20090817 Namoroka/3.6a2pre ID:20090817033832