Closed Bug 574006 Opened 12 years ago Closed 12 years ago

Support context menu and open-in-new-tab behavior


(Firefox for Android Graveyard :: General, defect)

Not set


(Not tracked)



(Reporter: mfinkle, Assigned: mfinkle)



(1 file, 3 obsolete files)

Attached patch wip 1 (obsolete) — Splinter Review
e10s will require context menu code to be split between chrome and content process. Also, the open-in-new-tab via CTRL+Tap needs to be supported as well.

This is a WIP patch. CTRL+Tap works, but context menu support doesn't yet. The "contextmenu" event is not being fired.
Attachment #453390 - Flags: feedback?(21)
Attachment #453390 - Flags: feedback?(mbrubeck)
Comment on attachment 453390 [details] [diff] [review]
wip 1

> ContentCustomClicker.prototype = {
>-  _dispatchMouseEvent: function _dispatchMouseEvent(aName, aX, aY) {
>+  _dispatchMouseEvent: function _dispatchMouseEvent(aName, aX, aY, aModifiers) {
>     let browser = this._browserView.getBrowser();
>     let [x, y] = Browser.transformClientToBrowser(aX, aY);
>-    browser.messageManager.sendAsyncMessage(aName, { x: x, y: y });
>+    browser.messageManager.sendAsyncMessage(aName, { x: x, y: y, modifiers: aModifiers });
>   },

aModifiers || null;
>   mouseDown: function mouseDown(aX, aY) {
>     // Ensure that the content process has gets an activate event
>     let browser = this._browserView.getBrowser();
>     let fl = browser.QueryInterface(Ci.nsIFrameLoaderOwner).frameLoader;
>     try {
>       fl.activateRemoteFrame();
>     } catch (e) {}
>-    this._dispatchMouseEvent("Browser:MouseDown", aX, aY);
>+    this._dispatchMouseEvent("Browser:MouseDown", aX, aY, null);
>   },

remove null here and everywhere

otherwise it looks fine, I'm not sure why the contextmenu is not working on e10s but it works on mozila-central. the merge from yesterday night seems to have took into account the change into nsEventStateManager done for handling it so i think it would be fine after rebuild
Attachment #453390 - Flags: feedback?(21) → feedback+
Attached patch wip 2 (obsolete) — Splinter Review
Adds some fixes now that I can actually see a context menu.
Adds some code from Vivien to enable on remote tabs

Still has dragging problems on local pages, but remote pages seem to work ok
Assignee: nobody → mark.finkle
Attachment #453390 - Attachment is obsolete: true
Attachment #453390 - Flags: feedback?(mbrubeck)
(In reply to comment #2)
> Created an attachment (id=454069) [details]
> wip 2
> Adds some fixes now that I can actually see a context menu.
> Adds some code from Vivien to enable on remote tabs
> Still has dragging problems on local pages, but remote pages seem to work ok

It seems to work for me when I remote the setTimeout around:

Also I think we need to ignore the contextmenu event if a pan has occured (MouseCancel), there is no clean way to cancel it now (or at least none that I know).
I'll be ok with a flag for now while we've not found a better solution.

A part of the code in input handler is also here to stop panning while the context menu is show (bug 555725)
Attached patch wip 3 (obsolete) — Splinter Review
This patch seems to have everything working, except canceling the "click" when a contextmenu has been shown.
Attachment #454069 - Attachment is obsolete: true
Attached patch patchSplinter Review
* Added mouse cancel check in ContentCustomClicker.singleClick

Seems to work well in e10s for local and remote pages
Attachment #454856 - Attachment is obsolete: true
Attachment #454858 - Flags: review?(21)
Comment on attachment 454858 [details] [diff] [review]

-          let rects = getContentClientRects(element);
-          sendAsyncMessage("Browser:Highlight", { rects: rects });
-        }, kTapOverlayTimeout);
+        this._sendMouseEvent("mousedown", element, x, y);
+        element.ownerDocument.releaseCapture();

A comment could be nice for the releaseCapture, because this can look unclear why there is a call to releaseCapture without any setCapture
Attachment #454858 - Flags: review?(21) → review+
added comment

Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.