176296 - [RFE][Type Ahead] Type Ahead Find preferences need ability to force start with / or ' only

Status: VERIFIED FIXED

(SeaMonkey :: Find In Page, enhancement)

Description

[Owen Marshall (Not reading bugmail)]

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.

Comment 1

[Adam Katz]

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.

Comment 2

[Owen Marshall (Not reading bugmail)]

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.

Comment 3

[Owen Marshall (Not reading bugmail)]

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...

Comment 4

[Aaron Leventhal]

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

Comment 5

[sairuh (rarely reading bugmail)]

confirming.

Comment 6

[Padmanabhan V. K.]

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

Comment 7

[Jan Pazdziora]

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.

Comment 8

[Matthew Miller]

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.

Comment 9

[Aaron Leventhal]

Attachment: Implements new bool pref accessibility.typeaheadfind.autostart, and avoids unnecessary keypress listeners

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

Comment 10

[Aaron Leventhal]

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 11

[Kyle Yuan]

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.

Comment 12

[Aaron Leventhal]

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?

Comment 13

[Jo Hermans]

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

Comment 14

[Aaron Leventhal]

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?

Comment 15

[Johnny Stenback (:jst)]

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

Comment 16

[Aaron Leventhal]

Attachment: Creates pref accessibility.typeaheadfind.autostart (0=force manual, 1=auto text, 2= auto links)

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 17

[Aaron Leventhal]

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 18

[Kyle Yuan]

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.

Comment 19

[Aaron Leventhal]

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

Comment 20

[Aaron Leventhal]

Attachment: Contains Kyle's changes, and adds autofind="false" to mail3PaneWindowVertLayout.xul

Comment 21

[Aaron Leventhal]

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

Carrying r=kyle

Comment 22

[Aaron Leventhal]

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

Comment 23

[(not reading, please use [e-mail] instead)]

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 24

[Johnny Stenback (:jst)]

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...

Comment 25

[Aaron Leventhal]

checked in

Comment 26

[R.K.Aa.]

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

Comment 27

[Owen Marshall (Not reading bugmail)]

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

Comment 28

[sairuh (rarely reading bugmail)]

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

Comment 29

[sairuh (rarely reading bugmail)]

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

Comment 30

[S. Burmeister]

' 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

Comment 31

[sairuh (rarely reading bugmail)]

reopening since this was effectively backed out.

Comment 32

[Aaron Leventhal]

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

Comment 33

[sairuh (rarely reading bugmail)]

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