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

RESOLVED FIXED

Status

Firefox for Android Graveyard
General
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: mfinkle, Assigned: mfinkle)

Tracking

Details

Attachments

(1 attachment, 3 obsolete attachments)

Created attachment 453390 [details] [diff] [review]
wip 1

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)
(Assignee)

Updated

8 years ago
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+
Created attachment 454069 [details] [diff] [review]
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
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:
element.ownerDocument.releaseCapture();


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)
Created attachment 454856 [details] [diff] [review]
wip 3

This patch seems to have everything working, except canceling the "click" when a contextmenu has been shown.
Attachment #454069 - Attachment is obsolete: true
Created attachment 454858 [details] [diff] [review]
patch

* 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]
patch

-          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

pushed:
http://hg.mozilla.org/mobile-browser/rev/50239a7fa3c3
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.