Add unadjustedMovement option to requestPointerLock #17525
Conversation
james-howard
requested review from
cdumez,
rniwa and
JonWBedard
as code owners
|
EWS run on previous version of this PR (hash 2974cbe) |
webkit-ews-buildbot
added
the
merging-blocked
james-howard
force-pushed
the
james-howard/PointerLockUnadjustedMovement
branch
from
2974cbe to
5a97860
Compare
|
EWS run on previous version of this PR (hash 5a97860) |
There was a problem hiding this comment.
Could this be a web platform test? Are there web platform tests for this feature?
There was a problem hiding this comment.
There aren't yet. The existing web platform tests for pointerlock in WebKit match what's in https://github.com/web-platform-tests/wpt/tree/master/pointerlock, and I did verify that they all still pass with my code changes.
I think the order of operations could go something like this:
- This PR gets merged and PointerLockOptions becomes an experimental feature in WebKit.
- Add lock options to requestPointerLock w3c/pointerlock#49 gets merged. The reason it hasn't been is because it only has an implementation in Chromium and nowhere else so far. I'm trying to solve that with this PR and unwedge it.
- Either I or someone from Chromium can propose a new web platform test that tests the unadjustedMovement option as well as the Promise return for requestPointerLock.
- Import the new WPT to WebKit and verify it passes.
- Mark PointerLockOptions as stable in WebKit.
There was a problem hiding this comment.
I agree that this should be a web platform test. Having a web platform test is also a prerequisite for having that PR merged (speaking as Chair of the WebApps WG).
There was a problem hiding this comment.
(not that it doesn't need to be sent to WPT first... you can add it here, and the we can upstream it to WPT)
There was a problem hiding this comment.
Let's aim to just make this a WPT that can be upstreamed later?
There was a problem hiding this comment.
When you say make it a WPT, do you mean leave it in LayoutTests/pointer-lock directory but write it against testharness.js instead of js-test-pre.js/pointer-lock-test-harness.js?
There was a problem hiding this comment.
I imagined (a) writing against testharness.js and (b) placing the test under LayoutTests/http/wpt/pointer-lock/
There was a problem hiding this comment.
OK, I added 3 new tests under LayoutTests/http/pointerlock.
@aprotyas @marcoscaceres please take a look and see if these match what you had in mind.
There was a problem hiding this comment.
Sorry, they should actually go into LayoutTests/imported/w3c/web-platform-tests/pointerlock/
There was a problem hiding this comment.
(oops, my apologies for the wrong info)
james-howard
force-pushed
the
james-howard/PointerLockUnadjustedMovement
branch
from
5a97860 to
c805f97
Compare
james-howard
requested review from
donny-dont,
zdobersek and
a team
as code owners
|
EWS run on previous version of this PR (hash c805f97) |
Ahmad-S792
added
the
UI Events
|
Going to try to satisfy all the bots today. Looks like there were a few tests (http/tests/pointer-lock) that didn't get run locally by The GTK bots seem like they're broken due to an infrastructure error. Hopefully after my next push they wake up because I need them to check my changes to the WebMouseEvent constructor which has a bunch of platform specific callers that I can't exercise locally. |
james-howard
force-pushed
the
james-howard/PointerLockUnadjustedMovement
branch
from
c805f97 to
b2a6a1e
Compare
|
EWS run on previous version of this PR (hash b2a6a1e) |
james-howard
force-pushed
the
james-howard/PointerLockUnadjustedMovement
branch
from
b2a6a1e to
f52100b
Compare
|
EWS run on previous version of this PR (hash f52100b) |
LayoutTests/fast/js-promise/js-promise-invalid-context-access.html
Outdated
Show resolved
Hide resolved
james-howard
force-pushed
the
james-howard/PointerLockUnadjustedMovement
branch
from
f52100b to
af693ab
Compare
|
I reworked the WebMouseEvent constructor change to require fewer modifications to unrelated files around the codebase. Hopefully that gets things building for GTK. |
|
EWS run on previous version of this PR (hash af693ab) |
There was a problem hiding this comment.
Thanks for the PR! This looks promising overall. I left a few comments, mostly nits.
I reckon I have two main suggestions:
- Consider holding an optional
PointerLockOptionsinstance inPointerLockController, rather than an individual boolean flag. - Consider making pointer-lock/pointer-lock-option-unadjusted-movement.html a WPT that we can later upstream.
There was a problem hiding this comment.
Can you break this up into multiple statements?
https://webkit.org/code-style-guidelines/#linebreaking-chained-assignments
There was a problem hiding this comment.
@james-howard I think you missed this one
There was a problem hiding this comment.
@james-howard I think this PR is almost there! I replied to some of the comments, but I'm not sure how to address the GTK build failure. I suspect we need GTK reviewers to opine here, but since webkit_dom_element_request_pointer_lock is deprecated and recommends use of the JS API instead, we could work around the build fix like this.
// WebCore::Element
void requestPointerLockSync()
{
if (document().page())
document().page()->pointerLockController().requestPointerLock(this);
}
// WebKitDOMElementGTK
void webkit_dom_element_request_pointer_lock(WebKitDOMElement* self)
{
#if ENABLE(POINTER_LOCK)
WebCore::JSMainThreadNullState state;
g_return_if_fail(WEBKIT_DOM_IS_ELEMENT(self));
WebCore::Element* item = WebKit::core(self);
item->requestPointerLockSync();
#else
UNUSED_PARAM(self);
WEBKIT_WARN_FEATURE_NOT_PRESENT("Pointer Lock")
#endif /* ENABLE(POINTER_LOCK) */
}Also, I have opened a standards position issue here: WebKit/standards-positions#254. Once this PR is good to land, we will wait on an official stance there.
There was a problem hiding this comment.
Can probably use shouldBeEqualToString here:
| shouldBe("error.name", "'WrongDocumentError'"); | |
| shouldBeEqualToString("error.name", "WrongDocumentError"); |
There was a problem hiding this comment.
I ended up having to change it a bit anyway to deal with a behavior difference between WK1 and WK2. I didn't want the test to test implementation differences that aren't covered by the spec, and I didn't really want to have separate test expectations for WK1 and WK2, so I changed the test like so:
const promise = targetDiv.requestPointerLock();
targetDiv.remove();
try {
await promise;
// depending on the implementation, requestPointerLock may be satisfied synchronously (WK1).
// This is OK, but now at this point the pointer should be unlocked.
} catch (error) {
// If an error does arise due to async pending pointer lock, it should be WrongDocumentError
// because the element lost its document that needed to be active and focused.
if (error.name != "WrongDocumentError")
testFailed(`Promise rejected with unexpected error ${error}`);
}
shouldBeNull("document.pointerLockElement");
finishJSTest();The reason I used if (error.name ... instead of shouldBeEqualToString was to avoid putting a line in the output that is conditional on WK2 vs WK1.
Alternative solutions I can see are:
- Make WK1 tests behave like WK2 by changing TestController's requestPointerLock to be:
static void requestPointerLock(WKPageRef page, const void*)
{
RunLoop::main().dispatch([=] {
WKPageDidAllowPointerLock(page);
});
}-
Define different test expectations for WK1 vs WK2.
-
Punt on this test entirely.
There was a problem hiding this comment.
Would it be safe to skip WK1? Or maybe yeah, option 1? @smfr?
There was a problem hiding this comment.
Another option is in Chromium, they have (edit: had?) a testRunner.setPointerLockWillRespondAsynchronously() method they can call. You can see the vestige of this in this test that's disabled in WebKit. I actually implemented that locally in WebKit when I was writing this test, but abandoned it when I found out I could get by without it. I could bring it back if you think that's the best solution here, though.
james-howard
force-pushed
the
james-howard/PointerLockUnadjustedMovement
branch
from
5d5b68d to
678ff3c
Compare
|
EWS run on previous version of this PR (hash 678ff3c) |
|
Thanks @marcoscaceres. I appreciate all your help on this patch as well as your work in getting the spec updates moving again.
My intent is to have acted on all open topics where the action was clear or else I'm waiting for help or clarification. But the PR has gotten quite big and I wouldn't be surprised if I missed something. Below are links to the open items I'm aware of. If there's something I've missed that you know about, please help me out with a link directly to the item. @smfr asked for IntPoint => IntSize. I wasn't sure if that was the right change or, if it is, if it should be tackled in a separate patch. I'm also happy to just do it if that helps move things along, I have no strong opinion on this point (heh). You asked for use of transient activation instead of processing user gesture to let us use promise_test instead of async_test in LayoutTests/imported/w3c/web-platform-tests/pointerlock/pointerlock_unadjustedMovement.html. But I was hoping to do that as a separate CL because there are security implications. You asked to cleanup LayoutTests/pointer-lock/pending-locked-element-removed-from-dom.html test runner behavior between WebKit 1 and WebKit 2 for whether requestPointerLock is synchronous or asynchronous. But I'm still not sure what the best approach is there.
I'm adding 3 new web platform tests that I want to upstream after the spec changes land:
I think you might be referring to the request from @aprotyas here, but that request was made before any of the WPTs were added. I left pointer-lock-option-unadjusted-movement.html also as a non-WPT test as well because I wanted to make sure that we had negative test coverage -- that is, I wanted a test that could still verify the old behavior of requestPointerLock when !PointerLockOptionsEnabled. |
aprotyas
dismissed
their stale review
Dismissing my review as I'm not a WebKit reviewer, but this PR is ready to land.
xingri
force-pushed
the
james-howard/PointerLockUnadjustedMovement
branch
from
678ff3c to
9a199c0
Compare
|
EWS run on previous version of this PR (hash 9a199c0) |
xingri
force-pushed
the
james-howard/PointerLockUnadjustedMovement
branch
from
9a199c0 to
2a48b6c
Compare
|
EWS run on previous version of this PR (hash 2a48b6c) |
|
@marcoscaceres Thanks for the review and approval. |
james-howard
force-pushed
the
james-howard/PointerLockUnadjustedMovement
branch
from
2a48b6c to
2e34bfa
Compare
|
EWS run on previous version of this PR (hash 2e34bfa) |
There was a problem hiding this comment.
Generally we use IntSize to represent a delta (or vector), and IntPoint to represent a geometric point. But OK to follow the other data members for now.
|
@james-howard no, there isn't, but you're not a committer so the |
james-howard
force-pushed
the
james-howard/PointerLockUnadjustedMovement
branch
from
2e34bfa to
2819e73
Compare
|
EWS run on previous version of this PR (hash 2819e73) |
🧪 api-ios
There was a problem hiding this comment.
nit: an early return if there is not lock pending would decrease indentation. Same in WebPageProxy::didDenyPointerLock
There was a problem hiding this comment.
ChromeClient isn't a great place for this because it isn't actually getting information from the client. It could just be a static function instead that returns different values based on HAVE(MOUSE_UNACCELERATED_MOVEMENT)
There was a problem hiding this comment.
Moved to a static method on PointerLockController: supportsUnadjustedMovement
james-howard
force-pushed
the
james-howard/PointerLockUnadjustedMovement
branch
from
2819e73 to
7848f89
Compare
|
EWS run on current version of this PR (hash 7848f89) |
https://bugs.webkit.org/show_bug.cgi?id=218054 Reviewed by Simon Fraser and Abrar Rahman Protyasha. This change implements the proposed modifications to the pointerlock spec described in w3c/pointerlock#49. The new behavior is controlled by the Runtime Flag PointerLockOptionsEnabled. When the flag is set, all WebKit platforms receive the ability to return a promise from requestPointerLock, which can be rejected with DOMExceptions describing why pointer lock couldn't be obtained. On macOS only, all mouse move events are annotated with unaccelerated movement, which is readily available on NSEvent (since macOS 10.15). When unadjustedMovement is requested via PointerLockOptions (and PointerLockOptionsEnabled is set), the unaccelerated values are provided in the movementX/Y attributes on MouseEvents. On other platforms, the promise returned from requestPointerLock can be used to learn that unadjustedMovement is not implemented for them (again, when PointerLockOptionsEnabled is set). When PointerLockOptionsEnabled is not set, requestPointerLock behaves as before: it does not return a promise and any options passed are ignored, meaning accelerated mouse motion will always be provided. * LayoutTests/fast/js-promise/js-promise-invalid-context-access.html: * LayoutTests/http/tests/pointer-lock/iframe-sandboxed-nested-disallow-then-allow-pointer-lock.html: * LayoutTests/http/tests/pointer-lock/iframe-sandboxed.html: * LayoutTests/http/tests/pointer-lock/requestPointerLock-can-not-transfer-between-documents.html: * LayoutTests/pointer-lock/lock-element-not-in-dom.html: * LayoutTests/pointer-lock/locked-element-removed-from-dom.html: Handle promise rejection if returned from requestPointerLock. * LayoutTests/platform/gtk/imported/w3c/web-platform-tests/pointerlock/pointerlock_unadjustedMovement-expected.txt: unadjustedMovement: true is unsupported so far on GTK, add expectation for that failure. * LayoutTests/platform/ios/TestExpectations: Add link to webkit.org/b/216621 for missing pointer lock support on iOS. * LayoutTests/platform/mac/pointer-lock/pointer-lock-option-unadjusted-movement-expected.txt: Mac platform supports unadjustedMovement and so its test output acknowledges this. * LayoutTests/platform/wpe/TestExpectations: Skip new wpt pointerlock tests on wpe because wpe does not support pointer lock at all (webkit.org/b/204001) * LayoutTests/pointer-lock/pointer-lock-option-unadjusted-movement-expected.txt: Added. * LayoutTests/pointer-lock/pointer-lock-option-unadjusted-movement.html: Added. Added tests for the unadjustedMovement option and the promise return value. * LayoutTests/pointer-lock/pending-locked-element-removed-from-dom-expected.txt: Added. * LayoutTests/pointer-lock/pending-locked-element-removed-from-dom.html: Added. Added test for the promise return value in the specific case of an element being removed while pointer lock is pending. * LayoutTests/imported/w3c/web-platform-tests/pointerlock/pointerlock_promise-expected.txt: Added. * LayoutTests/imported/w3c/web-platform-tests/pointerlock/pointerlock_promise.html: Added. WPT test that verifies requestPointerLock returns a promise. * LayoutTests/imported/w3c/web-platform-tests/pointerlock/pointerlock_unadjustedMovement-expected.txt: Added. * LayoutTests/imported/w3c/web-platform-tests/pointerlock/pointerlock_unadjustedMovement.html: Added. WPT test that verifies that unadjustedMovement option is handled according to the spec. * LayoutTests/imported/w3c/web-platform-tests/pointerlock/pointerlock_without_activation-expected.txt: Added. * LayoutTests/imported/w3c/web-platform-tests/pointerlock/pointerlock_without_activation.html: Added. WPT test that verifies that if requestPointerLock is called without user gesture that the returned promise is rejected with NotAllowedError. * Source/WTF/Scripts/Preferences/UnifiedWebPreferences.yaml: Added PointerLockOptionsEnabled * Source/WTF/wtf/PlatformHave.h: Added HAVE_MOUSE_UNACCELERATED_MOVEMENT for Mac * Source/WebCore/CMakeLists.txt: Added JSPointerLockOptions.cpp * Source/WebCore/DerivedSources-input.xcfilelist: Added PointerLockOptions.idl * Source/WebCore/DerivedSources-output.xcfilelist: Added JSPointerLockOptions.cpp * Source/WebCore/DerivedSources.make: Added JSPointerLockOptions.cpp * Source/WebCore/Headers.cmake: Added PointerLockOptions.h * Source/WebCore/Sources.txt: Added JSPointerLockOptions.cpp * Source/WebCore/WebCore.xcodeproj/project.pbxproj: * Source/WebCore/dom/Element+PointerLock.idl: Add PointerLockOptions and promise. * Source/WebCore/dom/PointerLockOptions.h: Added. * Source/WebCore/dom/PointerLockOptions.idl: Added. * Source/WebCore/dom/Element.cpp: (WebCore::Element::requestPointerLock): Pass options and return promise from requestPointerLock if PointerLockOptionsEnabled runtime flag is set. * Source/WebCore/dom/Element.h: Update requestPointerLock signature * Source/WebCore/page/PointerLockController.cpp: (WebCore::UnadjustedMovementPlatformMouseEvent::UnadjustedMovementPlatformMouseEvent): Added utility class to set PlatformMouseEvent movement to unadjusted movement. (WebCore::PointerLockController::~PointerLockController): Added empty destructor because DeferredPromise is a forward declaration in the header. (WebCore::PointerLockController::requestPointerLock): Resolve or reject promise with appropriate DOMException. (WebCore::PointerLockController::didAcquirePointerLock): Resolve promise if needed. (WebCore::PointerLockController::didNotAcquirePointerLock): Reject promise if needed (e.g. missing focus). (WebCore::PointerLockController::dispatchLockedMouseEvent): Send unadjusted movement delta if requested. (WebCore::PointerLockController::clearElement): Clear promise if saved (WebCore::PointerLockController::supportsUnadjustedMovement): Returns whether the unadjustedMovement option can be enabled. requestPointerLock needs to reject its promise if unadjustedMovement is true but the browser doesn't support that mode. * Source/WebCore/page/PointerLockController.h: Update requestPointerLock signature. Add supportsUnadjustedMovement static method. * Source/WebCore/platform/PlatformMouseEvent.h: (WebCore::PlatformMouseEvent::unadjustedMovementDelta const): Add accessor for unaccelerated pointer movement. * Source/WebCore/platform/mac/PlatformEventFactoryMac.h: Export WebCore::unadjustedMovementForEvent for use by WebKit::WebEventFactory::createWebMouseEvent. * Source/WebCore/platform/mac/PlatformEventFactoryMac.mm: (WebCore::unadjustedMovementForEvent): Ask NSEvent for unaccelerated mouse movement. (WebCore::PlatformMouseEventBuilder::PlatformMouseEventBuilder): Set unaccelerated mouse movement on PlatformMouseEvent as unadjustedMovementDelta. * Source/WebKit/Shared/WebEvent.serialization.in: * Source/WebKit/Shared/WebEventConversion.cpp: (WebKit::WebKit2PlatformMouseEvent::WebKit2PlatformMouseEvent): * Source/WebKit/Shared/WebMouseEvent.cpp: (WebKit::WebMouseEvent::WebMouseEvent): * Source/WebKit/Shared/WebMouseEvent.h: Added unadjustedMovementDelta to WebMouseEvent. All platforms get the field, but it's only set on Mac for now. * Source/WebKit/Shared/mac/WebEventFactory.mm: (WebKit::WebEventFactory::createWebMouseEvent): Ask NSEvent for unaccelerated mouse movement. * Source/WebKit/UIProcess/WebPageProxy.cpp: (WebKit::WebPageProxy::didAllowPointerLock): (WebKit::WebPageProxy::didDenyPointerLock): Remove ASSERT(m_isPointerLockPending && !m_isPointerLocked). Because didAllow/DenyPointerLock are not necessarily called synchronously by the UI Delegate, it is possible that the page itself has cancelled a pending pointer lock prior to the UI delegate making its decision to allow or deny pointer lock (e.g. by removing the target element). Instead, handle whatever cleanup actions are necessary based on the current state of m_isPointerLockPending and m_isPointerLocked individually. Canonical link: https://commits.webkit.org/276399@main
webkit-commit-queue
force-pushed
the
james-howard/PointerLockUnadjustedMovement
branch
from
7848f89 to
2ec55eb
Compare
|
Committed 276399@main (2ec55eb): https://commits.webkit.org/276399@main Reviewed commits have been landed. Closing PR #17525 and removing active labels. |
webkit-commit-queue
removed
the
merge-queue
2ec55eb
7848f89
🧪 ios-wk2-wpt🛠 🧪 jsc