Asaf Romano (gone) Reporter
	Description • 13 years ago

While working on bug 516753, I found out that setting a "contextmenu" listener in a content script only works in the first tab (main process).

I used the global manager to load my content script, with allowDelayed. I do see the script loaded. I was also able to track other events (well - click). Yet again, I do see contextmenu dispatched in the first tab.

	Asaf Romano (gone) Reporter
	Comment 1 • 13 years ago

I was able to reproduce this on current trunk (with --enable-e10s-compat), mac and windows.

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 2 • 13 years ago

I think we just don't forward contextmenu event atm.

	Asaf Romano (gone) Reporter
	Comment 3 • 13 years ago

Weird.

Per Olli's suggestion I did the following change in eventstatemanager:
PRBool
nsEventStateManager::HandleCrossProcessEvent(nsEvent *aEvent,
                                             nsIFrame* aTargetFrame,
                                             nsEventStatus *aStatus) {
  switch (aEvent->eventStructType) {
...
    case NS_MOUSE_EVENT:
      if (... ||
          aEvent->message == NS_CONTEXTMENU) {


We get there, yes. But the event isn't dispatched because, for some reason, nsEventStatus_eConsumeNoDefault is set. That fails here:

  if (*aStatus == nsEventStatus_eConsumeNoDefault ||
      !target ||
      !IsRemoteTarget(target)) {
...

(There's a target, and it's, as expected, a remote target).

I also verified that in the first tab (main process), getPreventDefault for the dispatched event returns false.

	:Felipe Gomes (needinfo for replies!)
	Comment 4 • 13 years ago

(In reply to Mano from comment #3)
> Weird.
> 
> Per Olli's suggestion I did the following change in eventstatemanager:
> PRBool
> nsEventStateManager::HandleCrossProcessEvent(nsEvent *aEvent,
>                                              nsIFrame* aTargetFrame,
>                                              nsEventStatus *aStatus) {
>   switch (aEvent->eventStructType) {
> ...
>     case NS_MOUSE_EVENT:
>       if (... ||
>           aEvent->message == NS_CONTEXTMENU) {

Yeah. Also take a look at http://mxr.mozilla.org/mozilla-central/source/content/events/src/nsEventStateManager.cpp#1641 . I don't think you'll need to make any changes there because the eventStructType for the contextmenu is just a NS_MOUSE_EVENT, right?

Oh, and make sure the serializer (see nsMouseEvent and nsMouseEvent_base in http://mxr.mozilla.org/mozilla-central/source/widget/public/nsGUIEventIPC.h ) contains all the fields you need.

> 
> 
> We get there, yes. But the event isn't dispatched because, for some reason,
> nsEventStatus_eConsumeNoDefault is set. That fails here:
> 
>   if (*aStatus == nsEventStatus_eConsumeNoDefault ||
>       !target ||
>       !IsRemoteTarget(target)) {
> ...

hmf, the preventDefault API makes this hard to track down because you can't easily tell who changed the status.. As a guess, it's _probably_ happening because the target now is a <browser> element rather than a node from content. Could it be this line?
http://mxr.mozilla.org/mozilla-central/source/content/xul/content/src/nsXULPopupListener.cpp#230

I also searched in nsContextMenu.js but didn't find anything.

	Asaf Romano (gone) Reporter
	Comment 5 • 13 years ago

Felipe! Hey, thanks for joining.

1. Why did you search nsContextMenu.js? we sure don't get that far (and we're not expected to as the event cannot bubble)
2. How would we get to popuplistener? there's no popup involved (yet).

	Asaf Romano (gone) Reporter
	Comment 6 • 13 years ago

Hrm, actually, we might be able to get to xulpopuplistener, but isn't the xul element handled within the chrome process?

	:Felipe Gomes (needinfo for replies!)
	Comment 7 • 13 years ago

TBH I didn't think much of it, I just searched contextmenu-related code to see what could be calling preventDefault().. (or it might be some C++ code directly setting the status).

Note that the event forwarding happens in PostHandleEvent, so any listeners for the contextmenu event in the parent process will already be run at that point.

	Asaf Romano (gone) Reporter
	Comment 8 • 13 years ago

So, yes, Felipe is right.

This is kind of a band-aid. I didn't want to rewrite the popup-listener.

There are still two issues though:
1) We don't really want to filter out all contextmenu events dispatched on browser - even a remote one - given that's this only way to actually generate a context menu later. For tabbbrowser, we could (and will) dispatch it on the tabbrowser element, but that isn't really a solution for any other use case.  However, I could not find a way to distinguish the initial parent event from "legitimate" cases.
2) The "preventDefault" handling later on in this function clearly doesn't make sense in e10s world. For now it's harmless, but ones e10s is on, this code should be removed.

	Asaf Romano (gone) Reporter
	Comment 9 • 13 years ago

IGNORE THE DEBUG PRINTF\n

	Asaf Romano (gone) Reporter
	Comment 10 • 13 years ago

The good news is that with this patch, I get the events in the right order - that is, if a site implemented its own context menu and set prevent-default, its context menu shows up first, and only then I get the event on TabChild (with prevent-default set)

	Olli Pettay [:smaug][bugs@pettay.fi]
	Comment 11 • 13 years ago

Comment on attachment 556759 [details] [diff] [review]
along these lines...


>+  nsCOMPtr<nsIContent> targetContent = do_QueryInterface(targetNode);
>+  if (!targetContent) {
>+    return NS_OK;
>+  }
>+  if (targetContent->Tag() == nsGkAtoms::browser &&
>+      targetContent->IsXUL() &&
>+      targetContent->AttrValueIs(kNameSpaceID_None, nsGkAtoms::Remote,
>+                                 nsGkAtoms::_true, eIgnoreCase)) {
>+    printf("IGNORING CONTEXT MENU EVENT");
>+    return NS_OK;
>+  }

  nsCOMPtr<nsIContent> targetContent = do_QueryInterface(targetNode);
  if (targetContent && 
      targetContent->Tag() == nsGkAtoms::browser &&
      targetContent->IsXUL() &&
      targetContent->AttrValueIs(kNameSpaceID_None, nsGkAtoms::Remote,
                                 nsGkAtoms::_true, eIgnoreCase)) {
    return NS_OK;
  }

	Asaf Romano (gone) Reporter
	Comment 12 • 13 years ago

Not landing this patch as it's not really need anymore.

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
	Comment 13 • 13 years ago

We still support out-of-process content.  We'll probably need this patch for b2g.