270297 - "search web for" selection ignores newline

Status: VERIFIED FIXED

(SeaMonkey :: Search, defect)

Description

[David desJardins]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.5) Gecko/20041107 Firefox/1.0
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.5) Gecko/20041107 Firefox/1.0

When I select a region containing a newline, then I right-click and choose the
menu option "Search web for <selection>", the search string sent to Google omits
the newline, so Google treats the last word on the first line and the first word
on the second line as a single word.  This is a bug in how Mozilla/Firebird is
constructing the search URL for Google.  Either substituting a space for the
newline (%20), or embedding an actual newline (%0A) will work.

Reproducible: Always
Steps to Reproduce:
1. Select text containing a newline.
2. Right click.
3. Select menu option "Search web for ...".

Actual Results:  
Search is performed but the search string is garbled.

Expected Results:  
Embed newline or space in the search string as a delimiter.

Comment 1

[Adam M. Costello]

I just noticed the same thing.  I never had much use for "search the web for
<selection>" until Google started accepting addresses and providing links to
maps.  This Google feature, combined with Firefox's search-the-web-for-selection
feature, are a match made in heaven, except that addresses are usually split
across multiple lines and therefore fall prey to this bug.

Comment 2

[Bart Kuik]

I often use this feature for web shops that don't host information for their
products. I select the brand and model, rightclick and 'Search Web For'. Some
shops use brand<br/>model, or <h1>brand</h1><h2>model</h2> or so, and I really
think that the 'Search Web For' function should treat the tags between words as
spaces, unless the sequence start with "http://", "ftp://" or "www.". I don't
think that searching for URL's is a reason to keep this feature broken.

So:

if search term qualifies as url: don't insert whitespaces and go to the url directly
else : replace tags with whitespace and search with Google.

Comment 3

[Adam Guthrie]

*** Bug 303153 has been marked as a duplicate of this bug. ***

Comment 4

[Elmar Ludwig]

*** Bug 313139 has been marked as a duplicate of this bug. ***

Comment 5

[David desJardins]

(continuing the discussion from bug 313139)

So, can we just change

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

to

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

as the easiest way to do the intended thing (remove leading and trailing 
whtespace) without the unintended consequences when the text contains newline 
characters?

Comment 6

[Elmar Ludwig]

Yes, something like this would work. I have a patch ready.

This was broken by the checkin for bug 221361 (see also the last comment there).

Comment 7

[Elmar Ludwig]

Attachment: simple patch

This patch restores two lines that were replaced by the checkin for bug 221361
to fix the newline issues here. The current code using the regular expression
/\s*(.*?)\s*$/ does not work correctly because:

1. The pattern is not anchored at the front, causing a match to start at an
   embedded newline just before the last line in the string (and removing the
   newline in the process).

2. Since "." does _not_ match newlines, the pattern will not match the complete

   string if the string contains embedded newlines, so that whitespace at the
   start is not removed.

Comment 8

[Elmar Ludwig]

CC'ing some people from bug 221361 since that was the cause of this bug.

Comment 9

[[e-mail]]

Comment on attachment 200285 [details] [diff] [review]
simple patch

D'oh!

Comment 10

[Elmar Ludwig]

Neil: Can you (or someone else?) please check this into the trunk?

I don't have CVS write access.

Comment 11

[[e-mail]]

I would have checked in earlier, but I was too busy watching TV :-P

Comment 12

[Brendan Eich [:brendan]]

You could use just one regexp, I think, by using the m(ultiline) flag:

js> s = "hello\nworld\n"
hello
world

js> s.replace(/\s*(.*?)\s*$/m, "$1")
hello
world

Should this bug be fixed for 1.8?

/be

Comment 13

[Elmar Ludwig]

> You could use just one regexp, I think, by using the m(ultiline) flag:

No. If you test this, you will see that trailing whitespace is not removed,
since it matches only on the first line (and adding a "g" modifier would be
even worse):

js> s="hello\nworld\n   "; s.replace(/\s*(.*?)\s*$/m, "$1")+"!"
hello
world
   !

That said, this _could_ be done in one pattern if you used this:

s.replace(/^\s*([\s\S]*?)\s*$/, "$1");    // note: anchored and no "m" flag

But I am still against using such a pattern, because:

- Trying to be too clever with regular expressions like this makes the code
  hard to read and may even introduce subtle bugs (like seen in bug 221361).
  Not everyone touching this code is an expert in regular expressions...

- Patterns that use non-greedy quantifiers and that are not well constructed
  are often slower than greedy patterns, so I suspect that using two simple
  replace() calls may even be faster here than one more complex call.

Comment 14

[Elmar Ludwig]

Comment on attachment 200285 [details] [diff] [review]
simple patch

Brendan Eich wrote:
> Should this bug be fixed for 1.8?

Trying to get branch approval. This is a simple low risk patch for a
regression.

Comment 15

[Brendan Eich [:brendan]]

Good points -- I'm plusing the bug, its patch should get approved shortly.

/be

Comment 16

[Elmar Ludwig]

Verified fixed using yesterday's trunk and branch builds on the testcase from
duplicate bug 313139.