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

RESOLVED FIXED

Status

defect
--
major
RESOLVED FIXED
11 years ago
9 years ago

People

(Reporter: mcsmurf, Assigned: neil)

Tracking

({regression})

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

11 years ago
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
(Reporter)

Comment 3

10 years ago
Should probably get fixed until final.
Flags: blocking-seamonkey2?
Keywords: regression
(Reporter)

Comment 4

10 years ago
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)
(Reporter)

Updated

10 years ago
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

Comment 5

10 years ago
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.
(Assignee)

Comment 6

10 years ago
(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

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

Comment 8

10 years ago
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?
(Reporter)

Comment 9

10 years ago
Posted 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.
(Assignee)

Comment 10

10 years ago
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).
(Reporter)

Comment 11

10 years ago
So the old version managed to only get keyboard input when it happened inside content?
(Assignee)

Comment 12

10 years ago
(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 ;-)
(Reporter)

Comment 13

10 years ago
Posted 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?
(Assignee)

Comment 15

10 years ago
(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).
(Assignee)

Comment 16

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

Comment 17

10 years ago
Posted 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)
(Reporter)

Comment 18

10 years ago
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.
(Reporter)

Updated

10 years ago
Attachment #364692 - Flags: review?(bugzilla) → review-
(Reporter)

Comment 19

10 years ago
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 "".
(Reporter)

Comment 20

10 years ago
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.
(Assignee)

Comment 21

10 years ago
Posted 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)

Updated

10 years ago
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.
(Reporter)

Comment 23

10 years ago
Comment on attachment 365313 [details] [diff] [review]
Revised patch

Thanks for working on this!
Attachment #365313 - Flags: review?(bugzilla) → review+
(Assignee)

Comment 24

10 years ago
Pushed changeset 893ff1345c2b to comm-central.
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED

Comment 25

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

Comment 26

10 years ago
Clearing flag as this bug is fixed.
Flags: blocking-seamonkey2?

Updated

9 years ago
Duplicate of this bug: 486985
You need to log in before you can comment on or make changes to this bug.