526247 - Focus handling doesn't work on TestGtkEmbed

Status: RESOLVED FIXED

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

Description

[Olli Pettay [:smaug][[e-mail]]]

I think focus re-write broke focus handling on embedding.
nsWebBrowser.cpp should be able to get NS_(DE)ACTIVATE to set the focus
correctly, but it doesn't do that, since nowadays those events are dispatched
only to top level widget.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

The patch for Bug 522533 fixes this for Gtk2, but we have to get this fixed also
for other platforms.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

But I want to actually fix this focus issue separately from Bug 522533.

Comment 3

[Neil Deakin]

What problem should I be seeing?

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Attachment: split from bug 522533

This was reviewed in bug 522533, but Neil, could you look at this too.

*At least* windows needs similar fix.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #3)
> What problem should I be seeing?

If you run TestGtkEmbed and load http://www.google.com to it and then try to
click the search form control, it doesn't get the focus.

Comment 6

[Neil Deakin]

Comment on attachment 409973 [details] [diff] [review]
split from bug 522533

>+  case NS_ACTIVATE: {
...
>+    nsCOMPtr<nsIDOMWindow> window = do_GetInterface(browser->mDocShell);
>+    nsCOMPtr<nsIFocusManager> fm = do_GetService(FOCUSMANAGER_CONTRACTID);
>+    if (fm && window) {
>+      fm->WindowRaised(window);
>+    }
>+
>+    break;

You could simplify this a bit by just calling browser->Activate() or browser->Deactivate();

Comment 7

[Karl Tomlinson (:karlt)]

Comment on attachment 409973 [details] [diff] [review]
split from bug 522533

When I reviewed this for electrolysis I suggested just skipping the
!mIsToplevel check in Dispatch(Activate|Deactivate)Event() so that
nsWidgetInitData::mTopLevelContentWidget and nsWindow::mIsTopLevelContent
would not be needed.

I'm kind of OK if this is landed as is for now on electrolysis because we
don't understand all the issues and it is holding up other things.

But for m-c I'd like to understand this better.

My understanding was that the only reason we did the mIsToplevel check was
that embedding handled activation itself.  If embedding still needs the
activation event, then there seems no need for the !mIsToplevel check.

This is only dispatched for the container window.  That is the window at the
top of the Gecko window hierarchy. i.e. for the nsWebShellWindow or for the
nsWebBrowser.

Smaug pointed out in bug 522533 comment 17, that NS_(DE)ACTIVATE are used by
nsObjectFrame.  This happens in nsObjectFrame::ProcessEvent() if there is a
.nativeMsg on the event, which there wouldn't be in this case.

Also, I don't follow how nsObjectFrame would ever get the NS_(DE)ACTIVATE, so
the code seems currently unused, because NS_(DE)ACTIVATE are currently only
dispatched to toplevels.

There is also the question of whether we should
DispatchActivateEventAccessible() for embedding.  We used to do that sometimes
but not other times before the focus rewrite, so I'm not too concerned about
getting that just right now.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Yeah, I could do still some more testing.

And there is at least one bug. The patch handles only content nsWebBrowser,
not chrome.
chrome nsWebBrowser may have content sub-docshells, and those have their own
widget, in which case we'd dispatch  NS_(DE)ACTIVATE in a bit different way
if mIsToplevel is removed.  I'll test that.

Comment 9

[Karl Tomlinson (:karlt)]

(In reply to comment #8)
> chrome nsWebBrowser may have content sub-docshells, and those have their own
> widget, in which case we'd dispatch  NS_(DE)ACTIVATE in a bit different way
> if mIsToplevel is removed.

They have their own widget, but they shouldn't be container widgets, so the widget for the ancestor nsWebBrowser should still dispatch the event (AFAIK).

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Attachment: v2

Karl, I think you're right.
I don't want to change nsObjectFrame here.

Comment 11

[Neil Deakin]

Comment on attachment 410240 [details] [diff] [review]
v2

Assuming that activate/deactivate will only be called for toplevel windows, this is ok.

What about the call in OnButtonPressEvent? Does that only get called on toplevel windows as well?

Comment 12

[Karl Tomlinson (:karlt)]

(In reply to comment #11)
> What about the call in OnButtonPressEvent? Does that only get called on
> toplevel windows as well?

It only gets called for the top Gecko window (which is not the toplevel native window when embedding).

Comment 13

[Karl Tomlinson (:karlt)]

I filed bug 526633 on nsObjectFrame.

I have to admit I don't understand the cocoa widget code well, but it sometimes sends NS_ACTIVATE/NS_DEACTIVATE events to the firstResponder, which I assume is not necessarily the top Gecko window.

http://hg.mozilla.org/mozilla-central/annotate/3d5cde53f929/widget/src/cocoa/nsWindowMap.mm#l255
http://hg.mozilla.org/mozilla-central/annotate/3d5cde53f929/widget/src/cocoa/nsChildView.mm#l5704

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

http://hg.mozilla.org/mozilla-central/rev/2b4f94beede5