161960 - Type ahead find shouldn't lose Accel+G memory after being cancelled or timing out

Status: VERIFIED FIXED

(SeaMonkey :: Find In Page, defect)

Description

[Aaron Leventhal]

Right now after type ahead find times out or is cancelled, it loses it's Accel+G
(find next) memory. Accel+G should continue to find the last thing type ahead
find found.

Comment 1

[Aaron Leventhal]

Attachment: Fixes Accel+G to keep memory after type ahead find cancels,

This also fixes / and ' so that they're part of the find string if they aren't
typed as the first char. Therefore, it understands '', //, '/ or /' so that '
or / become the first char to search for.

Also, if a link is focused, the find starts from there.

Comment 2

[Akkana Peck]

Maybe I'm misunderstanding what this is supposed to do ...  if I start up with
this patch (it goes to the start page, http://www.mozilla.org/start/ ), click in
the content area to set the focus, and type "dow", it goes to the Download link
in the fake-sidebar.  If I then type accel-G, nothing further happens.  It
doesn't matter whether I wait a while between typing w and ctrl-G, and it
doesn't matter whether I have the timeout set to 0 or not; I can't seem to make
accel-G go to the next link.  What am I missing?

I'm not clear why we have to store the string independantly of the global find
service's find string; is it a performance issue?

Comment 3

[Aaron Leventhal]

Akkana,

I could keep it all in 1 string, but then I'd have to change some fundamental
things.

Right now I check mTypeAheadBuffer.IsEmpty(), to see whether the mode is active.
Since that string gets set to "" when the timer times out, Accel+G would no
longer have the info it needs.

This is working on my system, I'm not sure why it doesn't work for you.

Comment 4

[Aaron Leventhal]

Attachment: Hopefully fixes performance regressions as well

This new patch simplifies type ahead find quite a bit.

It gets rid of the web progress listening and focus listening pieces.
Now we just attach a keystroke listener to all subwindows of any newly opened
window.

When we see a keystroke, we see if the window it's in is different from the
last window we saw a keystroke in. If it is, we get ready to start type ahead
find in the new window.

By not listening to every page load and focus event, I hope to return our Tp,
Txul and Ts stats almost back to where they were. I don't think I'll be able to
optimize too much further, without directly initializing type ahead find in
some core content document or window class.

Comment 5

[Akkana Peck]

Comment on attachment 96922 [details] [diff] [review]
Hopefully fixes performance regressions as well

Argh -- moz just stomped my detailed comments on the first half of
this patch.  Hint: don't ever make a long set of comments and leave
them unattended in a window where a -remote might request a load of
some other page -- backarrow takes back to the unedited attachment. :-(
Let's see if I can remember what my comments were -- sorry if this
is a bit disjointed.

Re my earlier comment when it didn't seem to be working right: the
problem there was that I was expecting find in all text, and I was
getting linksonly, forgetting that that was the default (I turn off
linksonly in my own profile).

> +  if (domWin != mFocusedWindow) {
> +    GetAutoStart(domWin, &mUseInFocusedWindow);
> +    if (mUseInFocusedWindow) {
> +      UseInWindow(domWin);
> +    }
> +  }
> +  if (!mUseInFocusedWindow) {
> +    mFocusedWindow = domWin;
>      return NS_OK;
>    }

I'm a bit confused about what mUseInFocusedWindow is.  I guess it
means "mFocusedWindow is a window that is doing autostart, i.e.
it will search when any key is pressed" -- is that right?  The
verb "use" confuses me.  Some comments about what the variable
means and under what conditions it's expected to be true would
probably make it a lot clearer.

I'm also somewhat worried about us doing extra work and slowing down
typing.  How far into this code does it get when focus is somewhere
where the user is typing, such as a text control or composer window?
We have some performance problems with typing on some slower machines,
so it's a concern.
It needn't block this patch -- people on slow machines can always
turn off typeahead find -- but I'd like to know where we stand.

> +  // ---------- Check the keystroke --------------------------------
> +  if (((charCode == 'g' || charCode=='G') && isAlt + isMeta + isCtrl == 1 && (

It's not new to this patch, but I really hate that this is hardwired
into the code.	What if the xul for Find Next is set to something
other than accel-G in some locale or platform?	We really need to
find a way not to compile this in, and I should file a separate bug
tracking that issue.

That goes double for the part with the platform ifdefs:

> +#ifdef XP_MAC
> +     // We don't use F3 for find next on Macintosh, function keys are user
> +     // definable there
> +     ) {
> +#else
> +    (keyCode == nsIDOMKeyEvent::DOM_VK_F3 && !isCtrl && !isMeta && !isAlt)) {
> +#endif
> +    // We steal Accel+G, F3 (find next) and Accel+Shift+G, Shift+F3
> +    // (find prev), avoid early return, so we can use our find loop


> -    if (bufferLength == 0) {
> +    if (bufferLength == 0 && !mLinksOnlyManuallySet) {
>        if (uniChar == '`' || uniChar=='\'' || uniChar=='\"') {
> +        mLinksOnlyManuallySet = PR_TRUE;
> +
>          // If you type quote, it starts a links only search
>          mLinksOnly = PR_TRUE;

I was confused about this, too, and eventually realized that
mLinksOnlyManuallySet meant that the value of mLinksOnly had been
set by the user typing ' or /, not that the flag mLinksOnly had been
set (to true, as opposed to cleared) manually (and I wasn't clear
what "manually" meant).  I don't have a clearer name to offer right
now, but for a name that's confusing like that, some comments would
make its function clearer.

>        // If you can see the selection (either not collapsed or
> -      // through caret browsing), start there
> +      // through caret browsing), or if already focused on a link, start there

If we're focused on a link but that link is off the screen, we'll
still start from there?  That's okay with me, I just want to make sure.
Why just links?  Should we consider starting from the selection position
if focus is in a form element on the page?

> +  if (!sIsFindingText) {

sIsFindingText isn't new with this patch, but I'm curious: if there's
only one nsWebBrowserFind object per app, I'm not clear why sIsFindingText
is declared static while values like mIsTypeAheadOn are not.

In CancelFind():
>    nsCOMPtr<nsISupports> windowSupports(do_QueryInterface(mFocusedWindow));
>    if (mManualFindWindows->IndexOf(windowSupports) >= 0) {
> -    RemoveCurrentKeypressListener();
> -    RemoveWindowFocusListener(mFocusedWindow);
>      RemoveCurrentSelectionListener();
>      RemoveCurrentScrollPositionListener();
> +    mUseInFocusedWindow = PR_FALSE;
>    }

You don't need to call RemoveKeypressListener?

> Index: src/nsTypeAheadFind.h
> ===================================================================
[ ... ]
> +  nsString mFindNextBuffer;
>    PRBool mLinksOnlyPref;
>    PRBool mStartLinksOnlyPref;
>    PRPackedBool mLinksOnly;
> @@ -167,6 +160,8 @@
>    PRBool mCaretBrowsingOn;
>    PRPackedBool mLiteralTextSearchOnly;
>    PRPackedBool mDontTryExactMatch;
> +  PRPackedBool mLinksOnlyManuallySet;
> +  PRBool mUseInFocusedWindow;


Why is mUseInFocusedWindow not a PRPackedBool?	Likewise for
mLinksOnlyPref and mStartLinksOnlyPref even though they're not
new with this patch.

Comment 6

[Aaron Leventhal]

Attachment: New patch based on Akkana's comments

Changes
- Changed mUseInFocusedWindow to mIsAutoFindWindow
- Added comments to describe mLinksOnlyManuallySet
- Changed static sIsFindingText to member variable mIsFindingText, no need for
static
- Added RemoveKeyPressListener() back into CancelFind() - you're right that
should be there.
- Checked for focus on any page element, instead of just links, when deciding
whether to start find from current position.

Typing Performance concerns:
- In a composer window, it sets the flag mIsAutoFindWindow to PR_FALSE after
the first keystroke
  after that, it exits the KeyPress() method pretty early until the window
we're in changes.
- In a textfield it exits as a result of this line:
  if (localSelection != mFocusedDocSelection || !mFocusedDocSelection) { ... }
  That occurs after it's use the event to get the window, keyCode, doc,
presshell and prescontext.
  We should check to see if it affects typing on slower machines.

Hardcoded keystrokes:
- I agree, we should find a way not to do this

Explanation of PRBool/PRPackedBool
- I used PRBool for member variables that get set by methods that take an
address of a PRBool as an argument. Otherwise I would have to use a temporary
variable to do the translation.

Comment 7

[Akkana Peck]

Comment on attachment 96981 [details] [diff] [review]
New patch based on Akkana's comments

r=akkana
Trivial nit in case you care: in a couple of places you say "it's" instead of
"its" for a possessive.  If you decide to fix that, no need to attach another
patch.

Comment 8

[Boris Zbarsky [:bzbarsky]]

+  nsCOMPtr<nsIWebNavigation> webNav;
+  ifreq->GetInterface(NS_GET_IID(nsIWebNavigation), getter_AddRefs(webNav));

How about:

nsCOMPtr<nsIWebNavigation> docShell(do_GetInterface(ifreq));

?  You may need to include nsIInterfaceRequestorUtils.h for this, maybe.

+      nsCOMPtr<nsIDOMWindow> domWin;
+      ifreq->GetInterface(NS_GET_IID(nsIDOMWindow), getter_AddRefs(domWin));

same here.

+  nsCOMPtr<nsISelectElement> selectEl(do_QueryInterface(domEventTarget));
+  if (selectEl) {

It may be faster to QI domEventTarget to nsIContent, check that it's an HTML
content (using IsContentOfType) and if it is getting the tag and comparing to
nsHTMLAtoms::select.  That involves a succeeding QI (which is much faster than a
failing QI, which is what this code would usually encounter) and some very cheap
getters/comparisons.

Could we name mAutoFindWindow something like mFindAllowedInWindow, maybe?  that
would make it clearer what it does.

+      // Get focused content from esm. If it's null, the document is focused.
+      // If not, the selection is in fron of the focused page element,
+      // because esm keeps the 2 in sync.

Hmm... I can select some text and then select a radio or checkbox... in that
case, focus and selection are not quite in sync, are they?

Definitely check how this affects typing in textareas on slower machines....

Comment 9

[Aaron Leventhal]

> Hmm... I can select some text and then select a radio or checkbox... in that
> case, focus and selection are not quite in sync, are they?

Thanks, I've fixed that scenario.

Also changed all the do_GetInterface() stuff and the variable name.

> It may be faster to QI domEventTarget to nsIContent, check that it's an HTML
> content (using IsContentOfType) and if it is getting the tag and comparing to
> nsHTMLAtoms::select.  That involves a succeeding QI (which is much faster than 
> a failing QI, which is what this code would usually encounter) and some very 
> cheap getters/comparisons.

Can I save that stuff for the next patch? Will that hold up sr= on this one?

Comment 10

[Aaron Leventhal]

Attachment: New patch based on bz's comments, except for nsIContent stuff

Comment 11

[Aaron Leventhal]

Comment on attachment 97114 [details] [diff] [review]
New patch based on bz's comments, except for nsIContent stuff

Carrying Akkana's r=

Comment 12

[Boris Zbarsky [:bzbarsky]]

+        // Get focused content from esm. If it's null, the document is focused.
+        // If not, the selection is in fron of the focused page element,
+        // because esm keeps the 2 in sync

This comment is out of sync with the code.  Fix that, and sr=me.

And yes, it's ok to do the nsISelectElement thing as part of a separate patch,
but make sure it happens, please.

Comment 13

[Aaron Leventhal]

checked in

Comment 14

[Aaron Leventhal]

Attachment: This patch just makes it so we're not registered w/ windowwatcher called unless the pref is set

If we don't check this in, we regress by 10 ms

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 97148 [details] [diff] [review]
This patch just makes it so we're not registered w/ windowwatcher called unless the pref is set

sr=bzbarsky

Comment 16

[John Keiser (jkeiser)]

Comment on attachment 97148 [details] [diff] [review]
This patch just makes it so we're not registered w/ windowwatcher called unless the pref is set

r=jkeiser

Comment 17

[sairuh (rarely reading bugmail)]

i'll get to this soon, just want to enumerate what needs to be verified here:

a. being able to find instances of // and '' in a page.
b. not losing accel-G (find again, F3) memory upon cancel (esc) or timeout.
c. if a link's already in focus, starting typeahead find from there, not from
the top of the page.

as for the perf fix, i'm just going to rs vrfy that.

Comment 18

[Aaron Leventhal]

> b. not losing accel-G (find again, F3) memory upon cancel (esc) or timeout.
The memory we're talking about is whether or not we're in "links only" or "all
text" search.
After Escape is pressed, it should forget that and go back to the default.
However, after timeout, it should remember that so that findnext/prev commands
work along the lines of the find that just occured.

Comment 19

[sairuh (rarely reading bugmail)]

tested using 2002.10.15.08 comm trunk builds.

(a) and (c) from comment 17 look fine.

however, (b) as described in comment 18 doesn't work as expected. Esc does
"forget" the mode and reset to whatever the default mode is (links or text).
however, i noticed that after the *timeout*, this also happened --ie, the
default mode was reinstated and the temporary mode (whether set by / or ', to
text or links, respectively) was forgotten

is this the expected behavior now? or, should i spin off another bug for that?
(or, alternatively, reopen this one.)

Comment 20

[Aaron Leventhal]

Sarah, (b) is working for me, for both timeout and Escape.

Maybe I have some fixes in my local tree that make it work, but I can't think of
what.

Comment 21

[Akkana Peck]

I have the opposite problem: I start typing a search string, but as soon as it
finds something that doesn't match, it stops accepting any more characters (and
starts beeping) so when I go to another tab/page and use accel-G, I'm forever
being surprised/annoyed that it only searches for the first two characters and I
have to type the search string again.  (That doesn't really belong in this bug,
I know, but I know of no other place to mention it and I'm reluctant to file a
new bug just for that.)

Comment 22

[sairuh (rarely reading bugmail)]

gonna verify this one, and spin off bug 175887 to cover the timeout/mode memory
issue.