313139 - "Search Web for" context menu item joins last two lines in selection but eliminates whitespace

Status: RESOLVED DUPLICATE of bug 270297

(Firefox :: Menus, defect)

Description

[Ted Gifford]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.12) Gecko/20050915 Firefox/1.0.7
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.12) Gecko/20050915 Firefox/1.0.7

At browser/base/content/browser.js:4312, we find the searchSelected function,
which is called by the Search Web for context-menu command. searchSelected
appears to try to join the lines in the selection with this line:

        searchStr = searchStr.replace(/\s*(.*?)\s*$/, "$1");

This only joins the last two lines, and it does it without putting a space in
between them. A simple fix is to add the g and m flags to the regexp, and a
space in the replacement string.

        searchStr = searchStr.replace(/\s*(.*?)\s*$/gm, "$1 ");

An even simpler (but wrong?) solution is to leave that line of code out
entirely. It works fine with google, but it might cause problems if another
search engine ignores query terms following a newline.

Probably the clearest solution is to do .replace(/\r\n|\n/gm," "), which
explicitly replaces newlines with spaces. 

The string displayed in the context menu should also be adjusted.

See patch in additional info

Reproducible: Always

Steps to Reproduce:
1. Select a few (short) lines of text in the browser window. Let's say you selected:

line1
line2
line3

2. Right-click and choose "Search Web for line1line2line3"

Actual Results:  
New tab with search results is opened, but query string contains incorrectly
joined lines: "line1%0D%0Aline2line3".

Expected Results:  
New tab with search results is opened, query string "line1 line2 line3".

--- browser/base/content/browser.js.bak 2005-07-07 06:21:06.000000000 -0400
+++ browser/base/content/browser.js     2005-10-20 09:32:23.170461600 -0400
@@ -4297,7 +4297,7 @@
 
         var searchSelectText;
         if (selection) {
-            searchSelectText = selection.toString();
+            searchSelectText = selection.toString().replace(/\r\n|\n/gm," ");
             if (searchSelectText.length > 15)
                 searchSelectText = searchSelectText.substr(0,15) + "...";
             result = true;
@@ -4322,7 +4322,7 @@
             pattern.test(searchStr);
             searchStr = RegExp.lastMatch;
         }
-        searchStr = searchStr.replace(/\s*(.*?)\s*$/, "$1");
+        searchStr = searchStr.replace(/\r\n|\n/gm," ");
         searchStr = searchStr.replace(/\s+/g, " ");
         return searchStr;
     },

Comment 1

[Elmar Ludwig]

> searchSelected appears to try to join the lines in the selection with this line

No, it doesn't. The line you quoted just removes leading and trailing whitespace.
The lines are joined by:

searchStr = searchStr.replace(/\s+/g, " ");

And this correctly inserts spaces in place of the original line breaks. The
steps to reproduce the behaviour you descibe work fine for me (on Linux with
the Firefox 1.5 beta 2 release). I get this search URL:

http://www.google.com/search?lr=&ie=UTF-8&oe=UTF-8&q=line1%20line2%20line3

You might want to try this on a newer build.

Comment 2

[Ted Gifford]

(In reply to comment #1)
> No, it doesn't.

Damnit you're right--I cooked up the line[123] example without testing it.
However, I'm still right about the code.

Select 

line1
line2
line3

and then do a javascript:alert(escape(window.getSelection().toString())), and
you'll see that there were no newlines to start with. Weird.

Now go to http://sanfrancisco.citysearch.com/profile/11489725/, and select the
address, i.e. 

150 Greenwich St
San Francisco, CA 94111

Do the context web search, and I get "150 Greenwich StSan Francisco, CA 94111"
for the query string.

Comment 3

[Ted Gifford]

Forgot to mention this is confirmed with 1.5 beta 2.

Comment 4

[Ted Gifford]

Looks like I should submit an additional bug: getSelection() converts newlines
to spaces in <pre/> elements.

Comment 5

[Ted Gifford]

Don't need the additional bug, the getSelection <pre/>  bug is #235937

Comment 6

[Elmar Ludwig]

Ok, I see this now. This is in fact caused by replace(/\s*(.*?)\s*$/, "$1") not
doing the right thing when the selection contains a newline in the middle of the
text. Your patch "fixes" this by removing this line from the code, but then any
leading and trailing whitespace will be included in the search string.

I'm marking this bug as a duplicate of bug 270297, so further discussions should
take place over there.

*** This bug has been marked as a duplicate of 270297 ***