403232 - Focused textfield loses focus when switching to another tab and back

Status: VERIFIED FIXED

(Core :: Widget: Cocoa, defect, P1)

Description

[Steven Michaud [:smichaud] (Retired)]

Steps to reproduce:

1) Open two tabs, at least one of which contains a text input field in
   the tab's content area (i.e. not in textfields shared by all pages,
   like the URL box or the Google search box).

2) Click on a textfield in the current tab -- the text input cursor
   (the I-bar cursor) will start blinking there.

3) Switch to the other tab, then back again -- the text input cursor
   will no longer be visible (or blinking), and the keyboard focus
   will no longer be in the textfield that you clicked on in step 2.

This only happens on the trunk on Mac OS X.  It doesn't happen on
Windows or Linux on the trunk, or on any of the three OSs on the 1.8
branch.

This regressed with the 2006-09-29-06-trunk nightly -- so it
cooincides with the switch to Cocoa widgets.

As best I can tell, this hasn't yet been reported ... which is
puzzling, since it seems quite a major problem.

On the face of it, this looks like a focus bug.  But it's different
from other focus bugs that have been reported (e.g. bug 354768 and bug
399471).

Comment 1

[Jesse Ruderman]

In step 3, what method are you using to switch tabs?

Comment 2

[Steven Michaud [:smichaud] (Retired)]

In my tests I always used the mouse.

Comment 3

[Steven Michaud [:smichaud] (Retired)]

I may have a simple fix for this ... stay tuned :-)

Comment 4

[Steven Michaud [:smichaud] (Retired)]

Attachment: Fix

I _do_ have a simple fix ... though a different one than I first
thought.

As my comment says, I don't yet fully understand why this patch works.
I hope to be able to fill out (and possibly correct) this comment
after I've had a chance to do more research (hopefully by sometime
next week).

This patch actually fixes two problems -- the original focus problem
(whose symptoms are reported in this bug and elsewhere), plus another
problem that was uncovered by my fix for the focus problem.  The
second problem is that sometimes you can get text-input cursors
flashing in more than one text field (in the same window).

Here's a tryserver build made using this patch.  I'm already quite
confident my patch fixed this bug (403232), bug 399471 and bug 404433.
But a number of other focus problems have been reported that I'm
unable to reproduce.  I'd really like to know if my patch also fixes
at least some of them (the Mac-specific ones).

https://build.mozilla.org/tryserver-builds/2007-11-29_13:[e-mail]-focus/

Comment 5

[Steven Michaud [:smichaud] (Retired)]

Attachment: Fix rev1 (deal with possible re-entrancy)

Here's a revision (one Josh asked for) that accounts for the
possibility that nsChildView::SetFocus() might be called re-entrantly.

(This might happen while Gecko processes an NS_LOSTFOCUS or
NS_GOTFOCUS event ... though it would probably be a bug.)

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+    // For reasons that aren't yet clear, focus changes within a window (as
+    // opposed to those between windows or between apps) should only trigger
+    // NS_LOSTFOCUS and NS_GOTFOCUS messages (to Gecko) in the context of a
+    // call to nsChildView::SetFocus() (or nsCocoaWindow::SetFocus(), which
+    // in any case re-routes to nsChildView::SetFocus()).  If we send these
+    // messages on every intra-window focus change (on every call to
+    // [ChildView becomeFirstResponder:] or [ChildView resignFirstResponder:]),
+    // the result will be strange focus bugs (like bmo bugs 399471, 403232
+    // and 404433).

I think this comment is really in the wrong place. It should be next to becomeFirstResponder.

How about making setInSetFocus into changeInSetFocus(PRInt32)? Maybe with an nsAutoInSetFocus helper class?

What's causing becomeFirstResponder to be called outside of SetFocus? Widget showing/hiding? Sounds like a similar Windows problem...

+      // Sometimes SetFocus() is called on an nsChildView object that's
+      // already focused. 

You mean it's already focused in AppKit but not in Gecko? How can that happen?

Comment 7

[Håkan Waara]

Steven, if you haven't, plase see the window delegate that does focus/blur magic. Probably it is relevant here.

Comment 8

[Steven Michaud [:smichaud] (Retired)]

(In reply to comment #7)

My first focus patch (bug 354768 attachment 279037 [details] [diff] [review]) already made some
changes to this code, and reduced the frequency of focus problems.
This patch should reduce the frequency still further ... but I doubt
it will eliminate them completely.

The focus code is very complex, and so far I've only scratched the
surface.  But over the next few months I'll be digging deeper, and
expect to keep finding bugs and fixing them.

(Here are some metrics that give you an idea how complex the focus
code really is:  I created a debugging patch that did no more than put
printf() statements into all the code (anywhere in the tree) having
something to do with focus (methods with "focus", "blur" or "activate"
in their names, the relevant parts of event handlers, and so forth).
This patch touches about 100 files, and is just under 500K!)

Comment 9

[Steven Michaud [:smichaud] (Retired)]

Attachment: Fix rev2 (use helper class, add comments)

> I think this comment is really in the wrong place. It should be next
> to becomeFirstResponder.

I actually think this comment makes better sense in
nsChildView::SetFocus(), since it concerns stuff "called" from there.
But I've added short comments at becomeFirstResponder and
resignFirstResponder that boil down the main comment, and refer people
back to it.

> How about making setInSetFocus into changeInSetFocus(PRInt32)? Maybe
> with an nsAutoInSetFocus helper class?

I wanted to preserve the fiction that the accessor functions are
setting and reading a boolean ... though if you object to that I can
change their names.

I did create a helper class, though.  (It also retains and releases
mView, which may be overkill.  But at least that does no harm.)

> What's causing becomeFirstResponder to be called outside of
> SetFocus? Widget showing/hiding?

I don't yet know ... though I hope to find out.

> +      // Sometimes SetFocus() is called on an nsChildView object that's
> +      // already focused.
>
> You mean it's already focused in AppKit but not in Gecko?

As best I can tell, it's already focused in Gecko ... which (if I'm
right) means that this part of my patch is really a workaround for
another bug (probably cross-platform).  I hope to find out what that
bug is (and fix it).  But in the meantime I think this workaround will
do.

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

What if a focus event triggers a modal dialog or something else that runs a nested event loop? Will we carry on thinking that inSetFocus is true, thereby triggering another instance of this bug when another focus change happens?

I'm thinking it might just be best to fix the underlying cause, whatever that is...

Comment 11

[Steven Michaud [:smichaud] (Retired)]

> What if a focus event triggers a modal dialog or something else that
> runs a nested event loop?

Can you imagine a reasonable way (or reasonably likely way) in which
this might happen?

I can't.  And I think it's better to land this patch now and wait for
something like this to come up.

By the way, it's the second part of my patch that (probably) masks a
Gecko bug -- the part that sends NS_LOSTFOCUS and NS_GOTFOCUS events
in quick succession if Gecko already has the focus when SetFocus() is
called.

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

How about <button onfocus="alert('hello')">?

Comment 13

[Steven Michaud [:smichaud] (Retired)]

> How about <button onfocus="alert('hello')">?

OK :-)

I'll test with this and see what I can come up with.

Comment 14

[Steven Michaud [:smichaud] (Retired)]

Roc, your onfocus example throws up a host of problems -- not least
with other browsers.  But it _doesn't_ cause any trouble with my
current patch for this bug (attachment 290893 [details] [diff] [review]).

For this and other reasons, I don't think we should try to deal with
the special case you came up with in comment #10.  So unless you can
come up with another (better) way to address these problems (plus
those that my patch addresses), or another testcase that actually
shows my patch behaving badly, I think my current patch (attachment
290893 [review]) should be landed more-or-less as is.

Yes (as I keep saying), I haven't yet discovered the "real" problem(s)
that my patch (in effect) papers over.  But accomplishing this will
probably take a long time (a thorough review of the focus code will
take months).  And (as best I can tell) the benefits of my current
patch _heavily_ outweigh the risks.

What follows is complex (and quite long).  I'll stick in some headers
in an attempt to keep it readable.

A) What I Tested With

I turned the example from comment #10 into the following HTML file.
The reasons for its (apparently) excess complexity should become clear
as we go along:

<HTML>
<HEAD>
<TITLE>OnFocus Alert</TITLE>
</HEAD>
<BODY>
<script language="javascript" type="text/javascript">
var count = 0;
function doAlert() {
  dump("blah1\n");
  if ((++count % 5) != 0) {
    dump("blah2\n");
    alert(count);
  }
  dump("blah3\n");
}
</script>
<button onfocus="doAlert();">OnFocus Alert</button>
</BODY>
</HTML>

(If you set browser.dom.window.dump.enabled (a boolean) to 'true' in
about:config, you'll see the output of the dump() commands in stdout.
If you don't do something to break the sequence of alert() windows,
Minefield (at least) will go into an infinite loop of alert() windows,
and you may need to force-quit it.)

B) The Onfocus Example Doesn't Cause Trouble With My Patch

As I tested my patch, I found that both nsChildView::SetFocus() and
[ChildView sendFocusEvent:] sometimes got re-entered (so the revision
that Josh suggested, mentioned in comment #5, is needed).

But SetFocus() never got "stuck" while waiting for user input (either
in the modal alert window or its parent browser window).  In other
words, mInSetFocusLevel was always '0' in this case.  So it's unlikely
that (with my patch) SetFocus() will set mInSetFocusLevel incorrectly,
even while the browser is running a modal event loop (even a multiply
nested one, as we'll see below).

With or without my patch, Minefield (unlike almost any other browser)
behaves more-or-less correctly with this example:  Every time the
"OnFocus Alert" button is newly focused, an alert window pops up.  And
it keeps popping up, each time you dismiss it, three more times (for a
total of four times).  If the button isn't already focused, it will
get focused when you click on it.  Or if it's already focused, it will
get focused again if you switch to another window (or app) and back
again.

C) The Onfocus Example _Does_ Cause Trouble with Trunk Event Handling

Looking at the example's code, you'd expect that the various dump()
statements would always be sent to stdout in numerical order.  But
this doesn't happen.  Instead you keep seeing "blah1" and "blah2"
until the fifth time the "OnFocus Alert" button is focused.  Then you
see one "blah1" and five "blah3"s.

Each time another alert window is displayed, the example's code is
re-entered, and its modal event loop is nested one level deeper (on
top of all the other alert windows' event loops).  Until the fifth
time through, when no alert window gets displayed.  (You see the same
thing if you observe the value of gXULModalLevel.)

I suspect this is a side effect of bug 301791.  But a fix for that bug
would be complex and risky, so it's unlikely to happen anytime soon.
Which means we'll have to live with its consequences for the time
being.

D) Which Makes It Impossible to Deal with Comment #10 Special Case

Which in turn means that Roc's special case from comment #10
is (basically) impossible to deal with (at least as far as I can
tell):

In principle, it should be possible to "correct" the value of
mInSetFocusLevel by subtracting from it the current modal depth (or
recursion depth).  But bug 301791 means that both of these values are
sometime much higher than they should be, which in turn means that
it's not reasonable to use their values to correct mInSetFocusLevel.

Comment 15

[Steven Michaud [:smichaud] (Retired)]

(Following up comment #14)

Here's my argument over again, boiled down as far as I can manage:

1) Roc's general objection from comment #10 is valid -- there _is_ a
   possibility that the return value of [ChildView inSetFocus] might
   be wrong if we're running modal (or are otherwise handling events
   recursively).

2) The testcase he suggests (the onfocus testcase) doesn't show this
   happening.  And in general I think it's very unlikely.

3) Bug 301791 makes it impossible to address Roc's objection (from
   comment #10) directly (and bug 301791 is itself so complex that
   it's unlikely to be fixed soon).

4) My current patch for this bug is a stopgap measure.  But it isn't
   just a superficial one -- though it's very simple, it fixes many
   different bugs, including one (bug 404433) that takes some thought
   to realize is a focus bug.

5) In sum, I think the benefits of my patch heavily outweigh its
   risks.

Comment 16

[Steven Michaud [:smichaud] (Retired)]

Side note:

While messing with all this I realized that ChildView's accessor
methods (setInSetFocus and inSetFocus) need to be public, and declared
in nsChildView.h.

I'll either post a new patch with this revision, or make the change
before I land the current patch.

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

My point is that inSetFocus is not a well-defined concept --- partly because of nested event loops, but in general because code that runs during a focus event can really be any code, so associating it with the focus event is at best a heuristic --- and therefore this stopgap patch is not architecturally sound. We've been doing a lot of hack-and-patch work in the Cocoa code and this is going to be another case where we hope the patch fixes more than it breaks, but we don't really understand what's going on so we don't really know. And even if it makes things work better from the user's point of view, we're actually decreasing code quality by adding known-bad hacks.

Now, maybe you're right that this is the best we can do for 1.9 and we should just land it and it will probably give better results for the next release than not landing it. If so, let's do that. But we need to recognize that widget/cocoa and focus in general is in bad shape and we can't continue this way indefinitely.

Comment 18

[Steven Michaud [:smichaud] (Retired)]

> Now, maybe you're right that this is the best we can do for 1.9 and
> we should just land it and it will probably give better results for
> the next release than not landing it.

I think so :-)

> But we need to recognize that widget/cocoa and focus in general is
> in bad shape and we can't continue this way indefinitely.

I suspect focus (on all platforms) is in a lot worse shape than
widget/cocoa.  It's certainly received less attention.

But even widget/cocoa will continue to need attention after the
Firefox 3.0 release.  If nothing else, the business about bug 301791
shows that.

I also suspect event handling on all platforms will continue to need
more work.

I'm quite willing to take on focus (and event handling) as special
projects, going forward from Firefox 3 (particularly in Mozilla 2.0).
I certainly won't be the only one working on event handling.  But I
don't detect much enthusiasm out there for working on focus code
... so that may end up largely in my lap :-)

I don't know enough now to come up with a plan for revising the focus
code.  But, over time, I hope to learn it from the ground up.  After a
few months of that, I should start to have some ideas.

Comment 19

[Mike Schroepfer]

> I'm quite willing to take on focus (and event handling) as special
> projects, going forward from Firefox 3 (particularly in Mozilla 2.0).
> I certainly won't be the only one working on event handling.  But I
> don't detect much enthusiasm out there for working on focus code
> ... so that may end up largely in my lap :-)
> 
> I don't know enough now to come up with a plan for revising the focus
> code.  But, over time, I hope to learn it from the ground up.  After a
> few months of that, I should start to have some ideas.
> 

SOLD!  

Comment 20

[Steven Michaud [:smichaud] (Retired)]

(In continued reply to comment #17)

> My point is that inSetFocus is not a well-defined concept --- partly
> because of nested event loops, but in general because code that runs
> during a focus event can really be any code, so associating it with
> the focus event is at best a heuristic

Actually, inSetFocus as I'm using it is quite narrowly defined -- I
only look at its results in [ChildView becomeFirstResponder:] or
[ChildView resignFirstResponder:].

Yes, it's a hack -- though I can see that it works very well, I don't
yet fully understand why it does so.  But I'm reasonably confident
that I'll eventually be able to remove this hack and replace it with
something that gets closer to the actual cause of these focus problems
(those fixed by my inSetFocus hack).

Howver, what I replace it with, though it will be less hackish, is
still likely to be something of a hack.  And I don't think that, by
itself, is a problem.  In any code that's as complex as the Mozilla
source tree, compromises are inevitable -- trade-offs that are needed
because it's not possible (or maybe not yet possible) to completely
solve two different problems simultaneously.

Yes, we need to keep trying to make Mozilla source clearer and more
logical (so that ideally every piece of code has a reasonably simple
explanation).  But we can't always get from here to there in one leap.

Comment 21

[Steven Michaud [:smichaud] (Retired)]

So, roc, are you willing to let this through? :-)

Comment 22

[Steven Michaud [:smichaud] (Retired)]

> SOLD!

I'll try not to break them as I play with them :-)

Comment 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 290893 [details] [diff] [review]
Fix rev2 (use helper class, add comments)

The problem is that just because we're inside a SetFocus call doesn't mean that
become/resignFirstResponder is meant to be happening in response to that SetFocus call. It might be
something conceptually unrelated that just happens to be triggered during focus processing. My alert() nested
event loop example was just one case of that. That is why "inSetFocus" is kind of meaningless ...
it tells us whether there's a SetFocus call on the stack, but *that* isn't a foolproof basis
for making any decisions on.

Comment 24

[Steven Michaud [:smichaud] (Retired)]

> The problem is that just because we're inside a SetFocus call
> doesn't mean that become/resignFirstResponder is meant to be
> happening in response to that SetFocus call. It might be something
> conceptually unrelated that just happens to be triggered during
> focus processing.

The point of my patch is to distinguish between those calls to
become/resignFirstResponder that happen because of (in response to) a
call to SetFocus and those that don't.  Only in the first case do I
call sendFocusEvent.

In the future I may have other ways of knowing when to call
sendFocusEvent (from become/resignFirstResponder) ... but right now I
don't.

> That is why "inSetFocus" is kind of meaningless ...  it tells us
> whether there's a SetFocus call on the stack, but *that* isn't a
> foolproof basis for making any decisions on.

I'd say that it's _possible_ for inSetFocus to have the wrong meaning
during calls to becomeFirstResponder or resignFirstResponder ... but
that it's very unlikely.

If I turn out to be wrong, we'll have to re-open the bug.  But if that
does happen, I hope that by then I'll know enough more about how the
focus stuff works to come up with a better workaround.

Thanks for your sr+.  If I've messed up it won't be your fault :-)
I'm pretty sure, though, that I haven't messed up.

Comment 25

[Steven Michaud [:smichaud] (Retired)]

Landed on trunk.

Comment 26

[Steven Michaud [:smichaud] (Retired)]

I had to back the patch out -- it failed some mochitests :-(

I'll deal with this tomorrow.

Comment 27

[Håkan Waara]

Some of the dependent bugs would be possible to get into the test suite, right? Perhaps that'd be a good idea in general with these focus bugs that are "fragile" and hard-to-find.

Comment 28

[Steven Michaud [:smichaud] (Retired)]

I agree that it's a good idea.

But it should (of course) wait until I've figured out how to stop my
patch from failing some of the Mochitests, and can re-land it.  (I'm
working on this.)

Comment 29

[Steven Michaud [:smichaud] (Retired)]

I've now reproduced the mochitest failures locally, and found out that
they're caused by a non-essential part of my patch (from
nsChildView::SetFocus()):

+    if ([mView isEqual:firstResponder]) {
...
+      if ([mView isKindOfClass:[ChildView class]]) {
+        [(ChildView *)mView sendFocusEvent:NS_LOSTFOCUS];
+        [(ChildView *)mView sendFocusEvent:NS_GOTFOCUS];
+      }
+    } else {

Non-essential, but still difficult (and necessary) to replace.  So I'm
re-opening this bug.  My guess is that coming up with an alternate way
of avoiding multiple text-input cursors will take 2-3 days.

Comment 30

[Steven Michaud [:smichaud] (Retired)]

Attachment: Fix rev3 (fix Mochitest problem, simplify)

This patch is work in progress.

It fixes the Mochitest problem without re-introducing the
multiple-text-input-cursors problem.  But it does reintroduce a small
focus problem (in fact a problem with this bug), so I want to keep
banging on it for a few more days, to see if I can do any better.

But I've run out of time before the Christmas vacation, so this will
have to wait until January, when I get back.

The problem that it re-introduces will (presumably) not be encountered
very often:

1) In a new browser window that has text-input fields
   (e.g. http://www.mozilla.org/projects/minefield/), tab until the
   text-input cursor reaches one of the in-page text-input fields (not
   the location bar or the Google search box).

2) Command-tab to open a new tab.

3) Use the mouse to navigate back to the previous tab: The text-input
   cursor won't be visible (though if you press the tab key it will
   become visible in the next text-input field after than).

This only happens once per window -- a repetition of these steps in
the same window won't cause any trouble.  It doesn't happen, even in a
new window, if you first click the mouse in one of the in-page
text-input fields.

I hope to leverage all this, once I get back, to find a fix.

By the way, roc, you'll notice that I no longer try to find out
whether [ChildView becomeFirstResponder:] or [ChildView
resignFirstResponder:] is called in response to a call to
nsChildView::SetFocus().  In retrospect the solution is obvious --
only send intra-window NS_GOTFOCUS or NS_LOSTFOCUS events from inside
SetFocus().  It just took me a while to see it :-)

Comment 31

[Steven Michaud [:smichaud] (Retired)]

> 2) Command-tab to open a new tab.

Command-t to open a new tab.

(Sorry.)

Comment 32

[Steven Michaud [:smichaud] (Retired)]

Here's a tryserver build made with my current (rev3) patch (attachment
294230 [review]):

https://build.mozilla.org/tryserver-builds/2007-12-21_08:[e-mail]-bugzilla403232-rev3/[e-mail]

Comment 33

[Steven Michaud [:smichaud] (Retired)]

Attachment: Fix rev4 (simplify further)

Here's a slightly revised (and pared down) version of my patch's
previous revision (rev3).  I've tested it in Minefield and Camino, on
OS X 10.4.11 and 10.5.1.  As far as I can tell it doesn't have any
problems.

This time I ran all the tests I could find at
http://developer.mozilla.org/en/docs/Mozilla_automated_testing.  I had
no problems with the Mochitests, or with any other set of tests but
the reftests.  The reftests had eight "unexpected failures" ... but
exactly the same eight "unexpected failures" happened in an unaltered
build (without my patch).  So I assume there was something wrong with
these eight reftests. and that none of the tests that run on build
will fail.

The problems I reported in comment #30 turned out to be ... well,
spurious :-)  Even with my rev3 patch, they only happened in one
particular set of (contrived) circumstances.  And even in those
circumstances, what I thought was a problem turns out not to be one.

Here's a tryserver build made with this patch:

https://build.mozilla.org/tryserver-builds/2008-01-10_09:[e-mail]-bugzilla403232-rev4/[e-mail]

Comment 34

[Steven Michaud [:smichaud] (Retired)]

Attachment: Fix rev5 (fix dumb mistake)

Josh found a dumb mistake in rev4 (attachment 296578 [details] [diff] [review]), so here's a new
revision that fixes it.  (You shouldn't make mInSetFocus false at the
top of nsChildView::SetFocus(), but only when this method returns (at
its bottom).  Otherwise you'll only prevent one level of re-entrancy.)

I redid my tests and found no problems, or differences in behavior.
This must mean that you rarely (if ever) get two or more levels of
re-entrancy.

I still see what I think are the same reftests failures (this time I
count ten of them, but I think I miscounted the first time).  So I
suppose these tests are still (somehow) broken.

Here's a tryserver build made with this patch:

https://build.mozilla.org/tryserver-builds/2008-01-14_11:[e-mail]-bugzilla403232-rev5/[e-mail]

Comment 35

[Steven Michaud [:smichaud] (Retired)]

Attachment: What I actually landed

Landed on trunk.

I had to change the patch a little to accomodate Mats Palmgren's
recent patch for bug 402505.  Here's what I actually landed.

Comment 36

[Steven Michaud [:smichaud] (Retired)]

This bug (bug 403232), like bug 404433 and bug 357535, was regressed
by my patch for bug 413882.

I have a new patch that fixes all these bugs, without regressing bug
413882.  I'm about to post a preliminary version at bug 404433, along
with a tryserver build made with that patch.

Comment 37

[Steven Michaud [:smichaud] (Retired)]

I just landed a patch for bug 404433 that also fixes this bug.

Comment 38

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Verified with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9b4) Gecko/2008030317 Firefox/3.0b4 ID:2008030317