862519 - Events handled by both content and chrome are not supported well in electrolysis

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

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

I'm not sure

Comment 1

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

Ugh, somehow hit enter too soon.

I'm not sure what's the case, but it might be in the event code. It looks like we're not getting a keypress event for backspace in the content process.

Comment 2

[(no longer active)]

I assume that this happens in very simple forms, right? (basically just an input type=text let's say?)

Comment 3

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

Yes, it seems to happen in all input fields.

Comment 4

[Tom S [:evilpie]]

Attachment: hack

This hack fixes the problem for me. Of course none of the keyset handlers in browser.xul are going to work. The previous code here was added in Bug 831421. (As a side note, how do you even trigger the backspace behavior in a normal build?)

Comment 5

[(no longer active)]

backspace is handled by the editor here: <http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/text/nsPlaintextEditor.cpp#367> and then we call PreventDefault on it (in nsEditor::HandleKeyPressEvent) to prevent it from being processed later.  If the editor doesn't capture this event, we fall back to handle it using <http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser-sets.inc#301> which involves the code you're patching here.

It seems like the content process just doesn't see the backspace event, perhaps because we're handling it in the chrome process before passing it down to the content process?  (Note sure where that logic lives.)

Comment 6

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

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #5)
> It seems like the content process just doesn't see the backspace event,
> perhaps because we're handling it in the chrome process before passing it
> down to the content process?
Yes, that is why evilpie's patch fixes this. But need to figure out some better, more generic
solution.

Comment 7

[:Felipe Gomes (needinfo for replies!)]

I've been thinking quite a bit about the system that we would want here (for a generic solution), and here are some thoughts.

The gist of the problem is that we need to be able to dispatch an event to the content process, have it call preventDefault(), and have the parent process be aware of that. This means that the parent process needs to wait for the content process to handle the event before continuing with its action, which is not done at the moment.

I'd like a solution that doesn't involve front-end handling in any way, similar to how it was done in bug 583976 (it might require front-end changes but no specific code to handle this). I think this is doable.

I think we also have a great opportunity here to fix the problem of webpages being able to block fundamental brower shortcuts such as ctrl+t, if we want. (note that this doesn't include plugins)

So I propose the following idea:

*Some* shortcut keys (but not all of them), will need to be marked with an attribute saying that they need async handling in the process boundary. We could do this with:

<key keycode="VK_BACK" command="cmd_handleBackspace" needsasync="true"/>

nsXBLWindowKeyHandler.cpp will look for this attribute and properly handle it.
- For keys not marked with it, it will keep working as it is now:
event hits the process boundary, is async-dispatched to the content process, and starts bubbling in the parent process, disregarding the handling done in content

- For keys marked with it, nsXBLWindowKeyHandler.cpp will prevent it from bubbling, generate an id associated with this event, and will wait for content to respond with an async EventHandled message.. Then the proper preventedDefault/propagationStopped flags will be set and the event resumes



If "event bubbling" can't be prevented or if bubbling is not really applicable here (not sure if keyHandler uses the same abstraction), replace it with "will wait the command action from being run until the async response is received"



What does this sound like? Correct? Doable?

Comment 8

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

I don't quite understand the proposal. 
"nsXBLWindowKeyHandler.cpp will look for this attribute and properly handle it."..
"For keys marked with it, nsXBLWindowKeyHandler.cpp will prevent it from bubbling..."
that all happens in parent process, right? So the bubbling is prevented where?
Note, nsXBLWindowKeyHandler runs pretty much in the top level of event target chain, so there isn't
much to bubble.

"will wait for content to respond with an async EventHandled message." so how long? What if the content process dies or hangs. Well, perhaps we could detect that and cancel such cases.

Hmm, perhaps this is doable. Just need to make sure we don't start spinning event loop or anything
for waiting, but really just add some kind of async event listener in parent process.

Comment 9

[(no longer active)]

One thing to note is that this stuff is already buggy -- see bug 289384 for example.  Actually it might be interesting for somebody to try out attachment 718825 [details] [diff] [review] and see if it affects anything.  That might enable us do this dance in the content process (but see bug 289384 comment 75 first.)

Comment 10

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

I worked around this on the larch branch with this checkin:
https://hg.mozilla.org/projects/larch/rev/9d86bba13b48
It just turns off the key handler for the backspace key. You won't be able to go back by hitting backspace, but that's disabled by default anyway.

Hopefully we can find a better fix for this that can be checked into inbound, but until then, this should suffice.

Comment 11

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

I just filed bug 899338 to work around this problem on mozilla-central.

Comment 12

[:Felipe Gomes (needinfo for replies!)]

I'm working on this

Comment 13

[:Felipe Gomes (needinfo for replies!)]

Attachment: WIP

From what we talked on IRC yesterday, this turned out to be simpler in two points:

- No need to set up an event listener in the child, because we can just tie this to the code that does the event dispatching (in TabChild::RecvRealKeyEvent), and thus we can just reply to the parent after dispatching has finished.

- It didn't need any new mechanisms to forward the event to the child, because ESM::HandleCrossProcessEvent already does that for every key event targeted to a <browser remote>.
Supposedly, the StopPropagation call (which I haven't added yet) won't prevent this from happening since ESM does that in PostHandleEvent.


Another thing missing from the patch is that nsXBLWindowKeyHandler will need to be a listener both for capturing and bubbling phases, as we want to stop propagation early for the events that we want to wait for a reply. The current handler is listening in the bubbling phase, which works but is not correct (it means that the event is being fully dispatched twice through in the UI)

Comment 14

[:Felipe Gomes (needinfo for replies!)]

Attachment: Patch

Finished version, just left one XX for you to tell me if it's necessary to do a null check.

I just sent it try, let's see how it goes: https://tbpl.mozilla.org/?tree=Try&rev=330b1313349a

Comment 15

[:Felipe Gomes (needinfo for replies!)]

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

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

::: content/xbl/src/nsXBLWindowKeyHandler.cpp
@@ +408,5 @@
>        do_QueryInterface(aEvent->GetInternalNSEvent()->originalTarget);
>      if (nsEventStateManager::IsRemoteTarget(originalTarget)) {
>        return NS_OK;
>      }
>    }

^ note to self: look into the original cset where this was added and see if it can now be removed too

Comment 16

[:Felipe Gomes (needinfo for replies!)]

(In reply to :Felipe Gomes from comment #15)
> Comment on attachment 8346637 [details] [diff] [review]
> Patch
> 
> Review of attachment 8346637 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: content/xbl/src/nsXBLWindowKeyHandler.cpp
> @@ +408,5 @@
> >        do_QueryInterface(aEvent->GetInternalNSEvent()->originalTarget);
> >      if (nsEventStateManager::IsRemoteTarget(originalTarget)) {
> >        return NS_OK;
> >      }
> >    }
> 
> ^ note to self: look into the original cset where this was added and see if
> it can now be removed too

I've checked and we still can't remove that with this patch, because the arrow keys are not listed as part of any <keyset>, so they don't get this special treatment. An alternative that fixes it would be to do the same treatment to all key events, not only the ones with matching handlers.
It's a fair proposal but it does suck a bit for every key event to have to wait on the content before running in the parent. Although this would be only for when focus is in content anyway.
But it also makes it harder to have a list of shortcut keys that cannot be ignored, which would be a nice benefit of the current approach (like disallowing content to interfere with ctrl + w, ctrl + t)

Comment 17

[:Felipe Gomes (needinfo for replies!)]

Ehsan, what is the code that makes the caret in an input box to move left or right when an arrow key is pressed? Content is receiving the event but is ignoring it, and I want to figure out why (fwiw it's not preventDefault as I've checked and it's not set)

Comment 18

[(no longer active)]

(In reply to :Felipe Gomes from comment #17)
> Ehsan, what is the code that makes the caret in an input box to move left or
> right when an arrow key is pressed? Content is receiving the event but is
> ignoring it, and I want to figure out why (fwiw it's not preventDefault as
> I've checked and it's not set)

nsFrameSelection::MoveCaret is the main function.  If you set a breakpoint there and it's not triggered, then that's a sign that one of the callers is busted somehow.  IIRC this is called from three distinct locations:

** <http://dxr.mozilla.org/mozilla-central/source/editor/libeditor/base/nsEditorCommands.cpp#733> coming from places like <http://dxr.mozilla.org/mozilla-central/source/content/xbl/builtin/editor-base.inc#4>
** <http://dxr.mozilla.org/mozilla-central/source/dom/base/nsGlobalWindowCommands.cpp#294> for caret browsing (Toggle "Always use the cursor keys to navigate withing pages" in prefs->advanced->general)
** <http://dxr.mozilla.org/mozilla-central/source/content/html/content/src/nsTextEditorState.cpp#452> for inputs and textareas.

Isn't this setup absolutely beautiful?  ;-)

Comment 19

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

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


>+  if (isXULKeyset) {
>+    // The capturing listener is only used for XUL keysets to properly handle
>+    // shortcut keys in a multi-process environment.
I don't understand this. Why keyset is special? Why we don't have the same behavior for platformHTMLBingings?


>+    manager->AddEventListenerByType(handler, NS_LITERAL_STRING("keydown"),
>+                                  dom::TrustedEventsAtSystemGroupCapture());
>+    manager->AddEventListenerByType(handler, NS_LITERAL_STRING("keyup"),
>+                                    dom::TrustedEventsAtSystemGroupCapture());
>+    manager->AddEventListenerByType(handler, NS_LITERAL_STRING("keypress"),
>+                                    dom::TrustedEventsAtSystemGroupCapture());
>+  }

Hmm, so listeners in default group would be called.


>+  // Inform the child process that this is a event that we want a reply
>+  // from.
>+  widgetEvent->mFlags.mWantReplyFromContentProcess = 1;
>+  aEvent->StopImmediatePropagation();
I think I'd call StopPropagation() here. It would behave less oddly.

>+bool nsXBLWindowKeyHandler::HasHandlerForEvent(nsIDOMKeyEvent* aEvent) {
bool to its own line and { to the next line

r- because I don't understand why special casing keysets, but not handle platform keybindings the same way.

Comment 20

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

Attachment: felipe's patch v2

I made the changes that Olli asked for. I also changed HasHandlerForEvent so that it doesn't return false when GetElement() is null. That seems to make the code work for platformHTMLBingings. That allows us to remove the special code added in bug 831421 to handle arrow keys.

Comment 21

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

Comment on attachment 8384053 [details] [diff] [review]
felipe's patch v2

Canceling review for now. I think I'll have to combine this with the patch for bug 972420.

Comment 22

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

Comment on attachment 8384053 [details] [diff] [review]
felipe's patch v2

I worked some more on that bug and I think they're independent.

Comment 23

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

Comment on attachment 8384053 [details] [diff] [review]
felipe's patch v2

>+bool
>+TabParent::RecvReplyKeyEvent(const WidgetKeyboardEvent& event)
>+{
>+  WidgetKeyboardEvent localEvent(event);
>+  // Set mNoCrossProcessBoundaryForwarding to avoid this event from
>+  // being infinitely redispatched and forwarded to the child again.
>+  localEvent.mFlags.mNoCrossProcessBoundaryForwarding = 1;
>+
>+  nsCOMPtr<mozilla::dom::EventTarget> target = do_QueryInterface(mFrameElement);
>+  NS_ENSURE_TRUE(target, true);
>+
>+  // Here we convert the WidgetEvent that we received to an nsIDOMEvent
>+  // to be able to dispatch it to the <browser> element as the target element.
>+  nsCOMPtr<nsIContent> content = do_QueryInterface(mFrameElement);
>+  NS_ENSURE_TRUE(content, true);
You're doing quite some QIs here, and none of them is needed.
mFrameElement is Element*, and Element inherits from nsIContent, and nsIContent
inherits nsINode which inherits EventTarget.


>+  nsIDocument* doc = content->OwnerDoc();
>+  nsPresContext* presContext = doc->GetShell()->GetPresContext(); // XXX null check GetShell?
Yes. In general Get* method may return null.

>+
>+  nsIDOMEvent* domEvent;
>+  nsEventDispatcher::CreateEvent(target, presContext,
>+                                 &localEvent, EmptyString(), &domEvent);
You leak the world here with nsIDOMEvent*. Running debug builds with export XPCOM_MEM_LEAK_LOG=1
is recommended ;)

>+
>+  domEvent->SetOwner(target);
>+
>+  bool dummy;
>+  target->DispatchEvent(domEvent, &dummy);
>+  return true;

You could just
nsEventDispatcher::Dispatch(target, presContext, &localEvent);

Comment 24

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

Attachment: events v3

Thanks for the help.

Comment 26

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

Comment on attachment 8388918 [details] [diff] [review]
events v3

>+TabParent::RecvReplyKeyEvent(const WidgetKeyboardEvent& event)
>+{
>+  NS_ENSURE_TRUE(mFrameElement, true);
>+
>+  WidgetKeyboardEvent localEvent(event);
>+  // Set mNoCrossProcessBoundaryForwarding to avoid this event from
>+  // being infinitely redispatched and forwarded to the child again.
>+  localEvent.mFlags.mNoCrossProcessBoundaryForwarding = 1;
Uh, does this even compile. Anyhow, true; not 1;

>+
>+  // Here we convert the WidgetEvent that we received to an nsIDOMEvent
>+  // to be able to dispatch it to the <browser> element as the target element.
>+  nsIDocument* doc = mFrameElement->OwnerDoc();
>+  nsIPresShell* docShell = doc->GetShell();
s/docShell/presShell/

>+nsXBLWindowKeyHandler::HandleEventOnCapture(nsIDOMKeyEvent* aEvent)
>+{
>+  WidgetKeyboardEvent* widgetEvent =
>+      aEvent->GetInternalNSEvent()->AsKeyboardEvent();
2 space indentation

>+
>+  if (widgetEvent->mFlags.mNoCrossProcessBoundaryForwarding == 1) {
>+    return;
>   }
just 
if (if (widgetEvent->mFlags.mNoCrossProcessBoundaryForwarding) {
  return;
}

>+nsXBLWindowKeyHandler::HasHandlerForEvent(nsIDOMKeyEvent* aEvent)
>+{
>+  bool trustedEvent = false;
>+  // Don't process the event if it was not dispatched from a trusted source
>+  aEvent->GetIsTrusted(&trustedEvent);
>+
>+  if (!trustedEvent) {
>+    return false;
>+  }
These days you can just do
if (!aEvent->InternalDOMEvent()->IsTrusted()) {
  return false;
}
You need to include "mozilla/dom/Event.h"



>+
>+  nsresult rv = EnsureHandlers();
>+  NS_ENSURE_SUCCESS(rv, false);
>+
>+  nsCOMPtr<Element> el = GetElement();
>+  if (el) {
>+    nsCOMPtr<nsIContent> content = do_QueryInterface(el);
Element inherits nsIContent.

>+    // skip keysets that are disabled
>+    if (content && content->AttrValueIs(kNameSpaceID_None, nsGkAtoms::disabled,
>+                                        nsGkAtoms::_true, eCaseMatters)) {
>+      return false;
>+    }
>+  }
I guess you just copied this from elsewhere.
I'd prefer having some helper method to check whether the element is disabled.
Maybe another version of GetElement() which return Element* and has outparam bool* aIsDisabled;
That could be used then also in nsXBLWindowKeyHandler::WalkHandlers


Would like to see those nits fixed.

Comment 27

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

Attachment: events v4

Comment 28

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/5232b7179148

Comment 29

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

Backed out because nsEventDispatcher got renamed last night.
https://hg.mozilla.org/integration/mozilla-inbound/rev/cb2cdeffc026

Comment 30

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

Relanded without the 'ns':
https://hg.mozilla.org/integration/mozilla-inbound/rev/9c3f1b715943

Comment 31

[Ryan VanderMeulen [:RyanVM]]

Backed out for mochitest failures.
https://hg.mozilla.org/integration/mozilla-inbound/rev/57a814e48f5f

https://tbpl.mozilla.org/php/getParsedLog.php?id=36342330&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=36342548&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=36342297&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=36342373&tree=Mozilla-Inbound

Comment 32

[Ryan VanderMeulen [:RyanVM]]

Those were a small sampling, btw. Please make sure this has a fully-green Try run before attempting to push again.

Comment 33

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

I had a green run last night.
https://tbpl.mozilla.org/?tree=Try&rev=0be014f0d5fb
If only that were enough to guarantee my changes would stick! I think we're all going to end up with PTSD from inbound some day.

Comment 34

[Ryan VanderMeulen [:RyanVM]]

And when you're running this through Try, don't succumb to the temptation to call the failures below bug 874423. They're perma-fail with your patch.
https://tbpl.mozilla.org/php/getParsedLog.php?id=36344450&tree=Mozilla-Inbound

Comment 35

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

Sorry I was so grumpy. I guess I did screw up here. I missed one of the tests when making the GetElement change, and it randomly happened to pass all Linux64 tests while failing everything else.

Green try push: https://tbpl.mozilla.org/?tree=Try&rev=fa182ce16474

https://hg.mozilla.org/integration/mozilla-inbound/rev/cce47e059431

Comment 36

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/cce47e059431