Closed Bug 345095 Opened 17 years ago Closed 17 years ago

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

Categories

(Toolkit :: Find Toolbar, defect)

x86
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.8.1beta2

People

(Reporter: u49640, Assigned: pkasting)

References

()

Details

(Keywords: fixed1.8.1)

Attachments

(3 files, 2 obsolete files)

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
Attached image screenshot
Attached patch patch (obsolete) — Splinter Review
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..
Attachment #229801 - Flags: review?
Attachment #229801 - Flags: review? → review?(aaronleventhal)
Attachment #229801 - Flags: review?(aaronleventhal) → review?(pkasting)
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.
Attachment #229801 - Flags: review?(pkasting) → review-
Taking
Assignee: nobody → pkasting
Severity: major → normal
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows XP → All
Target Milestone: --- → Firefox 2 beta2
Attached patch patch v1 (obsolete) — Splinter Review
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
Attachment #229801 - Attachment is obsolete: true
Attachment #229868 - Flags: review?(masayuki)
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?
(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 on attachment 229868 [details] [diff] [review]
patch v1

looks good. r=masayuki
Attachment #229868 - Flags: review?(masayuki) → review+
Attached patch patch v2Splinter Review
This is the same as patch v1, updated to apply cleanly on the trunk.
Attachment #229868 - Attachment is obsolete: true
Landed on trunk.
*** Bug 300428 has been marked as a duplicate of this bug. ***
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>
(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.
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
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.
Attachment #230674 - Flags: approval1.8.1?
Comment on attachment 230674 [details] [diff] [review]
safe version for branch

a=drivers. Please land this on the MOZILLA_1_8_BRANCH.
Attachment #230674 - Flags: approval1.8.1? → approval1.8.1+
Fixed on branch:

mozilla/toolkit/components/typeaheadfind/src/nsTypeAheadFind.cpp 1.9.4.7
Status: NEW → RESOLVED
Closed: 17 years ago
Keywords: fixed1.8.1
Resolution: --- → FIXED
Product: Firefox → Toolkit
You need to log in before you can comment on or make changes to this bug.