1255338 - Change the way pointerlock works on e10s

Status: RESOLVED FIXED

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

Description

[Xidorn Quan [:xidorn] UTC+10]

How pointerlock work in general:
For each mouse move, if the pointer stops at a location other than the center of the window, we ask the system to move it back to the center.
If we detect that an event is triggered by a mouse move we synthesized, we suppress it.

In the current settings for e10s, all the logic above works inside the content process, which means, for each mouse move event, we would have at least two more IPC messages (one for synthesizing the mousemove, one for the additional to-be-suppressed mousemove event). This is very inefficient.

We should move the main logic of pointerlock to the parent process, so that no additional IPC happens for mousemove events. In addition, issues like bug 1244546 is hard to fix for e10s due to coordinate alignment requirement of some platforms.

Comment 1

[Jim Mathies [:jimm]]

would love to see this api cleaned up, and the tests for it enabled. this is an enhancement though so doesn't block e10s.

Comment 2

[Xidorn Quan [:xidorn] UTC+10]

Without this change, pointer lock would not work properly on HiDPI Linux with e10s. The pointer position will constantly move upward even the user doesn't touch the mouse at all. If we can live with this issue, sure, it is not a blocker.

Note that it is not a regression. Before bug 1244546 gets fixed, pointer lock is not usable at all on Linux with HiDPI.

Comment 3

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

Well, we should disable pointerlock API on e10s-linux, if this wasn't fixed. It is just so broken.
We could have done it for bug 1244546 too for non-e10s case.

Comment 4

[Xidorn Quan [:xidorn] UTC+10]

It isn't broken for e10s-linux. It is only broken for e10s Linux with HiDPI. But yeah, if this isn't very high priority, we should probably disable pointer lock for HiDPI Linux with e10s enabled. But I still think we should fix pointer lock for e10s given it is currently so inefficient...

Comment 5

[Andrew Overholt [:overholt]]

Xidorn, do you think this is something you'll get to in the next few months?

Comment 6

[Xidorn Quan [:xidorn] UTC+10]

Yes, it is currently in my Q2 plan.

Comment 8

[Xidorn Quan [:xidorn] UTC+10]

It seems to me the HiDPI issue appears on OS X as well, when in fullscreen mode.

Comment 9

[Xidorn Quan [:xidorn] UTC+10]

I used to use pointer-lock-demo for testing pointer lock, but I just realized that the demo itself is poorly coded. So I guess we don't actually have any serious performance issue on PointerLock. (If there is really any performance difference for pointer-lock-demo between e10s and non-e10s, that should be because of handling tons of rAFs, not PointerLock).

Comment 10

[Xidorn Quan [:xidorn] UTC+10]

But we may still want to fix this, both for an even better performance, and for correceness on HiDPI Linux and Mac.

Comment 11

[Edgar Chen [:edgar]]

quote from bug 853160 comment# 13:

The issue here is that we are just repeatedly trying to move the pointer back to the center of the window, but we never really restrict the pointer within the window area.

IIRC all desktop platforms provide some kind of mouse movement restriction API, for example on macOS we may use CGAssociateMouseAndMouseCursorPosition to forbid the mouse cursor from being moved at all.

Comment 12

[Adam Vandolder [:avandolder]]

Attachment: WIP: Bug 1255338 - Part 1: Use native pointer grabbing API under X11. r?edgar,#dom-core

Using this API prevents the mouse pointer from escaping from
PointerLocked windows.

Comment 13

[Adam Vandolder [:avandolder]]

Attachment: WIP: Bug 1255338 - Part 2: Use native pointer grapping API under Windows. r?edgar,#dom-core

Comment 14

[Edgar Chen [:edgar]]

Attachment: Bug 1255338 - Part 1: Remove unused argument aElement from EventStateManager::SetPointerLock;

Comment 15

[Edgar Chen [:edgar]]

Attachment: WIP: Bug 1255338 - Part 1: Remove unused member from UIEvent;

Depends on D195726

Comment 16

[Edgar Chen [:edgar]]

Attachment: WIP: Bug 1255338 - Part 1: Make PointerLockManager::SetPointerLock won't dispatch pointerlockchange event;

by factoring out PointerLockManager::DispatchPointerLockChange() from
PointerLockManager::ChangePointerLockedElement().

This is needed for subsequent patches which tries to setup pointer lock in
chrome document as well when content request pointer lock and we don't want to
trigger pointerlock change event in chrome document in this case.

And this should not change currrent behavior.

Depends on D195728

Comment 17

[Edgar Chen [:edgar]]

https://treeherder.mozilla.org/jobs?repo=try&group_state=expanded&revision=6e4c55d29c14165261b8925601476e329ebf0158

Comment 18

[Edgar Chen [:edgar]]

https://treeherder.mozilla.org/jobs?repo=try&group_state=expanded&revision=3addcd2c8f7b11814fc5bb5165a248fa98fb83ea

Comment 19

[Edgar Chen [:edgar]]

Attachment: Bug 1255338 - Part 1: Use (-1, -1) as default value of sPreLockScreenPoint; r?smaug

Comment 20

[Edgar Chen [:edgar]]

Attachment: Bug 1255338 - Part 2: Capture pointer in parent process when pointer is locked;

Comment 21

[Phabricator Automation]

Comment on attachment 9367377 [details]
WIP: Bug 1255338 - Part 1: Remove unused member from UIEvent;

Revision D195728 was moved to bug 1880187. Setting attachment 9367377 [details] to obsolete.

Comment 22

[Edgar Chen [:edgar]]

Attachment: Bug 1255338 - Part 2: Reset pointer position for pointer lock from parent process; r?smaug

Comment 23

[Edgar Chen [:edgar]]

Hmm, Wayland currently has quite different setup on Pointer Lock, I need to think about how to make this change support that as well.

Comment 24

[Edgar Chen [:edgar]]

Attachment: WIP: Bug 1255338 - Part 3: Do not recenter on every mousemove for pointer lock; r?smaug

Recentering the pointer on every mousemove causes too much overhead when it is
handled in the parent process, as it prevents mousemove events from being
compressed or coalesced.

This patch introduces an optimization that recenters the pointer only when it
moves within a 25% boundary buffer near the edge of the window, reducing the
number of recentering operations. Since each platform now uses a native API to
"lock" the pointer, we do not need to worry as much about the pointer escaping
pointer lock by moving outside the browser window at high speed.

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/830b996ebc14
https://hg.mozilla.org/integration/autoland/rev/9f2c8078e6f8
Part 1: Use (-1, -1) as default value of sPreLockScreenPoint; r=smaug
https://github.com/mozilla-firefox/firefox/commit/1381d65aaf0c
https://hg.mozilla.org/integration/autoland/rev/4017fd844045
Part 2: Reset pointer position for pointer lock from parent process; r=smaug

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/bcaab0fc91e0
https://hg.mozilla.org/integration/autoland/rev/8524bfbd843e
Revert "Bug 1255338 - Part 2: Reset pointer position for pointer lock from parent process; r=smaug" for causing bp-nu bustages at /base/Document.h

Comment 27

[Silaghi Andreea]

Backed out for causing bp-nu bustages at /base/Document.h and mochitests failures
Backout Link
Push with failures
Failure Log
mochitests
Failure line /builds/worker/checkouts/gecko/dom/base/Document.h:1254:25: error: inline function 'mozilla::dom::Document::GetPresContext' is not defined [-Werror,-Wundefined-inline]

Comment 28

[Edgar Chen [:edgar]]

https://treeherder.mozilla.org/jobs?repo=try&landoInstance=lando-prod-2025&landoCommitID=61023

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/c0b364dee242
https://hg.mozilla.org/integration/autoland/rev/d3901bc87525
Part 1: Use (-1, -1) as default value of sPreLockScreenPoint; r=smaug
https://github.com/mozilla-firefox/firefox/commit/0cafe101b0d1
https://hg.mozilla.org/integration/autoland/rev/d4f48b8b585a
Part 2: Reset pointer position for pointer lock from parent process; r=smaug

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/b228dc82abc6
https://hg.mozilla.org/integration/autoland/rev/7944f8890ca2
Revert "Bug 1255338 - Part 2: Reset pointer position for pointer lock from parent process; r=smaug" for causing wpt failures in pointerlock-maintains-mousedown.html.

Comment 31

[Serban Stanca [:SerbanS]]

Reverted this because it was causing wpt failures in pointerlock-maintains-mousedown.html.

• Revert link
• Push with failures
• Failure Log
• Failure line: TEST-UNEXPECTED-PASS | /pointerlock/pointerlock-maintains-mousedown.html?unadjustedmovement=false | Tests that mousemove events during pointer lock report correct button state - expected FAIL

Comment 32

[Edgar Chen [:edgar]]

(In reply to Serban Stanca [:SerbanS] from comment #31)

• Failure line: TEST-UNEXPECTED-PASS | /pointerlock/pointerlock-maintains-mousedown.html?unadjustedmovement=false | Tests that mousemove events during pointer lock report correct button state - expected FAIL

Oh, this patch surprisingly fixes a test.
I think the test failed because there was an initial zero-movement mousemove event right after the pointer was locked. With recentering from the parent process, we no longer generate that initial zero-movement mousemove event.

Comment 33

[Edgar Chen [:edgar]]

https://treeherder.mozilla.org/jobs?repo=try&revision=d02bf31efa2b7e4d5e3f3d756b0064e37ef59d5b&group_state=expanded

Comment 34

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/e123aa17a22f
https://hg.mozilla.org/integration/autoland/rev/a27616a299ac
Part 1: Use (-1, -1) as default value of sPreLockScreenPoint; r=smaug
https://github.com/mozilla-firefox/firefox/commit/d98f16e74d90
https://hg.mozilla.org/integration/autoland/rev/799ad36be320
Part 2: Reset pointer position for pointer lock from parent process; r=smaug

Comment 35

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/a27616a299ac
https://hg.mozilla.org/mozilla-central/rev/799ad36be320

Comment 39

[Edgar Chen [:edgar]]

Attachment: Bug 1255338 - Part 1: Use (-1, -1) as default value of sPreLockScreenPoint;

Original Revision: https://phabricator.services.mozilla.com/D200837

Comment 40

[Phabricator Automation]

firefox-release Uplift Approval Request

• User impact if declined/Reason for urgency: This makes pointer lock works better on Windows.
• Code covered by automated testing?: yes
• Fix verified in Nightly?: yes
• Needs manual QE testing?: no
• Steps to reproduce for manual QE testing: None
• Risk associated with taking this patch: medium
• Explanation of risk level: This changes how pointer lock is handled. The changes have been in Nightly for a while, have been tested by several people, and no regressions have been reported. So I don't think this would be too risky.
• String changes made/needed?: None
• Is Android affected?: yes

Comment 41

[Edgar Chen [:edgar]]

Attachment: Bug 1255338 - Part 2: Reset pointer position for pointer lock from parent process;

This is gated behind a new pref: dom.pointer-lock.reset-to-center-from-parent.enabled.
The patch preserves the original behavior as much as possible when the pref is off.
There are some changes that affect both code paths, but they are minor and
should not affect the main logic.

Here are some major change when the pref is enabled:

• Pointer recentering and computation of the center point are now handled in the parent process.
• Locking native pointer is initiated from parent process as well.
• When a remote target requests pointer lock, the parent process captures the mouse
  on the top-level browser element. This is necessary because the center of the
  chrome window may not correspond to the web page (for example, when a DevTools
  panel is open and resized to be very large).
• This patch introduces an optimization that recenters the pointer only when it
  moves within a 25% boundary buffer near the edge of the window, reducing the
  number of recentering operations. Since each platform now uses a native API to
  "lock" the pointer, we do not need to worry as much about the pointer escaping
  pointer lock by moving outside the browser window at high speed.

Original Revision: https://phabricator.services.mozilla.com/D296321

Comment 42

[Pascal Chevrel (relman team) -> :pascalc]

Edgar, this is a fix for a 10 year old bug marked as P3/S3 and there is some risk of regression, is there a specific business need to have it uplifted to production now instead of letting it ship with 154 mid-August?

Comment 43

[Pulsebot]

https://github.com/mozilla-firefox/firefox/commit/7d1d08edaa56
https://hg.mozilla.org/releases/mozilla-release/rev/3f4eedd4c61b
https://github.com/mozilla-firefox/firefox/commit/b77c477f461f
https://hg.mozilla.org/releases/mozilla-release/rev/44d12294c895