Seems that everyone is griping about this in bug #30088. There should be a pref that allows me to pick between pressing a key to start it, or just typing to start it. To go a bit farther, this should, IMHO, default to using keys only.

see bug 169489 and bug 169489 comment 9 (text repeated for bugzilla's linking). suggesting marking this as a dup of aforementioned bug. i strongly feel that the default isearch method should require initiation. prefs (copied from my post, bug 169489 comment 9): (*) require manual initiation of isearches ( ) always in isearch links mode __ begin isearch of links __ begin reversed isearch of links ( ) always in isearch text mode __ begin isearch of all text __ begin reversed isearch of text "( )" is a radio button, "__" is a single-character input box defaults: require manual, ' links, " reverse links, / text, ? reverse text.

I disagree entirely here. The other bug was created to show other preferences in the UI. This bug is to create the preference so there can be a UI link. There is, IMHO, no dupe. I have to agree with you with the defaults, though -- the default should be "require a keypress to start". But there should be no UI elements. Only people who need it are going to turn it to autostart, and these people will know where and how to change it.

To expand on this, if I felt a UI link should be created, bug 169489 should depend on this bug. But, since I don't...

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

confirming.

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

To support the idea that there has to be a way to force a slash before the search start, please consider, that in Galeon, you can use the "vi" keys "j" and "k" to scroll around the page. And typeahead that came with Mozilla 1.2 and Galeon 1.2.7 breaks this. Search without extra windows popping up is nice but it should be strictly started by a defined keystroke, at least as an option.

I actually find myself avoiding using typeahead find, since it confuses me that there's no key to start searching. I know this is slightly weird behavior on my part, but there you have it.

Also fixes bug 183998 -- make / and ' start typeaheadfind in mailnews.

Comment on attachment 108790 [details] [diff] [review] Implements new bool pref accessibility.typeaheadfind.autostart, and avoids unnecessary keypress listeners Also seeking r=sspitzer for mailnews changes.

Comment on attachment 108790 [details] [diff] [review] Implements new bool pref accessibility.typeaheadfind.autostart, and avoids unnecessary keypress listeners Index: dom/src/base/nsGlobalWindow.cpp =================================================================== RCS file: /cvsroot/mozilla/dom/src/base/nsGlobalWindow.cpp,v retrieving revision 1.566 diff -u -r1.566 nsGlobalWindow.cpp --- dom/src/base/nsGlobalWindow.cpp 4 Dec 2002 02:51:47 -0000 1.566 +++ dom/src/base/nsGlobalWindow.cpp 9 Dec 2002 22:11:15 -0000 @@ -132,7 +132,8 @@ #include "nsIJSNativeInitializer.h" #include "nsIFullScreen.h" #include "nsIStringBundle.h" -#include "nsIScriptEventManager.h" // For GetInterface() +#include "nsITypeAheadFind.h" +#include "nsIScriptEventManager.h" // For GetInterface() #include "nsIConsoleService.h" here, you shouldn't introduce a new dependency of nsITypeAheadFind to nsGlobalWindow.cpp. other part looks good to me.

CC'ing jst for input on the dependency issue. Johnny, my patch adds a dependency on typeaheadfind in nsDOMWindowController::DoCommand(), so that it can start a type ahead find when the user presses / or ' Is this okay? If not, what do you suggest?

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

Comment on attachment 108790 [details] [diff] [review] Implements new bool pref accessibility.typeaheadfind.autostart, and avoids unnecessary keypress listeners Johnny, Kyle turned it down because of a dependency on nsITypeAheadFind in DOMWindowController. However, that's what the interface to typeaheadfind is for, control by the app. Would you rather have me use nsIObserver or something?

Yeah, I'd rather see the DOM code *not* depend on typeahead find which, at least in theory, is an optional extension.

A lot of work was necessary to do this right: 1. Adds / and ' to htmlBindings.xml 2. Inserts nsTypeAheadController into nsIControllers for each dom window opened 3. Implements pref for autostart in browser window. 4. Implements autostart="false" attribute for mailnews, to require manual typeahead start there 5. Only attach keypress and other listeners if typeahead is in autostart mode, or manually started. 6. Can dynamically attach or remove keypress and other listeners if autostart pref changes on the fly 7. When manual find is started with / or ', makes sure it starts in content window. Focuses one first if necessary. 8. Makes sure there is no key conflict with mailnews -- changes collapse all threads from / to \ 9. Implement new pref accessibility.typeaheadfind.autostart, eAutoStartNone = 0, eAutoStartText = 1, eAutoStartLinks = 2 10. Support old pref accessibility.typeaheadfind.linksonly by converting to autostart pref

Comment on attachment 109886 [details] [diff] [review] Creates pref accessibility.typeaheadfind.autostart (0=force manual, 1=auto text, 2= auto links) Seeking sr= from sspitzer because of the mailnews changes. Please see comments in bug about why this patch is large.

Comment on attachment 109886 [details] [diff] [review] Creates pref accessibility.typeaheadfind.autostart (0=force manual, 1=auto text, 2= auto links) r=kyle with a few comments. no new patch needed. >+NS_IMETHODIMP >+nsTypeAheadController::IsCommandEnabled(const char *aCommand, PRBool *aResult) >+{ >+ NS_ENSURE_ARG_POINTER(aResult); since you are using NS_ENSURE_ARG_POINTER here, please add it to other places, such as EnsureContentWindow(), GetStartWindow() etc. >+ >+ *aResult = PR_FALSE; >+ >+ if (!nsCRT::strcmp(sLinkFindString, aCommand) || >+ !nsCRT::strcmp(sTextFindString, aCommand)) { personally, I think |nsCRT::strcmp() == 0| is more readable than |!nsCRT::strcmp()|. but anyway, you can keep your own way. >+ >+ *aStartContentWin = startContentWin; >+ NS_ADDREF(*aStartContentWin); shouldn't you use NS_IF_ADDREF here? because you didn't check startContentWin in every cases. >+ return NS_OK; >+} >\ No newline at end of file need a CR here.

Thanks Kyle, I've made the changes you suggest.

Comment on attachment 110825 [details] [diff] [review] Contains Kyle's changes, and adds autofind="false" to mail3PaneWindowVertLayout.xul Carrying r=kyle

This is blocking about 3 bugs. Has r=, needs sr=.

Comment on attachment 110825 [details] [diff] [review] Contains Kyle's changes, and adds autofind="false" to mail3PaneWindowVertLayout.xul r=sspitzer on the mailnews parts of the patch

Comment on attachment 110825 [details] [diff] [review] Contains Kyle's changes, and adds autofind="false" to mail3PaneWindowVertLayout.xul - In nsTypeAheadFind::GetStartWindow(): +{ + // Return the root ancestor content window of aWindow + + *aStartWindow = aWindow; + + nsCOMPtr<nsIInterfaceRequestor> ifreq(do_QueryInterface(aWindow)); + NS_ASSERTION(ifreq, "Can't get interface requestor"); + if (!ifreq) + return; If there's ever a window that doesn't implement nsIInterfaceRequestor (which is unlikely, but still) and such a window is passed to this function you'll have an unbalanced reference count since you don't addref *aStartWindow in this early return, nor in any of the other ones later on in this method either. I'd say you should null out *aStartWindow at the beginning of this method, and set it to aWindow only at the end if !*aStartWindow at that point, then addref... +nsTypeAheadController::nsTypeAheadController(nsIFocusController *aFocusController) +: mFocusController(aFocusController) +{ +} Inconsistent placement of ':' before member intiialzers, other classes in this diff place the ':' at the end of the ctor declaration. - The body of nsTypeAheadController::IsCommandEnabled() is identical to the body of nsTypeAheadController::SupportsCommand(), couldn't one call the other? sr=jst if you have a look at the above...

checked in

This checkin seems to have caused bug 189011 (Mail composition fails.)

Aaron, was this backed out as per bug 189011? Do we need to reopen this?

yeah, it looks like aaronl's 12:14 checkin today took care of this.

er, make that his 01/14/2003 12:42 checkin.

' and / require pressing SHIFT+#, SHIFT+7 respectively on a German keyboard layout. Do not know about other keyboard layouts. I think this would be an argument to let the user choose between several keys to relate to typeahead, that do not cause any problems with other functions. I suggest - and # or + on a German keyboard for example. Maybe one could implement a drop down menu with key settings for different keyboard layout to keep the prefs clean. SHIFT+7 for the / does not work in my build whereas SHIFT+# works fine. The former does not work when choosing from the menu too. Mozilla/5.0 (Windows; U; Win 9x 4.90; en-US; rv:1.3b) Gecko/20030114

reopening since this was effectively backed out.

FIXED (Reversed partial backout). The partial backout caused / to no longer initiate text search, so that should be fixed too.

vrfy'd fixed with 2003.02.10 comm trunk bits on win2k, mac 10.2.3 and linux rh8.0.