Make requestPermission()'s behavior more backwards-compatible #129
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The changes added in #123 have introduced some behavior changes to the two requestPermission() operations that were not originally intended. Although the original steps were heavily modeled after the WebKit implementation and do not follow what other specifications normally do, some behavior changes from #123 have not been discussed and have been made more backwards-compatible: - Calls to "request permission to use" may return "prompt", while the previous version implicitly converted anything that was not "granted" to "denied" (possibly because the current Safari implementation shows a modal dialog asking for access, so users can only allow or deny the permission request, so remaining at the "prompt" state is not possible). From a specification perspective, if a user has not explicitly granted the permission request, "prompt" is essentially the same as "denied" anyway, so we now explicitly set anything that is not "granted" to "denied" again. - The previous steps only checked for transient activation if users had not made an explicit permission choice yet, which basically means that the permission state is set to "prompt". This change restores this behavior by first checking the current permission states for all required permission names and only throwing a NotAllowed exception if one of them is "prompt" and there is no transient activation. In other words, if a permission is either "granted" or "denied" then requestPermission() will resolve to one of the values without requiring transient activation. While here, refer to the right definitions of "granted" and "denied": use the PermissionState enum definitions, which is what the Permissions API algorithms we invoke return, rather than the non-Web IDL definitions that these enum values correspond to.
kenchris
reviewed
Jan 10, 2024
| @@ -306,20 +306,24 @@ The <dfn attribute for="DeviceOrientationEvent">absolute</dfn> attribute must re | |||
| The <dfn method for="DeviceOrientationEvent">requestPermission(<var>absolute</var>)</dfn> method steps are: | |||
|
|
|||
| 1. Let <var>global</var> be the <a>current global object</a>. | |||
| 1. If <a>this</a>'s <a>relevant global object</a> does not have <a>transient activation</a>, return <a>a promise rejected with</a> a "{{NotAllowedError}}" {{DOMException}}. | |||
| 1. Let <var>hasTransientActivation</var> be true if <a>this</a>'s <a>relevant global object</a> has <a>transient activation</a>, and false otherwise. | |||
There was a problem hiding this comment.
shouldn't it be "or false, otherwise" ? or "or else false"
There was a problem hiding this comment.
The "[...], and/or $SOMETHING otherwise" idiom is used in several specs, they just don't agree on whether to use "and" or "or".
kenchris
reviewed
Jan 10, 2024
| 1. Let <var>result</var> be <a>a new promise</a> in <a>this</a>'s <a>relevant Realm</a>. | ||
| 1. Run these steps <a>in parallel</a>: | ||
| 1. If <var>absolute</var> is true: | ||
| 1. Let <var>permissions</var> be « "<a permission>accelerometer</a>", "<a permission>gyroscope</a>", "<a permission>magnetometer</a>" ». | ||
| 1. Otherwise: | ||
| 1. Let <var>permissions</var> be « "<a permission>accelerometer</a>", "<a permission>gyroscope</a>" ». | ||
| 1. Let <var>permissionState</var> be "<a for="permission">granted</a>". | ||
| 1. <a for="list">For each</a> <var>name</var> of <var>permissions</var>: | ||
| 1. If <var>name</var>'s <a>permission state</a> is "{{PermissionState/prompt}}" and <var>hasTransientActivation</var> is false: |
There was a problem hiding this comment.
if you check hasTransientActivation here in each iteration and that value never changes, why not move that check outside?
There was a problem hiding this comment.
Not sure I understood the suggestion. The check here is basically "if the user needs to be prompted but there's no transient activation, reject the promise and stop", so I can't just check for transient activation separately.
kenchris
reviewed
Jan 10, 2024
| 1. Let <var>permissionState</var> be "<a for="permission">granted</a>". | ||
| 1. <a for="list">For each</a> <var>name</var> of <var>permissions</var>: | ||
| 1. If <var>name</var>'s <a>permission state</a> is "{{PermissionState/prompt}}" and <var>hasTransientActivation</var> is false: | ||
| 1. <a>Queue a global task</a> on the <a>device motion and orientation task source</a> given <var>global</var> to <a>reject</a> <var>result</var> with a "{{NotAllowedError}}" {{DOMException}}. |
There was a problem hiding this comment.
I would probably add a comma before "to reject"
There was a problem hiding this comment.
Do you mean "Queue a global task, to [...]"? Isn't it usual to just say "queue a task to xyz"?
reillyeon
approved these changes
Jan 18, 2024
github-actions bot
added a commit
that referenced
this pull request
Jan 22, 2024
SHA: 0b538e9 Reason: push, by rakuco Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The changes added in #123 have introduced some behavior changes to the two
requestPermission() operations that were not originally intended.
Although the original steps were heavily modeled after the WebKit
implementation and do not follow what other specifications normally do, some
behavior changes from #123 have not been discussed and have been made more
backwards-compatible:
previous version implicitly converted anything that was not "granted" to
"denied" (possibly because the current Safari implementation shows a modal
dialog asking for access, so users can only allow or deny the permission
request, so remaining at the "prompt" state is not possible).
From a specification perspective, if a user has not explicitly granted the
permission request, "prompt" is essentially the same as "denied" anyway,
so we now explicitly set anything that is not "granted" to "denied" again.
made an explicit permission choice yet, which basically means that the
permission state is set to "prompt".
This change restores this behavior by first checking the current
permission states for all required permission names and only throwing a
NotAllowed exception if one of them is "prompt" and there is no transient
activation. In other words, if a permission is either "granted" or
"denied" then requestPermission() will resolve to one of the values
without requiring transient activation.
While here, refer to the right definitions of "granted" and "denied": use
the PermissionState enum definitions, which is what the Permissions API
algorithms we invoke return, rather than the non-Web IDL definitions that
these enum values correspond to.
Preview | Diff