On my Windows7 x64 machine, when you lock the pointer with the browser window fullscreen on my secondary monitor, the mozMovement{X,Y} values in the generated MouseEvent events are stuck at a constant value. It doesn't seem to matter where I position my secondary monitor WRT my primary (i.e. above, below, left, right, etc). It's just pointer lock requests in the secondary monitor seem to have fairly constant mozMovement{X,Y} values. I'm not sure if this bug exists on non-windows platforms. Testing on Mozilla/5.0 (Windows NT 6.1; WOW64; rv:15.0) Gecko/15.0 Firefox/15.0a1 STR: 1. Configure your system to have a secondary monitor. 2. Open http://pearce.org.nz/fullscreen in Firefox Nightly. 3. With the firefox window on your primary display, press the 'p' key, or click "Fullscreen with pointer lock" button. 4. Move the mouse. Observe the "Pointer: delta=(*, *) screen=(*, *) client=(*, *)" text being updated as MouseEvents are dispatched. 5. Press ESC to exit fullscreen, move the Firefox window to your secondary monitor, press 'p', or click "Fullscreen with pointer lock" button. 4. Observe that the "Pointer: ..." text doesn't update the same as when pointer locked on the primary monitor. The "delta" values (the mozMovement{X,Y} values) are fairly constant.

On my linux box when I lock the pointer on my secondary display (following the STR in comment0) the mouse jumps to appear as visible on my primary, but remains hidden on my secondary display, and I still get incoherent mozMovement{X,Y} values.

Also note that the mozMovement{X,Y} values are coherent when in fullscreen move on the secondary monitor, it's only once the pointer is locked that they start reporting bogus values.

So when I change nsIntPoint nsEventStateManager::GetMouseCoords(nsIntRect aBounds) to just return (innerWidth/2, innerHeight/2), I get what looks like correct behaviour, and the mochitests still pass... Note aBounds has negative values in some fields in the secondary monitor case, whereas in the primary monitor case they're all >=0. Humph: Why do you need to pass the bounds into this function? And why is innerHeight used in the calculation but innerWidth isn't? It looks like SynthesizeNativeMouseEvent takes coords relative to the window/widget's top-left corner, rather than in system pixel coords, at least on windows it looks like it does: http://mxr.mozilla.org/mozilla-central/source/widget/windows/nsWindow.cpp#5716 Should nsIntPoint nsEventStateManager::GetMouseCoords() just be returning (innerWidth/2, innerHeight/2)?

The problem in brief is that we're using the widget's bounds in screen coords to calculate the mouse position and movement deltas, but the mouse/dom events and widget synthetic dispatch always assume the coords are in widget coords (i.e. offset from the widget origin, not the primary screen's origin). Specifically, nsEventStateManager::GetMouseCoords(bounds) is being used to calulate the center of the window containing the mouse event, but GetMouseCoords() is passed screenBounds (bounds in screen coords) and using that to seed its calculations causing the calculated mouse position to be in screen coords. We're dispatching synthetic mouse events to the screen coords, but the synthetic dispatch expects widget coords, causing the problem. Changes in this patch: * Change the window center calculation to return an offset relative to the widget's origin. This fixes the base problem. * Remove the "if (aEvent->refPoint == aEvent->lastRefPoint) aEvent->refPoint = sLastRefPoint;" case in nsEventStateManager::GenerateMouseEnterExit. In my testing this branch was only taken when aEvent->refPoint == sLastRefPoint, so it has no effect. * Save the mouse position in widget coords before going into pointer lock in nsEventStateManager::mPreLockPoint (previously it was saved in screen coords). * Synthesize mouse move to nsEventStateManager::mPreLockPoint when exiting, so mouse returns to same position upon pointer lock exit (previously mPreLockPoint was write only, and sLastScreenPoint was used for this purpose, except sLastScreenPoint is in screen coords, so it would behave incorrectly on secondary monitors). * Set nsEventStateManager::sLastRefPoint before entering/exiting pointer lock so that the synthetic mouse move fired when entering/exiting doesn't report movement as ((mouse position before lock) - (center of window)) as it did previously. (If that's not learn, play with http://pearce.org.nz/fullscreen/pointerlock-position.html and you should see what I mean)

While figuring out how pointer lock worked I added comments, removed some unused code, and simplified nsDOMUIEvent::GetMovementPoint(). This should make it easier to understand the pointer lock code in future. Note the nsDOMUIEvent::GetMovementPoint() code was converting from widget to screen to app units then back to pixels and then diffing, whereas we can instead simply just subtract the event's refPoint from the previous to get the delta. Much simpler this way.

Comment on attachment 633389 [details] [diff] [review] Patch 1 v1: Coords relative to widget, not screen. >- aEvent->widget->SynthesizeNativeMouseMove(aEvent->lastRefPoint); >+ nsIntPoint center = GetWidgetCenter(aEvent->widget); >+ aEvent->lastRefPoint = center; >+ // If this mouse move doesn't finish at the center of the widget, >+ // dispatch a synthetic mouse move to return the mouse back to the >+ // center. >+ if (aEvent->refPoint != center) { >+ aEvent->widget->SynthesizeNativeMouseMove(center); > } Why do we want to center in widget, and not in the web page? If browser chrome takes lots of space, the center can be over chrome.

Ooops, some of the changes from Patch 2 made it into this one. The new patch finds the center of the inner content area, rather than the center of the outer window/widget.

Rebased on new patch 1.

Comment on attachment 634256 [details] [diff] [review] Patch 1 v3: Coords relative to widget, not screen. >+// Returns the center point of the window's inner content area. >+// This is in widget coordinates, i.e. relative to the widget's top >+// left corner, not in screen coordinates. >+static nsIntPoint >+GetWindowInnerRectCenter(nsPIDOMWindow* aWindow, >+ nsIWidget* aWidget, >+ nsPresContext* aContext) >+{ >+ NS_ENSURE_TRUE(aWindow != nsnull, nsIntPoint(0,0)); >+ NS_ENSURE_TRUE(aWidget != nsnull, nsIntPoint(0,0)); >+ NS_ENSURE_TRUE(aContext != nsnull, nsIntPoint(0,0)); NS_ENSURE_TRUE(aWindow && aWidget && aContext, nsIntPoint(0,0));

(In reply to Chris Pearce (:cpearce) from comment #5) > Note the nsDOMUIEvent::GetMovementPoint() code was converting from widget to > screen to app units then back to pixels and then diffing, whereas we can > instead simply just subtract the event's refPoint from the previous to get > the delta. Much simpler this way. I don't understand this. The result should be in CSS pixels. With your patch, what guarantees that? (Perhaps I'm just missing something.)

Not needed for b2g.

(In reply to Olli Pettay [:smaug] from comment #11) > (In reply to Chris Pearce (:cpearce) from comment #5) > > Note the nsDOMUIEvent::GetMovementPoint() code was converting from widget to > > screen to app units then back to pixels and then diffing, whereas we can > > instead simply just subtract the event's refPoint from the previous to get > > the delta. Much simpler this way. > I don't understand this. The result should be in CSS pixels. With your > patch, what guarantees that? > (Perhaps I'm just missing something.) Ah yes, my mistake. You're correct, I was converting to dev pixels, not CSS pixels. I'll fix that. The change I'm trying to make here is to remove the unnecessary conversion from widget offset to screen coords. Let me update the patch again...

Since it's tagged as a games P2, we are not going to track this.

https://hg.mozilla.org/integration/mozilla-inbound/rev/4be7471e7a95 https://hg.mozilla.org/integration/mozilla-inbound/rev/d047caf12f04

https://hg.mozilla.org/mozilla-central/rev/4be7471e7a95 https://hg.mozilla.org/mozilla-central/rev/d047caf12f04

Can we are get this on Aurora (fx15)?

Comment on attachment 634256 [details] [diff] [review] Patch 1 v3: Coords relative to widget, not screen. [Approval Request Comment] Bug caused by (feature/regressing bug #): Mouse/pointer lock (bug 633602) User impact if declined: Pointer lock reports incorrect mouse movement values when the mouse is locked on a secondary monitor. Games etc won't work. FF15 is our big bang HTML5 Games Are Awesome Release, so it would be good to get this in FF15. Testing completed (on m-c, etc.): Local testing, and it's been on m-c for weeks. Risk to taking this patch (and alternatives if risky): Pretty low risk. String or UUID changes made by this patch: None.

Comment on attachment 634256 [details] [diff] [review] Patch 1 v3: Coords relative to widget, not screen. [Triage Comment] Low risk, and in support of a big bang :). Approved for Aurora 15.

Target Milestone is for m-c.

This is fixed in mozilla 15, so why shouldn't the target milestone be 15? Or is target milestone supposed to be for the release in which it first was pushed to m-c?

(In reply to Chris Pearce (:cpearce) from comment #23) > Or is target milestone supposed to be for the release in which it first was > pushed to m-c? Yes.