Closed Bug 343077 Opened 18 years ago Closed 18 years ago

Find Again toolbarbutton doesn't switch back to normal-find if the findbar is opened after links TAF

Categories

(Toolkit :: Find Toolbar, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla1.9alpha2

People

(Reporter: mmortal03, Assigned: asaf)

References

()

Details

Attachments

(1 file)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1a3) Gecko/20060623 BonEcho/2.0a3
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1a3) Gecko/20060623 BonEcho/2.0a3




Reproducible: Always

Steps to Reproduce:
1. Go to (just an example) http://www.cnn.com/2006/WORLD/meast/06/28/israel.soldier/index.html
2. hit the apostrophe key
3. type "Syria" without quotes

Actual Results:  
Quick find goes red and doesn't find the first instance.  Further, hit control-F, and click on the find button, and it still cannot find it. You have to delete "Syria" and retype it for it to find it.

Expected Results:  
Quick find finds the first instance of the string and highlights it.  After this, if you hit Ctrl-F, the string "Syria" should be present, and clicking Find should then bring up the next instance.
' is FAYT for links only.  This mode exists to make it easy to follow links using the keyboard.

/ is FAYT for all text.

So it makes sense that typing 'syria doesn't find the headline text, since it's not a link.

But it does seem wrong that after doing that, pressing Ctrl+F and then clicking Next doesn't find it.  I see that broken behavior on trunk too.
As far as I know, in 1.5 ctrl-f doesn't change the find mode, it simply focuses the find bar.  Find Next is doing the same thing ctrl-g would do, which is "find next" in whatever mode you're in.  So for that release, this behavior is incredibly confusing, but not unexpected :(.

However, on trunk and branch, ctrl-f DOES change the find mode, so we should get this right.  I haven't looked at it, but I suspect we have a shortcut in the code where, if the last match failed and we haven't changed the text, we don't even try a new find.

CCing Jeff Walden since this is sort of related to the quick find vs. find stuff.  If he doesn't get to it I can try to.
Status: UNCONFIRMED → NEW
Ever confirmed: true
|getBrowser().fastFind| caches the type of the last search and uses that for findNext and findPrevious, so basically, since the last search was a links-only search, the search that looks like it shouldn't fail is doing a links-only search again and failing.

Thinking about what to do here...
Assignee: nobody → jwalden+bmo
(In reply to comment #3)
> |getBrowser().fastFind| caches the type of the last search and uses that for
> findNext and findPrevious, so basically, since the last search was a links-only
> search, the search that looks like it shouldn't fail is doing a links-only
> search again and failing.

Ah.  I know the code you mean.

One idea that comes to mind is not making the "linksonly" part be a find parameter, but rather have it be an attribute, or provide some other way to more directly set it.  Then whenever the find bar changes modes, it sets the attribute as appropriate.

I think when the find bar changes modes it should also clear off its "not found" state (the red background in the textfield, and the Not Found icon/status).
(In reply to comment #4)
> One idea that comes to mind is not making the "linksonly" part be a find
> parameter, but rather have it be an attribute, or provide some other way to
> more directly set it.  Then whenever the find bar changes modes, it sets the
> attribute as appropriate.

That's what I've been thinking should happen, but unfortunately it would mean a new branch-only interface, which is a pain.  This is how it should be done on trunk, but I hope we don't have to go down that route on branch.

> I think when the find bar changes modes it should also clear off its "not
> found" state (the red background in the textfield, and the Not Found
> icon/status).

Yeah, the handling of various not-found state issues isn't exactly the best at the moment.  :-\
(In reply to comment #5)
> (In reply to comment #4)
> > One idea that comes to mind is not making the "linksonly" part be a find
> > parameter, but rather have it be an attribute, or provide some other way to
> > more directly set it.  Then whenever the find bar changes modes, it sets the
> > attribute as appropriate.
> 
> That's what I've been thinking should happen, but unfortunately it would mean a
> new branch-only interface, which is a pain.  This is how it should be done on
> trunk, but I hope we don't have to go down that route on branch.

Bug 189039 is going to require a branch-only interface as well, and that's definitely going in, so you should probably just pretend there's already such an interface in place.  Whichever of us gets there first can actually create it.
Oh, I forgot to mention one problem with my plan.  I already complained some in my bug 189039 discussion that syncing state via attributes like this is potentially problematic since there's not a 1:1 mapping of JS to C++ objects here.  So that's a downside of going the attribute route.  For the branch version of 189039, I'm probably going to use crazy CSS hackery to avoid state communications.  Maybe this should do something similar.
Attached patch patchSplinter Review
Assignee: jwalden+bmo → mano
Status: NEW → ASSIGNED
Attachment #246998 - Flags: review?(masayuki)
Priority: -- → P3
Target Milestone: --- → Firefox 3 alpha2
Attachment #246998 - Flags: review?(masayuki) → review+
OS: Windows XP → All
Hardware: PC → All
Summary: Quick Find doesn't find first string instance in a page, even though the string exists on the page → Find Again toolbarbutton doesn't switch back to normal-find if the findbar is opened after links TAF
mozilla/toolkit/components/typeaheadfind/public/nsITypeAheadFind.idl  1.9
mozilla/toolkit/components/typeaheadfind/src/nsTypeAheadFind.cpp 1.28
mozilla/toolkit/content/widgets/findbar.xml 1.2
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
tbd: unit-test.
Flags: in-testsuite?
Product: Firefox → Toolkit
Flags: in-testsuite?
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: