All users were logged out of Bugzilla on October 13th, 2018

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

RESOLVED FIXED in mozilla1.8.1beta2

Status

()

RESOLVED FIXED
12 years ago
10 years ago

People

(Reporter: u49640, Assigned: pkasting)

Tracking

({fixed1.8.1})

unspecified
mozilla1.8.1beta2
x86
All
fixed1.8.1
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

12 years ago
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
(Reporter)

Comment 1

12 years ago
Created attachment 229708 [details]
screenshot

Comment 2

12 years ago
Created attachment 229801 [details] [diff] [review]
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..
Attachment #229801 - Flags: review?

Updated

12 years ago
Attachment #229801 - Flags: review? → review?(aaronleventhal)

Updated

12 years ago
Attachment #229801 - Flags: review?(aaronleventhal) → review?(pkasting)
(Assignee)

Comment 3

12 years ago
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-
(Assignee)

Comment 4

12 years ago
Taking
Assignee: nobody → pkasting
Severity: major → normal
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows XP → All
Target Milestone: --- → Firefox 2 beta2
(Assignee)

Comment 5

12 years ago
Created attachment 229868 [details] [diff] [review]
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
Attachment #229801 - Attachment is obsolete: true
Attachment #229868 - Flags: review?(masayuki)

Comment 6

12 years ago
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?
(Assignee)

Comment 7

12 years ago
(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+
(Assignee)

Comment 9

12 years ago
Created attachment 230042 [details] [diff] [review]
patch v2

This is the same as patch v1, updated to apply cleanly on the trunk.
Attachment #229868 - Attachment is obsolete: true

Comment 10

12 years ago
Landed on trunk.
*** Bug 300428 has been marked as a duplicate of this bug. ***

Comment 12

12 years ago
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>
(Assignee)

Comment 13

12 years ago
(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

12 years ago
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
(Assignee)

Comment 15

12 years ago
Created attachment 230674 [details] [diff] [review]
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.
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+
(Assignee)

Comment 17

12 years ago
Fixed on branch:

mozilla/toolkit/components/typeaheadfind/src/nsTypeAheadFind.cpp 1.9.4.7
Status: NEW → RESOLVED
Last Resolved: 12 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.