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

RESOLVED FIXED in mozilla1.9alpha2

Status

()

Toolkit
Find Toolbar
P3
normal
RESOLVED FIXED
12 years ago
5 years ago

People

(Reporter: mmortal03, Assigned: mano)

Tracking

unspecified
mozilla1.9alpha2
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment)

(Reporter)

Description

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

Comment 1

12 years ago
' 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.

Comment 2

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

Comment 3

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

Comment 4

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

Comment 5

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

Comment 6

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

Comment 7

12 years ago
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.
Created attachment 246998 [details] [diff] [review]
patch
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
Last Resolved: 12 years ago
Resolution: --- → FIXED
tbd: unit-test.
Flags: in-testsuite?
Product: Firefox → Toolkit
You need to log in before you can comment on or make changes to this bug.