302962 - When creating a new tab in Camino, gecko steals focus from the url bar (early activate/deactivate fails)

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect, P1)

Description

[Simon Fraser [no longer active]]

When you make a new (empty) tab in recent camino builds, we focus the URL bar
explicitly, but the content area the steals focus from it, so you're left having
to refocus the url bar. This changed recently (I'm not sure why).

Some debugging shows that when we focus the URL bar, we are correctly
deactivating the content (via nsIWebBrowserFocus::Deactivate()), but this
happens early in loading, before we have a pres shell. Because of this,
nsWebBrowser::Deactivate() drops the deactivate on the floor.

I think this is wrong. If nsWebBrowser can get at a focus controller, it should
always pass on the Activate/Deactivate calls, even if we don't have a pres shell
yet. Currently nsGlobalWindow::Activate() NS_WARNS in that case, but it seems
legitimate to remove that.

Comment 1

[Simon Fraser [no longer active]]

Attachment: Patch

This patch simply removes the checks for non-null pres shell from the
nsWebBrowser Activate/Deactivate calls, and removes the NS_WARNING from
nsGlobalWindow if Activate() is called when the pres shell is null.

Comment 2

[Simon Fraser [no longer active]]

A regression window for this would be useful.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 191213 [details] [diff] [review]
Patch

So... I'm not really qualified to review this; I have no real idea what any of
our focus code does.  That said, the nsGlobalWindow change is basically a no-op
(see http://lxr.mozilla.org/seamonkey/source/xpcom/glue/nsDebug.h#225).

Comment 4

[Boris Zbarsky [:bzbarsky]]

One question I do have -- why was the check we're removing there in the first place?

Comment 5

[Simon Fraser [no longer active]]

The check was there to avoid an assertion in nsGlobalWindow::Activate (as noted
by the comment), which was then changed to a warning, which I here remove.

Comment 6

[Brian Ryner (not reading)]

We already force the docshell to create an about:blank content viewer when you
ask for random things like the document, or call GetInterface with certain IIDs.
 I think we should instead make this more consistent so that it calls
EnsureContentViewer from GetPresContext, which will make sure that the pres
shell is non-null.

Comment 7

[Simon Fraser [no longer active]]

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

Comment 8

[Simon Fraser [no longer active]]

Attachment: Patch to nsDochShell

This patch changes nsDocShell::GetPresContext() to call EnsureContentViewer(),
as suggested by bryner.

This patch is independent of the previous one, which I think is still valid.

Comment 9

[Simon Fraser [no longer active]]

Attachment: Revised nsDocShell patch

This patch doesn't spit out assertions if GetPresContext() is called when the
docshell is being destroyed.

Comment 10

[Simon Fraser [no longer active]]

Comment on attachment 191662 [details] [diff] [review]
Revised nsDocShell patch

This fixes a serious focus issue in Camino.

Comment 11

[Benjamin Smedberg]

Comment on attachment 191213 [details] [diff] [review]
Patch

Was this patch replaced by the other one, or do we need this one as well?

Comment 12

[Simon Fraser [no longer active]]

Fixed.

Since the fix is in core, I'm going to move this bug to Core in case people want
to search for it.

Comment 13

[Christian :Biesinger (don't email me, ping me on IRC)]

is this related to bug 210373?

Comment 14

[Christian Persch (GNOME) (away; not receiving bug mail)]

(In reply to comment #13)
> is this related to bug 210373?

GtkMozEmbed doesn't use nsIWebBrowserFocus, and instead sets the focus
controller's active state directly, and [de]activates the nsIPIDOMWindow. See
http://lxr.mozilla.org/seamonkey/source/embedding/browser/gtk/src/EmbedPrivate.cpp#731
ToplevelFocus[In|Out] and ChildFocus[In|Out]. So it's probably not related. Bug
226708 discusses the origins of the problems with gtkmozembed focus.

Comment 15

[Simon Fraser [no longer active]]

It looks like the docShell changes caused a slight increase in pageload time on
btek (930 -> 940ms).

Bryner, do you still think that eagerly doing EnsureContentViewer is the right
thing?

Comment 16

[jag (Peter Annema)]

It also increased Txul and Ts, see e.g. comet, monkey or luna.

Comment 17

[Simon Fraser [no longer active]]

Backing out the docShell change brought the numbers back down (and isn't
actually required to fix the bug). Should we leave it out?

Comment 18

[Robert Kaiser]

This also increased leak numers on brad, and the backout brought them back to
where they were before...

Comment 19

[Simon Fraser [no longer active]]

I backed out the nsDocShell.cpp patch above The bug remains fixed, even without
this change.