orientation-event: Add Permissions Policy integration tests #45428

rakuco · 2024-03-29T16:14:57Z

Add tests for deviceorientationabsolute, deviceorientation and devicemotion
as described in
https://w3c.github.io/deviceorientation/#permissions-policy-integration:

The deviceorientation event requires the "accelerometer" and "gyroscope"
features.
The deviceorientationabsolute event requires the "accelerometer",
"gyroscope" and "magnetometer" features.
The devicemotion event requires the "accelerometer" and "gyroscope"
features.

(WebKit currently requires the "magnetometer" feature for deviceorientation
events, see https://bugs.webkit.org/show_bug.cgi?id=271891).

The tests being added follow permissions-policy/README.md's recommendations:

Tests with different Permissions-Policy header values and how they
influence usage in the top-level frame, same-origin iframes and
cross-origin iframes.
Different values for the "allow" attribute and how they influece
same-origin and cross-origin iframes.
Usage of the "allow" attribute with redirection.

This is the minimum amount of coverage we need. These tests can be expanded
to include:

Calls to run_all_fp_tests_allow_self() to test the behavior of the
default policy for each feature (it requires making the function work with
more than one feature).
The relative orientation to absolute orientation fallback behavior that
depends on whether the "magnetometer" feature is enabled.
Not enabling all features (it may cause an explosion in the number of
tests though).

Add tests for deviceorientationabsolute, deviceorientation and devicemotion as described in https://w3c.github.io/deviceorientation/#permissions-policy-integration: - The deviceorientation event requires the "accelerometer" and "gyroscope" features. - The deviceorientationabsolute event requires the "accelerometer", "gyroscope" and "magnetometer" features. - The devicemotion event requires the "accelerometer" and "gyroscope" features. (WebKit currently requires the "magnetometer" feature for deviceorientation events, see https://bugs.webkit.org/show_bug.cgi?id=271891). The tests being added follow permissions-policy/README.md's recommendations: - Tests with different Permissions-Policy header values and how they influence usage in the top-level frame, same-origin iframes and cross-origin iframes. - Different values for the "allow" attribute and how they influece same-origin and cross-origin iframes. - Usage of the "allow" attribute with redirection. This is the minimum amount of coverage we need. These tests can be expanded to include: - Calls to `run_all_fp_tests_allow_self()` to test the behavior of the default policy for each feature (it requires making the function work with more than one feature). - The relative orientation to absolute orientation fallback behavior that depends on whether the "magnetometer" feature is enabled. - Not enabling all features (it may cause an explosion in the number of tests though).

reillyeon

LGTM with nits

...eviceorientationabsolute-allowed-by-permissions-policy-attribute-redirect-on-load.https.html

...policy/deviceorientation-allowed-by-permissions-policy-attribute-redirect-on-load.https.html

...ions-policy/devicemotion-allowed-by-permissions-policy-attribute-redirect-on-load.https.html

marcoscaceres · 2024-04-01T07:09:24Z

...tion/permissions-policy/devicemotion-enabled-on-self-origin-by-permissions-policy.https.html

+promise_test(async t => {
+  const helper = new SensorTestHelper(t, 'devicemotion');
+  await helper.grantSensorsPermissions();
+  await helper.initializeSensors();


This puts a dependency on create_virtual_sensor :( Are we sure there isn't some other way to test this?

With other permission policy tests, we usually just have a simple iframe that tries to use the API.

This puts a dependency on create_virtual_sensor :( Are we sure there isn't some other way to test this?

I don't think so. This API does not have any mechanisms to throw an exception when permissions policy checks fail like other APIs do, so we need a way to verify that the checks have failed, in which case we wait for readings not to be delivered. We could try to reach consensus on w3c/deviceorientation#118, but it wouldn't help with the case in which we need to verify that the checks do pass, as without virtual sensors we'd be entirely dependent on the presence of specific hardware for them to work.

Ok, yeah... I was trying to think of something too... and the only thing was trying to create the events in JS, which is silly.

Co-authored-by: Reilly Grant <[email protected]>

permissions-policy/resources/permissions-policy-orientation-event.sub.html

marcoscaceres · 2024-04-02T03:15:39Z

permissions-policy/resources/orientation-event.js

+
+async function waitForOrientationEvent(eventName) {
+  if (eventName !== 'devicemotion' && eventName !== 'deviceorientation' && eventName !== 'deviceorientationabsolute') {
+    return 'ERROR';


I wonder if we should throw real errors here. It might make things easier to debug if things go wrong.

marcoscaceres · 2024-04-02T03:16:53Z

permissions-policy/resources/orientation-event.js

+    // policy checks fail, so what we do here is wait for the timeout and the
+    // devicemotion event handler to race each other.
+    value = await new Promise((resolve, reject) => {
+      const timeoutId = window.setTimeout(() => {


This won't fly... it's gonna be super racy on testing infrastructure.

Do you have anything else in mind? This API doesn't throw any errors or fire any events when a permissions/permissions policy check fails, so the only other option would be adding a ReportingObserver and waiting for a permissions-policy-violation report. Would that be acceptable?

marcoscaceres · 2024-04-02T03:17:35Z

permissions-policy/resources/orientation-event.js

+    value = await new Promise((resolve, reject) => {
+      const timeoutId = window.setTimeout(() => {
+        window.removeEventListener(eventName, handler);
+        reject('NO-EVENT');


Reject with real error if we do this... otherwise we lose the error stack.

Is not having an error stack really a problem in this case? test_feature_availability_with_post_message_result() expects a string, so throwing an Error here would essentially require some plumbing that would ultimately result in a string comparison anyway.

The Promise.resolve() idea was copy-pasted from another file, but adding a load event handler makes it more clear and in line with other files in permissions-policy/resources. While here, use Promise.then() instead of async/await to match other files as well.

marcoscaceres · 2024-04-15T22:09:37Z

Hey, promise to take a look soon! At a conference this week, but will try to sneak these in!

rakuco requested review from reillyeon and marcoscaceres March 29, 2024 16:14

wpt-pr-bot added orientation-event permissions-policy wg-das wg-s_webappsec labels Mar 29, 2024

wpt-pr-bot assigned timvolodine Mar 29, 2024

wpt-pr-bot requested a review from timvolodine March 29, 2024 16:15

rakuco unassigned timvolodine Mar 29, 2024

rakuco removed the request for review from timvolodine March 29, 2024 16:15

reillyeon approved these changes Mar 29, 2024

View reviewed changes

marcoscaceres reviewed Apr 1, 2024

View reviewed changes

...ions-policy/devicemotion-allowed-by-permissions-policy-attribute-redirect-on-load.https.html Outdated Show resolved Hide resolved

marcoscaceres reviewed Apr 1, 2024

View reviewed changes

Raphael Kubo da Costa and others added 2 commits April 1, 2024 15:06

Fix typos

579a755

Co-authored-by: Reilly Grant <[email protected]>

Fix typos

8bd8ab4

Co-authored-by: Reilly Grant <[email protected]>

wpt-pr-bot assigned rakuco Apr 1, 2024

Raphael Kubo da Costa and others added 4 commits April 1, 2024 15:07

Fix typos

85232de

Co-authored-by: Reilly Grant <[email protected]>

Fix typos

1491d65

Co-authored-by: Reilly Grant <[email protected]>

Fix typos

d4e2578

Co-authored-by: Reilly Grant <[email protected]>

Fix typos

f3893ac

Co-authored-by: Reilly Grant <[email protected]>

marcoscaceres reviewed Apr 2, 2024

View reviewed changes

permissions-policy/resources/permissions-policy-orientation-event.sub.html Outdated Show resolved Hide resolved

marcoscaceres reviewed Apr 2, 2024

View reviewed changes

Raphael Kubo da Costa added 2 commits April 5, 2024 14:20

Reformat files using the prettier VS Code extension

9033cdb

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

orientation-event: Add Permissions Policy integration tests #45428

orientation-event: Add Permissions Policy integration tests #45428

rakuco commented Mar 29, 2024

reillyeon left a comment

marcoscaceres Apr 1, 2024

rakuco Apr 1, 2024

marcoscaceres Apr 2, 2024

marcoscaceres Apr 2, 2024

marcoscaceres Apr 2, 2024

rakuco Apr 5, 2024

marcoscaceres Apr 2, 2024

rakuco Apr 5, 2024

marcoscaceres commented Apr 15, 2024

orientation-event: Add Permissions Policy integration tests #45428

Are you sure you want to change the base?

orientation-event: Add Permissions Policy integration tests #45428

Conversation

rakuco commented Mar 29, 2024

reillyeon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcoscaceres commented Apr 15, 2024