Add WPT tests to increase PointerLock coverage [chromium/src : main]
Gaston Rodriguez (Gerrit)
Message from Gaston Rodriguez![]( "Open in Gerrit")
Set Ready For Review
Related details
- Gaston Rodriguez
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gaston Rodriguez (Gerrit)
Gaston Rodriguez voted and added 1 comment![]( "Open in Gerrit")
Votes added by Gaston Rodriguez
| Commit-Queue | +1 |
1 comment
Hi Olga or John, could you take a look please before I sent it for upstream review?
Related details
- John An
- Olga Gerchikov
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
John An (Gerrit)
John An added 4 comments![]( "Open in Gerrit")
Seems good overall
under the cursor (spec §2.1).nit: could you add a link to the spec below? just one is fine.
var container = document.getElementById("testContainer");Seems like this isn't doing anything.
document.addEventListener('pointerlockerror', function(event) {Why does this test get a pointerlockerror check and not the others?
Related details
- Gaston Rodriguez
- Olga Gerchikov
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gaston Rodriguez (Gerrit)
Gaston Rodriguez voted and added 3 comments![]( "Open in Gerrit")
Votes added by Gaston Rodriguez
| Commit-Queue | +1 |
nit: could you add a link to the spec below? just one is fine.
Done
Seems like this isn't doing anything.
Forgot to delete it after last patch, thanks.
document.addEventListener('pointerlockerror', function(event) {Why does this test get a pointerlockerror check and not the others?
- John An
- Olga Gerchikov
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
John An (Gerrit)
John An added 1 comment![]( "Open in Gerrit")
- Gaston Rodriguez
- Olga Gerchikov
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mustaq Ahmed (Gerrit)
Mustaq Ahmed added 2 comments![]( "Open in Gerrit")
Thanks for checking the coverage here.
- Have you checked possible dups in `external/wpt/pointerlock/`?
- Please move the new WPTs to the above folders whenever the test doesn't involve PointerEvents spec (e.g. the one for bubbling event).
The pointerlock spec is defined in https://www.w3.org/TR/pointerlock-2/Please link to the latest editor's draft instead.
Related details
- Gaston Rodriguez
- Olga Gerchikov
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gaston Rodriguez (Gerrit)
Gaston Rodriguez added 2 comments![]( "Open in Gerrit")
Thanks for checking the coverage here.
- Have you checked possible dups in `external/wpt/pointerlock/`?
- Please move the new WPTs to the above folders whenever the test doesn't involve PointerEvents spec (e.g. the one for bubbling event).
Turns out I had missed checking that directory. There were two tests that covered similar concerns, I migrated my logic into them and expanded what already existed.
I also moved most of them into `wpt/pointerlock`. Thanks!
The pointerlock spec is defined in https://www.w3.org/TR/pointerlock-2/Please link to the latest editor's draft instead.
- Mustaq Ahmed
- Olga Gerchikov
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gaston Rodriguez (Gerrit)
Gaston Rodriguez added 1 comment![]( "Open in Gerrit")
Gaston RodriguezThanks for checking the coverage here.
- Have you checked possible dups in `external/wpt/pointerlock/`?
- Please move the new WPTs to the above folders whenever the test doesn't involve PointerEvents spec (e.g. the one for bubbling event).
Turns out I had missed checking that directory. There were two tests that covered similar concerns, I migrated my logic into them and expanded what already existed.
I also moved most of them into `wpt/pointerlock`. Thanks!
Ended up moving all of them after further review.
Related details
- Mustaq Ahmed
- Olga Gerchikov
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mustaq Ahmed (Gerrit)
Mustaq Ahmed added 11 comments![]( "Open in Gerrit")
Nice, thanks for improving the test coverage for PointerLock and also checking for coverage overlap. I asked for a few minor changes in the second test, a rename, and the consideration to avoiding counting (possibly through another CL).
// Test 1: requestPointerLock() promise resolves on successNit: I would skip the test numbers to ease adding future tests in-between or moving tests. I personally prefer better `promise_test` label over such comment blocks but that's subjective.
// Set up listener BEFORE requesting lock so we consumeThe use of "consume" seems confusing to me, as if we can have at most one listener. I would suggest "to be able to handle...".
}, "requestPointerLock() promise resolves on success");"requestPointerLock() and exitPointerLock promises resolve on success"?
// Test 2: exitPointerLock() when not locked is a no-op.Ditto.
var eventFired = false;
var handler = function() {
eventFired = true;
};Let's make the error message more precise about which event has fired, and omit the Boolean:
```js
var handler = e => assert_unreached(`Unexpected firing of ${e.type}`);
```
// Test 3: pointerlockchange event does not bubble and is notDitto.
assert_false(lockEvent.bubbles,Please add an assert that `lockEvent` is not undefined, to avoid an uncaught reference error.
var lock_change_count = 0;The use of `lock_change_count` makes this test harder to follow compared to the test above because the test logic is scattered across multiple handlers. Can't we avoid this counting and instead rely on `await lockChangePromise` like your test above?
Same for all tests using this counter.
Alternatively you can rewrite these tests through a separate CL after this lands. Your call.
const kSuppressedEvents = ['mouseenter', 'mouseleave', 'mouseover',Please rename the test and text inside to avoid "compat mouse event". The suppressed events here are the boundary events, and "compat mouse events" are related to PointerEvents and unrelated to PointerLock.
Related details
- Gaston Rodriguez
- Olga Gerchikov
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gaston Rodriguez (Gerrit)
Gaston Rodriguez added 10 comments![]( "Open in Gerrit")
// Test 1: requestPointerLock() promise resolves on successNit: I would skip the test numbers to ease adding future tests in-between or moving tests. I personally prefer better `promise_test` label over such comment blocks but that's subjective.
I agree, removed the comments and improved the label where appropriate.
// Set up listener BEFORE requesting lock so we consumeThe use of "consume" seems confusing to me, as if we can have at most one listener. I would suggest "to be able to handle...".
Done
}, "requestPointerLock() promise resolves on success");"requestPointerLock() and exitPointerLock promises resolve on success"?
Done
// Test 2: exitPointerLock() when not locked is a no-op.Gaston RodriguezDitto.
Done
var eventFired = false;
var handler = function() {
eventFired = true;
};Let's make the error message more precise about which event has fired, and omit the Boolean:
```js
var handler = e => assert_unreached(`Unexpected firing of ${e.type}`);
```
Nice.
// Test 3: pointerlockchange event does not bubble and is notGaston RodriguezDitto.
Done
Please add an assert that `lockEvent` is not undefined, to avoid an uncaught reference error.
Done
assert_false(unlockEvent.bubbles,Gaston RodriguezSame.
Done
The use of `lock_change_count` makes this test harder to follow compared to the test above because the test logic is scattered across multiple handlers. Can't we avoid this counting and instead rely on `await lockChangePromise` like your test above?
Same for all tests using this counter.
Alternatively you can rewrite these tests through a separate CL after this lands. Your call.
I rewrote the tests to use the sequential model. It's more readable, thanks!
const kSuppressedEvents = ['mouseenter', 'mouseleave', 'mouseover',Please rename the test and text inside to avoid "compat mouse event". The suppressed events here are the boundary events, and "compat mouse events" are related to PointerEvents and unrelated to PointerLock.
- Mustaq Ahmed
- Olga Gerchikov
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gaston Rodriguez (Gerrit)
Gaston Rodriguez added 1 comment![]( "Open in Gerrit")
var lock_change_count = 0;Gaston RodriguezThe use of `lock_change_count` makes this test harder to follow compared to the test above because the test logic is scattered across multiple handlers. Can't we avoid this counting and instead rely on `await lockChangePromise` like your test above?
Same for all tests using this counter.
Alternatively you can rewrite these tests through a separate CL after this lands. Your call.
I rewrote the tests to use the sequential model. It's more readable, thanks!
Also, the latest version is 200 lines smaller than the one you commented on. Much better (:
Related details
- Mustaq Ahmed
- Olga Gerchikov
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mustaq Ahmed (Gerrit)
Mustaq Ahmed voted and added 3 comments![]( "Open in Gerrit")
Votes added by Mustaq Ahmed
| Code-Review | +1 |
3 comments
Awesome, thanks for cutting 200 lines and improve readability!
"Events fired: [" + events_on_other.join(", ") + "]");Nit: I think `assert_array_equals` emits the array content when fails. If this is correct, the last part of the string could be removed.
var lock_change_count = 0;Gaston RodriguezThe use of `lock_change_count` makes this test harder to follow compared to the test above because the test logic is scattered across multiple handlers. Can't we avoid this counting and instead rely on `await lockChangePromise` like your test above?
Same for all tests using this counter.
Alternatively you can rewrite these tests through a separate CL after this lands. Your call.
Gaston RodriguezI rewrote the tests to use the sequential model. It's more readable, thanks!
Also, the latest version is 200 lines smaller than the one you commented on. Much better (:
- Gaston Rodriguez
- Olga Gerchikov
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gaston Rodriguez (Gerrit)
Gaston Rodriguez added 1 comment![]( "Open in Gerrit")
Nit: I think `assert_array_equals` emits the array content when fails. If this is correct, the last part of the string could be removed.
I just double checked and you're right, removed this line.
Related details
- Olga Gerchikov
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Gaston Rodriguez (Gerrit)
Gaston Rodriguez voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Blink W3C Test Autoroller (Gerrit)
Message from Blink W3C Test Autoroller![]( "Open in Gerrit")
Exportable changes to web-platform-tests were detected in this CL and a pull request in the upstream repo has been made: https://github.com/web-platform-tests/wpt/pull/60707.
When this CL lands, the bot will automatically merge the PR on GitHub if the required GitHub checks pass; otherwise, ecosystem-infra@ team will triage the failures and may contact you.
WPT Export docs:
https://chromium.googlesource.com/chromium/src/+/main/docs/testing/web_platform_tests.md#Automatic-export-process
Related details
- Olga Gerchikov
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
18 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: third_party/blink/web_tests/external/wpt/pointerlock/pointerlock_all_mouse_events_to_locktarget.html
Insertions: 1, Deletions: 2.
@@ -58,8 +58,7 @@
assert_true(events_on_target.includes('click'),
"click should fire on lock target.");
assert_array_equals(events_on_other, [],
- "No mouse events should fire on non-lock-target element. " +
- "Events fired: [" + events_on_other.join(", ") + "]");
+ "No mouse events should fire on non-lock-target element.");
}, "mousedown, mouseup, click events go to the lock target");
</script>
</html>
```
Change information
Add WPT tests to increase PointerLock coverage
New tests in wpt/pointerlock/:
- pointerlock_all_mouse_events_to_locktarget: verifies that mouse events are routed to the lock target element and not the element under the cursor (spec §2.1).
- pointerlock_suppress_mouse_boundary_events_when_locked.html: verifies that mouse boundary events (out, leave, etc) do not fire while the pointer is locked (spec §2.1).
- pointerlock_target_change: verifies that calling requestPointerLock() on a different element while already locked changes the lock target, fires pointerlockchange, and that subsequent pointermove events are dispatched to the new lock target (spec §3 step 6.2, plus Pointer Events routing during lock).
- pointerlock_api_behavior: verifies miscellaneous behaviours of calling pointerlock functions. Has two tests: (a) exitPointerLock() is a no-op when not locked, and (b) the pointerlockchange event fired by the UA matches spec (does not bubble, is not cancelable, targets the document) (spec §2.3, §3, §4).
Replaced/strengthened test in wpt/pointerlock/:
- pointerlock_remove_target: replaces the previous manual-style test with an automated version that verifies pointer lock exits when the locked element is removed from the DOM (spec §8).
The pointerlock spec is defined in https://w3c.github.io/pointerlock/
- A third_party/blink/web_tests/external/wpt/pointerlock/pointerlock_all_mouse_events_to_locktarget.html
- A third_party/blink/web_tests/external/wpt/pointerlock/pointerlock_api_behavior.html
- M third_party/blink/web_tests/external/wpt/pointerlock/pointerlock_remove_target.html
- A third_party/blink/web_tests/external/wpt/pointerlock/pointerlock_suppress_mouse_boundary_events_when_locked.html
- A third_party/blink/web_tests/external/wpt/pointerlock/pointerlock_target_change.html
- A third_party/blink/web_tests/external/wpt/pointerlock/test-helper.js
Code-Review: +1 by Mustaq Ahmed
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Blink W3C Test Autoroller (Gerrit)
Message from Blink W3C Test Autoroller![]( "Open in Gerrit")
The WPT PR for this CL has been merged upstream! https://github.com/web-platform-tests/wpt/pull/60707
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |