189039 - Allow type ahead find to position the cursor in text boxes

Status: VERIFIED FIXED

(SeaMonkey :: Find In Page, enhancement)

Description

[Alexander Eberts]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.3a) Gecko/20021207 Phoenix/0.5
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.3a) Gecko/20021207 Phoenix/0.5

Type ahead find is a great feature - I thought that it might be a good method to
allow the user to position the cursor text boxes as well as finding text on a page.

Reproducible: Always

Steps to Reproduce:
1.
2.
3.

Comment 1

[Marc Bejarano]

see also bug 120568

Comment 2

[rbs]

My patch for bug 58305 fixes this bug as well out-of-the box.

Comment 3

[:deanis74]

Since 58305 was checked in, find-as-you-type seems to be searching in text
fields but the text isn't highlighted at all.  This is very confusing to the
user.  Personally, I liked the old behavior of skipping text fields better.

Comment 4

[Aaron Leventhal]

This has created a bug as far as the user is concerned.
I'll review changes to find as you type, but I'm not actively developing it
right now.
-> rbs, the developer of the new feature.

Another problem with the fix, other than no selection:
I think there are some other weirdnesses with the way it works with Find As You
Type. For example, when FAYT is finished (via Escape to cancel or timeout), the
focus goes outside the textarea even if that's where the text was found. You
should be in the text area with the text selected at that point.

Comment 5

[rbs]

Attachment: patch to make FAYT ignore matches inside textareas

I have this simple patch to revert to the old behavior in FAYT (i.e., ignore
matches in textareas). But it has an issue because FAYT steals findNext,
causing the normal Find-In-Page not to find anymore in textareas. 

The problem is this. Although the documentation of FAYT says that FAYT takes
over the findNext if FAYT was the last find, the behavior I am seeing is more
intrusive than that. Indeed, as soon as you make a _first_ use to FAYT (using
'/'), all subsequent Find-In-Page (using Ctrl-F) are short-circuited by FAYT.
Therefore findNext (form Ctrl-F or Ctrl-G) goes into FAYT which now ignores the
matches in the textareas if this patch is applied. That's not what is intended.


What is desirable is for FAYT not to steal findNext if Ctrl-F is used.

Comment 6

[Aaron Leventhal]

Comment on attachment 149479 [details] [diff] [review]
patch to make FAYT ignore matches inside textareas

That's a cool way, I never new about NS_FRAME_INDEPENDENT_SELECTION

Anyway, too bad we can't have FAYT work in text fields, but it's better than
what's happening new.

Comment 7

[Aaron Leventhal]

Comment on attachment 149479 [details] [diff] [review]
patch to make FAYT ignore matches inside textareas

Actually this bug should not get closed by this fix, since the summary is about
getting FAYT to *work* inside textboxes, not about turning that off because
it's currently got problems.

Comment 8

[Aaron Leventhal]

Comment on attachment 149479 [details] [diff] [review]
patch to make FAYT ignore matches inside textareas

Actually this bug should not get closed by this fix, since the summary is about
getting FAYT to *work* inside textboxes, not about turning that off because
it's currently got problems.

Comment 9

[:deanis74]

As long as FAYT works the same way as the normal Find.  They should both either
find text in textboxes or ignore text in textboxes.

Comment 10

[Peter Kasting]

Taking.  I've been working on this and have half a solution.

Comment 11

[Peter Kasting]

Attachment: Find/highlight in textareas without correct focus/caret

OK, here's my attempt at the first half of a fix.  This reverts the previous patch (allowing the find bar to find matches in textfields again) and makes it so these matches are correctly highlighted as you find them.

To do this, I ripped the selection coloring code out of findBar.js and moved it into the C++ side of things, and now track what selection controller I'm looking at.  As far as I can tell, editable elements have their own selection controllers, which is why the old code wasn't working.  So when we see some anonymous content, we try to find an editable element containing it, and grab the selection controller off that.

As far as I can tell, this all works beautifully, though I admit I've only been testing with a very simple testcase.  I haven't yet tried all the permutations of using caret browsing, find links only, etc.  If anyone who normally uses those wants to tell me if I broke something I'd be happy to hear it.

However, there is a glaring problem.  If you find a match in a textfield, hit escape to close the find bar, and then try to move your selection (say, hit shift-right to add a character), the caret has clearly been left at the previous find match instead of being placed in the textfield.  I have been banging my head on this one for a couple days without solving it.  The focus-handling code in findBar.js might have to all come out and move into the C++ side of things too, I'm not sure.

The other thing I'm a little worried about is whether I've cleared off mSelectionController in the right places-- I don't want to leak documents if the find bar gets cached or something, but I'm not quite sure how to tell if I've got this right.

Comment 12

[Peter Kasting]

Some background info:
As far as I know, the way moving the caret works is that you first (optionally) tell some element what selection to use for the caret position, and then you focus the element, which moves the caret to it.  I think.

The findBar.js code doesn't really goof with the caret part of that.  It just messes with focus.  So when you dismiss the bar, for example, it tries to focus the correct element.  Right now, that's basically "the last selected link" (if you found one), or "the current document" (otherwise).  Clearly, if we want to move the caret to a textfield, we'll have to shoehorn that into here somewhere.

This still leaves the problem of how to tell the textfield what selection to use for its caret, or whether it needs to be told anything at all.  Luckily, that one's more solvable, because at the time we find the match in the textfield we can call whatever we need here.

Unfortunately, all my attempts to do these sorts of things have produced no result at all.  The Seamonkey implementation of FAYT is not much help here either, because it doesn't use a find bar, and thus doesn't need to worry about keeping the bar focused and only moving focus when it's dismissed.  Instead, it just moves focus immediately when you find a match, using a listener to steal the keypresses and keep FAYT working.  I suppose Firefox could go that route, updating the find bar's search box when the listener caught a keypress, but that seems really hairy to me and I'd rather avoid it.

Here are some random lines of code from Seamonkey that may trigger someone's memory or provide useful LXR fodder:

***

nsIEventStateManager *esm = presContext->EventStateManager();
esm->SetContentState(nsnull, NS_EVENT_STATE_FOCUS);
esm->MoveFocusToCaret(PR_TRUE, &isSelectionWithFocus);

***

  nsCOMPtr<nsICaret> caret;
  aPresShell->GetCaret(getter_AddRefs(caret));
  nsCOMPtr<nsILookAndFeel> lookNFeel(do_GetService(kLookAndFeelCID));
  if (!caret || !lookNFeel) {
    return;
  }

  if (aEnabled) {
    // Set caret visible so that it's obvious we're in a live mode
    caret->SetCaretDOMSelection(mFocusedDocSelection);
    caret->SetVisibilityDuringSelection(PR_TRUE);
    caret->SetCaretVisible(PR_TRUE);
    mFocusedDocSelCon->SetCaretEnabled(PR_TRUE);
  }
  else {
    PRInt32 isCaretVisibleDuringSelection = 0;
    lookNFeel->GetMetric(nsILookAndFeel::eMetric_ShowCaretDuringSelection,
                         isCaretVisibleDuringSelection);
    caret->SetVisibilityDuringSelection(isCaretVisibleDuringSelection != 0);
    nsCOMPtr<nsISelection> caretDomSelection;
    caret->GetCaretDOMSelection(getter_AddRefs(caretDomSelection));
    if (mFocusedDocSelection == caretDomSelection)  {
      mFocusedDocSelCon->SetCaretEnabled(isCaretVisibleDuringSelection != 0);
    }
  }

Comment 13

[Peter Kasting]

Attachment: Updated version with working focus

OK, I think I got it.  I've tested this patch out not just in my normal usage but with caret browsing, in FAYT mode ('/') and links-only mode ('''), and I think it does the right thing for all keypresses in all cases (including tab/enter in the non-"normal" find modes).

HOWEVER, there is one known issue with the code still: if an editable area contains an actual link (some kind of MIDAS situation or something?  not just a string that looks like a link), and we search in non-normal mode (i.e. not ctrl-f), then we'll still find the appropriate matches, but the find bar won't do the other special behaviors it normally would for that link, such as putting the href in the statusbar, drawing the 1px pseudo-focus box, and following the link when the user hits Enter.  The cause of this is the conditional in nsTypeAheadFind.cpp:FindItNow() that does not set mFoundLink if mFoundEditable is non-null.  I'm not sure how to solve this; I've written a few comments there.  More explanation is available if desired.

Other than that issue, I think this works, and I think that's a rare enough case with decent enough behavior anyway that I'm looking for review on this.

Comment 14

[Aaron Leventhal]

Comment on attachment 220464 [details] [diff] [review]
Updated version with working focus

Big patch -- who owns the find bar and type ahead find these days? I'm pretty swamped which is why I gave it up. I'll review if I need to, but I might not be able to get to it until the end of May.

Comment 15

[Peter Kasting]

Attachment: Oops, include all the files

Whoops!  I didn't get all the changes in that last patch.  I wondered why it was smaller than my WIP version.  Hopefully this one has everything.

As to ownership and review... I would have said Blake, but he's gone AFAIK.  I don't know of anyone else, really.  I do know some other folks who'd like to see this make 2.0, so it'd be nice to get it baking on the trunk.  Since I'm so new you'd probably know more than I would about who else has touched this code enough to know it; the module owners page lists you as sole owner with no peers :)

Comment 16

[Peter Kasting]

Comment on attachment 220464 [details] [diff] [review]
Updated version with working focus

Forgot to mark last patch obsolete

Comment 17

[Aaron Leventhal]

> HOWEVER, there is one known issue with the code still: if an editable area
> contains an actual link (some kind of MIDAS situation or something?  not just 
> a string that looks like a link), and we search in non-normal mode (i.e. not
> ctrl-f), then we'll still find the appropriate matches, but the find bar won't
> do the other special behaviors it normally would for that link, such 
> as putting the href in the statusbar, drawing the 1px pseudo-focus box, and 
> following the link when the user hits Enter.  The cause of this is the 
> conditional in nsTypeAheadFind.cpp:FindItNow() that does not set mFoundLink if 
> mFoundEditable is non-null.  

Maybe I'm misunderstanding something, but I'm pretty sure we don't want to focus links in a MIDAS area. Links and controls within an editable area should not receive focus. Is this attempting to change that?

Comment 18

[Aaron Leventhal]

Comment on attachment 220517 [details] [diff] [review]
Oops, include all the files

r+

A toolkit peer needs to review those changes. I looked at it all and the patch looks good to me, although I'll ask you to consider the following:
- I don't think NS_FRAME_INDEPENDENT_SELECTION is exactly the same as being in anonymous content. Is it equivalent? Perhaps name the parameter aUsesIndependentSelection.
- Someone like bz or neil should sr= it, especially the part where we know we're in an independent selection and we're trying to get our editor. You might be able to get to the root where the editor is as O(1) instead of walking the parents.
- I saw a grammatical error in the comments, so it wouldn't hurt to do a round of cleanup on those. Perhaps "Walk the DOM" should be changed to say "Walk the parent chain" to be clearer.
- The boolean in param ChangeSelectionColor is hard to understand when you're reading the code. I recommend changing the method name to something like UseAttentionSelection(PRBool); -- you can see right away what the boolean is going to do.
- Looks like we agree on not focusing links in an editor, so ignore my previous comment on that.

Comment 19

[Aaron Leventhal]

BTW, make sure you do check this with Midas, including when there is a link in the editable text for that.

Comment 20

[Mike Beltzner [:beltzner, not reading bugmail]]

Thanks for the review, Aaron; much appreciated. pkasting, do we have some solid testcases (different uses of textboxes with and without listeners, midas, etc) for this?

Comment 21

[Peter Kasting]

I've been testing using the various input areas on this bugzilla page.  There's also a usable midas box at http://www.kevinroth.com/rte/demo.htm that I've tried testing the link-finding stuff on.  It actually seems to do what I want right now, which is that (somehow) it recognizes links as links, still shows me the link destinations, but doesn't go to them on hitting <enter>.  Not sure why that all works, but I won't complain.

I don't have a testcase for a textfield with a listener; my utter ignorance of HTML/JavaScript is hindering me here, since I'm really not exactly sure what you want me to test.  If there's a page out there, or some sample code, that you'd like me to throw this at, I'll be happy to oblige.

One strangeness I did notice is that if you do a links-only search and land on a midas result, then hit "tab" to move focus forward, it moves focus out of the midas area, leaving the selection inside untouched.  This certainly looks weird, although I'm not convinced it's wrong.  I don't really know how that tab navigation stuff is supposed to work so I'm not sure what a user expects to see if they do this.  We could change this behavior pretty easily.

I'm going to attach another patch shortly which addresses aaronleventhal's comments.

Comment 22

[Peter Kasting]

Attachment: Updated version, includes aaronleventhal review feedback

This patch should address Aaron's comments.  I made one safety fix to ensure we grab some kind of selection controller even if we trace up the parent chain (in the editable case) and find nothing, and added comments about that case.  Cleaned up various other comments/prototypes.  The ChangeSelectionColor() stuff was changed to more transparently indicate what was actually getting called (we now expose an interface very much like the selection controller's SetDisplaySelection()).

I don't know of an O(1) way to grab the right selection controller in the editable case, but I'd love to hear one, as I asked about such a thing when consulting darin/bryner about how to get the right selection controller, and they didn't know of anything.  I'm not worried about the speed of this, but the other way would just be cleaner and easier.

Comment 23

[Martijn Wargers (dead)]

There are 2 implementations of typeaheadfind in Mozilla, aren't there (which one is the best?)? They should both get fixed, not?

Comment 24

[Martijn Wargers (dead)]

Ok, I just tried the latest patch, and it works splendidly. Thank you, Peter!
I see two very minor issues.
When the find function selects something, I first see a flash of the normal selection color, before it turns into the lime color.
When you have found something in a textcontrol, and you click somewhere in the page area, the find selection in the textcontrol is not removed (or made invisible).

Comment 25

[Peter Kasting]

(In reply to comment #23)
> There are 2 implementations of typeaheadfind in Mozilla, aren't there (which
> one is the best?)? They should both get fixed, not?

There are two places in the source tree I'm aware of.  No product uses both.  One is the Seamonkey TAF code, and one is the Firefox code.  The two work differently and supposedly Seamonkey isn't affected by this problem, so this is a Firefox-only bug.

(In reply to comment #24)
> When the find function selects something, I first see a flash of the normal
> selection color, before it turns into the lime color.

I can't reproduce the color flashing bug you describe.  Perhaps my machine is too fast or something?  In the code, we first add the new range to the selection, then scroll the selection into view, then set the color to green and repaint, so I suppose what you're seeing is possible.  I can try swapping the color-setting and the scrolling to see if that makes things look any better for you.

> When you have found something in a textcontrol, and you click somewhere in
> the page area, the find selection in the textcontrol is not removed (or made
> invisible).

Yes, I had seen this, but had at the time decided it wasn't a bug.  Now that I've played with things more I think it may be inconsistent behavior after all.  Specifically, if you successfully find text in a textarea, then (without dismissing the find bar) either select text elsewhere on the page or (in non-ctrl-f mode) hit tab, we don't collapse/cancel textarea selections.  Since we DO cancel/collapse normal selections here I think I should change this.

While testing this I also noticed that hitting tab while in ctrl-f mode will send the tab to the find bar, thus switching focus away without dismissing it or cancelling the current focus in the page.  This seems bogus to me.  I think tab here should behave similarly to tab in the other two search modes, dismissing the find bar and being sent to the page instead.

My local tree is a bit crazy at the moment but I'll work on putting together a patch that does these.

Incidentally, I'm still worried about the consequences of holding my selection controller as a strong, rather than weak, reference.  I'm not experienced enough to know whether this potentially introduces leaks.

Comment 26

[Martijn Wargers (dead)]

I very much suspect Seamonkey is also affected by this problem, but ctrl-f there is searching with normal selection, so in Seamonkey there is no problem finding stuff in textareas with ctrl-f. But the typeahead find problem is most likely also there.

Probably, the real problem is that typeahead find is forked between Firefox and Seamonkey. I guess it needs to be unforked one day, in order not to get these problems.

Yes, I have a slow machine (800 Mhz and running debug build with the fix). I think that swapping the color-setting and the scrolling would help indeed.

Comment 27

[Peter Kasting]

Supposedly Seamonkey is moving to Toolkit (the Firefox codebase) at some point so perhaps at that point the separate Seamonkey implementation of TAF will vanish.  I confess ignorance of most things Seamonkey.  I'm ignorant enough of the Firefox codebase already :)

It wouldn't surprise me if Seamonkey TAF couldn't handle textareas, as I didn't see any of the crazy selection controller swapping in its code that I'm adding in this patch.  Perhaps I was misunderstanding others' assurances that "it works in Seamonkey" when they really meant that the Find box, not TAF, works in Seamonkey.

In any case, I'm not prepared to tackle fixing TAF in Seamonkey, as the code is rather different.  I suspect someone motivated could probably use the changes in my patch to do the work themselves, if they wanted the feature before whatever future date the Toolkit implementation becomes the only one.

Comment 28

[Peter Kasting]

I filed bug 337192 for the weird focus changing/bar dismissal issue, as that's not related to my patch.

Comment 29

[Peter Kasting]

Attachment: Better selection handling

This takes into account comment #24/comment #25:
(1) Change selection color before scrolling, in hopes of removing flicker on slow machines
(2) Collapse selections in editable areas when clicking on the page or in certain cases of dismissing FAYT

Hopefully, the behavior of selections in editable areas now matches the behavior of selections in non-editable areas everywhere.  (Fingers crossed)

Comment 30

[Peter Kasting]

Comment on attachment 221389 [details] [diff] [review]
Better selection handling

After some discussion with bryner today, I'm convinced this patch does, in fact, leak.  I'm also not convinced it does all the right magic when searching while jumping forward/back between pages.  I need to think hard about some of that stuff and then post an updated patch, hopefully within the next few days.

Comment 31

[Peter Kasting]

In addition to the leak issues, this patch also suffers from the following issues:
* When the find bar is not focused, we normally move the focus/caret to found matches.  This does not happen for textfields; the focus box remains around the previous element.  (Therefore, typed keystrokes do not replace the highlighted match.)
* When changing locations and then using the "back" button to return to a page you were using Find on, there are problems unselecting matches in editable areas or restarting the search from the "next" place.
* Selections in editable areas are collapsed on any mouse click in the tabbrowser (for example, dragging the scrollbar).

The first and last issues are part of the same problem: focus handling here is _extremely_ tricky.  I really need the help of someone who knows the focus/selection code well.  The middle issue is due to the use of multiple selection controllers; it could perhaps be solved with (once again) the help of someone who knows the selection code well.

It may be that this functionality is "good enough" as is and should have the leak patched and then get checked in, with follow up bugs for this other stuff.  Personally I'd rather see it get fixed, but after a couple of weeks of rewriting the FAYT code and banging my head into these sorts of issues I'm beginning to run out of options for how I know how to fix things.

Comment 32

[Mike Beltzner [:beltzner, not reading bugmail]]

pkasting, any updates here? this is a really, really nice-to-have for Fx2 if we can get it by beta1.

Comment 33

[Peter Kasting]

The memleak and the first of the three problems I report in comment #31 are probably fixable.  The other two issues I'm fairly sure cannot be solved without rewriting how selections work (bug 339400).  Those other two are somewhat noticeable, I don't know if you think this is still worthwhile without them.

If so, I can probably buckle down and try and get the other issues solved.

Comment 34

[Peter Kasting]

Attachment: candidate patch v1

OK, this patch should fix the memleak and make focus handling a bit better.  Now we put focus on editable areas when the find bar's textfield doesn't currently have focus.  This basically does "the right thing", although there's a shockingly large amount of code needed to check whether focus is, in fact, in the find bar's textfield.

This patch does not address the other problems I listed in previous comments, such as that a selection in a textarea will disappear if you click anywhere in the page (including the scrollbar) while the find bar has focus, or that selection handling can be odd when trying to find across pages in different places in your history.  If this patch goes in, I can file followup bugs on those issues, though I don't believe they will be addressable in the near future.

I think this patch is probably a win overall.  In most of the common usage scenarios, it seems to do what you want, and certainly the feature is highly requested.  However, since it's large and in places a bit hackish, it could use a careful review, and as much testing as possible.

Comment 35

[Peter Kasting]

Nominating for blocking Fx2 B1, since this blocks a bug that's blocking-firefox2+.

Of course really this IS the same as bug 252371.  I haven't yet figured out in what way one should be duped to the other.

Comment 36

[Mike Schroepfer]

Cleaning the blocking since the other FF2 bug is already on the blocking list.  This is clearly something we'd love to have if we can get a patch in time.

Comment 37

[Martijn Wargers (dead)]

I found 1 issue with "candidate patch v1":
Find some word inside a text input/textarea, the word is highlighted green.
Now right-click on the text input/textarea, the highlighted green selection is collapsed. I don't think that should be happening. You should be able to cut/copy/delete the green selection. Also, when you select something in the text input/textarea afterwards, the selection is also green, when it should be the normal selection color.

Please don't block the checking in of this patch over any issues I find, they are really small, especially compared to the disability of not being able to find anything in text inputs/textareas.

Comment 38

[Peter Kasting]

(In reply to comment #37)
> I found 1 issue with "candidate patch v1":
> Find some word inside a text input/textarea, the word is highlighted green.
> Now right-click on the text input/textarea, the highlighted green selection is
> collapsed. I don't think that should be happening. You should be able to
> cut/copy/delete the green selection.

Yep, that's bullet point 3 in comment #31.  That's an artifact of my incredibly hackish solution to the problem of selections not properly collapsing when a different selection is created.  I know I said this probably can't be fixed without rewriting selections, but I suppose that one theoretical fix would be to check the target of the click and see if it is
(a) not the editable area our match is in and
(b) focusable
...and then only collapse the selection when those are true.  That's a closer match to what the "correct" behavior would be.

I don't really know how to pull this off, though.  I guess I could try and give it a shot today, but no promises :)

> Also, when you select something in the
> text input/textarea afterwards, the selection is also green, when it should be
> the normal selection color.

That is bug 209989, which I have a patch waiting for SR on.  It turns out you can trigger that in basically the same circumstances in non-textareas, and it's a longstanding bug, but obviously people select text in non-editable content much less frequently, so this patch makes that bug more noticeable.  I agree this is annoying, hopefully the other bug can be patched.

Comment 39

[Peter Kasting]

Attachment: candidate patch v2

OK, in theory this should fix the "scrolling clears selections" and "right-clicking clears selections" bugs, I hope.

I'd like to get this baking on trunk so we can see if it's good enough for B1.

Comment 40

[Peter Kasting]

Comment on attachment 226369 [details] [diff] [review]
candidate patch v2

This isn't right, something broke when I merged in the fix for bug 209989.  Hold on.

Comment 41

[Peter Kasting]

Attachment: candidate patch v3

OK, this should get things right after my fix for bug 209989 went in.

Also, I was wrong about fixing the "clears editable selections when right-clicking while the find bar has focus" bug; I just wasn't testing that properly.  (I had been surprised to see it disappear...)  I don't see a way to solve that one, since I don't know (in the find bar blur handler) where the user is right-clicking.  Shrug.

Comment 42

[Peter Kasting]

Comment on attachment 226422 [details] [diff] [review]
candidate patch v3

mconnor suggested I try masayuki to see if I could get faster review.  I'm hoping to get this on trunk ASAP to try to get it on branch before B1 if at all possible.

Comment 43

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

findBar.js

> +const nsISelectionController = Components.interfaces.nsISelectionController;

Note that you add the new global object in findBar.js that is included by some files. You should test on browser window of Fx, help window of Fx and Tb.

> +  mFoundEditable: null,

I don't think that |mFoundEditable| is good name. Because it looks like boolean value. How about |mFoundEditor| or |mFoundEdiableElement|?


nsITypeAheadFind.idl

>+  void setSelectionModeAndRepaint(in short toggle);

I think that the method name is redundant. Because the user of the method may hope to repaint automatically. How about |setSelectionMode| or |changeSelectionMode|?

> +  readonly attribute nsIDOMElement foundEditable;

same as |mFoundEditable| in findBar.js.

nsTypeAheadFind.cpp

> +nsTypeAheadFind::GetSearchContainers(nsISupports *aContainer,
> +                                     nsISelectionController *selectionController,

Use |aSelectionController| instead of |selectionController|.

> +            // Check if find field is focused, if so do nothing
> +            nsCOMPtr<nsISupports> container = presContext->GetContainer();
> +            nsCOMPtr<nsPIDOMWindow> window = do_GetInterface(container);
> +            if (window) {
> +              nsIFocusController* focusController = 
> +                window->GetRootFocusController();
> +              if (focusController) {
> +                nsCOMPtr<nsIDOMElement> focusedElement;
> +                focusController->GetFocusedElement(
> +                  getter_AddRefs(focusedElement));
> +                nsCOMPtr<nsIContent> content = 
> +                  do_QueryInterface(focusedElement);
> +                if (content) {
> +                  nsCOMPtr<nsIDOMElement> boundElement = 
> +                    do_QueryInterface(content->GetBindingParent());
> +                  if (boundElement) {
> +                    nsAutoString idStr;
> +                    if (NS_SUCCEEDED(boundElement->GetAttribute(
> +                      NS_LITERAL_STRING("id"), idStr)) &&
> +                        idStr.EqualsLiteral("find-field")) {
> +                      break;
> +                    }
> +                  }
> +                }
> +              }

I don't like here.

First, you should move the code to a new function. If you do it, you can use 'early return'. So, we can escape from the hell of nests. And the function name will document what you do.

Second, it refers the id attribute and its value. I think that this is not good. This class should be independent from Find Toolbar. I think that the class should have the editor element that is set from findBar.js. (Or the class should have the root element of find toolbar, and you check whether the editor is a descendant. This way is strong for redesigning in future.)

> +        // If we reach here without setting mFoundEditable, then something
> +        // besides editable elements can cause us to have an independent
> +        // selection controller.  I don't know whether this is possible.
> +        // Currently, we simply fall back to grabbing the document's selection
> +        // controller in this case.  Perhaps we should reject this find match
> +        // and search again.

Why don't you use NS_ASSERTION?

Comment 44

[Peter Kasting]

(In reply to comment #43)
> findBar.js
> 
> > +const nsISelectionController = Components.interfaces.nsISelectionController;
> 
> Note that you add the new global object in findBar.js that is included by some
> files. You should test on browser window of Fx, help window of Fx and Tb.

Better yet, I can just make it non-global (as long as I make it non-const as well, sadly).  Then I can't possibly conflict.

> > +  mFoundEditable: null,
> 
> I don't think that |mFoundEditable| is good name. Because it looks like
> boolean value. How about |mFoundEditor| or |mFoundEdiableElement|?

I'm not sure those look less boolean... if you really think they're a lot clearer I can of course change it easily, but they don't seem much better or worse to me.

> >+  void setSelectionModeAndRepaint(in short toggle);
> 
> I think that the method name is redundant. Because the user of the method may
> hope to repaint automatically. How about |setSelectionMode| or
> |changeSelectionMode|?

I don't think I understand your comment "the user...may hope to repaint automatically".  The method explicitly repaints, so it seemed important to me to call that out in the name.  If I drop that callers might not know this will actually cause a repaint, which seems bad.  It seems like you're saying callers would expect "setSelectionMode" would repaint, but that's not how the C++ function works, so I'm not sure why they'd expect that.

> Second, it refers the id attribute and its value. I think that this is not
> good. This class should be independent from Find Toolbar. I think that the
> class should have the editor element that is set from findBar.js. (Or the
> class should have the root element of find toolbar, and you check whether the
> editor is a descendant. This way is strong for redesigning in future.)

I'm not opposed to this, but I don't know to actually do it.  How can the C++ class get the appropriate element?  Would browser.xml (or similar) have to set this up somehow?  If you have some pseudocode that would be really helpful.

I know checking IDs feels like bad practice, but the very design of all this stuff makes the C++ and JS sides tightly coupled already.  Just doing different behaviors based on whether focus is in the find bar's input is pretty wrong if one side is supposed to work independent of the other.  I don't think this makes that problem worse.

> > +        // If we reach here without setting mFoundEditable, then something
> > +        // besides editable elements can cause us to have an independent
> > +        // selection controller.  I don't know whether this is possible.
> 
> Why don't you use NS_ASSERTION?

Because I'm not convinced it's impossible to fall through to that point.  Doing what I'm doing now makes my patch no worse than the old behavior in such a case.  Adding an assertion means we'll assert if we get there.  I dunno, I'm pretty sure it can't happen, so I suppose I could add it.  I was trying to have as light a footprint as possible.

Comment 45

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #44)
> (In reply to comment #43)
> > > +  mFoundEditable: null,
> > 
> > I don't think that |mFoundEditable| is good name. Because it looks like
> > boolean value. How about |mFoundEditor| or |mFoundEdiableElement|?
> 
> I'm not sure those look less boolean... if you really think they're a lot
> clearer I can of course change it easily, but they don't seem much better or
> worse to me.

I'm not a native English speaker, of course. (and very poor my English.) Therefore, if the name is natural (and clear) for native spearkers, I don't have a objection.

> > >+  void setSelectionModeAndRepaint(in short toggle);
> > 
> > I think that the method name is redundant. Because the user of the method may
> > hope to repaint automatically. How about |setSelectionMode| or
> > |changeSelectionMode|?
> 
> I don't think I understand your comment "the user...may hope to repaint
> automatically".  The method explicitly repaints, so it seemed important to me
> to call that out in the name.  If I drop that callers might not know this will
> actually cause a repaint, which seems bad.  It seems like you're saying callers
> would expect "setSelectionMode" would repaint, but that's not how the C++
> function works, so I'm not sure why they'd expect that.

Yeah, I think that the clear name is good too. This is not a objection. This is a proposal. I don't adhere to this. But I looked that the name is redundant.

> > Second, it refers the id attribute and its value. I think that this is not
> > good. This class should be independent from Find Toolbar. I think that the
> > class should have the editor element that is set from findBar.js. (Or the
> > class should have the root element of find toolbar, and you check whether the
> > editor is a descendant. This way is strong for redesigning in future.)
> 
> I'm not opposed to this, but I don't know to actually do it.  How can the C++
> class get the appropriate element?  Would browser.xml (or similar) have to set
> this up somehow?  If you have some pseudocode that would be really helpful.

Add an attribute that type is nsIDOMElement to nsITypeAheadFind. And set the value of |document.getElementById("find-field")| to the attribute from findBar.js. (findBar.js already depends on Find Toolbar.)

> > > +        // If we reach here without setting mFoundEditable, then something
> > > +        // besides editable elements can cause us to have an independent
> > > +        // selection controller.  I don't know whether this is possible.
> > 
> > Why don't you use NS_ASSERTION?
> 
> Because I'm not convinced it's impossible to fall through to that point.  Doing
> what I'm doing now makes my patch no worse than the old behavior in such a
> case.  Adding an assertion means we'll assert if we get there.  I dunno, I'm
> pretty sure it can't happen, so I suppose I could add it.  I was trying to have
> as light a footprint as possible.

I worry that the bug cannot be found. Even if you're right, but that condition is 'currently'. If Gecko spec is changed in future, it will help to find the new bug. And I don't think that the footprint is large that it is worried.

Comment 46

[Peter Kasting]

(In reply to comment #45)
> Add an attribute that type is nsIDOMElement to nsITypeAheadFind. And set the
> value of |document.getElementById("find-field")| to the attribute from
> findBar.js. (findBar.js already depends on Find Toolbar.)

So, I'd need to keep an extra member variable (for the setter function to update) on the C++ side, right?  And then make sure I set this on the JavaScript side just before I call any function that might reach that find code?  There isn't a 1:1 correspondence between JS findbars and C++ find objects... although I think it's one find bar to many C++ objects, not many to one or many to many, so maybe I only need to set this up "in the constructor" somewhere.  (That was why I suggested browser.xml or something, I don't know what's the closest analog to the constructor in this case).

I dunno, seems pretty sketchy to me.  I think it'd be better to call up to a JS function asking if focus was currently in the find field, but I'm not sure how to pull that off either, and it doesn't achieve the sort of separation you wanted.

Comment 47

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Ah, right. I'll think about that. Please wait tomorrow. I'll sleep soon, sorry.

Comment 48

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I think that you can check whether nsIDOMElement.ownerDocument.location.protocol is  "chrome:". I think that we don't need to find in XUL document's input control.

Comment 49

[Peter Kasting]

Wouldn't that return true when the focus is in, say, the urlbar?  That wouldn't do the right thing, since I need to detect ONLY when the focus is in the find bar.

Comment 50

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #49)
> Wouldn't that return true when the focus is in, say, the urlbar?  That wouldn't
> do the right thing, since I need to detect ONLY when the focus is in the find
> bar.

Oh, sorry, I misread your patch. You want to check current focused element.

O.K. I think that as you said, nsITypeAheadFind and gFindBar are not 1:1. But there is exclusive transaction between them. That begins/ends by nsITypeAheadFind::SetDocShell.

And I think you should not check the focused element for flexibility. Instead of it, you add a new attribute that is a boolean flag. That attribute means whether you allow the focus changing by FindItNow.

Comment 51

[Peter Kasting]

But FindItNow() makes certain types of attempts to change focus regardless of where focus currently is.  It's only for editable elements that we need to be cautious.  So this attribute would be very confusing to use.  And I'm still worried about losing sync on the attribute when we switch which nsITypeAheadFind we're talking to.  We'd need to make sure the attribute gets updated properly every time the find bar focuses or blurs AND any time SetDocShell gets called.  (And it makes life harder for a rewrite patch simmering on my back burner that makes the relationship between the C++ and JS sides of this a little clearer.)

It's doable, but it all seems more fragile and even more hackish than my current code.  Is there a particular scenario you're trying to design for?

If the problem is that you don't think nsITypeAheadFind should rely on the findBar.js implementation, we already rely on it in various other behavioral ways.  This doesn't make that any worse.  (If someone were to come along and plug in a different find bar implementation, then they could choose to use that ID or not depending on whether they wanted the focus behavior.  They're already pretty constrained in how they'd design this stuff anyway, and plus I don't really think anyone besides the find bar is using nsITypeAheadFind as opposed to the more general Find APIs).

If the problem is that you're concerned about people using that ID in their webpages, I can add the chrome check you suggested to ensure that won't trigger this.  (Although maybe that's unnecessary?  Wouldn't my use of GetBindingParent() eliminate any potential problems here?)

If the problem is that you're worried someone might change the ID in the find bar code itself, I don't think that's very likely (why bother?), but we can add appropriate comments.

I'm just not sure what problem is worth the pain of the solution you've proposed.

Comment 52

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #51)
> But FindItNow() makes certain types of attempts to change focus regardless of
> where focus currently is.  It's only for editable elements that we need to be
> cautious.  So this attribute would be very confusing to use.

I don't think so. Because the attribute will be clear the meaning with suitable name.

> And I'm still
> worried about losing sync on the attribute when we switch which
> nsITypeAheadFind we're talking to.  We'd need to make sure the attribute gets
> updated properly every time the find bar focuses or blurs AND any time
> SetDocShell gets called.  (And it makes life harder for a rewrite patch
> simmering on my back burner that makes the relationship between the C++ and JS
> sides of this a little clearer.)

I think that if the sync is lost, there are other problems. You only change the attribute on find field getting/losing the focus. I think that it's enough for the sync.

> It's doable, but it all seems more fragile and even more hackish than my
> current code.  Is there a particular scenario you're trying to design for?

No, but I cannot estimate the current design at 1.5 release. I think that we should keep independency and flexibility for future if it's possible.

> If the problem is that you don't think nsITypeAheadFind should rely on the
> findBar.js implementation, we already rely on it in various other behavioral
> ways.  This doesn't make that any worse.

I don't think so, findBar.js depends on nsITypeAheadFind, but nsITypeAheadFind does not depends on findBar.js. Cannot use it from other code?

> If the problem is that you're concerned about people using that ID in their
> webpages, I can add the chrome check you suggested to ensure that won't trigger
> this.  (Although maybe that's unnecessary?  Wouldn't my use of
> GetBindingParent() eliminate any potential problems here?)

I don't have the worry.

Comment 53

[Peter Kasting]

(In reply to comment #52)
> I think that if the sync is lost, there are other problems. You only change
> the attribute on find field getting/losing the focus. I think that it's enough
> for the sync.

It's already possible (though unlikely due to craziness in tabbrowser.xml) to switch to a different nsITypeAheadFind instance underneath a single findBar.js.  And my future rewrite patch will make that happen on every tab change.  So what if you switch away, change the focuse state, and switch back?  Now we've lost sync.  That's why I think this would have to happen at SetDocShell() time too.  But that makes nsITypeAheadFind depend very explicitly on findBar.js.

> > It's doable, but it all seems more fragile and even more hackish than my
> > current code.  Is there a particular scenario you're trying to design for?
> 
> No, but I cannot estimate the current design at 1.5 release. I think that we
> should keep independency and flexibility for future if it's possible.

I think my proposed solution is more flexible, honestly.  It requires less work on the part of the findBar.js side and it doesn't actually tightly tie the two together.  The C++ code isn't calling any functions, just checking an ID.  If a future implementation wants to control the focus behavior it can simply use that ID or not.

Plus, as I've mentioned, I've done some pretty extensive redesign/rewrite work in my spare time, and I have a decent idea of where the code can be improved and modified.  This solution works much more cleanly with that.

Finally, I'm not sure when or why we'd want to change one side of this picture without changing the other.  So what "flexibility" are we buying here?  Let's design for our current problems, not overdesign for our future ones (a problem throughout the entire Mozilla codebase...).

> > If the problem is that you don't think nsITypeAheadFind should rely on the
> > findBar.js implementation, we already rely on it in various other behavioral
> > ways.  This doesn't make that any worse.
> 
> I don't think so, findBar.js depends on nsITypeAheadFind, but nsITypeAheadFind
> does not depends on findBar.js. Cannot use it from other code?

Look at, for example, the way we goof with selection color changing and collapsing.

The C++ code may not depend on the JS code in a binary-dependence sense, but it's certainly closely tied to it in a behavioral sense.  As I said, I'm not aware of anyone besides findBar.js actually using nsITypeAheadFind, and if anyone did, they'd need to hew pretty closely to the existing findBar.js behavior.

It was a mistake in the first place to split this stuff into a C++ "service provider" and JS "consumer" in toolkit.  We need to move in the direction of making the JS side UI-only and pushing all the functionality into the C++ side of things.

Comment 54

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #53)
> (In reply to comment #52)
> > I think that if the sync is lost, there are other problems. You only change
> > the attribute on find field getting/losing the focus. I think that it's enough
> > for the sync.
> 
> It's already possible (though unlikely due to craziness in tabbrowser.xml) to
> switch to a different nsITypeAheadFind instance underneath a single findBar.js.
>  And my future rewrite patch will make that happen on every tab change.  So
> what if you switch away, change the focuse state, and switch back?  Now we've
> lost sync.  That's why I think this would have to happen at SetDocShell() time
> too.  But that makes nsITypeAheadFind depend very explicitly on findBar.js.

You think that my approach breaks the current sync, would you add the parameter to |find()|, |findNext()| and |findPrevious()|? I cannot accept the ID checking in cpp code still now. (And maybe, I will not change my mind except there is no way.)

Comment 55

[Peter Kasting]

I can live with that.  Let me see if I can cobble together a working patch in a bit.

Comment 56

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to comment #55)
> I can live with that. 

If you hate my approach, please propose new approach in new patch. I don't stick to the way. I only hate the ID checking.

Comment 57

[Peter Kasting]

Attachment: candidate patch v4

OK, this addresses masayuki's previous comments as follows:
* Replaces check for CSS IDs with aHasFocus parameter.  findBar.js now tracks when the find field has focus and sends this down on any find requests.
* Assertion failure if we run into an element with a selection controller when we weren't expecting one.  This should probably be tested against a page using designmode... I haven't tried that.
* Changed selectionController to aSelectionController in one place.

I haven't done a lot of testing on this version of the patch, but I did quickly make sure the focus stuff looked like it was kinda working.

Comment 58

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 227302 [details] [diff] [review]
candidate patch v4

r=me.

Note that if you want to take this to 1.8 branch, I think that you should use ID checking way instead of this. Because it doesn't need to change current method of interface and it's enough for the fix.

Comment 59

[Peter Kasting]

(In reply to comment #58)
> Note that if you want to take this to 1.8 branch, I think that you should use
> ID checking way instead of this. Because it doesn't need to change current
> method of interface and it's enough for the fix.

Even with the ID checking method, I changed the interface and its UUID.  So no matter what, either the branch version of the patch will have to declare a _MOZILLA_1_8_BRANCH interface or break interface compatibility on this object.  Of course, it WOULD be a tiny bit simpler to do this using the ID checking method because it's that many fewer interface changes...

Comment 60

[Peter Kasting]

Attachment: candidate patch v5

OK, this has two additional changes due to comments from darin:

* Comment the .idl a bit since I'm touching it anyway
* Guard against GetEditor() returning null in my C++ code.  I don't think this should ever happen, so I made it assert.

I think this patch is ready to go into the trunk for baking.

Comment 61

[Darin Fisher]

Comment on attachment 227599 [details] [diff] [review]
candidate patch v5

sr=darin (thanks for the IDL documentation, but we tend to favor doxygen style comments)

Comment 62

[Darin Fisher]

fixed-on-trunk

Comment 63

[Peter Kasting]

*** Bug 252371 has been marked as a duplicate of this bug. ***

Comment 64

[Peter van der Woude [:Peter6]]

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a1) Gecko/20060629 Minefield/3.0a1 ID:2006062914 [cairo]

verified/fixed

Comment 65

[Peter Kasting]

Attachment: branch patch v1

Here's a branch version of the patch.  The major changes from the trunk version are:

(1) Adds an nsITypeAheadFind_MOZILLA_1_8_BRANCH interface, which inherits from nsITypeAheadFind, to avoid changing the existing interface.  createInstance() calls in [tab]browser.xml now use this new interface.

(2) The focusLinks parameter in the old interface, which I removed in my trunk patch, has been made to do nothing in the branch version.

(3) Uses the CSS ID-matching method from my candidate patch v3 instead of the later parameter-based match.  This is as suggested by masayuki, and avoids more interface changes on the branch.  I did break this out into its own function to avoid the nesting hell from my original implementation.

Otherwise I only changed bits as needed for branch compatibility.

I tried to interdiff the patches, but go weird errors about rejected hunks, probably because the context is different in the trunk and branch patches :(  A good graphical diff viewer should be able to show you the differences between this patch and the trunk patch though.

I'm desperately trying to make it onto the branch before the code freeze tonight, if this looks clean...

Comment 66

[Darin Fisher]

Comment on attachment 228198 [details] [diff] [review]
branch patch v1

>Index: toolkit/components/typeaheadfind/src/nsTypeAheadFind.cpp

>+        // We may be inside an editable element, and therefore the selection
>+        // may be controlled by a different selection controller.  Walk up the
>+        // chain of parent nodes to see if we find one.
>+        nsCOMPtr<nsIDOMNode> node;
>+        returnRange->GetStartContainer(getter_AddRefs(node));
>+        while (node) {
>+          nsCOMPtr<nsIDOMNSEditableElement> editable = do_QueryInterface(node);
>+          if (editable) {

nit: As we discussed, it might be nice to only loop to find
the non-null editable.  Then, after the loop, do the stuff
that depends on the editable.


>+PRBool
>+nsTypeAheadFind::FindFieldHasFocus(nsPresContext *aPresContext)
>+{
>+  NS_ENSURE_ARG_POINTER(aPresContext);
>+
>+  nsCOMPtr<nsISupports> container = aPresContext->GetContainer();
>+  nsCOMPtr<nsPIDOMWindow> window = do_GetInterface(container);
>+  if (!window) {
>+    return false;

nit: s/false/PR_FALSE/


sr=darin

Comment 67

[Peter Kasting]

Attachment: branch patch v2

This fixes the PR_FALSE issue.  I didn't rework the loop, because I'd rather keep the branch in sync with the trunk here.  It's not actually any less efficient this way, it's just a small readability issue, so it's not worth accidentally screwing something up in a quick rewrite :).  I can make the trunk code a little cleaner-looking some day in the future.

bryner/darin suggested this patch probably didn't need two levels of review given the differences from the trunk version, so I'm just carrying over the a? request.

Comment 68

[Brendan Eich [:brendan]]

+1 for inclusion in beta 1.

/be

Comment 69

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 228225 [details] [diff] [review]
branch patch v2

Please get this landed on the branch ASAP.

Comment 70

[Pam Greene]

Landed on branch:

toolkit/components/typeaheadfind/content/findBar.js: 1.21.2.19
toolkit/components/typeaheadfind/public/nsITypeAheadFind.idl: 1.5.2.2
toolkit/components/typeaheadfind/src/Makefile.in: 1.5.8.1
toolkit/components/typeaheadfind/src/nsTypeAheadFind.cpp: 1.9.4.5
toolkit/components/typeaheadfind/src/nsTypeAheadFind.h: 1.5.4.2
toolkit/content/widgets/browser.xml: 1.70.2.13
toolkit/content/widgets/tabbrowser.xml: 1.103.2.53

Comment 71

[Ria Klaassen (not reading all bugmail)]

*** Bug 356688 has been marked as a duplicate of this bug. ***