782729 - Pointer lock does not work on FPS "PlayCanvas" demo

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Alon Zakai (:azakai)]

http://apps.playcanvas.com/playcanvas/scifi/latest

is an FPS demo. Press space to go to fullscreen. In that mode, the mouse has no effect on Linux, but people tell me it works on other platforms, so seems platform-specific.

Comment 1

[Alon Zakai (:azakai)]

Note that the mouse does seem to work while the permission dialog is shown.

Comment 2

[Chris Pearce [:cpearce (Not reading bugmail)]]

Are you running two monitors? And/or nvidia twin view?

Comment 3

[Alon Zakai (:azakai)]

Nothing fancy on my system, just a macbook running linux. No external monitors.

I have ATI and their binary drivers enabled.

bjacob said he also saw this bug, not sure what his setup is.

Comment 4

[Benoit Jacob [:bjacob] (mostly away)]

I have the same issue on a Thinkpad W520, Ubuntu 12.04 64bit, NVIDIA.  No external monitor or pointing device.

Comment 5

[Chris Pearce [:cpearce (Not reading bugmail)]]

Nightly 2012-06-21-05-30-48 pointer lock doesn't work on either my primary or secondary monitor.
Nightly 2012-06-20-06-51-38 pointer locks works on my secondary monitor but not on my primary monitor.

Therefore I assume the regression range is:

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=c3190d715044&tochange=10e019421e6b

It seems it's a safe assumption that regressor is these changesets:

Wed Jun 20 16:38:48 2012 -0700
d047caf12f04 Chris Pearce — Bug 756936 - Minor tidy ups to pointer lock code. r=smaug
4be7471e7a95 Chris Pearce — Bug 756936 - Ensure MouseEvent.mozMovement{X,Y} values are correct on secondary monitors. r=smaug

Comment 6

[Martin Best (:mbest)]

It would be good to have this for fx16.

Comment 7

[Martin Best (:mbest)]

We are not sure on 15 and 16 status.  The regression seems in range of 16 so it seems likely that it's affected.

Comment 8

[Lukas Blakk [:lsblakk] use ?needinfo]

We're already about to build our final RC for FF15, so not tracking for that.  Will track for 16, since this is a regression.

Comment 9

[Alex Keybl [:akeybl]]

Chris - can you investigate and suggest a backout, fix, or wontfix for FF16? This affects the bananabread demo.

Comment 10

[Chris Pearce [:cpearce (Not reading bugmail)]]

We should backout Bug 756936 on beta, and fix this bug on Aurora.

Bug 756936 fixes pointer lock in a few edge cases, notably when there is multiple monitors on Windows. AFAICT there's been no-one complaining about the issues Bug 756936 fixes, so I think we can live without Bug 756936's changes in release for another six weeks.

Comment 11

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Backout bug 756936 on beta for Firefox 16

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 756936
User impact if declined: Pointer lock won't work on Linux. However with this patch pointer lock won't work on secondary monitors (on Windows and Linux, maybe Mac too), but we had this behaviour in FF14 and no one complained (bug 756936 shipped in FF15, fixing pointer lock on secondary monitors but breaking linux pointer lock).
Testing completed (on m-c, etc.): mochitests pass locally. They passed before bug 756936 landed, so once we back it out they should still pass.
Risk to taking this patch (and alternatives if risky): Low, since this is a backout...
String or UUID changes made by this patch:  None.

Comment 12

[Alon Zakai (:azakai)]

(In reply to Alex Keybl [:akeybl] from comment #9)
> Chris - can you investigate and suggest a backout, fix, or wontfix for FF16?
> This affects the bananabread demo.

Hmm, I doublechecked and actually it looks like this does *not* affect other demos than the one reported here. BananaBread is ok, as is http://media.tojicode.com/q3bsp/ , I checked on 16, 17 and 18.

So the problem appears specific to the playcanvas demo for some unknown reason.

Given that, I would recommend not making any changes to aurora or beta. But it would be good to find out why that demo does not work on nightly even though others do (and since the demo does work in other browsers).

Comment 13

[Chris Pearce [:cpearce (Not reading bugmail)]]

Hmmm, indeed my pointer lock demo at http://pearce.org.nz/fullscreen/pointerlock-position.html works ok in Linux, but *only* when I don't have a secondary monitor active. I'll cancel the approval request until we can figure out what's going on here...

Comment 14

[Chris Pearce [:cpearce (Not reading bugmail)]]

I will fix the issue of pointer lock not working on Linux with multiple displays in bug 791121, and we'll leave this one for tracking down why http://apps.playcanvas.com/playcanvas/scifi/latest doesn't work with pointer lock on Linux.

Comment 15

[Chris Pearce [:cpearce (Not reading bugmail)]]

This is not Linux specific, PlayCanvas also doesn't work for me in [Mozilla/5.0 (Windows NT 6.1; WOW64; rv:18.0) Gecko/18.0 Firefox/18.0].

Please disregard the regression range I reported in comment 5, that's a regression range for the problem with Linux and pointer lock with multiple monitors. I misunderstood the bug report here. Sorry about that.

Comment 16

[Chris Pearce [:cpearce (Not reading bugmail)]]

I take it back. After re-doing a regression range search on Windows using the PlayCanvas demo as a test case I have discovered that this bug has the same regression range I reported in comment 5, i.e. the regression range is 

https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=c3190d715044&tochange=10e019421e6b

Thus it's probably a regression from Bug 756936.

Comment 17

[Chris Pearce [:cpearce (Not reading bugmail)]]

Confirmed this with PlayCanvas on Linux too. It's definitely a regression from Bug 756936's 4be7471e7a95 changeset.

Comment 18

[Alex Keybl [:akeybl]]

Since this only affects a single demo (as opposed to pointer lock on Linux in general), we're no longer going to track for release.

Comment 19

[Chris Pearce [:cpearce (Not reading bugmail)]]

The problem with the PlayCanvas demo was caused by removing this if branch in 4be7471e7a95:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4be7471e7a95#l1.65

With that if-branch resurrected, plus all calls to SynthesizeNativeMouseMove in screen coords, and with Windows' SynthesizeNativeMouseMove implementation altered to accept screen coords, pointer lock appears to work properly; the PlayCanvas demo works fine, on multiple monitors, and bug 782777 also seems to be fixed. Patches to follow...

Comment 20

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch 1 v1: Pass device pixels to nsIWidget::SynthesizeNativeMouseMove()

I was wrong in changeset 4be7471e7a95 to switch to passing widget coords to nsIWidget::SynthesizeNativeMouseMove(). We should pass in screen coords. All the nsIWidget::Synthesize* methods are documented that they take screen coords, and the widget backends expect screen coords (except for Windows, which is a bug that I'm going to fix in the next patch). See Bug 791121 comment 2 for more info.

With this patch pointer lock now behaves correctly on non-Windows platforms with multiple monitors. This will break Windows pointer lock on secondary monitors without the next patch.

Comment 21

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch 2 v1: Resurrect pointer lock refPoint restore on centering

In changeset 4be7471e7a95 I removed this branch:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4be7471e7a95#l1.65

I removed it because I was never able to observe it having any affect. Guess I was wrong.

Removing this branch caused the PlayCanvas demo to break.

Comment 22

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch 3 v1: Make Windows nsWindow::SynthesizeNativeMouseEvent accept coords in screen coords, not widget coords

The other widget backends' implementation of SynthesizeNativeMouseMove() accept a coord in device pixels, but the Windows implementation is accepting it in widget coords (relative to the window's top left). All implementations should accept coords in the same coordinate system, otherwise cross platform code will be wrong.

Comment 23

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 662500 [details] [diff] [review]
Patch 2 v1: Resurrect pointer lock refPoint restore on centering

Trying to figure out the reason for this...

Comment 24

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 662502 [details] [diff] [review]
Patch 3 v1: Make Windows nsWindow::SynthesizeNativeMouseEvent accept coords in screen coords, not widget coords

This needs a review from Windows widget peer.

Comment 25

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 662500 [details] [diff] [review]
Patch 2 v1: Resurrect pointer lock refPoint restore on centering

Hmm, shouldn't we actually check that if
refPoint == sLastRefPoint and don't dispatch anything in that case.
We do want to dispatch 0,0 movement when mouse is back to center, right?

Re-ask review if I'm wrong here :)
The code needs some comments here.

Comment 26

[Jim Mathies [:jimm]]

Comment on attachment 662502 [details] [diff] [review]
Patch 3 v1: Make Windows nsWindow::SynthesizeNativeMouseEvent accept coords in screen coords, not widget coords

This has been this way since it landed in 09, so please run through try to confirm this doesn't break any tests.

Comment 27

[Chris Pearce [:cpearce (Not reading bugmail)]]

Green on Try, modulo random orange:
https://tbpl.mozilla.org/?tree=Try&rev=93b00a03d636

Comment 28

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch 2 v2: Cancel synth centering pointer moves, comment code.

The previous "refPoint restore" code which I was resurrecting in my previous patch was confusing, so I changed the code to do what I *think* it meant to do, and what makes sense to me, and added I comments to explain how it works.

I explicitly look out for and cancel the "synthetic native" mouse event that we dispatch to re-center the pointer, and this fixes the PlayCanvas demo, and bug 782777 as well.

Explicitly tracking when we should cancel the mouse event also prevents us from cancelling mouse events that are targeted at the center of the window and which follow a pointer-lock re-centering mouse move (earlier versions of *this* patch had mochitests failures because the pointerlock mochitests dispatch synthetic mouse moves to the center of the window, I don't know if existing pointer lock test orange is/was caused by this).

Greenish: https://tbpl.mozilla.org/?tree=Try&rev=a2eed6362fc4

Comment 29

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 663914 [details] [diff] [review]
Patch 2 v2: Cancel synth centering pointer moves, comment code.

Why do we set sSynthCenteringPoint = center; after dispatching the
synth event?

Comment 30

[Olli Pettay [:smaug][[e-mail]]]

...and not before.

Comment 31

[Chris Pearce [:cpearce (Not reading bugmail)]]

So we know to expect a synthetic event, so that we can cancel the next event (which is dispatched asynchronously based on my testing). It can be set before dispatching I imagine.

Some of the mochitests were failing because they also dispatch synthetic events to the center of the window, and we were cancelling them.

Comment 32

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch 2 v3: Cancel synth centering pointer moves, comment code.

Setting sSynthCenteringPoint before the synthetic centering move dispatch.

Comment 33

[Chris Pearce [:cpearce (Not reading bugmail)]]

Thanks for reviews!

https://hg.mozilla.org/integration/mozilla-inbound/rev/74a9ba7336cf
https://hg.mozilla.org/integration/mozilla-inbound/rev/1f87f13f8da4
https://hg.mozilla.org/integration/mozilla-inbound/rev/a88b3cd6f28d

Comment 34

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/74a9ba7336cf
https://hg.mozilla.org/mozilla-central/rev/1f87f13f8da4
https://hg.mozilla.org/mozilla-central/rev/a88b3cd6f28d

Comment 35

[Paul Silaghi, QA [:pauly]]

Verified fixed 32.0a1 (2014-05-18), Ubuntu 13.04