User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7) Gecko/20040707 Firefox/0.9.2 Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7) Gecko/20040707 Firefox/0.9.2 Quick searches can be perfomed by highlighting / selecting some text and then dragging it to the address bar (to display search results in current window) or by dragging on to a blank piece of the Tab Bar (to display search results in a new tab). One of the easiest ways to select a word is to double-click on it. However when you do this, Windows (rather stupidly) includes a trailing space. If you then drag this selected word (including trailing space) on to the address bar, then the search works as expected, (I think was a recent bug fix) BUT if you do the same onto the TAB BAR, then the search breaks, and nothing happens. If you go back and manually select the same word (by dragging your mouse over the text rather than double clicking on it) so that the selection does NOT include the trailing space, then dropping the selected text onto the tab bar performs a quick search and opens the results in a new tab (as expected). This problem also exists for multiple word selection where spaces are in the middle of the selected text. Reproducible: Always Steps to Reproduce: 1. Open a web page that contains some plain text. 2. Double click on a word to select it. 3. Drag this selection onto a blank piece of tab bar and let go. Actual Results: Nothing whatsoever happens. The Quick search breaks. Expected Results: The word dropped onto the tab bar should be used to perform a quick search and the results should be displayed in a new tab. I believe that Bookmark keywords and the Address Bar had a similar problem (but they have now been fixed, as I can sucessfully use spaces in both). The same fix has not been applied to the tab bar.

Confirming. Really annoying, especially when i try to drag "bug <whatever>" to my tab bar and nothing happens instead of my bugzilla keyword kicking in. :-) I think this should be in the "Toolbars" category though? Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.3) Gecko/20040902 Firefox/1.0 PR (NOT FINAL)

Well, this fixes it (like the url already pointed out).

Comment on attachment 199844 [details] [diff] [review] patch hmm, regexps are bad for catching bad schemes, as a general rule. Converting to an nsIURI and checking schemeIs is preferred, so if we're touching this, let's fix that too. I'm not sure this is the right thing to do anyway, we explicitly blocked this for a reason, and without knowing why I'm not going to remove that check.

(In reply to comment #3) > I'm not sure this is the right thing to do anyway, we explicitly blocked this > for a reason, and without knowing why I'm not going to remove that check. Blake added it when he "fixed View->Character encoding", so it seems to me like he just arbitrarily decided that things with spaces aren't URIs and shouldn't be dragged. The comment seems pretty consistent with that theory, I think. I guess you'd have to ask him :).

Maybe Blake took it over from Hyatt: http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/xpfe/global/resources/content/bindings/tabbrowser.xml&rev=1.108&root=/cvsroot#1209 Maybe dragDropSecurityCheck needs can have nsIScriptSecurityManager.DISALLOW_SCRIPT_OR_DATA instead of nsIScriptSecurityManager.STANDARD? Then that stuff can be removed. I don't think anything useful would break. http://lxr.mozilla.org/seamonkey/search?string=dragDropSecurityCheck

It would break dragging data: or javascript: to the new-tab, new-window and downloads buttons, but that's not all that useful anyways. It would also remove the need for a bunch of additional checkLoadURI calls.

Comment on attachment 199844 [details] [diff] [review] patch Obsoleting per Mike's comment.

I'd like to point out that dragging a word to an empty space on the tab bar currently does NOT perform a quick search - it simply goes to www.foo.com (where foo is the dragged word). Given this behavior, I think the test for whitespace is correct, as the dragged text is expected to be part of a domain name, which can not contain whitespace. A request that dragging text into the tabbar should be filed as a separate bug (unless there's already a bug filed on it).

(In reply to comment #8) > A request that dragging text into the tabbar should be filed as a separate bug > (unless there's already a bug filed on it). Yes, bug 311011.

(In reply to comment #8) Ugh, sorry. Ignore my comment #8. I have non-standard settings in my profile which disable search from the URL bar with no keyword (which I incorrectly(?) referred to as "quick search").

If someone else wants to take this, please do.

Better late than never! This patch will make the drop perform a search using the current search engine if possible.

Comment on attachment 720802 [details] [diff] [review] Dropping text containing spaces will now perform a search on the given sentence Hi Lukas, thanks for the patch! While the effect seems roughly correct, there are a couple of ways we could simplify: Rather than calling getSubmission ourselves, we can set allowThirdPartyFixup: true in the aOptions parameter to loadOneTab, and pass Ci.nsIWebNavigation.LOAD_FLAGS_ALLOW_THIRD_PARTY_FIXUP to loadURIWithFlags for the "load in an existing tab" case.

I updated the patch to using thirdpartyfixup instead. A little question about that: How does firefox choose which thirdparty that get the honour?

review ping?

Comment on attachment 721183 [details] [diff] [review] Updated patch using thirdpartyfixup Sorry for the delay, Lukas! This looks much better, but removing the calls to getShortcutOrURI regresses some functionality (the ability to drag a bookmark keyword to the tab). So I think we need to keep those. It would also be nice to have a test for this particular change that covers the issues I've pointed out, if there isn't one already. browser/base/content/test/browser_drag.js seems to do something like this already, you could use that as inspiration. Feel free to find me or anyone else on IRC ("gavin" on #fx-team) if you have further questions about testing.

Imagine a case where the user has saved either a bookmark keyword or search keyword as 'abc', if the text 'abc 123' would be dropped from a page wouldn't this trigger a bookmark/search jump with the getShortcutOrURI? Should it be that way or what would be a better fix?

(In reply to Lukas Nordin from comment #17) > Imagine a case where the user has saved either a bookmark keyword or search > keyword as 'abc', if the text 'abc 123' would be dropped from a page > wouldn't this trigger a bookmark/search jump with the getShortcutOrURI? > > Should it be that way I don't think it should be that way. The dragged text is remote content whereas keywords are user-defined and supposed to be typed by the user. The idea that web content would intentionally include keywords for the user to drag them seems backwards, and the idea that web content would unintentionally and randomly include keywords that would automatically do the right thing when dragged to the UI seems pretty sketchy.

Comment on attachment 721183 [details] [diff] [review] Updated patch using thirdpartyfixup This looks good to me.

Well, sure, I guess we can revisit whether it's useful to allow dropping keywords, but that seems rather orthogonal to this bug as summarized.

As I understand it, comment 0 wasn't even talking about keywords (although its last paragraph refers to keywords in a way that I don't fully understand), and then comment 1 claimed that this bug would be about keywords too. So I suggest we morph this back to what it was filed on.

The test browser_tabDrop.js already checked if a url with spaces was being dropped so i modified it to count it like a passed test instead.

Comment on attachment 728278 [details] [diff] [review] Bug fix and testcase This version removes one getShortcutOrURI call but leaves the other. I guess I don't feel strongly which way we go (remove them or leave them), but either way we should be consistent and remove both or leave both. Looks good otherwise, though.

Oops missed that one, hehe. The attached patch has no calls to getShortcutOrURI.

Comment on attachment 728619 [details] [diff] [review] Fix and test case UPDATED Looks good - thanks for sticking with it Lukas :)