Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use permissions API for XR feature requests #900

Merged
merged 8 commits into from
Feb 24, 2020
152 changes: 120 additions & 32 deletions index.bs
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,9 @@ Abstract: This specification describes support for accessing virtual reality (VR
<pre class="link-defaults">
spec:infra;
type:dfn; text:string
type:dfn; text:tuple
spec:permissions-1;
type:dfn; text:powerful feature
</pre>

<pre class="anchors">
Expand Down Expand Up @@ -346,11 +349,15 @@ When this method is invoked, the user agent MUST run the following steps:
1. Abort these steps.
1. Let |session| be a new {{XRSession}} object.
1. [=Initialize the session=] with |session|, |mode|, and |device|.
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}}.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Duplicate line here.

1. Let |status| be an {{XRPermissionStatus}}, initially <code>null</code>
1. [=Request the xr permission=] with |descriptor| and |status|.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ping about this

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

image

1. If |status|' {{PermissionStatus/state}} is {{PermissionState/"denied"}} run the following steps:
1. [=Reject=] |promise| with a "{{NotSupportedError}}" {{DOMException}}.
1. If |immersive| is <code>true</code>, set [=pending immersive session=] to <code>false</code>.
1. Abort these steps.
1. Set |session|'s [=list of enabled features=] to |status|' {{XRPermissionStatus/granted}}.
1. Potentially set the [=active immersive session=] as follows:
<dl class="switch">
<dt> If |immersive| is <code>true</code>
Expand Down Expand Up @@ -496,36 +503,6 @@ 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.

<div class="algorithm" data-algorithm="resolve-features">

To <dfn>resolve the requested features</dfn> given |requiredFeatures| and |optionalFeatures| for an {{XRSession}} |session|, the user agent MUST run the following steps:

1. Let |consentRequired| be an empty [=/list=] of {{DOMString}}.
1. Let |consentOptional| be an empty [=/list=] of {{DOMString}}.
1. Add every [=feature descriptor=] in the [=default features=] table associated with |session|'s [=XRSession/mode=] to the indicated feature list if it is not already present.
1. For each |feature| in |requiredFeatures| perform the following steps:
1. If |feature| is not a valid [=feature descriptor=], return <code>false</code>.
1. If the requesting document's origin is not allowed to use any [[#feature-policy|feature policy]] required by |feature| as indicated by the [=feature requirements=] table, return <code>false</code>.
1. If |session|'s [=XRSession/XR device=] is not [=capable of supporting=] the functionality described by |feature| or the user agent has otherwise determined to reject the feature, return <code>false</code>.
1. If the functionality described by |feature| requires [=explicit consent=], append it to |consentRequired|.
1. Else append |feature| to |session|'s [=list of enabled features=].
1. For each |feature| in |optionalFeatures| perform the following steps:
1. If |feature| is not a valid [=feature descriptor=], continue to the next entry.
1. If the requesting document's origin is not allowed to use any [[#feature-policy|feature policy]] required by |feature| as indicated by the [=feature requirements=] table, continue to the next entry.
1. If |session|'s [=XRSession/XR device=] is not [=capable of supporting=] the functionality described by |feature| or the user agent has otherwise determined to reject the feature, continue to the next entry.
1. If the functionality described by |feature| requires [=explicit consent=], append it to |consentOptional|.
1. Else append |feature| to |session|'s [=list of enabled features=].
1. If |consentRequired| or |consentOptional| are not empty, request [=explicit consent=] to use the functionality described by those features.
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, return <code>false</code>.
1. Else append |feature| to |session|'s [=list of enabled features=].
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. Else append |feature| to |session|'s [=list of enabled features=].
1. Return <code>true</code>

</div>

Session {#session}
=======

Expand Down Expand Up @@ -2334,6 +2311,117 @@ 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}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, okay.

---------

The [[!permissions]] API provides a uniform way for websites to request permissions from users and query which permissions they have.

The <dfn enum-value for="PermissionName">"xr"</dfn> [=powerful feature=]’s permission-related algorithms and types are defined as follows:


<dl>
<dt>[=permission descriptor type=]</dt>
<dd>

<pre class="idl">
dictionary XRPermissionDescriptor: PermissionDescriptor {
XRSession session;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

sequence&lt;any&gt; requiredFeatures;
sequence&lt;any&gt; optionalFeatures;
};
</pre>

where {{PermissionDescriptor/name}} is {{PermissionName/"xr"}}.


</dd>

<dt>[=permission result type=]</dt>
<dd>
<pre class=idl>
dictionary XRPermissionStatus: PermissionStatus {
sequence&lt;any&gt; granted;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @toji opinions on naming?

};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@mounirlamouri mounirlamouri Dec 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@Manishearth Manishearth Feb 25, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

</pre>

</dd>


<dt>[=permission request algorithm=]</dt>
<dd>
<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. Let |result| be the result of [=resolve the requested features|resolving the requested features=] given |descriptor|'s {{XRPermissionDescriptor/requiredFeatures}}, {{XRPermissionDescriptor/optionalFeatures}}, and {{XRPermissionDescriptor/session}}.
1. If |result| is <code>null</code>, run the following steps:
1. Set |status|'s {{XRPermissionStatus/granted}} to an empty {{FrozenArray}}.
1. Set |status|'s {{PermissionStatus/state}} to {{PermissionState/"denied"}}.
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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"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.

1. If |feature| is not in |granted|, append |feature| to |granted|.
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|.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good point

1. Set |status|'s {{PermissionStatus/state}} to {{PermissionState/"granted"}}.

Note: The user agent may choose to batch up permissions prompts for all requested features when gauging if there is a clear signal of [=user intent=].

</div>


<dt>[=permission query algorithm=]</dt>
<dd>
<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=].
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :(

1. If |status|'s {{PermissionStatus/state}} is {{PermissionState/"denied"}}, set |status|'s {{XRPermissionStatus/granted}} to an empty {{FrozenArray}} and abort these steps.
1. Let |result| be the result of [=resolve the requested features|resolving the requested features=] given |descriptor|'s {{XRPermissionDescriptor/requiredFeatures}}, {{XRPermissionDescriptor/optionalFeatures}}, and {{XRPermissionDescriptor/session}}.
1. If |result| is <code>null</code>, run the following steps:
1. Set |status|'s {{XRPermissionStatus/granted}} to an empty {{FrozenArray}}.
1. Set |status|'s {{PermissionStatus/state}} to {{PermissionState/"denied"}}.
1. Abort these steps.
1. Let <code>(|consentRequired|, |consentOptional|, |granted|)</code> be the fields of |result|.
1. Set |status|'s {{XRPermissionStatus/granted}} to |granted|.
1. If |consentRequired| [=list/is empty=] and |consentOptional| [=list/is empty=], set |status|'s {{PermissionStatus/state}} to {{PermissionState/"granted"}} and abort these steps
1. Set |status|'s {{PermissionStatus/state}} to {{PermissionState/"prompt"}}.

</div>

</dd>

</dl>

<div class="algorithm" data-algorithm="resolve-features">

To <dfn>resolve the requested features</dfn> given |requiredFeatures| and |optionalFeatures| for an {{XRSession}} |session|, the user agent MUST run the following steps:

1. Let |consentRequired| be an empty [=/list=] of {{DOMString}}.
1. Let |consentOptional| be an empty [=/list=] of {{DOMString}}.
1. Let |granted| be a [=/list=] of {{DOMString}} initialized to |session|'s [=list of enabled features=].
1. Add every [=feature descriptor=] in the [=default features=] table associated with |session|'s [=XRSession/mode=] to the indicated feature list if it is not already present.
1. For each |feature| in |requiredFeatures| perform the following steps:
1. If |feature| is not a valid [=feature descriptor=], return <code>null</code>.
1. If |feature| is already in |granted|, continue to the next entry.
1. If the requesting document's origin is not allowed to use any [[#feature-policy|feature policy]] required by |feature| as indicated by the [=feature requirements=] table, return <code>null</code>.
1. If |session|'s [=XRSession/XR device=] is not [=capable of supporting=] the functionality described by |feature| or the user agent has otherwise determined to reject the feature, return <code>null</code>.
1. If the functionality described by |feature| requires [=explicit consent=], append it to |consentRequired|.
1. Else append |feature| to |granted|.
1. For each |feature| in |optionalFeatures| perform the following steps:
1. If |feature| is not a valid [=feature descriptor=], continue to the next entry.
1. If |feature| is already in |granted|, continue to the next entry.
1. If the requesting document's origin is not allowed to use any [[#feature-policy|feature policy]] required by |feature| as indicated by the [=feature requirements=] table, continue to the next entry.
1. If |session|'s [=XRSession/XR device=] is not [=capable of supporting=] the functionality described by |feature| or the user agent has otherwise determined to reject the feature, continue to the next entry.
1. If the functionality described by |feature| requires [=explicit consent=], append it to |consentOptional|.
1. Else append |feature| to |granted|.
1. Return the [=tuple=] <code>(|consentRequired|, |consentOptional|, |granted|)</code>

</div>

Acknowledgements {#ack}
================

Expand Down