201996 - Caret blinking in field, but focus is somewhere else

Status: VERIFIED FIXED

(Core :: DOM: UI Events & Focus Handling, defect)

Description

[Ere Maijala (slow)]

Running trunk build 2003041208 on WinXP, although this has been a problem with a
few builds lately (I'm going to see if I find a time frame when this started).

I have a frameset where there is a menu on the left and a form on the right.
When the form is shown, the focus is set to the field using javascript. Caret
blinks happily in the field, but typing initiates find as you instead. A reduced
test case is available at http://atp.fi/~ere/frametest. I'll also attach it.
Press the form link on the left and try typing. If it works, click reload and
try again.

Comment 1

[Ere Maijala (slow)]

2003040209 works, 2003040308 fails. Brian, could your fix for bug 170811 have
caused this?

Comment 2

[Brian Ryner (not reading)]

seems likely.  and this has nothing to do with 'form manager'.

Comment 3

[Ere Maijala (slow)]

Oops, sorry. Should have thought about it a little more.

Comment 4

[Brian Ryner (not reading)]

Can't reproduce on a recent Linux build.  I tried reloading several times.  I'll
check Mac and Windows next.

Comment 5

[Brian Ryner (not reading)]

Can't reproduce on windows either :(  (just-pulled cvs build, windows xp)

Comment 6

[Ere Maijala (slow)]

I can still reproduce, pulled today about 8 hours ago. I just click Form, type
something (works), click Reload and type something (doesn't).

Comment 7

[Ere Maijala (slow)]

Attachment: Zipped testcase

This is the test case zipped.

Comment 8

[Ere Maijala (slow)]

Happens on Linux too using the latest nightly trunk build. Not every time, but
after a few reloads it always happens.

Comment 9

[Ere Maijala (slow)]

About nsPresShell::CheckForFocus(). The latest version is missing an old check
to see if focusedWindow == domWindow (aOurWindow). Without this check it seems
to steal the focus from somewhere else. Now, the check makes me wonder if the
call to ourWin->Focus() is necessary at all. Everything I tried works fine
without it. Why focus a window that already has focus? The only reason I could
find for this is if focus controller just _thinks_ ourWindow has focus although
it doesn't. I don't know if such a case exists. 

Anyway, having the check in place, or removing the Focus() call altogether,
makes the test case work perfectly and doesn't break tinderbox iframes (bug 170811).

Comment 10

[Ere Maijala (slow)]

Attachment: Proposed patch

This patch will restore the check in CheckForFocus() to focus the window only
if it's the active one. This avoids stealing the focus from someone else. I've
tested that scrolling after clicking a link in a tinderbox iframe still works
(bug 170811), my test case works and http login dialog (bug 191896) works.

Btw. the reason Focus() needs to be called is that when suppressed, only focus
controller is updated and the window doesn't really get the focus before this
call. This happens for example when creating the http auth login dialog.

Comment 11

[Ere Maijala (slow)]

The test case at http://atp.fi/~ere/frametest is improved a bit. Reloading it
with focus in the input field should definitely screw the focus without the patch.

Comment 12

[Ere Maijala (slow)]

Just for the record: the test case only works if it loads quickly enough, so to
test it should be copied to a local server or disk.

Comment 13

[sairuh (rarely reading bugmail)]

able to repro on mac/nscp, but for some reason cannot (so far) do so on camino.
(both trunk builds from today, 2003.04.18)

Comment 14

[Michael Buckland]

Discussed in edt bug triage.  Plussing.

Comment 15

[[e-mail]]

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

This patch doesn't fix bug 203117 because it's the call to
aFocusController->SetFocusedWindow that's breaking that bug.

Comment 16

[Ere Maijala (slow)]

That problem is probably bug 201242 then.

Comment 17

[[e-mail]]

Going from the "form" page to mozilla.org, back, forward, lost focus :-(

Comment 18

[Brian Ryner (not reading)]

Attachment: fix for a number of focus issues

This patch fixes this bug, bug 200735, bug 203117, and bug 194104.  Summary of
the changes:

- No longer require a frame to be present for all events.  In particular, don't
require one for focus events.  This ensures that focus events get through to
the focus controller even if we haven't reflowed the document yet.

- General cleanup and commenting in nsEventStateManager::PreHandleEvent().  I
fixed some cases where we should be reusing the event struct but aren't.

- In nsHTMLInputElement::SetFocus() and Select(), make sure to set the focused
window as well as the focused element if the focus controller is inactive. 
This fixes setting latent focus to a text input inside an iframe.

- Fixed handling of certain conditions in PresShell's CheckForFocus().	The
previous behavior could be incorrect depending on whether the parent or child
document unsuppressed painting first.

- General cleanup in PresShell::HandleEvent() and HandleEventInternal(). 
Allowed focus events to go through with no frame, updated some comments,
normalized indentation.

Comment 19

[Brian Ryner (not reading)]

Attachment: diff -w of the above, for easier reviewing

Comment 20

[[e-mail]]

Comment on attachment 122185 [details] [diff] [review]
diff -w of the above, for easier reviewing

>@@ -2648,6 +2661,8 @@ static void CheckForFocus(nsPIDOMWindow*
> 
>     nsCOMPtr<nsIDocument> parentDoc;
>     curDoc->GetParentDocument(getter_AddRefs(parentDoc));
>+    if (parentDoc == aDocument)
>+      return;
>     curDoc = parentDoc;
>   }
> 
I can verify that this fixes bug 203117.

Comment 21

[Ere Maijala (slow)]

Thumbs up for bryner, the patch works great :)

Comment 22

[sairuh (rarely reading bugmail)]

nominating for 1.4beta, as this fixes a bunch of focus issues. in addition to
the ones mentioned in comment 18, bug 202180 also appears to be fixed.

Comment 23

[Johnny Stenback (:jst)]

Comment on attachment 122184 [details] [diff] [review]
fix for a number of focus issues

sr=jst

Comment 24

[John Keiser (jkeiser)]

Comment on attachment 122184 [details] [diff] [review]
fix for a number of focus issues

r=me if you fix the comment about sending a blur event to the window, which the
NS_GOTFOCUS code doesn't do.  I am a little sad that this fix is restricted to
focus events, but it's a start and won't be hard to remove the restriction :)

Comment 25

[Brian Ryner (not reading)]

Comment on attachment 122184 [details] [diff] [review]
fix for a number of focus issues

drivers wants this for 1.4, right?

Comment 26

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

Comment on attachment 122184 [details] [diff] [review]
fix for a number of focus issues

a=sspitzer, as this is a 1.4 blocker

Comment 27

[Brian Ryner (not reading)]

Fix checked in.

Comment 28

[sairuh (rarely reading bugmail)]

vrfy'd fixed with 2003.05.12 comm builds.