345095 - findfast only search for the first character if searchstring is copied into search bar

Status: RESOLVED FIXED

(Toolkit :: Find Toolbar, defect)

Description

[u49640]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1b1) Gecko/20060710 Firefox/2.0b1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1b1) Gecko/20060710 Firefox/2.0b1

1. copy the string "3000" into your clipboard
2. make sure the findfast bar is closed and has no search string in the text field
3. goto the url
4. press ctrl + f to open the find bar
5. press ctrl + c to copy "3000" into the search bar

--> it selects the "3" from #302737.

it looks like firefox only searches for the first character on the page and not the full text. even if i add some text to the search bar, the "3" keeps selected

see attached screenshot for example

i think this happened with 1.5 too

Reproducible: Always

Comment 1

[u49640]

Attachment: screenshot

Comment 2

[Jan de Mooij]

Attachment: patch

Afaik the repeated char function and NO_LINK_CYCLE_ON_SAME_CHAR aren't used any more? 

This is my first patch, I think it's okay, but please double-check..

Comment 3

[Peter Kasting]

Comment on attachment 229801 [details] [diff] [review]
patch

This patch fixes the bug, but I'd like to see a lot more of the cruft here ripped out.

AFAIK, originally typeaheadfind could only get here when the new string was one character longer or shorter than the old string, because users were just typing in letters and there was no textbox to paste.  The Find Bar UI breaks that assumption, so mAllTheSameChar doesn't get set/cleared correctly and the code you're trying to remove triggers when it shouldn't.

Since Blake kludged off the RepeatingChar sort of functionality, you're correct in assuming this code can be removed because it shouldn't ever actually be triggered.  But that means that things like mAllTheSameChar, eRepeatingChar, etc. can all go away.

I actually already did this in a different local checkout, so I'll just go ahead and pull that work into a patch and attach it here.

Comment 4

[Peter Kasting]

Taking

Comment 5

[Peter Kasting]

Attachment: patch v1

This patch fixes the bug and also rips out all the associated cruft.  This enabled quite a bit of cleanup in various places.

I also cleaned up a couple other miscellaneous other functions that were annoying me.  There is TONS of crappiness still left in this file, though :D

Comment 6

[Jan de Mooij]

Thanks a lot for reviewing, next time I will try to be more complete. I was afraid of breaking something.

Btw, do you know where this file is for: seamonkey/source/extensions/typeaheadfind/src/nsTypeAheadFind.cpp? I think some parts are copy-pasted between these two files, maybe it's a good idea to change this one too?

Comment 7

[Peter Kasting]

(In reply to comment #6)
> Btw, do you know where this file is for:
> seamonkey/source/extensions/typeaheadfind/src/nsTypeAheadFind.cpp? I think some
> parts are copy-pasted between these two files, maybe it's a good idea to change
> this one too?

The Seamonkey code still uses the old typeaheadfind UI, and therefore this stuff is still applicable there, AFAIK.

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 229868 [details] [diff] [review]
patch v1

looks good. r=masayuki

Comment 9

[Peter Kasting]

Attachment: patch v2

This is the same as patch v1, updated to apply cleanly on the trunk.

Comment 10

[Pam Greene]

Landed on trunk.

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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

Comment 12

[Sergei Dolgov]

For some misterious reason BeOS build is busted now:

 /home/develop/mozilla/toolkit/components/typeaheadfind/src/nsTypeAheadFind.cpp
/boot/home/develop/mozilla/toolkit/components/typeaheadfind/src/nsTypeAheadFind.cpp: 
In method `nsresult nsTypeAheadFind::FindItNow(nsIPresShell *, int, int, int, int, PRUint16 *)':
/boot/home/develop/mozilla/toolkit/components/typeaheadfind/src/nsTypeAheadFind.cpp:393: 
ambiguous overload for `bool ? nsCOMPtr<nsISelectionController> & : int'
/boot/home/develop/mozilla/toolkit/components/typeaheadfind/src/nsTypeAheadFind.cpp:393: 
candidates are: operator ?:(bool, nsCOMPtr<nsISelectionController>, 
nsCOMPtr<nsISelectionController>) <builtin>

/boot/home/develop/mozilla/toolkit/components/typeaheadfind/src/nsTypeAheadFind.cpp:393:                 
operator ?:(bool, nsDerivedSafe<nsISelectionController> *, 
nsDerivedSafe<nsISelectionController> *) <builtin>

Comment 13

[Peter Kasting]

(In reply to comment #12)
> For some misterious reason BeOS build is busted now:

That wasn't due to this checkin, that was due to bug 344337, and the fix is waiting for checkin on bug 345584.

Comment 14

[Sergei Dolgov]

Per comment 13:
Peter, thanks for info.
I tried to follow bonsai, but didn't find patch in bug to which bonsai checkin info pointed at

Comment 15

[Peter Kasting]

Attachment: safe version for branch

This is a branch version of the fix for this bug.  It's equivalent to the patch originally submitted by Jan de Mooij, which means that it fixes the bug in a very safe way instead of doing the more aggressive cleanup my trunk patch did.

I consider this patch no-risk (it just rips out a small chunk of code that was never supposed to trigger in the Find Bar world anyway) and good to have on branch, but this bug is not a Fx2 blocker, so if drivers want to punt on it I can live with that.  If that is the case, let me know if it would be appropriate to nominate this for any 2.0.1/following releases.

Comment 16

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 230674 [details] [diff] [review]
safe version for branch

a=drivers. Please land this on the MOZILLA_1_8_BRANCH.

Comment 17

[Peter Kasting]

Fixed on branch:

mozilla/toolkit/components/typeaheadfind/src/nsTypeAheadFind.cpp 1.9.4.7