673157 - Drag selected text and drop in Firefox will not cause a search

Status: RESOLVED WONTFIX

(Firefox :: General, defect)

Description

[ebrahim]

Attachment: draganddroptosearch.png

User Agent: Mozilla/5.0 (Windows NT 6.1; rv:8.0a1) Gecko/20110720 Firefox/8.0a1
Build ID: 20110720030844

Steps to reproduce:

I dragged a text from a webpage inside Firefox and dropped that to Firefox tab bar.


Actual results:

Firefox can not detect it is not an URI and is a text and must be searched in my web search engine instead asked from DNS server.


Expected results:

I want dropped text to be searched with my default search engine.
Thanks :)

Comment 1

[Brian R. Bondy [:bbondy]]

Confirmed

Comment 2

[Brian R. Bondy [:bbondy]]

Attachment: Patch for drag selected text and drop in Firefox will not cause a search

Description of task:
Now when you drop something that is not a URL on a tab (or at some position in the tabbar to create a new tab), it will do a search with your default search engine.
This behavior is consistent with how the urlbar itself works when something is typed in which is not a URL.

Description of mochitest for task:
- It will simulate a drop of text using all of the different drop types.
- It will add a default search engine so that tests don't get run to google.com and instead go to mochi.test:8888
- For each drop type, it will verify that the loaded page has the right URL (i.e if it is not a URI it should contain the search term info)
- This test fully passes (27 pass) with this patch and fails without this patch applied.

Comment 3

[Brian R. Bondy [:bbondy]]

Attachment: Rebased patch for drag selected text and drop in Firefox will not cause a search

Had to rebase for mozilla-central changes

Comment 4

[Marco Bonardo [:mak]]

Comment on attachment 549540 [details] [diff] [review]
Rebased patch for drag selected text and drop in Firefox will not cause a search

Review of attachment 549540 [details] [diff] [review]:
-----------------------------------------------------------------

I can give you some feedback but I'm not the best person to review this, gavin is probably the best one considering the code you are touching and the fact I'm not sure if we want to take the privacy implications of passing selected text to a third party without a sort of confirmation (like enter in the locationbar), sure dropping is a clear action but it's easier to wrongly drop something.
This seems to also allow fixup for any other invalid url dropped, that may be unwanted.

::: browser/base/content/tabbrowser.xml
@@ +3855,5 @@
>          let dt = event.dataTransfer;
>          if (dt.dropEffect != "link")
>            return;
>  
> +        let uri = browserDragAndDrop.drop(event, { });

please avoid changing names or indentation unless it is really necessary, since that is going to:
1. destroy code blame that may be useful
2. increase size of the patch (thus time needed to review it)

Moreover in this case uri makes me think this returns an nsIURI, that it not the case.

@@ +3863,5 @@
>            return;
> +        }
> +
> +        let bgLoad = Services.prefs
> +                             .getBoolPref("browser.tabs.loadInBackground");

ditto on unneded changes

::: browser/base/content/test/browser_bug673157.js
@@ +41,5 @@
> +var testIndex = -1;
> +// holds the a custom engine which will return results to mochi.test
> +var engine;
> +// Holds the preference service and search service
> +var searchSvc, prefSvc;

you don't need these, use Services shortcuts instead

@@ +53,5 @@
> +  function observer(aSubject, aTopic, aData) {
> +    switch (aData) {
> +      case "engine-added":
> +        let engineName = "Bug 673157";
> +        gEngine = searchSvc.getEngineByName(engineName);

you don't have any gEngine, you have a global engine var...

@@ +55,5 @@
> +      case "engine-added":
> +        let engineName = "Bug 673157";
> +        gEngine = searchSvc.getEngineByName(engineName);
> +        prefSvc = Cc["@mozilla.org/preferences-service;1"]
> +                  .getService(Ci.nsIPrefBranch);

Use Services.prefs

@@ +56,5 @@
> +        let engineName = "Bug 673157";
> +        gEngine = searchSvc.getEngineByName(engineName);
> +        prefSvc = Cc["@mozilla.org/preferences-service;1"]
> +                  .getService(Ci.nsIPrefBranch);
> +        let pref = "browser.search.defaultenginename";

put this into a const DEFAULT_SEARCHENGINE_PREF

@@ +58,5 @@
> +        prefSvc = Cc["@mozilla.org/preferences-service;1"]
> +                  .getService(Ci.nsIPrefBranch);
> +        let pref = "browser.search.defaultenginename";
> +        oldDefaultSearch = prefSvc.getCharPref(pref);
> +        prefSvc.setCharPref(pref, engineName);

rather here use registerCleanupFunction to restore the original value, you won't need a global variable and if your test timeouts it will ensure next tests won't fail due to this

@@ +69,5 @@
> +  }
> +  
> +  // Obtain the search service and get what the URL bar should contain
> +  searchSvc = Components.classes["@mozilla.org/browser/search-service;1"]
> +              .getService(Components.interfaces.nsIBrowserSearchService);

Services.search

@@ +73,5 @@
> +              .getService(Components.interfaces.nsIBrowserSearchService);
> +
> +  // Add an engine that will have search results in the http://mochi.test host 
> +  Services.obs.addObserver(observer, "browser-search-engine-modified", false);
> +  let u = "http://mochi.test:8888/browser/browser/base/content/test/673157.xml";

name variables in some meaningful way... btw this should be a const

@@ +80,5 @@
> +}
> +
> +function test() {
> +  waitForExplicitFinish();
> +  initSearchEngine(); // will also kick off the tests by calling initTests()

I'd rather add a callback argument so it's invoked like initSearchEngine(initTests);

@@ +89,5 @@
> +  // Setup a series of tests for each drag type for dropping on the tab bar
> +  var mimeTypes = ["text/plain", "text/unicode", "text/x-moz-url"];
> +  var effects = ["move", "copy", "link"];
> +  for (e in effects) {
> +    for (m in mimeTypes) {

for good code practice, don't use for..in on arrays

@@ +160,5 @@
> +  ++testIndex;
> +  var currentTest = tests[testIndex];
> +  if (!currentTest) {
> +    searchSvc.removeEngine(gEngine);
> +    let pref = "browser.search.defaultenginename";

you use this twice, a const can't really hurt

@@ +166,5 @@
> +    finish();
> +    return;
> +  }
> +
> +  gBrowser.addEventListener("load", validateTest, true);

would be better to wait for load on your specific tab.

@@ +168,5 @@
> +  }
> +
> +  gBrowser.addEventListener("load", validateTest, true);
> +  
> +  // Simulate a dorp of the passed in term

typo: dorp

Comment 5

[Brian R. Bondy [:bbondy]]

Thanks for the review comments, I read them over.  Before applying them I wait for Gavin's thoughts on the task itself. 

How other browsers work: 
Chrome12: Supports drop of text and hyperlinks, text goes to default search engine.
IE9: supports drop of hyperlinked URLs but not text URLs nor text.
Safari5.1: Supports dropped hyperlinks and text URLs but not text searches.
Opera11.5: Does not support drag and drop at all on tabs.
 
Also worth noting, in Firefox, when you drop a URL in the URL bar it loads the site.  When you drop text in the URL bar it accepts the text but does not start the search.  

For what it's worth, this is a feature I would use, but I understand your concerns.

Comment 6

[Brian R. Bondy [:bbondy]]

Just an idea, perhaps we could enable this via a preference that is disabled by default.

Comment 8

[Nicolas Barbulesco]

Why having this feature disabled ?

Comment 9

[Brian R. Bondy [:bbondy]]

Disabled was just said based on Comment 4. I personally like the feature.

Comment 10

[ebrahim]

With fix of 673080 (thanks for it!), seems Firefox sometimes accepts drop selected text (not link) from an external application and sometimes not (not for search of course!).
I just want mention that drop text (not link) from an external application (e.g. google talk) must be considered in patches and tests.
Thanks :)

Comment 11

[Brian R. Bondy [:bbondy]]

ebraminio, that is the patch provided in this task but it seems there are some  privacy implications as per Comment 4 that need approval before we can do this.

Comment 12

[ebrahim]

Oh, okay, I understand. I just wanted combination of this request and my request on 673080 that together mean, drop texts(not links) from external application to Firefox, considered in tests, nothing more :)
However I can not understand what is difference of making query from DNS Server with from default search engine in view of user privacy..., but not important :)
Thank you :)

Comment 13

[Brian R. Bondy [:bbondy]]

New drag and drop stuff in HTML5 allows you to simply drop a file and the page can read the contents of the file even.  So I don't think that dropping text to go to default search engine is a big deal personally. 

Awaiting your input whenever you get a chance Gavin for want/don't want this as per Comment 0.

Comment 14

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

Comment on attachment 549540 [details] [diff] [review]
Rebased patch for drag selected text and drop in Firefox will not cause a search

I'm not really the best person to answer that question - someone from the UX team probably is.

Comment 15

[Alex Faaborg [:faaborg] (Firefox UX)]

Comment on attachment 549540 [details] [diff] [review]
Rebased patch for drag selected text and drop in Firefox will not cause a search

I'm going to recommend that we go with dragging the text to the search or location bars.  The tab strip seems like it is more likely to pick up unintentional drops than intentional ones, and firing the search off without a more explicit action (like the go or google button) could create some unintentional data leakage that user's wouldn't always appreciate.

Comment 16

[Brian R. Bondy [:bbondy]]

Thanks for the input Alex.
Marking this as WONTFIX based on comment 15.

Comment 17

[ebrahim]

Okay. Can we have this as an option in Firefox (in about:config for example) or an add-on?
Thanks :)

Comment 18

[Brian R. Bondy [:bbondy]]

ebraminio: Not as an option in about:config built in, but someone is free to do an addon for it.

Comment 19

[Brian R. Bondy [:bbondy]]

I cannot myself but if you find an extension developer then they can look at the attached obsolte patch as a starting point.

Comment 20

[Nicolas Barbulesco]

What about a security confirmation ? In which we could say yes just for this dragging. And we could say yes once and for all in the prefs or in a checkbox of this security confirmation.