466994 - New FAYT implementation prevents entering any text in text input boxes while search is active

Status: RESOLVED FIXED

(SeaMonkey :: Find In Page, defect)

Description

[Frank Wein [:mcsmurf]]

To reproduce:
1. Search for a word on a website that does not exist, either use the / or the menu entry in the Edit menu to start the search
2. As soon as "Text not found: foo" appears in the status bar, try to enter any text in the URL bar or in any HTML text input box (even in another tab).

Results:
The text does not appear.

After a few seconds (FAYT timeout?) entering text works again.

Comment 1

[Jürgen 'JiHa' Harter]

(In reply to comment #0)

> After a few seconds (FAYT timeout?) entering text works again.

Hitting ESC makes text entering possible again, too.

Comment 2

[Tony Mechelynck [:tonymec]]

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1b3pre) Gecko/20081126 SeaMonkey/2.0a2pre - Build ID: 20081126132932

The bug is not limited to Windows.

Comment 3

[Frank Wein [:mcsmurf]]

Should probably get fixed until final.

Comment 4

[Frank Wein [:mcsmurf]]

BTW: This also happens in the normal FAYT case, type in a word that exists on the current web page and now try to enter any text in a text input box (comment field in Bugzilla for example or URL Bar in SeaMonkey). Observe that it tries to FAYT again.

Comment 5

[jag (Peter Annema)]

Can we add a focus or click handler that detects when the user focuses an input field (url bar, form field), and cancel FAYT?

Or, better yet, any time focus changes due to a user generated event? I seem to recall I could click anywhere in the page to cancel FAYT. And of course switching tabs or creating a new one would also cancel FAYT. Right now it clears the status bar, but the moment you start typing it continues FAYT on the old tab.

Can you look at how Firefox handles this? They've got this all working.

Comment 6

[[e-mail]]

(In reply to comment #5)
> Can you look at how Firefox handles this?
They don't have to bother. When you FAYT on Firefox it opens the Find bar, so the question of content fields getting focus does not apply.

Comment 7

[jag (Peter Annema)]

Ah, right. And losing focus from the find bar (triggered by any of the things I mentioned above) means you cancel FAYT.

Comment 8

[jag (Peter Annema)]

From irc:

<NeilAway> we might be able to use a selection listener (view source uses one to track your line when you click) but I don't know how well that works across frames
<jag> How about the following: as part of the FAYT key handler, you set a flag at the beginning of the method, and clear it at the end. If focus changes while the flag is set, it's prolly due to FAYT, and you do nothing. If it changes when the flag isn't set, that means the user did something to change focus, so you end FAYT.
<jag> Which I guess leaves you with hooking up a focus change listener when you start FAYT, and removing it when you end FAYT. Not sure where to hook it up though, I guesss that's the selection listener Neil's thinking of?

Comment 9

[Frank Wein [:mcsmurf]]

Attachment: WIP

Ok, this is a patch that works, but it's probably not finished. Guess the event listeners should be removed on stopFind again. Comments welcome.

Comment 10

[[e-mail]]

I had a look at the old FAYT, and it didn't use a focus listener. It did use a selection listener (and also a scroll listener but unfortuantely that particular version of the listener is C++ only).

Comment 11

[Frank Wein [:mcsmurf]]

So the old version managed to only get keyboard input when it happened inside content?

Comment 12

[[e-mail]]

(In reply to comment #11)
> So the old version managed to only get keyboard input when it happened inside
> content?
No, I meant to cancel the find... obviously it had a key listener too ;-)

Comment 13

[Frank Wein [:mcsmurf]]

Attachment: WIP 2

Fixes entering text into chrome text boxen, does not yet fix entering text into text boxen in content (websites, etc.). Problem seems to be the removal of the selection listener, need to take another look at that.

Comment 14

[jag (Peter Annema)]

You probably want to stopFind(true) in notifySelectionChanged.

I just got to wondering about what happens if a user closes a tab in the middle of FAYT. Your patch doesn't change this behaviour, but I'm still curious :-) Do we keep the tab alive until FAYT times out? Do we clear all state correctly?

Comment 15

[[e-mail]]

(In reply to comment #14)
> You probably want to stopFind(true) in notifySelectionChanged.
1.x does the equivalent of stopFind(false) (well, the status message at least).

Comment 16

[[e-mail]]

(In reply to comment #14)
> Do we keep the tab alive until FAYT times out? Do we clear all state correctly?
Yes and yes.

Comment 17

[[e-mail]]

Attachment: Proposed patch

* The unload listener deals with closing tabs.
* The blur listener deals with finding text in a field, and clicking elsewhere.
* The selection listener deals with finding text in a field and clicking within the field, and also finding not in a field and clicking anywhere.
The old FAYT listener always used the window selection, which was invisible in text fields (or contenteditable areas) but avoided the need for a blur handler.

Comment 18

[Frank Wein [:mcsmurf]]

Comment on attachment 364692 [details] [diff] [review]
Proposed patch

This patch does not work for the following case: FAYT for something on a webpage that has a input element on it. Now type (append) another character to the search string so that this search string does not exist on the webpage. Now click in the input element and try to enter some text. Note that the text gets appended to the search string instead.

Comment 19

[Frank Wein [:mcsmurf]]

Comment on attachment 364692 [details] [diff] [review]
Proposed patch

Another problem I noticed with this patch: It sometimes happens that starting FAYT does not work anymore (also not in a new browser window), Venkman brings up those two errors:
Error ``this.statusTextField is null'' [x-] in file ``chrome://navigator/content/nsBrowserStatusHandler.js'', line 139, character 0.
Error ``TypeError: this.statusTextField is null'' [x-] in file ``chrome://navigator/content/nsBrowserStatusHandler.js'', line 139, character 0.

When stepping into the setOverLink function, "this" only seems to contain member attributes that are all null or "".

Comment 20

[Frank Wein [:mcsmurf]]

Forget an error and the exception before the error:
Error: uncaught exception: [Exception... "'[JavaScript Error: "this.statusTextField is null" {file: "chrome://navigator/content/nsBrowserStatusHandler.js" line: 139}]' when calling method: [nsIXULBrowserWindow::setOverLink]"  nsresult: "0x80570021 (NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS)"  location: "JS frame :: file:///F:/mozilla/tree-hg/obj-suite/mozilla/dist/bin/components/nsTypeAheadFind.js :: anonymous :: line 262"  data: yes] (from JS Console)
Error ``Components is not defined'' [x-] in file ``chrome://navigator/content/nsBrowserStatusHandler.js'', line 57, character 0.
Error ``ReferenceError: Components is not defined'' [x-] in file ``chrome://navigator/content/nsBrowserStatusHandler.js'', line 57, character 0.

Comment 21

[[e-mail]]

Attachment: Revised patch

This fixes cancelling find after a failed match by always using the focused element, and it fixes the XULBrowserWindow error by not cancelling the previous find unless we're pretty sure it still needs to be cancelled.

Comment 22

[jag (Peter Annema)]

Comment on attachment 365313 [details] [diff] [review]
Revised patch

Looks good, though I'd add a comment before the two places where you remove the blur and selection listeners. Something like:

// We're about to blur and/or change the selection. Stop listening until we're done.

Comment 23

[Frank Wein [:mcsmurf]]

Comment on attachment 365313 [details] [diff] [review]
Revised patch

Thanks for working on this!

Comment 24

[[e-mail]]

Pushed changeset 893ff1345c2b to comm-central.

Comment 25

[doctor__j]

There's another behaviour difference on the new FAYT implementation.
I have filed Bug 470175 for that issue.  Pls take a look.

Comment 26

[Frank Wein [:mcsmurf]]

Clearing flag as this bug is fixed.