Closed Bug 167921 Opened 21 years ago Closed 21 years ago

Web pages should be able to use letters without conflicting with typeaheadfind by calling evt.preventDefault()

Categories

(SeaMonkey :: Find In Page, defect, P1)

Product:
SeaMonkey
Component:
Find In Page
Platform:
x86
All
Type:
defect

Tracking

(Not tracked)

Status:
VERIFIED FIXED
Milestone:
mozilla1.2beta

People

(Reporter: aaronlev, Assigned: aaronlev)

References

Details

Attachments

(2 files, 11 obsolete files)

Test page to see if preventDefault() is deactivating typeaheadfind for keystrokes handled by the page
530 bytes, patch
Details | Diff | Splinter Review
Uses NS_NAMED_LITERAL_STRING instead of L"foo"
52.79 KB, patch
aaronlev
: review+
aaronlev
: superreview+
Details | Diff | Splinter Review 
It is a legal application of the dom to use letter keypresses in script.
However, typeaheadfind also wants to use these letters. The web page needs to be
able to prevent typeaheadfind from finding links as the user presses keypresses
trapped by it's script.

Here's an example page that exhibits the problem:
http://labs.google.com/cgi-bin/keys?q=mozilla

I feel that as long as we can tell script authors a way to code their pages to
work along with us, we've done our job. Mac IE also searches for links as you
type, and scripts need to be able to be ready for this feature in whatever
browser it crops up in. After all, it's quite nice the way it is.
I have a solution working if I make typeaheadfind a bubbling listener. I've
tested it with a script that just calls evt.preventDefault();
Typeaheadfind then use GetPreventDefault() to see if it can use the key.

Unfortunately, at the moment typeaheadfind needs to be a capturing listener so
that it can grab Accel+[shift]+G or [shift]+F3 before normal find does (when
we're active). That's because we override the normal find next behavior when in
typeaheadfind mode. So it looks like I need to fix the way that works.




So, in order to fix this, I need to change the way we override the normal "find
next".

My proposal is the let the normal find-next check with typeaheadfind first and
see if it wants to do it. 

That means this would also fix bug 165315 - getting rid of hardcoded key
constants in typeaheadfind.

While we're at it, we might as well also make find-prev work for normal find,
since we'll need to have that parallel command in the normal find code when we
do this switch. In this way we can get a three-for-one fix.
Blocks: isearch, 167786
Severity: normal → major
Status: NEW → ASSIGNED
Priority: -- → P1
Target Milestone: --- → mozilla1.2beta
Summary: Web pages should be able to use letters without conflicting with typeaheadfind → Web pages should be able to use letters without conflicting with typeaheadfind by calling evt.preventDefault()
Reviewers -

The patch for this bug is longer than you'd think, here's the reason:

In order to fix this bug I had to change the way typeaheadfind received
find-next commands. Now nsWebBrowserFind calls it directly via
nsITypeAheadFind::FindNextIfActive(), instead of typeaheadfind trying to steal
the Accel+G or F3 keystrokes before nsWebBrowserFind did. This was all
necessary because in order for prevent default to work, I needed to be a
bubbling listener, and that wouldn't let me steal the keystrokes before
nsWebBrowserFind did.

So everything is much cleaner. However, this meant I also had to implement Find
Previous in all of the browser's components. Actually, in order to keep the
functionality I had, I needed to implement Accel+Shift+G, F3 and Shift+F3, as
well as a menu item for Find previous -- just doing the right thing there.

Here was my implementation and testing chart:

		  browser help editor im  mail viewsource  messagecompose
  Accel+shift+G   x	   x	  x	   x	   x		  x
  Menu item	  x    no menus   x	   x	   x		  x
  F3		  x	   x	  x	   x	   x		  x
  Shift+F3	  x	   x	  x	   x	   x		  x
  
Note that IM will require a Bugscape bug.

Here are all of the bugs that ended up getting fixed with this patch:
bug 167921 (this one) - typeaheadfind doesn't work with pages that contain
letter handlers 
bug 165315 - typeaheadfind shouldn't hardcode kbd shortcuts
bug 77704  - add accelerator for find prev (also added menu item)
bug 167783 - ctrl+g for typeaheadfind after repeating characters. 
	     This got fixed because I cleaned up what gets
	     done before/after a successful letter is typed 
	     into typeaheadfind vs. the new findnextifactive command
bug 168408 - don't add bad chars to type ahead buffer
bug 71832 - add F3 for find next (also shift+F3)
bug 167786 - bitflux editor broken by typeaheadfind - (they need to use
preventdefault)

Seeking r=/sr=
Someone please help me get this reviewed, it's clogging my tree and preventing
me from getting stuff done :-)
Attached patch New, better patch after testing (obsolete) — — Splinter Review 
While testing with this patch, I found that navigator.xul was grabbing
Backspace for go back in history before I could get it. I had to move the
backspace handling into platformHTMLBindings.xml and nsGlobalWindow.cpp's
nsDOMWindowController.

Also now fixes bug 168408, so that backspace does the right thing after a bad
character is typed.
Attachment #99195 - Attachment is obsolete: true
Comment on attachment 99340 [details] [diff] [review]
New, better patch after testing

--
+      mTypeAheadBuffer = Substring(mTypeAheadBuffer, 0,
+				    mTypeAheadBuffer.Length() - 1);
mTypeAheadBuffer.Truncate(mTypeAheadBuffer.Length() - 1) is more efficiency.

--
Index: mozilla/embedding/components/find/src/nsWebBrowserFind.h
===================================================================
...
+#include "nsITypeAheadFind.h"
You introduced a new dependency here - if we turned off typeaheadfind when
compile, nsITypeAheadFind.h will not be generated. So if
embedding/components/find is a optional module, it should be depends on
typeaheadfind; If it isn't, typeaheadfind shouldn't be optional either.

r=kyle for the typeaheadfind module if above questions get addressed.
You should ask someone else who is Front-End expert to take a look at the FE
part.
Comment on attachment 99484 [details] [diff] [review]
Adds MOZ_TYPEAHEADFIND so that we can still build nsWebBrowserFind.cpp without typeaheadfind. Everything else is the same.

Carrying forward Kyle's r=

Still need r= on the chrome changes (.dtd, .js and .xul files)

Also seeking r=cls on configure.in and MozillaBuildFlags.txt parts
Attachment #99484 - Flags: review+
Comment on attachment 99489 [details] [diff] [review]
Adds MOZ_TYPEAHEADFIND so that we can still build nsWebBrowserFind.cpp without typeaheadfind. Everything else is the same.

Carrying Kyle's r=
Attachment #99489 - Flags: review+

        Attachment #99489 -
        Attachment is obsolete: true

    
  

        Attachment #99493 -
        Flags: review+

    
    

    
  
I'm still waiting to hear back from drivers@mozilla.org regarding whether we
still have the restriction that add-on modules (psm, mailnews, extensions)
should not require additional defines to build.


    
    

    
  
Joe, can you give me r= for the chrome changes (.js/.xul/.dtd)?

    
    

    
  
Regarding chrome files (xul, js, dtd): in ComposerCommands.js can you collapse
nsFindNextCommand and nsFindPrevCommand into one class, say nsFindAgainCommand,
if they are similar?  The new class can have a ctor that takes the direction.

So the following 
  controller.registerCommand("cmd_findNext", nsFindNextCommand);
may change to something along the lines
  controller.registerCommand("cmd_findNext", new nsFindAgainCommand('next'));

The chrome changes look fine to me.

    
    

    
  
Is there a way we can do this without resorting to ifdefs?

    
    

    
  
No, not unless typeaheadfind is always built.
We need the ifdef's.

    
    

    
  
Why wouldn't it be built always?  It will rust and break if it's not on by
default, and if it's on by default, why can't it just be always on?

Or failing that, why can't you use tried-and-true XPCOM techniques to condition
things at runtime?

/be

    
    

    
  
I'm opene to making it build always. When it was new, that wasn't really an
option because it would have messed everyone up. Now that it's stable it seems
like a good idea.

However, it would really help me to get this in like it is and file a separate
bug on making it always build. We should probably move it out of extensions if
that's the case.

    
    

    
  
aaronl: you should check with embedding folks, maybe they don't want this code
in all builds.  How hard would it be to leave the code in extensions, and hook
it up via XPCOM services and such, without #ifdefs?

/be

    
    

    
  
Right, the issue is that I need to be able to include nsITypeAheadFind.h from that
webbrowserfind component. How can runtime xpcom methods help with that?

    
    

    
  
I should add that embeddors can simply leave out the .dll if they don't want the
code bloat.

It just still needs to be able to compile nsWebBrowserFind.
If the .dll is gone, it won't be able to get nsITypeAheadFind from the service
manager, so it won't try to call nsITypeAheadFind::FindNextIfActive().

    
    

    
  
The tried-and-true XPCOM techniques here would probably involve the
webbrowserfind component defining an interface for per-keystroke handling. 
That's how network protocols work, and command-line handlers, and such: the
extended system, not the extension, defines the interface and provides the headers.

But it sounds like the right thing is to just whack it all into our
webbrowserfind component.  Let's do that.

    
    

    
  
> But it sounds like the right thing is to just whack it all into our
> webbrowserfind component.  Let's do that.
It sounds like you're being facetious here, am I right?

The way I have it now works quite well, the typeaheadfind dll is not required in
a give install, so you can save the bloat. There is only 1 small part in
webbrowserfind.

    
    

    
  
So perhaps implement something like an nsIFindNextListener() on nsWebBrowserFind?
Something like that would be alright.

    
    

    
  
This looks like a job for nsIObserver/nsIObserverService.

    
    

    
  


  
seeking r=kyle on the new changes to nsTypeAheadFind that support the new
interfaces

seeking r=akkana for the nsWebBrowserFind changes

seeking r=cls on the build stuff, I got rid of the MOZ_TYPEAHEADFIND stuff. The
build changes that need r= are so that nsIWebBrowserFindHandler.idl and
nsIWebBrowserFindManager.idl get built. I wasn't sure if they should go under
SDK_XPIDLSRCS or XPIDLSRCS, but it seems to be fine under SDK_XPIDLSRCS. Should
I change it?

sgehani gave r= for the chrome changes over email

        Attachment #99493 -
        Attachment is obsolete: true

    
    

    
  
Comments:

+interface nsIWebBrowserFindHandler : nsISupports
+{
+  // Return true if handled 
+  boolean FindNext(in boolean aIsReverse);
+};

Use leading lowercase for interface methods. 'findNext'.

+
+      <!-- Supporting IE forward and back accelerators out of courtesy
+           to transitioning IE users -->
+      <handler event="keypress" keycode="VK_BACK" command="cmd_browserBack"/>
+      <handler event="keypress" keycode="VK_BACK" modifiers="shift"
command="cmd_browserForward" />

Will this affect embedders? If so, will they want that? Will it interfere with
the delete key in forms?

I don't really like the changes. It seems weird to me that our nsWebBrowserFind
should delegate the entire Find operation to some other optional component, just
to fix an event handling issue, and in the process add two new interfaces. If
you really have to do this, why not just override all of nsIWebBrowserFind by
providing your own implementation of the entire API?

    
    

    
  
The Backspace key already goes back in history on Windows, so the form issue is
the same as it was.

As far as embeddors wanting Backspace to go back or not, we implement a host of
keystrokes that they may or may not want. This is no different. We should
probably make it easier for them to modify keystrokes, but they can make their
own htmlBindings.xml or platformHTMLBindings.xml files.

If I implemented nsIWebBrowserFind on nsTypeAheadFind, how would I make sure my
impl overrode the nsWebBrowserFind impl?

    
    

    
  
Simon, we're not delegating the entire find operation -- just "find next" and
"find previous".  Type Ahead Find needs to handle those operations when it's the
last active find.

    
    

    
  
Comment on attachment 99711 [details] [diff] [review]
Creates 2 new interfaces to get rid of dependency and #ifdef's (nsIWebBrowserFindHandler and nsIWebBrowserFindManager)

The idls should only be listed in SDK_XPIDLSRSCS if they are frozen (@status
FROZEN) .

    
    

    
  
we looked at this today and there is a problem with implementing the
nsIWebBrowserFind interfaces completely (I should note here that I am not
avocating that we check aarons code in as-in.).  nsIWebBrowserFind is a per
docshell object.  This is a problem for the current design of typeaheadfind
since typeaheadfind is a singleton.  Currently, there is no way to keep track of
which docshell was doing what search.  We would have to break out typeaheadfind
so that we can create an object per docshell.  (aaron, why is typeaheadfind a
service again?)

However, I don't see any reason why typeaheadfind couldn't replace the default
find after these changes are made.  I just think that alot of work is going have
to be done in order for typeaheadfind to be massaged into overriding the default
find.


    
    

    
  
aaron, could you write a wrapper for |findNext()| in js code - looking for 
nsITypeAheadFind firstly, call nsITypeAheadFind ::FindNextIfActive if it got 
loaded, or pass the call to nsIWebBrowserFind::FindNext. That could avoid to 
add new dependency between nsIWebBrowserFind and nsITypeAheadFind, and you 
don't need worry about how to find "nsITypeAheadFind.h" in the js code :)
I don't dig the code so deeply, just for inspiration.

    
    

    
  
Kyle, thanks for the suggestions. We can't do that, because it won't solve the
problem for embeddors using nsIWebBrowserFind.

    
    

    
  


  
This patch deals with all of the concerns that have been brought to me.

After talking with Simon, we agreed that using nsIObserver was better than
creating new interfaces. I'm able to pass back whether type ahead find
succeeded via an nsISupportsPRBool for the nsISupports *aSubject

        Attachment #99756 -
        Attachment is obsolete: true

    
  

        Attachment #99890 -
        Flags: review+

    
    

    
  
Comment on attachment 99890 [details] [diff] [review]
Uses nsIObserver so that nsTypeAheadFind can observe cmd_findagain, and either execute the command or return false

r=kyle on the TypeAheadFind changes. Just one question:
+	 observerService->NotifyObservers(didExecute, "cmd_findagain",
mFindBackwards? L"down": L"up");
Is this portable? Why don't just use NS_LITERAL_STRING?

    
    

    
  
Kyle, nsIObserverService->NotifyObservers has been around for a while. It takes
const PRChar* and const PRUnichar*, not nsAStrings. Don't know why, but that's
what it does.

    
    

    
  
L notation is not portable. You need to use NS_LITERAL_STRING.

    
    

    
  


  
Ah, okay - but I had to append .get() to the literal strings.
I've had problems with NS_LITERAL_STRING in ? : syntax before, so I used
NS_NAMED_LKITERAL_STRING like this:

NS_NAMED_LITERAL_STRING(downString, "down");
NS_NAMED_LITERAL_STRING(upString, "up");
observerService->NotifyObservers(didExecute, "cmd_findagain", mFindBackwards?
downString.get(): upString.get());

        Attachment #99890 -
        Attachment is obsolete: true

    
    

    
  


  
Thanks for pointing me to NS_L("foo"), Kyle.

        Attachment #99943 -
        Attachment is obsolete: true

    
    

    
  
Comment on attachment 99944 [details] [diff] [review]
Even better, uses | mFindBackwards? NS_L("down"): NS_L("up") |

r=kyle

        Attachment #99944 -
        Flags: review+

    
    

    
  
sorry, I mean r=kyle on the TypeAheadFind changes.

    
    

    
  
Please note that |NS_L| only expands to a wide string on platforms with native
wide string support.  This macro is not for use by individual programmers. 
|NS_L| is just a tool for use in the bigger macros you are eschewing, except in
the case where you are doing literal concatenation, e.g.,

  NS_MULTILINE_LITERAL_STRING(
    NS_L("this is the first line")
    NS_L(" this is the second line")
    NS_L(" this is the third line, note the lack of commas") )

It really shouldn't be used outside of this context.  If

  expr ? NS_LITERAL_STRING("a") : NS_LITERAL_STRING("b")

is giving you problems, then your answer is comment #42, above, is the best answer.

I strongly urge you not to use |NS_L| directly, as this will lead to mixed size
comparisons and syntax errors on any platform where |sizeof(wchar_t) !=
sizeof(PRUnichar)|.  Including and especially platforms where there is no |wchar_t|.


    
    

    
  
Comment on attachment 99943 [details] [diff] [review]
Uses NS_NAMED_LITERAL_STRING instead of L"foo"

Seeking sr= for this patch instead, because it uses NS_NAMED_LITERAL_STRING
instead of NS_L.

        Attachment #99943 -
        Attachment is obsolete: false

        Attachment #99943 -
        Flags: review+

    
    

    
  
Comment on attachment 99944 [details] [diff] [review]
Even better, uses | mFindBackwards? NS_L("down"): NS_L("up") |

This one is now obsolete, as per scc's comments.

        Attachment #99944 -
        Attachment is obsolete: true

    
    

    
  
Don't observe "cmd_findagain", since this will conflict with command manager
command update notifcations. Choose a unique string, like
"nsWebBrowserFind_FindAgain".

    
    

    
  
Comment on attachment 99943 [details] [diff] [review]
Uses NS_NAMED_LITERAL_STRING instead of L"foo"

I received sr=brendan over email

sfraser, I have your changes in there too.

        Attachment #99943 -
        Flags: superreview+

    
    

    
  
checked in
Status: ASSIGNED → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED

    
    

    
  
vrfy'd fixed using 2002.09.26.08 mozilla trunk builds.

when visiting a page that uses letter handlers, those letter keypresses take
precedence over typeahead find. however, typeahead find otherwise still works on
such pages. tested on http://labs.google.com/cgi-bin/keys?q=mozilla
Status: RESOLVED → VERIFIED

    
    

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

    
    

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

    
    

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

    
  
Component: Keyboard: Navigation → Keyboard: Find as you Type

    
    

    
  
Don't know whether this should be reopened or should I file a new bug for this.
I think type-ahead find should have priority over DOM keybindings if it is
already active, since that's what users would except.

Since the first report used google labs, I'll continue that trend:

Open http://labs.google.com/cgi-bin/keys?q=bug

Start typing: bugnet

Excepted behavior: the link to first hit "bugnet" should be found and selected.

Observed behavior: first three words are found and highlighted but after
pressing "n" googlelabs script takes over and takes you to next page of results,
even when the type-ahead find is already active.


    
    

    
  
*** Bug 215024 has been marked as a duplicate of this bug. ***
Product: Core → SeaMonkey

          You need to log in
          before you can comment on or make changes to this bug.