794621 - Work - Remove the event overlay and send input directly to content

Status: RESOLVED FIXED

(Firefox for Metro Graveyard :: General, defect)

Description

[Jim Mathies [:jimm]]

So some background. In the current implementation of metrofx, we use the same event overlay xul fennec uses. What this does basically is trap input events and forward them over to remote content. While the current incarnation of metrofx does not use remote content we've kept the e10s related boundaries in place based on the assumption that we would like to enable remote content down the road.

With in-process content, this mechanism is causing a lot of problems, specifically:

- Front end event handling is way too complex. There are a series of event traps in chrome code that catch events and forward them over to out-of-process content code, both of which are running in the same process. We've had to deal with a number event issues like event loops and capture listeners that receive events they do not expect. Event often have to be recreated and re-targeted multiple times so that they can be delivered to content.

- Front end code is currently segregated into chrome and content modules which communicate through message manager. This adds additional complexity we do not have to live with if we are not going to support e10s. Also, message manager has been shown to be rather slow on Windows due to the way is handles in-process events via runnables. (This could be fixed, but wouldn't need to be if we weren't using message manager so heavily for in process communication.)
 
- Chrome can't directly access content. Again we spend a lot of time forward messages that are interpreted by content script which then manipulate content and forward the results back. This is all uneeded overhead and complexity at this point.

- Performance is bad. Recent work to get w3c touch events implemented exposed this in a bad way - touch events had to be recreated and forwarded twice to get the events to a web page. On desktop drawing on various touch demos is performant, in metrofx the browser was unable to keep up with more than one or two touch points.

So currently the team is leaning toward removing all of this code and streamlining the front end post the release of our preview this month. This would mean we would have solve the e10s problem later if we decided to implement out-of-process content. Potentially this could be done in the backend (automatic event forwarding?) without involving front end script.

One flip side argument - there are concerns about panning and zoom performance without e10s. The preview release may well put those concerns to bed, or bring them to light. We do plan to get OMTC going for in-process content which will help.

cc'ing a few people who might have an opinion either way.

Comment 1

[Jim Mathies [:jimm]]

I should add, we don't need to remove all the e10s modularity from front end code. Some components like selection handling can continue to work through the message manager. But we would remove those that pose a performance problem, and we would want to remove the input overlay and a number of capture hooks / forwarding in chrome that deal with timely input.

Comment 2

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

I don't know enough about metrofox to have a strong opinion one way or another, but fyi touch events are implemented fully generically for cross-process content (they go through normal DOM dispatch on parent side), and we've optimized the crap out of cross-process dispatch for b2g.  There are several gaia apps (regular HTML content) that easily hit 60fps with pan/zoom gestures on the lowest-end smartphone chipset that qualcomm makes nowadays.

YMMV though; some of this perf depends on having a separate compositor thread (in addition to content processes).

Comment 3

[Jim Mathies [:jimm]]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #2)
> I don't know enough about metrofox to have a strong opinion one way or
> another, but fyi touch events are implemented fully generically for
> cross-process content (they go through normal DOM dispatch on parent side),
> and we've optimized the crap out of cross-process dispatch for b2g.  There
> are several gaia apps (regular HTML content) that easily hit 60fps with
> pan/zoom gestures on the lowest-end smartphone chipset that qualcomm makes
> nowadays.
> 
> YMMV though; some of this perf depends on having a separate compositor
> thread (in addition to content processes).

Is the event forwarding in b2g happening automatically down in the dom? With metrofx (as with xul fennec) the events are received by chrome, re-targeted there at the (maybe oop) content window, then the content window re-targets again at the actual content that should receive them.

event -> js chrome -> (oop?) js content window -> content

Adding out-of-process content would be a breeze if the dom handled event forwarding for us. If it does now, which is why I ask about b2g, then we can remove all this craziness from our front end code today.

Comment 4

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

They're dispatched through the full document hierarchy

 <xul:window>
   <!-- XUL chrome -->
   <browser remote=true>
       \./ [IPC] \./
       <!-- HTML content -->

There's no retargeting needed.

In fact, you don't want to even attempt to retarget, because it destroys fast paths ;).

Comment 5

[Jim Mathies [:jimm]]

(In reply to Chris Jones [:cjones] [:warhammer] from comment #4)
> They're dispatched through the full document hierarchy
> 
>  <xul:window>
>    <!-- XUL chrome -->
>    <browser remote=true>
>        \./ [IPC] \./
>        <!-- HTML content -->
> 
> There's no retargeting needed.
> 
> In fact, you don't want to even attempt to retarget, because it destroys
> fast paths ;).

Sounds like the code we started with is a bit out of date.

Does the current implementation conform to the dom event model? For example, do events bubble up properly to chrome from content across processes?

Comment 6

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

We break DOM semantics at the process boundary: events don't bubble up (well, back to the chrome DOM anyway).  We have to break full DOM event semantics anyway or chrome would be blocked on content, which is exactly what we don't want to happen.

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

Yes, the input overlay in XUL Fennec predates much the the new code regarding event forwarding. One of the other reasons for using the overlay was implementing the sliding sidebars in XUL Fennec. Panning the content would cause the chrome sidebars to slide into view when you hit a content edge. This was implemented bby handling all input in the chrome side first, then passing the mouse events to content if we were not sliding a sidebar.

Metro does not do anything like this, I think. We should try to remove as much of the old code as possible.

Comment 8

[Jim Mathies [:jimm]]

(In reply to Mark Finkle (:mfinkle) from comment #7)
> Yes, the input overlay in XUL Fennec predates much the the new code
> regarding event forwarding. One of the other reasons for using the overlay
> was implementing the sliding sidebars in XUL Fennec. Panning the content
> would cause the chrome sidebars to slide into view when you hit a content
> edge. This was implemented bby handling all input in the chrome side first,
> then passing the mouse events to content if we were not sliding a sidebar.
> 
> Metro does not do anything like this, I think. We should try to remove as
> much of the old code as possible.

We still use it as the source of the custom scroller for content scrolling, but we can make it invisible using pointer-events: none. I'm looking at this now. 

It looks like we can split this up into a couple tasks - 

1) remove the overlay trap and deliver most input directly to content. Touch up everything that breaks. We'll keep our mouse input capture listeners in input.js for scrolling for now. This will fix the performance issues we have with certain events, like touch input. This will also allow us to get rid of TapDown, TapUp, and TapMove events, and all the message manager code associated with processing them.

2) Remove the custom scroller code and use anonymous scroll bars, and in so doing remove the need for the capture listeners and custom scroller for content in input.js. This is longer term and is out of the scope of this bug.

These changes shouldn't force us to remove the isolation we have between certain modules. Longer term we can decide if this is still needed, and if not, merge these together so that we don't need the message manager at all, or keep them and implement oop content.

Comment 9

[Jim Mathies [:jimm]]

Attachment: starting point

Comment 10

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

For maximum performance, you'll want to add a fast-path dispatch like

http://mxr.mozilla.org/mozilla-central/source/widget/gonk/nsWindow.cpp#300

This skips display-list hit-testing and DOM event dispatch in the parent process when a TabParent is "capturing" a touch series.  Everything will work just fine without that fast path, though.  (Even better would be making the fast-path widget-backend neutral, but I wasn't able to find a way to do that.)

Comment 11

[Matt Brubeck (:mbrubeck)]

Attachment: WIP

Comment 12

[Matt Brubeck (:mbrubeck)]

Attachment: patch

Everything is in a working state now as far as I know, with no known regressions and most of the dead code removed.  I'll request review as soon as I've taken a second look over everything and done some final cleanup.

Comment 13

[Matt Brubeck (:mbrubeck)]

Comment on attachment 684317 [details] [diff] [review]
patch

This touches a lot of code, but at the core it is just replacing message passing with regular event dispatch.

Instead of intercepting input events with a transparent overlay, we let them go straight to content.  Instead of catching the resulting events in the content process and sending messages back to chrome, we wait for the events to bubble up to chrome.  (Note: this won't work with out-of-process-content.  Longer-term I expect we'll be using the e10s-aware AsyncPanZoomController rather than chrome JavaScript code here.)

MouseModule now listens for touch events instead of mouse events (so I renamed it to TouchModule).  This breaks treatMouseAsTouch mode, but I have a patch started to restore that mode for testing purposes.

This patch depends on bug 808847.

Comment 14

[Jim Mathies [:jimm]]

Comment on attachment 684317 [details] [diff] [review]
patch

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

Can we work up a new rev with all the commented out code removed? I'd like to take this for a spin but unfortunately tim's patch isn't applying.

::: browser/metro/base/content/input.js
@@ +93,5 @@
>   * the defaultDragger prototype property.
>   */
>  
> +var TouchModule = {
> +  _debugEvents: true,

nit - false

Comment 15

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Attachment: Metro Widget Input Overhaul

Sorry for the delay!  Attached is the current widget patch, rebased to apply cleanly to current elm tip.

High-level description of patch:
  Moves input-related code out of FrameworkView, MetroWidget, and GestureInput (GestureInput has now been removed) into a newly-named MetroInput.  

Bugs fixed by this patch:
  Bug 745071 - This patch adds touch support, and improves the performance of our touch events over what we had previously implemented. On my Samsung Series 7 slate, metro touch is no longer visibly slower than desktop touch.
  Bug 800908 - This was due to us sending two keypress events for the 'tab' key
  Bug 815253 - This was due to us sending no keypress events for the 'delete' key
  Bug 808847 - We now send mousedown/mouseup for touch events only if a 'tap' gesture is detected
  Bug 795063 - This patch takes advantage of the Tapped and RightTapped events that GestureRecognizer raises, avoiding the need to implement our own tap, click, and doubleclick detection.

Comment 16

[Matt Brubeck (:mbrubeck)]

Attachment: front-end patch v2

No code changes since my previous patch; just rebased to avoid a minor conflict on elm tip.

Comment 18

[Matt Brubeck (:mbrubeck)]

Comment on attachment 686331 [details] [diff] [review]
Metro Widget Input Overhaul

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

::: widget/windows/winrt/GestureInput.cpp
@@ +801,5 @@
> +            velocities.Linear.X >= SWIPE_MIN_VELOCITY
> +         && abs(delta.Translation.X) >= SWIPE_MIN_DISTANCE;
> +  bool isVerticalSwipe =
> +            velocities.Linear.Y >= SWIPE_MIN_VELOCITY
> +         && abs(delta.Translation.Y) >= SWIPE_MIN_DISTANCE;

Drive-by comment: I think you need to use abs() for the velocities too.  With this patch applied, I get DOWN and RIGHT swipe events but not UP or LEFT.

Comment 19

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Attachment: back-end patch

(In reply to Matt Brubeck (:mbrubeck) from comment #18)
> Comment on attachment 686331 [details] [diff] [review]
> Metro Widget Input Overhaul
> 
> Review of attachment 686331 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/windows/winrt/GestureInput.cpp
> @@ +801,5 @@
> > +            velocities.Linear.X >= SWIPE_MIN_VELOCITY
> > +         && abs(delta.Translation.X) >= SWIPE_MIN_DISTANCE;
> > +  bool isVerticalSwipe =
> > +            velocities.Linear.Y >= SWIPE_MIN_VELOCITY
> > +         && abs(delta.Translation.Y) >= SWIPE_MIN_DISTANCE;
> 
> Drive-by comment: I think you need to use abs() for the velocities too. 
> With this patch applied, I get DOWN and RIGHT swipe events but not UP or
> LEFT.

Good point.  Apparently I managed to test swiping in only 2 directions ;)

Actually this reminds me that I haven't tested the following:
  Pinch events
  Rotate events
  Pen input

I found my stylus, so I can test pen input, but does anyone know a good resource for testing the other two?

I haven't been able to apply both patches at once yet, so it's possible that some of these issues are resolved with the frontend patch applied, but I noticed some undesired behavior while testing the backend patch:
  Tapping causes a "click" event to be received by the page for the first tap, but not for subsequent taps. I attached a debugger and followed along, and the widget code seemed to be sending the correct events.
  Right-tapping (tap and hold) on the background of a page causes the app bar to appear.
  Middle-clicking does not cause a "click" event to be received by the page.

I'll re-test these with the frontend patch applied, but I figured I'd point them out in case they're real issues.

Comment 20

[Jim Mathies [:jimm]]

(In reply to Tim Abraldes (:tabraldes) from comment #19)
> Actually this reminds me that I haven't tested the following:
>   Pinch events
>   Rotate events

At this point all we can do is make sure the simple gestures get sent, since these aren't hooked up to anything. We may not even use these once we get the async controller working.

Comment 21

[Jim Mathies [:jimm]]

Hmm, input doesn't seem to work at all with both of these applied. Lets chat on irc today.

Comment 22

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

(In reply to Tim Abraldes (:tabraldes) from comment #19)
> [...]
>
>   Tapping causes a "click" event to be received by the page for the first
> tap, but not for subsequent taps.

With the front-end patch applied, this is no longer an issue.

> [...]
>
>   Right-tapping (tap and hold) on the background of a page causes the app
> bar to appear.
>   Middle-clicking does not cause a "click" event to be received by the page.

These two are both still present, even with the front-end patch applied.

>
> [...]
>

Still to do: Test pen input, test magnify, test rotate

Comment 23

[Jim Mathies [:jimm]]

Comment on attachment 686340 [details] [diff] [review]
front-end patch v2

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

I'm going to mark this a r+, although it would be nice if we could get double-tab on the tab bar and page selection working. If not, please file a follow up.

::: browser/metro/base/content/bindings/notification.xml
@@ +39,5 @@
>            return this.parentNode.customDragger;
>          </getter>
>        </property>
>  
>        <method name="captureEvents">

Can't we get rid of all this?

::: browser/metro/base/content/contenthandlers/Content.js
@@ +175,5 @@
>  function elementFromPoint(x, y) {
>    // browser's elementFromPoint expect browser-relative client coordinates.
>    // subtract browser's scroll values to adjust
>    let cwu = Util.getWindowUtils(content);
> +  /* XXX mbrubeck

ditto

@@ +284,5 @@
>      addMessageListener("Browser:CanUnload", this);
>      addMessageListener("Browser:CanCaptureMouse", this);
>  
>      // Asyncronous messages from the browser we only receive when out-of-process 
> +    /* XXX mbrubeck

ditto

@@ +295,5 @@
>  
>      // Synchronous events from the browser we only receive when in-process
> +    addEventListener("touchstart", this, false);
> +    addEventListener("touchend", this, false);
> +    /* XXX mbrubeck

ditto

@@ +351,5 @@
> +        let touch = aEvent.changedTouches[0];
> +        this._genericMouseDown(touch.clientX, touch.clientY);
> +        break;
> +
> +        /*

ditto

@@ +481,5 @@
>      // Set the target element to active
>      this._doTapHighlight(element);
>    },
>  
> +  // XXX mbrubeck unused

ditto

::: browser/metro/base/content/input.js
@@ +260,5 @@
>        }
>      }
>  
>      // When panning starts over an input field, focus should not change
> +    /*

ditto

@@ +286,2 @@
>      // Move the caret to the end of the target input field and focus it
> +    /*

ditto

@@ +353,5 @@
>            this._targetScrollbox.dispatchEvent(event);
>          }
>        }
> +    }
> +    /*else if (!dragData.dragging && this._downUpEvents.length) {

ditto

Comment 24

[Jim Mathies [:jimm]]

Comment on attachment 686439 [details] [diff] [review]
back-end patch

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

sweet work, thanks for all the reformulating and code cleanup.

::: widget/windows/winrt/GestureInput.cpp
@@ +2,5 @@
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +// Header we're implementing

nit - no need for this comment

@@ +378,5 @@
> +  // The limited documentation available on MSDN suggests that these
> +  // functions will return true only if that is the button that caused
> +  // this input event to occur, which implies that only one can be true
> +  // at a time, and that they will only be true for PointerPressed and
> +  // PointerReleased events.

If I remember correctly, these are only valid on PointerPressed. Might have been fixed since I wrote the original code. Please test to confirm your assumption is correct.

@@ +945,5 @@
> +// is never read.  This allows us to avoid the (admittedly small) overhead
> +// of creating a new nsEventStatus every time we dispatch an event.
> +void
> +MetroInput::DispatchEventIgnoreStatus(nsGUIEvent *aEvent) {
> +  mWidget->DispatchEvent(aEvent, sThrowawayStatus);

are we sure mWidget is always valid here and below?

@@ +979,5 @@
> +  aData->isChanged = false;
> +  return PL_DHASH_NEXT;
> +}
> +
> +MetroInput::~MetroInput()

Please move this up top.

@@ +1209,5 @@
> +{
> +  return sVirtualKeyMap[aKey];
> +}
> +
> +MetroInput::MetroInput(MetroWidget* aWidget,

ditto, lets move this up top. it's sort of a mozilla convention.

@@ +1210,5 @@
> +  return sVirtualKeyMap[aKey];
> +}
> +
> +MetroInput::MetroInput(MetroWidget* aWidget,
> +                           UI::Core::ICoreWindow* aWindow)

nit - spacing

Comment 25

[Matt Brubeck (:mbrubeck)]

Attachment: front-end patch v3

Addresses Jim's review comments and removes some other related dead code. (Thanks Jim.)  Also includes trivial changes to fix some bugs with tap highlights and contextmenu event positioning.  Carrying r=jimm.  This patch is ready for checkin.

Comment 26

[Matt Brubeck (:mbrubeck)]

Attachment: fixes for SelectionHelperUI

Migrate from custom Tap* events to standard click/touch events.  As a follow-up we should also consider how to make SelectionHelperUI play nicely with mouse, for users who are using both mouse and touch.

Comment 27

[Jim Mathies [:jimm]]

Comment on attachment 687188 [details] [diff] [review]
fixes for SelectionHelperUI

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

::: browser/metro/base/content/helperui/SelectionHelperUI.js
@@ +328,5 @@
>      messageManager.addMessageListener("Content:SelectionDebugRect", this);
>  
>      // selection related events
> +    window.addEventListener("click", this, true);
> +    window.addEventListener("dblclick", this, true);

Do we currently receive this from widget or do we need a follow up on it?

Comment 28

[Matt Brubeck (:mbrubeck)]

Attachment: completely remove "Tap" events

And this fixes the last few bits of code that were still using TapSingle/TapDouble/etc.

Comment 29

[Jim Mathies [:jimm]]

Comment on attachment 687193 [details] [diff] [review]
completely remove "Tap" events

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

::: browser/metro/base/content/bindings/selectionoverlay.xml
@@ -130,5 @@
> -        <parameter name="aEvent"/>
> -        <body>
> -          <![CDATA[
> -            Util.dumpLn("selection-overlay:", aEvent.type);
> -            switch (aEvent.type) {

I had some plans for these but never got around to it.

Comment 30

[Matt Brubeck (:mbrubeck)]

Attachment: front-end patch v4

I deleted slightly too much code and broke the SelectHelper.  This version restores the code from _genericMouseClick and calls it in response to "click" events.

Comment 31

[Matt Brubeck (:mbrubeck)]

(In reply to Jim Mathies [:jimm] from comment #27)
> > +    window.addEventListener("click", this, true);
> > +    window.addEventListener("dblclick", this, true);
> 
> Do we currently receive this from widget or do we need a follow up on it?

Yes, we do receive both of these from widget, and the SelectionHelperUI code is now working correctly with this patch... but for some reason double-tap in the tab strip is still not working with touch.  I'll file a follow-up bug for that.

Comment 32

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Attachment: back-end patch

(In reply to Jim Mathies [:jimm] from comment #24)
> Comment on attachment 686439 [details] [diff] [review]
> back-end patch
> 
> Review of attachment 686439 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> sweet work, thanks for all the reformulating and code cleanup.
> 
> ::: widget/windows/winrt/GestureInput.cpp
> @@ +2,5 @@
> >  /* This Source Code Form is subject to the terms of the Mozilla Public
> >   * License, v. 2.0. If a copy of the MPL was not distributed with this
> >   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> >  
> > +// Header we're implementing
> 
> nit - no need for this comment

Removed.

> @@ +378,5 @@
> > +  // The limited documentation available on MSDN suggests that these
> > +  // functions will return true only if that is the button that caused
> > +  // this input event to occur, which implies that only one can be true
> > +  // at a time, and that they will only be true for PointerPressed and
> > +  // PointerReleased events.
> 
> If I remember correctly, these are only valid on PointerPressed. Might have
> been fixed since I wrote the original code. Please test to confirm your
> assumption is correct.

Thanks for pointing this out!  Investigating this uncovered that these functions in fact don't work for PointerReleased.  Additionally, it turns out that we don't receive PointerPressed/PointerReleased events for each mouse button push.  We'll probably have to move away from Pointer events for mouse input (but keep Pointer events for touch and pen) - filed bug 817061.  Updated this patch to maintain our own info about which button is pressed.
 
> @@ +945,5 @@
> > +// is never read.  This allows us to avoid the (admittedly small) overhead
> > +// of creating a new nsEventStatus every time we dispatch an event.
> > +void
> > +MetroInput::DispatchEventIgnoreStatus(nsGUIEvent *aEvent) {
> > +  mWidget->DispatchEvent(aEvent, sThrowawayStatus);
> 
> are we sure mWidget is always valid here and below?

We set mWidget in the constructor and don't destroy it until our destructor.  It's a ComPtr so I don't see a way that mWidget would become invalid.  If the mWidget can become invalid without the ComPtr becoming null, then we'll have to check for that.

> @@ +979,5 @@
> > +  aData->isChanged = false;
> > +  return PL_DHASH_NEXT;
> > +}
> > +
> > +MetroInput::~MetroInput()
> 
> Please move this up top.

Done!  But I moved some of the initialization into a private InitializeEvents() method :)

> @@ +1209,5 @@
> > +{
> > +  return sVirtualKeyMap[aKey];
> > +}
> > +
> > +MetroInput::MetroInput(MetroWidget* aWidget,
> 
> ditto, lets move this up top. it's sort of a mozilla convention.

Done, same as constructor.

> @@ +1210,5 @@
> > +  return sVirtualKeyMap[aKey];
> > +}
> > +
> > +MetroInput::MetroInput(MetroWidget* aWidget,
> > +                           UI::Core::ICoreWindow* aWindow)
> 
> nit - spacing

Fixed.



During testing I noticed that gesture events were being fired for mouse input.  Specifically, clicking while flinging the mouse to one direction caused swipe events to be generated.  I added a check in our swipe detection code to make sure that swipes aren't generated from mouse input.

Pressing and holding the left mouse button causes our context menu to appear.  I thought this was widget-related but after some debugging I'm thinking that this is happening in the front-end.  Also, strangely, middle-clicking seems to be bringing up the context menu, but only after clicking a bunch with the left and middle mouse buttons.  Matt, can you check if these are indeed front-end issues?

I tested pen input which seems to work fine, though it's currently impossible to scroll with pen input (bug 817163).

Comment 33

[Jim Mathies [:jimm]]

Comment on attachment 687318 [details] [diff] [review]
back-end patch

Let's get all this landed a file follow ups for remaining issues.

Comment 34

[Matt Brubeck (:mbrubeck)]

(In reply to Tim Abraldes (:tabraldes) from comment #32)
> Pressing and holding the left mouse button causes our context menu to
> appear.  I thought this was widget-related but after some debugging I'm
> thinking that this is happening in the front-end.  Also, strangely,
> middle-clicking seems to be bringing up the context menu, but only after
> clicking a bunch with the left and middle mouse buttons.  Matt, can you
> check if these are indeed front-end issues.

I don't think this is a front-end bug.  As far as I can tell, the front-end code displays the context menu only when it receives a "contextmenu" event from widget.

Comment 35

[Jim Mathies [:jimm]]

We're currently firing that from widget - 

http://mxr.mozilla.org/projects-central/source/elm/widget/windows/winrt/FrameworkView.cpp#494

Comment 36

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

The backend patch no longer cleanly applies.  I've resolved the merge conflicts and I'm building locally to verify that we're ready to land.  When the build completes I'll do some cursory testing and land the 4 patches in this bug on elm.

Comment 37

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Attachment: back-end patch

Rebased back-end patch.  Carrying forward r+

Comment 38

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Pushed to elm

https://hg.mozilla.org/projects/elm/rev/b36e74e1a195 [front-end]
https://hg.mozilla.org/projects/elm/rev/fc6eda083e17 [SelectionHelperUI]
https://hg.mozilla.org/projects/elm/rev/bb9170302715 ["Tap" events]
https://hg.mozilla.org/projects/elm/rev/e38fa39ccf69 [back-end]

This build contains the patches:
https://tbpl.mozilla.org/?tree=Elm&rev=e38fa39ccf69

Comment 39

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Filed a couple follow-up bugs:
Bug 817780 - Holding down left mouse button causes contextmenu
Bug 817783 - Middle-click sometimes causes contextmenu

Bug 817780 was an issue even before the patches in this bug, but bug 817783 seems to have appeared more recently (and maybe only happens on my slate?)

Comment 40

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Pushed this fix for debug build bustage:
https://hg.mozilla.org/projects/elm/rev/87f4998eaa4e

Follow the build here:
https://tbpl.mozilla.org/?tree=Elm&rev=87f4998eaa4e

Comment 41

[Matt Brubeck (:mbrubeck)]

Followup to remove debugging code that I didn't mean to land:
https://hg.mozilla.org/projects/elm/rev/856ac363ea09

Comment 42

[Matt Brubeck (:mbrubeck)]

Resolving bugs in the Firefox for Metro product that are fixed on the elm branch.  Sorry for the bugspam.  Search your email for "bugspam-elm" if you want to find and delete all of these messages at once.