831421 - Arrow keys are broken in test-ipc.xul

Status: RESOLVED FIXED

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

Description

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

If you run test-ipc.xul and visit google.com and start typing in the search box, the arrow keys don't work. (Note that drawing is kind of broken in test-ipc.xul, but it's usually possible to force it to draw by moving the window around.)

It looks like the keypress event is not being sent to the child process because it has already been consumed by the parent.

Comment 1

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

If I make this change to nsGlobalWindow.cpp, it sort of fixes the problem:

-    if (privateRoot == static_cast<nsIDOMWindow*>(this)) {
+    if (privateRoot == static_cast<nsIDOMWindow*>(this) && XRE_GetProcessType() == GeckoProcessType_Content) {
       nsXBLService::AttachGlobalKeyHandler(mChromeEventHandler);
     }

However, this is pretty kludgey. It works if all <browser> elements are remote, but it breaks for non-remote <browser> elements.

Comment 2

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

Attachment: disable global bindings for remote process

https://tbpl.mozilla.org/?tree=Try&rev=022fc352b339

Comment 3

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 703428 [details] [diff] [review]
disable global bindings for remote process

Thanks Olli! This works for both remote and non-remote <browser> elements.

Comment 4

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

Comment on attachment 703428 [details] [diff] [review]
disable global bindings for remote process

I don't know who could actually review this :/

But, felipe, what you think of this. We have the global bindings also in
child process, so using them in parent doesn't really make sense to me.

Comment 5

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 703428 [details] [diff] [review]
disable global bindings for remote process

Review of attachment 703428 [details] [diff] [review]:
-----------------------------------------------------------------

Yeah let's try this, it's a simpler change that gets what we need to work. If it breaks something else or doesn't work properly we can think of a larger change later.. My only concern is if the b2g structure is depending on these handlers in the parent.

Comment 6

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

Comment on attachment 703428 [details] [diff] [review]
disable global bindings for remote process

Neil, you might remember how these global XBL handlers work ;)

If this causes problems in b2g, that means that something there needs to
manually prevent these key events in parent, IMHO.

Comment 7

[[e-mail]]

Comment on attachment 703428 [details] [diff] [review]
disable global bindings for remote process

> bool
> nsXBLWindowKeyHandler::WalkHandlersAndExecute(nsIDOMKeyEvent* aKeyEvent,
>                                               nsIAtom* aEventType,
>                                               nsXBLPrototypeHandler* aHandler,
>                                               uint32_t aCharCode,
>                                               bool aIgnoreShiftKey)
> {
>-  nsresult rv;
I don't think this is the right method to change, my preference would be for HandleEvent, which is three calls further up in the stack.

>+  if (sXBLSpecialDocInfo && aHandler &&
>+      aHandler->GetPrototypeBinding() &&
>+      aHandler->GetPrototypeBinding()->XBLDocumentInfo() ==
>+        sXBLSpecialDocInfo->mHTMLBindings.get()) {
I think the correct way to do this test is to see whether the result of GetElement() is null.

Comment 8

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

(In reply to [e-mail] from comment #7)
> I don't think this is the right method to change, my preference would be for
> HandleEvent, which is three calls further up in the stack.
Doesn't really matter, but I can move it up.

> I think the correct way to do this test is to see whether the result of
> GetElement() is null.
I don't understand this comment.

Comment 9

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

Attachment: simpler

Comment 10

[[e-mail]]

Comment on attachment 705413 [details] [diff] [review]
simpler

>+    nsCOMPtr<mozilla::dom::Element> originalTarget =
>+      do_QueryInterface(aEvent->GetInternalNSEvent()->originalTarget);
>+    if (nsEventStateManager::IsRemoteTarget(originalTarget)) {
[I guess it doesn't matter that IsRemoteTarget only needs an nsIContent*]

Comment 11

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

https://hg.mozilla.org/mozilla-central/rev/1a3f731ae54a