141295 - focus in content area is erratic (timing?)

Status: RESOLVED FIXED

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

Description

[sairuh (rarely reading bugmail)]

this a spinoff from bug 37638. basically, the problem is that focus in the
content area in those cases is erratic. :-/

here are the main cases where focus should initially be in the content area of a
browser window. there are others, but bug 37638 primarily fixes these cases in
particular.

(a) upon startup of the application, when the browser is the startup app (which
it is by default).

(b) when opening a new browser window via either accel+N (shortcut) or File >
New > Navigator window (toplevel menu).

i have seen this in both branch (eg, 2002.04.30.0x-1.0.0) and trunk (eg,
2002.04.30.0x) commercial builds, all platforms.

when i test this, i typically have my home page set to http://mozilla.org/
(namely to avoid the interfering popups generated from
http://home.netscape.com). sometimes (a) occurs, but (b) is fine --or vice
versa, or even both.

perhaps this is a timing issue --in any case, it blocks my reliably verifying
bug 37638.

Comment 1

[sairuh (rarely reading bugmail)]

saari, bryner et al., any ideas why this might be occurring?

nearly forgot to add:

* expected behavior for both (a) and (b):
when focus is successfully in the content area, i should be able to hit space,
pageup, pagedown, arrow keys, etc to scroll. shouldn't need to click or hit tab.

* actual behavior:
hitting pace, pageup, pagedown, arrow keys, etc to scroll does nothing. not sure
where focus is. workaround is to hit tab twice (1st->urlbar, 2nd->content area)
to move focus to the content area (or, clicking there w/mouse).

Comment 2

[sairuh (rarely reading bugmail)]

amendment to comment 1, part (b): sometimes hitting the tab key does nothing.
focus is just lost, in which case i must click with the mouse to bring focus to
the content area (or urlbar, whichever i prefer at the time).

Comment 3

[Aaron Leventhal]

I think this is messing with type ahead find (bug 30088), at least on Linux
platforms.

Comment 4

[Akkana Peck]

I see this fairly often on small, simple pages -- it's not clear where focus
ends up (it's not in the urlbar or in the content, and keys like page up/down do
nothing -- but on more elaborate pages, focus usually ends up in the body as
expected.  It's very reproducible for me on linux, and doesn't depend on whether
I use pointer or click focus.

Comment 5

[Samir Gehani]

Nav triage team: nsbeta1+/adt3

Comment 6

[Brian Ryner (not reading)]

Attachment: patch

This fixes the problem for me on Linux and Mac (I was unsable to reproduce it
on Windows).  The problem is that windowwatcher's active window may be out of
sync with the active focus controller, because windowwatcher marks a window as
being active right when SetVisibility(true) is called on the nsXULWindow.  So,
if navigator.js's timer callback happens between the call to SetVisibility and
the actual NS_ACTIVATE event for the window, it will try to focus the page
content and we'll get confused (because the widget isn't actually visible yet,
I think).

Comment 7

[saari (gone)]

The fix sounds correct and should help eliminate the problem. I'd prefer if danm
reviewed the window watcher changes, but r=saari for the focus controller
changes and the conceptual fix.

Comment 8

[Dan M]

Comment on attachment 93943 [details] [diff] [review]
patch

r=danm. Note (just because I had to double-check this) that this patch removes
code in nsWindowMediator which resets WindowWatcher's activeWindow when a
window is destroyed. This is OK because there is already code which clears
WindowWatcher's active window if the destroyed window was that window. (And
this patch adds a (more accurate) notification mechanism for window activation,
which should automatically spring in soon after.)

Comment 9

[Alec Flett]

Comment on attachment 93943 [details] [diff] [review]
patch

> nsFocusController::SetActive(PRBool aActive)
> {
>   mActive = aActive;
>+
>+  // Inform the window watcher of the new active window.
>+  nsCOMPtr<nsIWindowWatcher> wwatch = do_GetService("@mozilla.org/embedcomp/window-watcher;1");
>+  if (wwatch && mCurrentWindow) {

wouldn't it make more sense to check mCurrentWindow first, before spending the
time to retrieve the WindowWatcher service? i.e.

>+  if (!mCurrentWindow) return NS_OK;
>+
>+  // Inform the window watcher of the new active window.
>+  nsCOMPtr<nsIWindowWatcher> wwatch = do_GetService("@mozilla.org/embedcomp/window-watcher;1");
>+  if (wwatch) {

(and in fact I find the code a whole lot more readable if you don't put the
entire purpose of the function inside a giant if() - how about

>+ if (!wwatch) return NS_OK;

Comment 10

[Brian Ryner (not reading)]

Attachment: patch v2

Cleaned up according to alecf's suggestions.  I also just eliminated the null
check for the QI to nsIDocShellTreeItem - we have exactly one implementation of
nsIDocShell, and it does implement nsIDocShellTreeItem.

Comment 11

[Aaron Leventhal]

Believe it or not, this patch changes the order of
nsIWebProgressListener::OnStateChange and sgo->SetNewDocument calls.
It used to be that I would always get at least 1 OnStateChange() after the last
SetNewDocument(), but for the first document in a new window, this is no longer
true. This messes me up because now my dom listeners are gone via
SetNewDocument's RemoveAllListeners() call, and I have no hook to put them back

Comment 12

[Aaron Leventhal]

This patch is definitely regressing what is focused on startup, on my windows box.

It used to be that the nsGlobalWindowImpl::SetDocument() was called twice on
startup, once for the chrome window, and then once for the content window.

With this patch it's called only once, on the chrome window.

What really confuses me is that space bar, pageup/dn, etc. still work, even
though the chrome window is focused.

Comment 13

[Brian Ryner (not reading)]

Aaron, I'm confusd about what you've said.  SetDocument() isn't directly related
to focus.  What prompts you to say that the chrome window is focused?

Comment 14

[Aaron Leventhal]

Sorry, I meant nsGlobalWindow::Focus()

Comment 15

[Aaron Leventhal]

Attachment: Simple test case - dumps "window focused" to console when it's focused. Used to work, before patch is applied.

Brian, the patch in attachment 94564 [details] [diff] [review] prevents the a dom focus event from being
fired on the content window.

Comment 16

[Aaron Leventhal]

Attachment: Simple test case - dumps "window focused" to console when it's focused. Used to work, before patch is applied.

Brian, the patch in attachment 94564 [details] [diff] [review] prevents the a dom focus event from being
fired on the content window.

Comment 17

[Brian Ryner (not reading)]

Attachment: patch v3

This should fix the problem that aaronl found.	What was happening is that the
focus controller was made active before it had a focused window set, and there
was no update of the window watcher then when the focused window was set.  This
caused the code in navigator.js to call setFocusedWindow on the command
dispatcher instead of just calling .focus() on the window as it should have
(since the window was indeed active, the window watcher just didn't think it
was).

This patch should not be checked in until bug 163918 is fixed, as it aggravates
the problem and will cause a crash on startup if you use the profile manager
(the window is never unregistered with the window watcher so it has a dangling
pointer).

Comment 18

[Greg K.]

Re: comment 1, the content pane should not receive focus when the URL is
[about:blank]. In that case, the URL Bar should get focus.

Also, the content pane should receive focus (and presently doesn't) when loading
a URL sent by MailNews. (Should I file a separate bug about that?)

Comment 19

[Greg K.]

To follow up on comment 18, paragraph 1, that is to say any "blank" browser
window should put focus on the URL Bar and not the content pane.

Comment 20

[Brian Ryner (not reading)]

In testing this, I found one other situation where initial focus is wacky that
this patch doesn't address.  This happens if navigator.js focuses "_content"
after the old document (about:blank) has had its reference to the DOMWindow
released, but before the paint suppression timer fires.  The EventStateManager
will be unable to get the DOMWindow from the document and therefore won't give
it the DOM focus event, which means the focus controller isn't notified of the
new focused window.  So, when the paint suppression timer fires, it doesn't
think the DOMWindow is focused and doesn't focus the new document's widget.

I have a patch which fixes this by using the "new" container property of the
document to get at the DOMWindow through the DocShell even if the document has
released its ScriptGlobalObject reference.  I'll post it here after I test it on
Mac and Linux.

Comment 21

[Brian Ryner (not reading)]

Attachment: patch v4

Ok, here is the pacth I mentioned in the previous comment.

This actually has the potential to affect a number of things so it needs some
testing.  Particularly:

- initial focus (new window)
- focus memory (switch to another window and switch back)
- windows should not jump to the front when a page finishes loading

I was able to entirely get rid of the focus controller member variable from
nsDocument/nsXULDocument and the GetFocusController method on nsIDocument.  I
also have some miscellaneous cleanup here of nsEventStateManager and
nsIEventStateManager, where some methods were virtual for no reason.

Comment 22

[Johnny Stenback (:jst)]

Comment on attachment 97667 [details] [diff] [review]
patch v4

+    if (!document) break;

There's lots of these towards the end of the diff. Maybe it's just me, but I
really dislike one-line if-statements, please put the break on it's own line,
it helps when debugging (setting breakpoints), if nothing else.

Other than that, sr=jst

Comment 23

[Brian Ryner (not reading)]

Sure.  I was mainly just changing the API in that section of code to not use
nsIDocument::GetFocusController(), but it looked pretty messy as far as if
blocks/indenting so I tried to clean it up.

Comment 24

[sairuh (rarely reading bugmail)]

tested focus and keyboard scrolling with builds from bryner (9/3) on linux
rh7.2, win2k and mac os x 10.1.5. looks good!

Comment 25

[Dan M]

Comment on attachment 97667 [details] [diff] [review]
patch v4

r=danm on the new, bigger! patch.

Comment 26

[Asa Dotzler [:asa]]

Comment on attachment 97667 [details] [diff] [review]
patch v4

a=asa (on behalf of drivers) for checkin to 1.1

Comment 27

[Asa Dotzler [:asa]]

um, I mean 1.2a. 

Comment 28

[Brian Ryner (not reading)]

fix checked in.

Comment 29

[WD]

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

Comment 30

[Greg K.]

The cases I mentioned in comment 18 now seem to work properly. I noticed there's
still one that doesn't, though. A mail message opened in its' own window doesn't
focus the content pane. New bug?