252441 - cannot drag&drop text containing spaces to the tab bar

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[Oomingmak]

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.

Comment 1

[kstahl]

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)

Comment 2

[Martijn Wargers (dead)]

Attachment: patch

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

Comment 3

[Mike Connor [:mconnor]]

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.

Comment 4

[:Gavin Sharp [email: [e-mail]]]

(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 :).

Comment 5

[Martijn Wargers (dead)]

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

Comment 6

[:Gavin Sharp [email: [e-mail]]]

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 7

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 199844 [details] [diff] [review]
patch

Obsoleting per Mike's comment.

Comment 8

[Uri Bernstein]

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).

Comment 9

[:Gavin Sharp [email: [e-mail]]]

(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.

Comment 10

[Uri Bernstein]

(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").

Comment 11

[:Gavin Sharp [email: [e-mail]]]

If someone else wants to take this, please do.

Comment 12

[Lukas Nordin]

Attachment: Dropping text containing spaces will now perform a search on the given sentence

Better late than never!

This patch will make the drop perform a search using the current search engine if possible.

Comment 13

[:Gavin Sharp [email: [e-mail]]]

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.

Comment 14

[Lukas Nordin]

Attachment: Updated patch using thirdpartyfixup

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

Comment 15

[Lukas Nordin]

review ping?

Comment 16

[:Gavin Sharp [email: [e-mail]]]

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.

Comment 17

[Lukas Nordin]

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?

Comment 18

[Dão Gottwald [:dao]]

(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 19

[Dão Gottwald [:dao]]

Comment on attachment 721183 [details] [diff] [review]
Updated patch using thirdpartyfixup

This looks good to me.

Comment 20

[:Gavin Sharp [email: [e-mail]]]

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.

Comment 21

[Dão Gottwald [:dao]]

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.

Comment 22

[Lukas Nordin]

Attachment: Bug fix and testcase

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 23

[:Gavin Sharp [email: [e-mail]]]

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.

Comment 24

[Lukas Nordin]

Attachment: Fix and test case UPDATED

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

Comment 25

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 728619 [details] [diff] [review]
Fix and test case UPDATED

Looks good - thanks for sticking with it Lukas :)

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a3acc0eaed0c

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a3acc0eaed0c