Closed Bug 831421 Opened 11 years ago Closed 11 years ago

Arrow keys are broken in test-ipc.xul

Categories

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

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla21

People

(Reporter: billm, Assigned: smaug)

Details

Attachments

(2 files)

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.
Assignee: nobody → bugs
Component: DOM: Events → Event Handling
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 on attachment 703428 [details] [diff] [review]
disable global bindings for remote process

Thanks Olli! This works for both remote and non-remote <browser> elements.
Attachment #703428 - Flags: feedback?(wmccloskey) → feedback+
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.
Attachment #703428 - Flags: feedback?(felipc)
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.
Attachment #703428 - Flags: feedback?(felipc) → feedback+
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.
Attachment #703428 - Flags: review?(neil)
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.
Attachment #703428 - Flags: review?(neil) → review-
(In reply to neil@parkwaycc.co.uk 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.
Attached patch simplerSplinter Review
Attachment #705413 - Flags: review?(neil)
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*]
Attachment #705413 - Flags: review?(neil) → review+
https://hg.mozilla.org/mozilla-central/rev/1a3f731ae54a
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
Component: Event Handling → User events and focus handling
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: