Closed Bug 466994 Opened 12 years ago Closed 12 years ago

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

Categories

(SeaMonkey :: Find In Page, defect)

defect
Not set
major

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: mcsmurf, Assigned: neil)

References

Details

(Keywords: regression)

Attachments

(1 file, 3 obsolete files)

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.
(In reply to comment #0)

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

Hitting ESC makes text entering possible again, too.
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.
OS: Windows XP → All
Hardware: PC → All
Should probably get fixed until final.
Flags: blocking-seamonkey2?
Keywords: regression
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.
Summary: New FAYT implementation prevents entering any text when text has not been found → New FAYT implementation prevents entering any text in text input boxes (html, xul)
Summary: New FAYT implementation prevents entering any text in text input boxes (html, xul) → New FAYT implementation prevents entering any text in text input boxes while search is active
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.
(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.
Ah, right. And losing focus from the find bar (triggered by any of the things I mentioned above) means you cancel FAYT.
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?
Attached patch WIP (obsolete) — Splinter Review
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.
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).
So the old version managed to only get keyboard input when it happened inside content?
(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 ;-)
Attached patch WIP 2 (obsolete) — Splinter Review
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.
Attachment #363999 - Attachment is obsolete: true
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?
(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).
(In reply to comment #14)
> Do we keep the tab alive until FAYT times out? Do we clear all state correctly?
Yes and yes.
Attached patch Proposed patch (obsolete) — Splinter Review
* 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.
Assignee: nobody → neil
Attachment #364283 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #364692 - Flags: superreview?(jag)
Attachment #364692 - Flags: review?(bugzilla)
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.
Attachment #364692 - Flags: review?(bugzilla) → review-
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 "".
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.
Attached patch Revised patchSplinter Review
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.
Attachment #364692 - Attachment is obsolete: true
Attachment #365313 - Flags: superreview?(jag)
Attachment #365313 - Flags: review?(bugzilla)
Attachment #364692 - Flags: superreview?(jag)
Attachment #365313 - Flags: superreview?(jag) → superreview+
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 on attachment 365313 [details] [diff] [review]
Revised patch

Thanks for working on this!
Attachment #365313 - Flags: review?(bugzilla) → review+
Pushed changeset 893ff1345c2b to comm-central.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
There's another behaviour difference on the new FAYT implementation.
I have filed Bug 470175 for that issue.  Pls take a look.
Clearing flag as this bug is fixed.
Flags: blocking-seamonkey2?
Duplicate of this bug: 486985
You need to log in before you can comment on or make changes to this bug.