Use permissions API for XR feature requests #900

Manishearth · 2019-11-06T01:30:34Z

This makes our feature request algorithm reuse machinery from the Permissions API, making it possible to do things like query consent status.

We define a single "xr" feature, however the query is done through a new XRPermissionDescriptor type which contains additional information about the requested features and the session.

In the current implementation, requested features are not persisted across sessions (which enables sessions with different modes, and sessions using different devices to have different granted features). However, there is nothing stopping the user agent from persisting granted features where it feels it prudent, since the spec relies on there being a "clear signal of user intent".

I'm not 100% sure of this approach, and I suspect this may go through a bunch of changes, hence the draft PR.

r? @NellWaliczek @toji

/fixes #794, /fixes #722, /fixes #702, /fixes #725

(maybe also #725?)

toji · 2019-11-07T22:07:36Z

index.bs

@@ -496,33 +503,113 @@ Note: {{XRReferenceSpaceType/"local"}} is always included in the [=requested fea

 Note: For example, several VR devices support either configuring a safe boundary for the user to move around within or skipping boundary configuration and operating in a mode where the user is expected to stand in place. Such a device can be considered to be [=capable of supporting=] {{"bounded-floor"}} {{XRReferenceSpace}}s even if they are currently not configured with safety boundaries, because it's expected that the user could configure the device appropriately if the experience required it. This is to allow user agents to avoid fully initializing the [=XRSession/XR device=] or waiting for the user's environment to be recognized prior to [=resolve the requested features|resolving the requested features=] if desired. If, however, the user agent knows that the boundary state at the time the session is requested without additional initialization it may choose to reject the {{"bounded-floor"}} feature if the safety boundary not already configured.

+Permissions API Integration {#permissions}
+---------


I understand why this was put here, but it feels currently like it's kind of "breaking the flow" of the document. (As much as a spec can have any sort of proper flow to it, anyway.) It's not really part of the "Initialization" topic, nor is it a critical piece to using the API. As a result, despite the strong relationship to the requiredFeatures and optionalFeatures, I feel this would be better placed near the end of the spec (in the "Integrations" section).

Works for me.

toji

Hoo boy, this is some dense stuff. Thanks for working out how this can all fit together, I'm sure it was a pain. Overall it's looking really good, and I like the direction you're taking it. I've got some questions that primarily relate to my lack of knowledge about the permissions API, and a couple of issues that should be addressed.

toji · 2019-11-08T05:08:40Z

index.bs

-        1. [=Resolve the requested features=] given by |options|' {{XRSessionInit/requiredFeatures}} and |options|' {{XRSessionInit/optionalFeatures}} values for |session|, and let |resolved| be the returned value.
-        1. If |resolved| is <code>false</code>, run the following steps:
+        1. Let |descriptor| be an {{XRPermissionDescriptor}} initialized with |session|, |options|' {{XRSessionInit/requiredFeatures}}, and |options|' {{XRSessionInit/optionalFeatures}}.
+        1. Let |descriptor| be an {{XRPermissionDescriptor}} initialized with |session|, |options|' {{XRSessionInit/requiredFeatures}}, and |options|' {{XRSessionInit/optionalFeatures}}.


Duplicate line here.

toji · 2019-11-08T05:46:03Z

index.bs

+    1. Abort these steps.
+  1. Let <code>(|consentRequired|, |consentOptional|, |granted|)</code> be the fields of |result|.
+  1. For each |feature| in |consentRequired| perform the following steps:
+      1. If a clear signal of [=user intent=] to enable |feature| has not been given, set |status|'s   {{PermissionStatus/state}} to {{PermissionState/"denied"}} and abort these steps.


This may just be me not groking the permissions flow, but it's not clear to me where in this algorithm the user intent should be prompted? [=resolve the requested features=] identifies what features need consent, and then this fails if they don't have consent, and it seems like there's not an opportunity in between to actually prompt for it?

"Clear signal of user intent" is when they can be prompted, but that signal can also be UA preferences or something else. We could perhaps firm up the language around that, maybe adding a definition for "clear signal of user intent" that includes something about prompting. The permissions spec itself is rather wishy washy about prompting as well.

FWIW "clear signal of user intent" was copied over from the old feature request code. I wasn't quite sure what to do for prompting so I left it as is.

[=Resolve the requested features=] is about filtering which features need to actually be requested.

toji · 2019-11-08T05:49:39Z

index.bs

+<div class=algorithm data-algorithm="xr-permission-query-algorithm">
+To query the "xr" permission with an {{XRPermissionDescriptor}} |descriptor| and a {{XRPermissionStatus}} |status|, the UA MUST run the following steps:
+
+  1. Set |status|'s {{PermissionStatus/state}} to |descriptor|'s [=permission state=].


I'll admit that I don't really get what this line is doing, but I also see that other permissions do something similar so I'm going to assume it's a good thing.

Yeah, this confuses me too. Is there someone we can ask to validate this isn't an obsolete thing to be doing?

This is basically the base algorithm to read the permission state. This is entirely fine to do. However, the way it's described in the spec makes it unclear to me whether this is taking into account the optional and required features. Unfortunately, I'm running out of time to review this :(

toji · 2019-11-08T05:54:42Z

index.bs

+  1. For each |feature| in |consentOptional| perform the following steps:
+      1. If a clear signal of [=user intent=] to enable |feature| has not been given, continue to the next entry.
+      1. If |feature| is not in |granted|, append |feature| to |granted|.
+  1. Set |status|'s {{XRPermissionStatus/granted}} to |granted|.


Should the session's [=list of enabled features=] be updated to |granted| here? I know that happens above, but that's only for the requestSession() method, while this could also be invoked by the Permissions.request() method.

Manishearth · 2019-11-18T18:29:47Z

/agenda to ask people to look at it

NellWaliczek

Wow, this must have been a beast to write up. Thank you so much for taking it on! I've left a few nits and one or two questions, but other than that LGTM!

index.bs

NellWaliczek · 2019-11-20T00:35:54Z

index.bs

+<div class=algorithm data-algorithm="xr-permission-query-algorithm">
+To query the "xr" permission with an {{XRPermissionDescriptor}} |descriptor| and a {{XRPermissionStatus}} |status|, the UA MUST run the following steps:
+
+  1. Set |status|'s {{PermissionStatus/state}} to |descriptor|'s [=permission state=].


Yeah, this confuses me too. Is there someone we can ask to validate this isn't an obsolete thing to be doing?

index.bs

NellWaliczek

LGTM!!!

mounirlamouri · 2019-11-21T18:07:40Z

self mentioning to have it in my queue @mounirlamouri

It will avoid to have PR author sending rendered output as in immersive-web#900

mounirlamouri

Sorry for the late comments, thanksgiving week was on the way :-/

mounirlamouri · 2019-12-03T18:11:24Z

index.bs

+
+<pre class="idl">
+dictionary XRPermissionDescriptor: PermissionDescriptor {
+  XRSession session;


It seems odd to me that XRPermissionDescriptor would contain an XRSession as this is what developers would need to call permissions.query() and ideally, they may want to do that before calling requestSession(). However, requestSession() is what provides the session and querying about the permission status at that point becomes moot.

It's because there's no user-exposed handle for the "device" except for the session itself. The permissions integration needs to check against a device, and also needs to be able to deal with the device being different -- e.g. if you plug in a device that supports bounded-floor spaces, you want the permissions check to work.

We could replace this with a mode enum, and use the "current XR device" instead, setting the list of granted features on the device instead of the session.

Why does the permission need to be checked against a device?

Some devices don't support a feature, period.This is the existing behavior of XR features, I'm wary about changing how this works without broader agreement.

index.bs

mounirlamouri · 2019-12-03T18:14:29Z

index.bs

-        1. If |resolved| is <code>false</code>, run the following steps:
+        1. Let |descriptor| be an {{XRPermissionDescriptor}} initialized with |session|, |options|' {{XRSessionInit/requiredFeatures}}, and |options|' {{XRSessionInit/optionalFeatures}}.
+        1. Let |status| be an {{XRPermissionStatus}}, initially <code>null</code>
+        1. [=Request the xr permission=] with |descriptor| and |status|.


It seems that "Request the xr permission" could return "status" instead of having it as in in/out parameter.

It can't, that's how the permission request algorithm works. I agree that it is awkward, but that's a problem for upstream.

https://wicg.github.io/permissions-request/#permission-request-algorithm

This is a different spec FWIW. You could re-use https://wicg.github.io/permissions-request/#boolean-permission-request-algorithm the same way https://w3c.github.io/permissions/#media-devices does.

ping about this

The media spec is using it incorrectly: it operates on the return value of the boolean permissions request algorithm, which does not exist, and it returns a value from its own permissions request algorithm, which according to the permissions spec should not exist. I'd be more than happy to clean this up but as written the permissions spec forces you to use an inout parameter. Perhaps it should be changed to use return values in many places so that it can be used in the way the media spec expects it to be.

mounirlamouri · 2019-12-03T18:15:07Z

index.bs

@@ -2334,6 +2309,121 @@ The [=default allowlist=] for this feature is <code>["self"]</code>.

 Note: If the document's origin is not allowed to use the <code>"xr-spatial-tracking"</code> feature policy any [=immersive sessions=] will be blocked, because all [=immersive sessions=] require some use of spatial tracking. [=Inline sessions=] will still be allowed, but restricted to only using the {{XRReferenceSpaceType/"viewer"}} {{XRReferenceSpace}}.

+
+Permissions API Integration {#permissions}


Shouldn't this section have a disclaimer that it's temporary and ought to move to the Permissions API?

I'm not sure that's the case? E.g. web bluetooth doesn't have this disclaimer, and it would be really weird to upstream all of this machinery to that API I think.

https://w3c.github.io/permissions/#permission-registry is here exactly for that

index.bs

mounirlamouri · 2019-12-03T18:22:59Z

index.bs

+<dd>
+<pre class=idl>
+dictionary XRPermissionStatus: PermissionStatus {
+  sequence&lt;any&gt; granted;


nit: I would call this features, not granted because it's hard to understand what that is without a lot of context. It kind of relates to #860 too.

Personally I find granted clearer than features. But open to changing if others feel differently.

PermissionStatus has a field called state that can be set to granted. It's super confusing that PermissionStatus which reflects the status of the permission has a field called granted that just return a list of any.

cc @toji opinions on naming?

mounirlamouri · 2019-12-03T18:25:26Z

index.bs

+<div class=algorithm data-algorithm="xr-permission-request-algorithm">
+To <dfn lt="request the xr permission">request the "xr" permission</dfn> with an {{XRPermissionDescriptor}} |descriptor| and a {{XRPermissionStatus}} |status|, the UA MUST run the following steps:
+
+  1. Set |status|'s {{XRPermissionStatus/granted}} to an empty {{FrozenArray}}.


style: I don't know why this spec is using "1." for bullet points but it makes it a bit hard to compare to the generated spec.

mounirlamouri · 2019-12-03T18:25:40Z

index.bs

+<pre class=idl>
+dictionary XRPermissionStatus: PermissionStatus {
+  sequence&lt;any&gt; granted;
+};


This said, I wonder if we could delay this addition. Are there strong use cases for this? It would require implementations to be able to provide the list of enabled features without creating the session which may or may not an easy thing to do. If we don't have a strong use case, keeping the change small for now may be good?

would require implementations to be able to provide the list of enabled features without creating the session which may or may not an easy thing to do

I don't understand why? The spec already requires you to compute which features have been granted when you requestSession(), this just exposes that list.

In requestSession(), you create an XRSession and have the list of available features. Here, you should be able to check for permissions without a session.

ping about this. I think dropping the attribute entirely would be better. The only benefit of returning the list of features is to tell which optional features are granted. It doesn't seem critical and if someone really cares, they shouldn't have them as optional in the first place.

No, this was part of the point of this change, see #794 . It's important for apps to know what optional features were granted.

So yeah, there was a strong use case, this was one of the main reasons we wanted to make this pull request.

mounirlamouri · 2019-12-03T18:26:42Z

index.bs

+  1. The user agent MAY at this point perform any necessary steps, like showing permissions prompts, to determine if there is a clear signal of [=user intent=] for any features in |requiredFeatures| and |optionalFeatures|.
+  1. For each |feature| in |consentRequired| perform the following steps:
+      1. The user agent MAY at this point perform any necessary steps, like showing permissions prompts, to determine if there is a clear signal of [=user intent=] for |feature|.
+      1. If a clear signal of [=user intent=] to enable |feature| has not been determined, set |status|'s {{PermissionStatus/state}} to {{PermissionState/"denied"}} and abort these steps.


FWIW, the Permissions API has language for that. Maybe we can try to re-use it? #request-permission-to-use for example

As mentioned above, this is not useful here, #request-permission-to-use takes a whole permission descriptor, we're requesting stuff for individual subfeatures. It would be nice if the permissions spec exposed a more scoped way to do prompts, but it doesn't, everything has to use a descriptor.

Re-using the wording would be a first step.

mounirlamouri · 2019-12-03T18:31:31Z

index.bs

+<div class=algorithm data-algorithm="xr-permission-query-algorithm">
+To query the "xr" permission with an {{XRPermissionDescriptor}} |descriptor| and a {{XRPermissionStatus}} |status|, the UA MUST run the following steps:
+
+  1. Set |status|'s {{PermissionStatus/state}} to |descriptor|'s [=permission state=].


This is basically the base algorithm to read the permission state. This is entirely fine to do. However, the way it's described in the spec makes it unclear to me whether this is taking into account the optional and required features. Unfortunately, I'm running out of time to review this :(

TrevorFSmith · 2019-12-03T19:37:30Z

Sorry that I'm late to the review party due to holidays but from an API consumer / framework writer perspective this looks really solid and addresses all of my concerns. Great work, Manish!

Manishearth · 2019-12-30T10:51:15Z

Addressed. The last commit is the one moving stuff over to using devices, not sessions.

Manishearth · 2020-01-21T18:38:17Z

@mounirlamouri review ping, let's get this merged 😄

Manishearth · 2020-02-10T21:05:36Z

/agenda to check for any final issues before landing

cabanier · 2020-02-12T00:19:38Z

This is implemented on the Oculus side.
@ravi, doesn't Magic Leap have this implemented as well?

Manishearth · 2020-02-12T03:15:42Z

Please let me know if there were any issues implementing the API and if you think it could be better!

mounirlamouri

I see this has merged. FWIW, I did review this last week but didn't submit my changes. My concern was that basically most of my comments were ignored so I'm not sure what you wanted me to look at.

mounirlamouri · 2020-02-18T02:31:06Z

index.bs

@@ -256,6 +262,7 @@ Each time the [=list of immersive XR devices=] changes the user agent should <df
  1. [=Shut down the session|Shut down=] any active {{XRSession}}s.
  1. Set the [=XR compatible=] boolean of all {{WebGLRenderingContextBase}} instances to <code>false</code>.
  1. [=Queue a task=] to [=fire an event=] named {{devicechange!!event}} on the [=context object=].
+  1. [=Queue a task=] to fire appropriate <code>change</code> events on any {{XRPermissionStatus}} objects who are affected by the change in the [=XR/immersive XR device=] or [=inline XR device=].


Why would an update in the list of available device have an impact on the permissions?

Because permissions being granted is the intersection of the device supporting it and the user allowing it. This could be changed, but I was trying to keep the behavior of permissions as close to the existing features behavior as possible.

This is basically the fallout of making it no longer require an XRSession (which was definitely a good decision): features are still tied to devices, so changing the device can invalidate permissions.

It could be expanded to not require this, we can file a followup issue about that. There's some concern about how much permissions should persist between devices anyway.

mounirlamouri · 2020-02-18T02:32:38Z

index.bs

-        1. If |resolved| is <code>false</code>, run the following steps:
+        1. Let |descriptor| be an {{XRPermissionDescriptor}} initialized with |session|, |options|' {{XRSessionInit/requiredFeatures}}, and |options|' {{XRSessionInit/optionalFeatures}}.
+        1. Let |status| be an {{XRPermissionStatus}}, initially <code>null</code>
+        1. [=Request the xr permission=] with |descriptor| and |status|.


ping about this

mounirlamouri · 2020-02-18T02:35:40Z

index.bs

+<dd>
+<pre class=idl>
+dictionary XRPermissionStatus: PermissionStatus {
+  sequence&lt;any&gt; granted;


PermissionStatus has a field called state that can be set to granted. It's super confusing that PermissionStatus which reflects the status of the permission has a field called granted that just return a list of any.

mounirlamouri · 2020-02-18T02:38:35Z

index.bs

+<pre class=idl>
+dictionary XRPermissionStatus: PermissionStatus {
+  sequence&lt;any&gt; granted;
+};


ping about this. I think dropping the attribute entirely would be better. The only benefit of returning the list of features is to tell which optional features are granted. It doesn't seem critical and if someone really cares, they shouldn't have them as optional in the first place.

It will avoid to have PR author sending rendered output as in immersive-web#900

This was referenced Nov 6, 2019

Informing apps which optional features were granted? #794

Closed

Querying user consent status #722

Closed

Privacy: Interaction of existing permission-based features with WebXR feature consent #702

Closed

toji reviewed Nov 7, 2019

View reviewed changes

Use permissions API for XR feature requests

ee370bd

Manishearth force-pushed the consent-tracking branch from 470686b to ee370bd Compare November 7, 2019 22:37

toji reviewed Nov 8, 2019

View reviewed changes

Manishearth added 2 commits November 7, 2019 22:12

remove duplicate line

c69936c

Grant features for the session in the request algorithm

82495dc

Manishearth marked this pull request as ready for review November 8, 2019 18:36

toji approved these changes Nov 13, 2019

View reviewed changes

avadacatavra pushed a commit to avadacatavra/webxr that referenced this pull request Nov 14, 2019

removed Querying consent section per immersive-web#900

dc120f3

toji pushed a commit that referenced this pull request Nov 14, 2019

removed Querying consent section per #900

c2619f7

NellWaliczek approved these changes Nov 20, 2019

View reviewed changes

Review fixes

6914665

NellWaliczek reviewed Nov 20, 2019

View reviewed changes

index.bs Outdated Show resolved Hide resolved

Manishearth added 2 commits November 20, 2019 10:36

Unconditionally null out |granted|

e980903

Explicitly deal with permissions prompts and such

68670dc

NellWaliczek approved these changes Nov 20, 2019

View reviewed changes

toji added the agenda Request discussion in the next telecon/FTF label Dec 2, 2019

mounirlamouri added a commit to mounirlamouri/webxr that referenced this pull request Dec 2, 2019

Update .pr-preview.json to use the right index.bs

c000c2b

It will avoid to have PR author sending rendered output as in immersive-web#900

mounirlamouri mentioned this pull request Dec 2, 2019

Update .pr-preview.json to use the right index.bs #931

Merged

mounirlamouri reviewed Dec 3, 2019

View reviewed changes

AdaRoseCannon added agenda Request discussion in the next telecon/FTF and removed agenda Request discussion in the next telecon/FTF labels Dec 13, 2019

Use language from #request-permission-to-use

13babff

Make permissions query stuff use devices, not sessions

8a4a443

Manishearth force-pushed the consent-tracking branch from 54f830c to 8a4a443 Compare December 30, 2019 10:50

toji added this to the Pre-CR milestone Jan 6, 2020

Manishearth mentioned this pull request Jan 20, 2020

Expose a way to query a session about the supported features #952

Closed

Manishearth merged commit 41612a7 into immersive-web:master Feb 24, 2020

Manishearth deleted the consent-tracking branch February 24, 2020 18:37

mounirlamouri reviewed Feb 25, 2020

View reviewed changes

stephenmcgruer mentioned this pull request Feb 25, 2020

XRPermissionStatus is a dictionary but inherits from an interface #965

Closed

kearwood pushed a commit to kearwood/webxr that referenced this pull request Mar 11, 2020

removed Querying consent section per immersive-web#900

e8fdc47

kearwood pushed a commit to kearwood/webxr that referenced this pull request Mar 11, 2020

Update .pr-preview.json to use the right index.bs

df035db

It will avoid to have PR author sending rendered output as in immersive-web#900

		@@ -2334,6 +2309,121 @@ The [=default allowlist=] for this feature is <code>["self"]</code>.

		Note: If the document's origin is not allowed to use the <code>"xr-spatial-tracking"</code> feature policy any [=immersive sessions=] will be blocked, because all [=immersive sessions=] require some use of spatial tracking. [=Inline sessions=] will still be allowed, but restricted to only using the {{XRReferenceSpaceType/"viewer"}} {{XRReferenceSpace}}.


		Permissions API Integration {#permissions}

Use permissions API for XR feature requests #900

Use permissions API for XR feature requests #900

Conversation

Manishearth commented Nov 6, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

toji 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

Manishearth commented Nov 18, 2019

NellWaliczek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NellWaliczek left a comment

Choose a reason for hiding this comment

mounirlamouri commented Nov 21, 2019

mounirlamouri 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mounirlamouri Dec 3, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Manishearth Feb 25, 2020 • edited Loading

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

TrevorFSmith commented Dec 3, 2019

Manishearth commented Dec 30, 2019

Manishearth commented Jan 21, 2020

Manishearth commented Feb 10, 2020

cabanier commented Feb 12, 2020

Manishearth commented Feb 12, 2020

mounirlamouri 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

Manishearth commented Nov 6, 2019 •

edited

Loading

mounirlamouri Dec 3, 2019 •

edited

Loading

Manishearth Feb 25, 2020 •

edited

Loading