cannot drag&drop text containing spaces to the tab bar

RESOLVED FIXED in Firefox 22

Status

()

Firefox
Tabbed Browser
RESOLVED FIXED
13 years ago
4 years ago

People

(Reporter: Oomingmak, Assigned: Lukas Nordin)

Tracking

Trunk
Firefox 22
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

13 years ago
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

13 years ago
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)

Updated

13 years ago
Assignee: firefox → p_ch
Status: UNCONFIRMED → NEW
Component: General → Search
Ever confirmed: true
QA Contact: firefox.general → firefox.search
Assignee: p_ch → gavin.sharp
OS: Windows 2000 → All
Hardware: PC → All
Summary: Trailing space breaks quick search from Tab Bar (i.e. results in new tab) → cannot drag&drop text containing spaces to the tab bar (bookmark keywords, quick search)
Target Milestone: --- → Firefox1.6-
Version: unspecified → Trunk
Created attachment 199844 [details] [diff] [review]
patch

Well, this fixes it (like the url already pointed out).
Attachment #199844 - Flags: review?(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.
(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.
Attachment #199844 - Attachment is obsolete: true
Attachment #199844 - Flags: review?(mconnor)
Status: NEW → ASSIGNED
Priority: -- → P2
Blocks: 138245
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").
Priority: P2 → P4
If someone else wants to take this, please do.
Component: Search → Location Bar and Autocomplete
QA Contact: search → location.bar
Priority: P4 → --
Target Milestone: Firefox 2 → ---
Assignee: gavin.sharp → nobody
Status: ASSIGNED → NEW
(Assignee)

Comment 12

4 years ago
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.
Attachment #720802 - Flags: review?
(Assignee)

Updated

4 years ago
Attachment #720802 - Flags: review? → review?(gavin.sharp)
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.
Attachment #720802 - Flags: review?(gavin.sharp) → review-
(Assignee)

Comment 14

4 years ago
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?
Attachment #720802 - Attachment is obsolete: true
Attachment #721183 - Flags: review?(gavin.sharp)
(Assignee)

Comment 15

4 years ago
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.
Attachment #721183 - Flags: review?(gavin.sharp) → review-
(Assignee)

Comment 17

4 years ago
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.
Attachment #721183 - Flags: feedback+
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.

Updated

4 years ago
Summary: cannot drag&drop text containing spaces to the tab bar (bookmark keywords, quick search) → cannot drag&drop text containing spaces to the tab bar

Updated

4 years ago
Assignee: nobody → lukasnordin11

Updated

4 years ago
Component: Location Bar → Tabbed Browser
(Assignee)

Comment 22

4 years ago
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.
Attachment #721183 - Attachment is obsolete: true
Attachment #728278 - Flags: review?(gavin.sharp)
Attachment #728278 - Flags: review?(dao)
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.
Attachment #728278 - Flags: review?(gavin.sharp)
Attachment #728278 - Flags: review?(dao)
Attachment #728278 - Flags: review-
(Assignee)

Comment 24

4 years ago
Created attachment 728619 [details] [diff] [review]
Fix and test case UPDATED

Oops missed that one, hehe. The attached patch has no calls to getShortcutOrURI.
Attachment #728278 - Attachment is obsolete: true
Attachment #728619 - Flags: review?(gavin.sharp)
Comment on attachment 728619 [details] [diff] [review]
Fix and test case UPDATED

Looks good - thanks for sticking with it Lukas :)
Attachment #728619 - Flags: review?(gavin.sharp) → review+
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/a3acc0eaed0c
Flags: in-testsuite+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/a3acc0eaed0c
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 22

Updated

4 years ago
Duplicate of this bug: 727523
You need to log in before you can comment on or make changes to this bug.