Last Comment Bug 252441 - cannot drag&drop text containing spaces to the tab bar
: cannot drag&drop text containing spaces to the tab bar
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Tabbed Browser (show other bugs)
: Trunk
: All All
: -- normal with 4 votes (vote)
: Firefox 22
Assigned To: Lukas Nordin
:
Mentors:
: 727523 (view as bug list)
Depends on:
Blocks: 138245
  Show dependency treegraph
 
Reported: 2004-07-21 06:32 PDT by Oomingmak
Modified: 2013-11-15 13:29 PST (History)
11 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (1.40 KB, patch)
2005-10-17 13:58 PDT, Martijn Wargers [:mwargers] (not working for Mozilla)
no flags Details | Diff | Splinter Review
Dropping text containing spaces will now perform a search on the given sentence (2.29 KB, patch)
2013-03-04 10:41 PST, Lukas Nordin
gavin.sharp: review-
Details | Diff | Splinter Review
Updated patch using thirdpartyfixup (1.91 KB, patch)
2013-03-05 04:23 PST, Lukas Nordin
gavin.sharp: review-
dao+bmo: feedback+
Details | Diff | Splinter Review
Bug fix and testcase (2.53 KB, patch)
2013-03-22 09:39 PDT, Lukas Nordin
gavin.sharp: review-
Details | Diff | Splinter Review
Fix and test case UPDATED (2.52 KB, patch)
2013-03-23 06:58 PDT, Lukas Nordin
gavin.sharp: review+
Details | Diff | Splinter Review

Description Oomingmak 2004-07-21 06:32:46 PDT
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 2004-09-05 05:11:03 PDT
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 [:mwargers] (not working for Mozilla) 2005-10-17 13:58:52 PDT
Created attachment 199844 [details] [diff] [review]
patch

Well, this fixes it (like the url already pointed out).
Comment 3 Mike Connor [:mconnor] 2005-10-17 18:05:50 PDT
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: gavin@gavinsharp.com] 2005-10-18 05:31:32 PDT
(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 [:mwargers] (not working for Mozilla) 2005-10-18 08:24:50 PDT
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: gavin@gavinsharp.com] 2005-10-18 08:54:44 PDT
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: gavin@gavinsharp.com] 2005-10-26 08:49:52 PDT
Comment on attachment 199844 [details] [diff] [review]
patch

Obsoleting per Mike's comment.
Comment 8 Uri Bernstein (Google) 2005-10-26 09:58:52 PDT
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: gavin@gavinsharp.com] 2005-10-26 10:00:46 PDT
(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 (Google) 2005-10-26 10:28:10 PDT
(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: gavin@gavinsharp.com] 2006-04-18 06:25:09 PDT
If someone else wants to take this, please do.
Comment 12 Lukas Nordin 2013-03-04 10:41:37 PST
Created attachment 720802 [details] [diff] [review]
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: gavin@gavinsharp.com] 2013-03-04 16:52:06 PST
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 2013-03-05 04:23:09 PST
Created attachment 721183 [details] [diff] [review]
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 2013-03-13 13:21:30 PDT
review ping?
Comment 16 :Gavin Sharp [email: gavin@gavinsharp.com] 2013-03-14 16:04:30 PDT
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 2013-03-15 05:55:37 PDT
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] 2013-03-15 06:09:38 PDT
(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] 2013-03-15 06:11:21 PDT
Comment on attachment 721183 [details] [diff] [review]
Updated patch using thirdpartyfixup

This looks good to me.
Comment 20 :Gavin Sharp [email: gavin@gavinsharp.com] 2013-03-15 12:07:46 PDT
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] 2013-03-15 12:20:54 PDT
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 2013-03-22 09:39:32 PDT
Created attachment 728278 [details] [diff] [review]
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: gavin@gavinsharp.com] 2013-03-22 18:56:11 PDT
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 2013-03-23 06:58:36 PDT
Created attachment 728619 [details] [diff] [review]
Fix and test case UPDATED

Oops missed that one, hehe. The attached patch has no calls to getShortcutOrURI.
Comment 25 :Gavin Sharp [email: gavin@gavinsharp.com] 2013-03-25 14:18:27 PDT
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] 2013-03-27 10:59:55 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/a3acc0eaed0c
Comment 27 Ryan VanderMeulen [:RyanVM] 2013-03-27 18:59:22 PDT
https://hg.mozilla.org/mozilla-central/rev/a3acc0eaed0c
Comment 28 [:Aleksej] 2013-11-15 13:29:50 PST
*** Bug 727523 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.