103197 - Focus/Activate event and InitialReflow() race condition

Status: RESOLVED WORKSFORME

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

Description

[kinmoz]

People have been filing bugs regarding the fact that they are sometimes
unable to type into or give focus to text widgets (textfields, textareas, etc)
in forms or in XUL dialogs.

I've never been able to reproduce the problem in form textwidgets, but I
do see the problem on WindowsNT when bringing up the prefs dialog a 2nd time.

To reproduce, the pref dialog must start with a pref panel that contains
a text widget. (For example the Navigator or Mail & News panels) The very
first time you bring up the pref dialog you will be able to click in
the text widget on the pref panel, to place the caret, and then type something.

If you close the pref dialog, and bring it up again. You'll notice that
clicking in the text widget does not place the caret, nor are you allowed
to edit it.

There seems to be a race condition between the InitialReflow() of the
contents of the pref's XUL window and the processing of the XUL window's
native activate and focus events.

Things work fine when you bring up the dialog the first time because
the XUL code is not retrieved from the XUL cache ... which allows the
InitialReflow of the dialog contents to happen before the focus/activate
events are processed.

Every time you bring up the pref dialog after that, the XUL code is
retrieved from the XUL cache which for some reason, produces enough
of a delay that allows the window's native activate and focus events
to be processed at a time when there is no frame data that exists,
so the events are basically dropped/ignored.

The events are dropped/ignored because the viewPort frame is not
created and associated with the rootView for the window, leaving
it's mClientData == null, until after InitialReflow() is called,
so nsView::HandleEvent() does not dispatch the synthesized NS_ACTIVATE
event neccessary to activate the focusManager.

After the dialog comes up, clicking in the text widgets doesn't bring
them into focus or place the caret because there is some code, added
by Hyatt:

    nsHTMLInputElement.cpp revision 1.165
    r=saari, sr=brendan

in nsHTMLInputElement::SetFocus() which bails early if the focusManager is not
active, so the focus notifications neccessary to "wake up" the editor and place
focus in the text widget are never fired.

I did verify that skipping over this piece of code gets things working.

I guess I'm filing this bug to address the race condition, and to figure
out from Hyatt and others why his fix was necessary. Comments?

Comment 1

[kinmoz]

Adding nsbranch keyword as per conversation with kmcclusk.

Comment 2

[David Hyatt]

My fix is necessary in order to avoid focus stealing.  If you allowed windows to
come to the front when focus happens on text fields, then you can have one
window "jump" to the front, stealing focus from the other.  (The classic Google
example of kicking off Google's load, switching to another window, and then
having the Google window leap back to the front again when it finishes loading.)

Comment 3

[saari (gone)]

This falls into a more general category of bug, IMO, where we need an event
queue that is ready to accept events before the window is ready to process them
to avoid things falling on the floor. Tom, do you agree with that?

Comment 4

[Ken Foskey]

This occurred to me on Google (Moz 0.9.6) and always occurs in Open Office bug
(moz 0.9.5) tracker.   I just lost a heap of memory so I was down to 64 meg and
a very stressed machine when I upgraded.   When I use Office bugtracker
(bugzilla) I typically am doing builds etc in the background and therefore
stressing the machine a bit.

LInux Debian Woody  Kernel 2.4 (not premptive),was 64Meg  Pentium 350 (now 256)

Comment 5

[saari (gone)]

I can't reproduce this on my win2k P3 500 anymore, but there probably is a
system out there that can reproduce this

Comment 6

[joki (gone)]

*** Bug 121917 has been marked as a duplicate of this bug. ***

Comment 7

[Charles Manske]

*** Bug 125111 has been marked as a duplicate of this bug. ***

Comment 8

[Charles Manske]

Notes from bug 125111. 
You can get this to happen with Composer:
1. Launch Composer
2. Click in Image toolbar button. You should be able to place caret and type in
the Location inputfield.
3. Close dialog, then immediately bring it up again. 
Now you can't place caret and type in location field.

Note that user can "fix" it by switching focus to another window, then returning,
or by minimizing, then restoring the window.
This is bad and should be fixed sooneer than 1.2, imho.
It must be a regression - I never saw this behavior before 2/23/02

Comment 9

[Kathleen :Brade]

Is there some workaround that could be done without developing the whole event
queue?  Could we force focus to the text widget when a window is opened or ???
We need something done for mozilla 1.0.

Comment 10

[saari (gone)]

The fix in nsHTMLInputElement::SetFocus()
 is necessary to keep pages, like google, from poping their window forward when
it finshes loading in a background window.

Now, what I don't understand is why this shows up when things are in the XUL
cache, but not on the first load. Wouldn't the frame model for the window be up
and running *faster* with the XUL cache and thus ready to take events? This
seems backwards to me given Kin's analysis.

Comment 11

[saari (gone)]

To be clear

"Things work fine when you bring up the dialog the first time because
the XUL code is not retrieved from the XUL cache ... which allows the
InitialReflow of the dialog contents to happen before the focus/activate
events are processed."

So retrieving from the XUL cache allows the InitialReflow of the dialog contents
to happen before the focus/activate events are processed. And that causes the
problem? 

Comment 12

[kinmoz]

No, when we retrieve from the XUL cache, the InitialReflow() happens *after* 
focus/activate events have been dispatched and processed.

My guess (and it's just a guess) is that the act of parsing the non-cached 
XUL triggers some content sink callbacks that create some frames and fires off 
an InitialReflow() on them before the window actually shows up on the screen.

I'm not sure if we parse cached XUL or if it's already in some sort of binary 
format that doesn't got through the parser/content sink mechanism.

Comment 13

[Dan M]

 Oy. While the prospect of potentially dozens of important messages going
unanswered is sobering, I argue that the demonstrated fault of a single dropped
message doesn't warrant an entire deferred event mechanism. Seems like posting
an NS_ACTIVATE ourselves at some opportune time might be easier and more
appropriate.
  But actually I don't think either solution would help. (On my build, the input
element is unfocused even the first time. So for the duration of this comment
I'm going to pretend the XUL cache issue isn't...) The focus that goes largely
ignored; the one affected by Dave's patch to nsHTMLInputElement, isn't a
response to any Windows message. It comes from script running in an onload
handler. And the window isn't made visible until its chrome is loaded -- we
become aware of that in another onload handler.
  The focus is generated (eventually) in nsDocLoaderImpl::doStopDocumentLoad
when it calls FireOnStateChange(... STATE_IS_DOCUMENT). The visibility (and
activation) is generated (eventually) in nsDocLoaderImpl::doStopDocumentLoad
when it calls FireOnStateChange(... STATE_IS_WINDOW), which it does immediately
following.
  I don't see how saving up OS events would have any effect here. Anybody have
any idea why the focus controller isn't giving focus to the input element when
its window becomes active, as it should? The window I saw getting the activate
events was consulting a different RootFocusController than the one used by the
nsHTMLInputElement. That's as far as I've gotten here.

Comment 14

[saari (gone)]

For the dialog that is showing the problem, put the onload handler's focus
method into a JS timeout. So timeout of 0 before we focus. I think that hack
might work.

Comment 15

[Dan M]

Yes, this hacks/fixes the image dialog in my build:

Index: editor/ui/dialogs/content/EdImageProps.js
===================================================================
RCS file: /cvsroot/mozilla/editor/ui/dialogs/content/EdImageProps.js,v
retrieving revision 1.96
diff -u -r1.96 EdImageProps.js
--- editor/ui/dialogs/content/EdImageProps.js	14 Feb 2002 02:13:28 -0000	1.96
+++ editor/ui/dialogs/content/EdImageProps.js	14 Feb 2002 22:19:03 -0000
@@ -109,7 +109,7 @@
     gDialog.widthUnitsMenulist.selectedIndex == 0 &&
     gDialog.heightUnitsMenulist.selectedIndex == 0;
 
-  SetTextboxFocus(gDialog.srcInput);
+  setTimeout("SetTextboxFocus(gDialog.srcInput)", 0);
 
   SetWindowLocation();
 }

A correction or clarification to comment 13 that I noticed after printf
debugging; the only way to be sure for focus/activate problems: The sequence
goes like this: The text field gets focus, which becomes just a notation in the
focus controller because the window isn't active. Later, Windows sends the
expected WM_ACTIVATE/WM_FOCUS messages to the image dialog window, but the
WebShellWindow is never notified. It's within the WSW's NS_GOTFOCUS handler that
the focus controller is used to actually focus the DOM window, so while at this
time a perfectly good focus controller is totally aware that the input field
wants focus, no one ever asks his opinion and the text field remains fuzzy.

Comment 16

[Brendan Eich [:brendan]]

kin, the XUL cache holds prototype XUL elements -- no parsing required after
cache fill (first load of a XUL window of a given type).  This is "brutal
sharing", it avoids reparsing and recompiling XUL, JS, CSS, and XBL.

danm, thanks for the diagnosis -- do use the faster form of setTimeout:

-  SetTextboxFocus(gDialog.srcInput);
+  setTimeout(SetTextboxFocus, 0, gDialog.srcInput);

/be

Comment 17

[Brendan Eich [:brendan]]

danm, can you take this bug and target at 0.9.9?  We want the workaround with an
ugly XXX comment citing this bug, if not a better fix, for 1.0.  Thanks,

/be

Comment 18

[Dan M]

I know I'll regret this...

Comment 19

[saari (gone)]

r=saari for the tried and true hack

Comment 20

[Brendan Eich [:brendan]]

I can't see it, but my third eye says danm is using the righteous funarg form of
setTimeout, and commenting with an XXX comment containing a big ugly citation of
this bug, so sr=[e-mail].

/be

Comment 21

[Dan M]

I could never refuse something named funarg. (Improved) patch is in.

Comment 22

[Charles Manske]

HEY! Can you say "module owner" ? 
This problem may crop up in other dialogs. 
If you look at SetTexboxFocus, you'll see that it is called by every dialog!
(good factoring). So the fix is to put the timeout in that method,
not the image dialog!

Comment 23

[Charles Manske]

Attachment: Much better fix

Comment 24

[Charles Manske]

reviews please

Comment 25

[Charles Manske]

Oh yea, i'll back out the checked in fix as part of this :)

Comment 26

[Charles Manske]

And I changed the comment to:
"//XXX Using the setTimeout is hacky workaround for bug 103197"

Comment 27

[Dan M]

Sorry; we should have consulted you first.

I didn't want to put the timeout where you do because it's a hack and it hadn't
been demonstrated that there was a problem with the other dialogs. And because
in that place, it'll affect every call to set the focus, not just the ones done
in the onload handler. If you think the potential repercussions are acceptable,
I'll just duck out of here.

r=me though Brendan will want you to use the funarg version of setTimeout. And
I'm curious why the non-zero timeout.

Comment 28

[Brendan Eich [:brendan]]

Comment on attachment 69596 [details] [diff] [review]
Much better fix

What I and danm said: cite the bug #, use the funarg form of setTimeout, and
why can't you use a delay of 0 ms?  There should be no reason to use 1 (I
hope!).

/be

Comment 29

[Brendan Eich [:brendan]]

Oh sorry -- cmanske said he changed the comment to cite the bug number.  All I
want is the final patch with funarg, 0-delay setTimeout to sr=, though -- pretty
please?

/be

Comment 30

[Kathleen :Brade]

Why was the WRONG fix checked in???  Why does the comment for this checkin say
that *I* reviewed it instead of danm/brendan?

Comment 31

[kinmoz]

So, I'm just wondering why this "generic" bug was changed to be about Composer's 
dialogs (I noticed the summary of the bug changed) ... did someone fix this 
problem for *all* the other windows/dialogs/panels in the product?

Is the real problem going to be addressed at some point?

Comment 32

[Kathleen :Brade]

The Composer-specific fix is covered in bug #125346.
This bug should be to cover the issue for the whole product.
Reassign back to danm; back to general summary.

Comment 33

[Kathleen :Brade]

really reassign

Comment 34

[saari (gone)]

kin sez "Is the real problem going to be addressed at some point?" 

To which my response is "I hope so, but not when there is a known work around
and there are other serious bugs without a known work around."

Sorry, I know it isn't a good answer, but it is the current reality.

Comment 35

[Charles Manske]

This problem is cropping up in other dialogs now, and I bet not only Composer
dialogs, as the blocker list attests! 

Comment 36

[saari (gone)]

That doesn't change my answer, unless damn, bryner, hyatt or another suspect
wants to take it on. I'm too doomed to consider it considering there is a work
around.

Comment 37

[Ere Maijala (slow)]

Charles: Yes, this is causing a lot trouble (125060, 125065 being the ones with
highest visibility IMO). If I were asked, this should be fixed in 0.9.9.

Comment 38

[Bill Law]

Note that we've come up with a similar workaround in the <dialog> xbl binding 
code; see bug 126850.

That fixes instances using <dialog>.  It doesn't do anything for <window>s 
(e.g., the Address Book New Card dialog, the new account wizard).

Comment 39

[[e-mail]]

Is this problem affecting radio buttons as well?

Comment 40

[Ere Maijala (slow)]

Neil: yes.

Comment 41

[Peter Trudelle]

nsbeta1- per ADT triage team, due to workaround (Use the hack, Luke).  ->1.2

Comment 42

[Aleksey Nogin]

Another issue that might be caused by this is that sometimes the URL bar will
stay empty (with only the icon in it) wherever I go and will only react to
pressing "Enter" in it by reloading the page, ignoring what I have typed in it.
This only ever happens with the first browser window that is opened and seems to
depend on the order things happen on startup. When  I see Mail Window -> Master
Password Window -> Browser Window, then things are OK, but when Browser Window
pops up before the Master Password Window, the URL bar would usually fail to work. 

BuilID 2002033118 on RedHat Linux 7.2

Comment 43

[Randell Jesup [:jesup] (needinfo me)]

This is an "Make RC1 Not Suck" bug.  Any progress?  It also blocks another RC1
not Suck bug (bug 130581)

Comment 44

[Aleksey Nogin]

Is bug 158030 related?

Comment 45

[Jesse Ruderman]

I ran into a similar problem while fixing bug 68891, "mail search window should
focus textbox automatically".  Focusing the textbox right away, or even on a 0ms
timeout, did not work.  My solution was to change the xul textbox constructor to
notice that the textbox has focus and move focus to the inner box.  I don't know
whether my change fixes this bug or whether a fix for this bug would make my
change unnecessary.

Comment 46

[Charles Manske]

Bug 133121 in composer looks like a similar problem.
The caret isn't turned on when you start the image dialog on an existing image
(so there's a URL in the location input field.)
I tried Jesse's patch to textbox.xul (new constructor code), but it didn't help.
 

Comment 47

[Benjamin Smedberg]

*** Bug 138797 has been marked as a duplicate of this bug. ***

Comment 48

[Juan José Mata]

Changing target milestone to 'Future' since these seem to have missed the 1.2
alpha.  If you are still considering a fix for 1.2 please re-target accordingly.

Comment 49

[[e-mail]]

Hmm... I can't duplicate this bug under Linux any more...
(after removing the setTimeout from dialog.xml)

Comment 50

[Ere Maijala (slow)]

To comment #42: this is probably caused by bug 90337.

Comment 51

[Torben]

*** Bug 136903 has been marked as a duplicate of this bug. ***

Comment 52

[Torben]

*** Bug 140082 has been marked as a duplicate of this bug. ***

Comment 53

[Serge Gautherie (:sgautherie)]

Acting on Duplicate resolution from bug 140082 comment 3:

*Moving 'Blocks: bug 140346' from bug 140082 to bug 103197.
 (Take further action as needed.)

Comment 54

[Wayne Mery (:wsmwk)]

bug 130581 was resolved without this bug being fixed, so removing this as blocker.

is the premise of this bug still valid?

Comment 55

[Emilio Cobos Álvarez (:emilio)]

I think this is working.