55052 - search text field changes what I type

Status: VERIFIED FIXED

(SeaMonkey :: Search, defect, P3)

Description

[Warren Harris]

If I go to google and search for something, the search sidebar panel
automatically springs open and lets me see elements inserted into it 2 or 3
times. Bug 1: This shouldn't be automatic -- the user should control it.

If I try to drag select the text in the Search input field in the sidebar to
type something else, I can't select anything. I have to first click in the field
to select it (and it gets a slightly thicker border) and then I can drag select
the text. Bug 2: Drag selecting should automatically select the input field.

If my search input field says "roofing" and I augment it to say "+wallace
roofing" and hit return, it will change my text to just "wallace roofing"
dropping the '+'. Bug 3: It shouldn't change what I type.

After hitting return to search for "+wallace roofing" I see the results fill up
5 or 6 times before stopping. And every time the panel lays out again, the set
of results seems to be different. Bug 3: List of results should just be appended
to, and shouldn't relayout so many times.

Comment 1

[Warren Harris]

Oops, make that last point bug 4.

Comment 2

[Claudius Gayle]

c'mon warren, are you venting or are you actually hoping this 'bug' becomes a muddled mess
that is impossible to resolve/qa/verify? Anyway...

issue #1: the user can control this, Edit|Preferences, Navigator|Internet Search. Uncheck
"Open the Search tab in My Sidebar when search results are available"
this pref stopped working recently and the regression has been filed as a bug.

issue #2 That's a textfield like any other in the app right, nothing special? Regardless, this
WFM with the 2000100408 branch builds. I do note that the textfield has the bold 'focused'
style all the time for me though. Well, not really, now I notice the focused style goes away
when another widget in the panel gets focus - so no problems whatsoever.

issue #3 Wierd search voodoo. actually this is a real bug. I do note that the search query
is built correctly (includes your '+') this '+' is replaced by a space in the search text field.

issue #4 Yup, that's already written up too, strangely it's more pronounced on a single search
than multiple.

I once thought I saw #2, it looked like a drag initiated but I haven't seen it in 30 tries since.

OK so that leaves us with issue #3 as the only new bug, morphing this bug.
Any other issue should be written as a separate issue/bug please :-)

Comment 3

[Warren Harris]

I'm not venting... just calling them like I see them. I can reproduce the drag 
problem every time (start from the left edge of the text).

I had no idea about the pref for this panel. Seems like the default is wrong, 
or maybe the pref should have a checkbox right on the panel.

Comment 4

[Claudius Gayle]

ok I can do the drag thing (issue #2) but that really needs to be a separate bug. It seems
there are a few pixels to the left of where the text begins, extending to outside the textfield
where your cursor will change to an I-bar but the text still won't pick up the selection. So, to
see this (other)bug start the cursor to the left of the textfield and try to select as soon as the
cursor changes to an I-bar. I believe this to be a separate and general ender bug - I'll look
into that.

Comment 5

[Marcia Knous [:marcia]]

Netscape Nav triage team: this is a Netscape beta stopper.

Comment 6

[Viswanath Ramachandran]

reassigning to morse

Comment 7

[Paul Chen]

nav triage team:

Marking nsbeta1+

Comment 8

[Viswanath Ramachandran]

nav triage team: steve, could you schedule this into mozilla0.8 or mozilla0.9 as 
it is a beta stopper. thanks, Vishy

Comment 9

[Stephen P. Morse]

Seems like this is being done by design.  See specifically the following code 
which occurs twice in xpfe/components/search/resources/search-panel.js

   // convert pluses (+) back to spaces
   target = target.replace(/+/i, " ");
   textNode.value = unescape(target);

If you don't want the pluses to be changed, these lines should be removed.  If 
you do want them to be replaced and there is more than one (note the plural on 
"pluses"), then the /i should be replaced with /g.

The converting of pluses to spaces was added by rjc on April 2, 2000.  
Reassigning to rjc so he can determine which he meant.

Comment 10

[Robert John Churchill]

As I'm sure we all know, spaces often get converted to pluses when encoded in a 
URL.  The code in question attempts to undo that.  In Warren's case, searching 
for a string with a "+" in it should have encoded the plus (perhaps to a hex 
value)... why that didn't happen, I don't know, however I believe that the code 
is probably correct as is.

Moving this bug back to morse.

Comment 11

[Stephen P. Morse]

OK, if you want to do the plus decoding, you need to separate out the case of 
the searchstring coming from the URL or coming from what the user typed into the 
sidebar search text.  Furthermore, the .replace has to have a /g instead of a /i 
so it can do a global replace (otherwise it is replacing only the first + in the 
url).

Attaching a patch that does both of these.

Comment 12

[Stephen P. Morse]

Attachment: do a global replace of +, and do it only if it came from URL

Comment 13

[Stephen P. Morse]

Attachment: same as previous patch but with better formating

Comment 14

[Stephen P. Morse]

Ignore previous patches.  Upon further investigation I now realize that they are 
wrong.  (There is one part about them that is correct and that is the changing 
the /i to /g in the regular expression for the .replace.)

The problem is being caused by the following code in FindInternetSearchResults 
routine of nsInternetSearchService.cpp:

  nsCAutoString escapedSearchText;
  escapedSearchText.AssignWithConversion(searchText);
  nsCAutoString aCharset;
  aCharset.AssignWithConversion(mQueryEncodingStr);
  PRUnichar *uni = nsnull;
  if (NS_SUCCEEDED(rv = textToSubURI->UnEscapeAndConvert
      (aCharset, escapedSearchText, &uni)) && (uni)) {
    char *convertedSearchText = nsnull;
    if (NS_SUCCEEDED(rv = textToSubURI->ConvertAndEscape
        ("UTF-8", uni, &convertedSearchText))) {
      searchText.AssignWithConversion(convertedSearchText);
      Recycle(convertedSearchText);
    }
    Recycle(uni);
  }

The above code is doing an unescape followed by an escape.  Seems harmless on 
the surface.  But consider the case of the original string being:

   a+%2B+B

After doing the escape we get

   a%2B%2B%2Bb

and then the unescape gives us

   a+++b

This is the source of the confustion between quoted pluses and normal pluses and 
is the reason we are stripping off all pluses in the sidebar search texfield.

Comment 15

[Stephen P. Morse]

Certainly by removing this escape/unescape, the bug reported here goes away.  
This code was added by nhotta to fix bug 42221, so we can't simply remove it.  
I'll investigate further to try to find a patch that corrects this problem 
without reintroducing bug 42221.

Comment 16

[Stephen P. Morse]

Actually my example above was a little wrong.  The code is doing unescape 
before escape.  So the corrected example that illustrates the problem is:


Consider the case of the original string being:

   a+%2B+b

After doing the unescape we get

   a+++b

and then the escape gives us

   a%2B%2B%2Bb

The conclusion is the same -- quoted pluses and regular pluses are no longer 
distinguishable.

Comment 17

[Stephen P. Morse]

I now have the patch that I'm about to attach.  It corrects three problems:

1. Preserves distinction between quoted pluses and regular pluses (that's the 
change to nsInternetSearchService.cpp).

2. Preserves all pluses, not just the first.  That's the change from /i to /g in 
search-panel.js.  This problem was being hidden by the previous one, which is 
why nobody every noticed it.

3. Avoids a false update in the sidebar search textfield.  Otherwise the pluses 
will be erased initially and then restored after the search was completed.  This 
problem always existed but it wasn't noticed as long as the same string was 
being written in both times.  Now that the plus problem has been fixed, the 
double updating becomes apparent.

Comment 18

[Stephen P. Morse]

Attachment: Patch that corrects the underlying problem (previous patches did not)

Comment 19

[nhottanscp]

r=nhotta

Comment 20

[Stephen P. Morse]

cc'ing alecf for super review.

Comment 21

[Alec Flett]

ok, the fixing of + and " " seem fine, but I don't like what you're doing with
the whole /0 /1 thing... it will work, sure, but what if some future code
encounters this string with this proprietary escaping mechanism? Why can't we
re-escape the "%" in %2B with "%25"?

I'm thinking something like re-escaping the whole string 
escapedString.ReplaceSubstring("%25", "%2B25");
escapedString.ReplaceSubstring("+", "%25");

and then to reverse it:

escapedString.ReplaceSubstring("+", "%25");
escapedString.ReplaceSubstring("%2B25", "%25");

My technique may be ugly, but I'd like to find something less proprietary than
the /0 /1 magic tokens to solve this.

Comment 22

[Stephen P. Morse]

I have absolutely no objection to the encoding/decoding that you are proposing.  
But I don't understand why.  It's certainly much uglier and less untuitive than 
the encoding/decoding I was using.

You asked what would happen if future code encounters my encoded string.  But 
that can't happen.  This encoding is very temporary -- it merely enables the 
l10n group to do their magic with unescape/escape that somehow converts the 
underlying character set.  I encode just before they do their magic and then two 
lines later after their magic is performed I unencode.  Nobody else will ever 
encounter my encoded string.

Your encoding is really strange.  You are encoding

  % <%>  to  % <+> <%>   // where <> indicate hex code for character
  +      to  % <%>

A more logical encoding along those lines would probably be

  % <+>  to  % <%> <+>
  +      to  % <+>

So I'll revise the patch for nsInternetService.cpp to be as I just indicated.

Comment 23

[Stephen P. Morse]

Attachment: revised patch using encoding suggested by alecf

Comment 24

[Alec Flett]

well, what mine allows one to do is nsUnescape the string twice and get back to
the original string.
But in any case, I know it seems temporary now, but like all code, this is not
the last time this file will be modified. someone, somewhere down the line may
change code in between the "escape" and "unescape", and I would rather have the
string live in a somewhat recognizable format.

Comment 25

[Stephen P. Morse]

OK, I'll post another patch that uses exactly the encoding that you outlined 
(correcting for an error that you have in the parameter ordering on the third 
replace statement.

Comment 26

[Stephen P. Morse]

Attachment: re-revised patch using exact encoding requested by aledf

Comment 27

[Alec Flett]

nice catch.. sr=alecf then!

Comment 28

[Stephen P. Morse]

fix checked in.

Comment 29

[sairuh (rarely reading bugmail)]

mass-verifying claudius' Fixed bugs which haven't changed since 2001.12.31.

if you think this particular bug is not fixed, please make sure of the following
before reopening:

a. retest with a *recent* trunk build.
b. query bugzilla to see if there's an existing, open bug (new, reopened,
assigned) that covers your issue.
c. if this does need to be reopened, make sure there are specific steps to
reproduce (unless already provided and up-to-date).

thanks!

[set your search string in mail to "AmbassadorKoshNaranek" to filter out these
messages.]