Closed Bug 796275 Opened 7 years ago Closed 7 years ago

Context menu fires on wrong target out of process

Categories

(Firefox OS Graveyard :: General, defect, P2)

defect

Tracking

(blocking-basecamp:+, firefox18 fixed, firefox19 fixed)

RESOLVED FIXED
blocking-basecamp +
Tracking Status
firefox18 --- fixed
firefox19 --- fixed

People

(Reporter: ghtobz, Assigned: baku)

References

(Blocks 1 open bug)

Details

(Whiteboard: [LOE:S])

Attachments

(1 file, 1 obsolete file)

[GitHub issue by daleharvey on 2012-07-27T20:23:21Z, https://github.com/mozilla-b2g/gaia/issues/2913]
When I listen to contextmenu events, their target is the parent iframe when used out of context

@jlebar I will diagnose this, but if you have any insight it would help, Thanks
[GitHub comment by jlebar on 2012-07-27T20:30:06Z]
> When I listen to contextmenu events, their target is the parent iframe when used out of [process]

You mean, if you long-press on something inside <iframe mozbrowser>, the target is that iframe, rather than the something you long-pressed on?  What do you expect should happen?

You can't reference a DOM node in another process, so certainly we couldn't fire the event at the node which you long-pressed on.
[GitHub comment by daleharvey on 2012-07-27T20:42:54Z]
I wasnt sure where the context menu got triggered from a long press, but I would expect it to be in the same process or at least have access to the node that triggered the context menu, otherwise capturing the context menu support in BEC seems kinda pointless no?

https://github.com/mozilla-b2g/mozilla-central/blob/master/dom/browser-element/BrowserElementChild.js#L143
[GitHub comment by jlebar on 2012-07-27T20:45:01Z]
>  but I would expect it to be in the same process or at least have access to the node that triggered the context menu, 
> otherwise capturing the context menu support in BEC seems kinda pointless no?

Oh, I understand what you're seeing now.   Yes, that sounds pretty wrong to me!
[GitHub comment by daleharvey on 2012-07-27T20:48:25Z]
Just for tracking since I am working on something else right now, 

https://github.com/mozilla-b2g/mozilla-central/blob/master/content/events/src/nsEventStateManager.cpp#L1863

is what fires the context menu, and that is in the child content process right? is this a problem with the parent capturing the same event and disabing / overriding the child (I vaguelly remember a conversation about this)
[GitHub comment by jlebar on 2012-07-27T20:49:48Z]
> and that is in the child content process right?

I have no idea, myself.  Felipe might know.
[GitHub comment by jcarpenter on 2012-08-21T16:18:12Z]
@erikziegler Can you send Dale the visual design for the menus?
[GitHub comment by daleharvey on 2012-08-21T16:25:28Z]
I created a new issue for the vis dev, cheers @jcarpenter 

https://github.com/mozilla-b2g/gaia/issues/3650
[GitHub comment by nhirata on 2012-08-29T17:15:52Z]
Marking as blocked as this prevents things such as context menu for links as well as in bookmark tabs.  Another wish list item.  @autonome
[GitHub comment by daleharvey on 2012-09-18T13:04:32Z]
Just an update since I almost let this one slip.

Debugging with @jlebar we found that 

1. We fire duplicate contextmenu events as we capture them during bubbling
2. We dont propogate the context menu events across process boundaries (so they never get fired within children)

Tests pass since we manually trigger the event within the child, it should be a matter of capturing the event, finding its target and manually firing in the child
[GitHub comment by jlebar on 2012-09-18T14:44:21Z]
> We fire duplicate contextmenu events as we capture them during bubbling

Just to be clear, it's more like we *observe* duplicate events because the event bubbles.

So there's no Gecko event-handling bug.  The only issue in Gecko is that we don't currently dispatch the event down into the correct process.
[GitHub comment by benfrancis on 2012-09-28T11:46:06Z]
@jlebar @daleharvey Are either of you likely to get to this today? I have a feeling you might both be otherwise engaged. If so can you let us know so we can try and re-assign?
[GitHub comment by jlebar on 2012-09-28T21:56:12Z]
This is a bug, not a feature.  We should not be scrambling to get this in, particularly since he and I spent a few hours at a work week trying to understand what was going on and I don't think anyone else is up to speed here.
[GitHub comment by benfrancis on 2012-10-01T13:19:57Z]
OK fair enough, I thought this bug was blocking the feature development but according to @daleharvey there's already an implementation, it's just that there are bugs in both Gaia and the platform preventing it from working.
I think I need more input on this, it was more complicated than I thought (as usual)

On the device, a long press triggers the contextmenu on the outer frame, the contextmenu event is never propogated to the content process, this is exactly the same as the diagnostics for pressing right click inside b2g desktop

    I/Gecko   ( 1175): BrowserElementChild - Got contextmenu
    I/Gecko   ( 1175): BrowserElementChild - inside: app://browser.gaiamobile.org/index.html on element: IFRAME
    I/Gecko   ( 1175): BrowserElementChild - Got contextmenu
    I/Gecko   ( 1175): BrowserElementChild - inside: app://system.gaiamobile.org/index.html on element: IFRAME

However, if I long press on b2g desktop I get the correctly triggered contextmenu on the content as well as the events fired on its parents, however the click event is still fired and will follow the link as well as fire its contextmenu

    BrowserElementChild - Got contextmenu
    BrowserElementChild - inside: http://browser.gaiamobile.org:8080/index.html on element: IFRAME
    BrowserElementChild - Got contextmenu
    BrowserElementChild - inside: http://system.gaiamobile.org:8080/ on element: IFRAME
    BrowserElementChild - Got contextmenu
    BrowserElementChild - inside: http://yayhooray.net/ on element: #text

I think the first question to answer is why longpress generates contextmenu inside content on b2g desktop but not on the device
Dale, are you waiting for assistance here, or do you have a path forward at the moment?  If you're really spinning your wheels here, would you like to see if Olli or someone else can take over this bug?  I'm all for learning by doing, but there is such a thing as being in over one's head.
Blocks: browser-api
Whiteboard: [label:browser][label:blocked][label:system][LOE:S] → [LOE:S]
I wasnt waiting for assistance but I am somewhat stuck at the moment, I can follow what is actually happening but trying to figure out what is supposed to happen has me stuck. 

If someone a bit more familiar with that code could jump in I would appreciate it. There is another bug that prevents context menus from displaying when the event is correctly shown, but I will file a bug and submit a patch for that one.
Assignee: dale → nobody
https://bugzilla.mozilla.org/show_bug.cgi?id=797297 (I dont really know if either depends on or blocks the other, but figured knowing helps)
> If someone a bit more familiar with that code could jump in I would appreciate it.

Can you detail what you know at this point?
I wonder if this can be related to some device specific code path (src/widget/gonk/nsAppShell.cpp). CC'ing mwu.
When Dale and I looked at this in Sao Paulo, we saw that basically the contextmenu event was not forwarded down to the child process.

The two contextmenu events he was seeing were both occurring in the parent process.  He was seeing two due to event bubbling.

But Dale said things have mysteriously changed since the last time he looked at it; I don't know what the current status is.
In Sao Paulo I was testing using only a right click to trigger a context menu - https://bugzilla.mozilla.org/show_bug.cgi?id=796275#c14 has the most information I could figure out at this point, basically 

long press inside b2g desktop: context menu all works great, but extra click event
long press on device: context menu only fired on outer 2 frames
right click on b2g desktop: same as above

I am finished the larger chunks of my gaia code, so back on this until it gets fixed since I keep dropping it, but I will likely need help, right now I am figuring out where code code path differs between right click and long press on b2g desktop
So without being able to debug child processes I am most definitely stuck here 

http://pastebin.mozilla.org/1864937

Is a diagnoses of what I am trying to do, what goes wrong. If there are any questions about above then please ask, but right now I am getting no work done being stuck on this and I think its best worked on by someone who knows more about the event system.
Assignee: nobody → dale
Priority: -- → P2
Hey Alex

Not sure why 1. this is P2 since its blocking+, and 2. I got reassigned it as I just posted that I am pretty stuck when dealing with the gecko event system.

I will get back to trying to find my way with it when I have a bunch of other bugs off my plate, but it is will be best of someone with more experience in this area could pick up this bug
> 1. this is P2 since its blocking+,

I think we're trying to prioritize the blockers themselves, to avoid the situation where all our "important" bugs are P1's.
Assignee: dale → nobody
Component: Gaia → General
Assignee: nobody → amarchesini
Attached patch patch 1 (obsolete) — Splinter Review
Attachment #679235 - Flags: review?(jones.chris.g)
Blocks: 809525
Comment on attachment 679235 [details] [diff] [review]
patch 1

>diff --git a/dom/ipc/TabChild.cpp b/dom/ipc/TabChild.cpp

> bool
>+TabChild::RecvHandleLongTap(const nsIntPoint& aPoint)

>+  RecvMouseEvent(NS_LITERAL_STRING("contextmenu"), aPoint.x, aPoint.y, 2, 1, 0, false);
>+

Please briefly document what |2, 1, 0, false| mean ;).

Looks good.
Attachment #679235 - Flags: review?(jones.chris.g) → review+
Attached patch patchSplinter Review
https://tbpl.mozilla.org/?tree=Try&rev=a9c65270dd15
waiting for green on try.
Attachment #679235 - Attachment is obsolete: true
Attachment #679662 - Flags: review+
Keywords: checkin-needed
We do not have any test for the gestures. I created a bug for it: Bug 809525
https://hg.mozilla.org/mozilla-central/rev/f3d9d7b86339
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.