-
Notifications
You must be signed in to change notification settings - Fork 22.5k
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
Update Storage Access API content #26558
Conversation
Preview URLs (8 pages)
Flaws (44)Note! 6 documents with no flaws that don't need to be listed. 🎉 URL:
URL:
External URLs (3)URL:
(comment last updated: 2023-05-16 08:50:28) |
Note: Tech reviewed by Chris Fredrickson from the Chromium engineering team (he chose to give me feedback via email); updates made. |
LGTM, thank you! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple of comments and fixes outstanding, but otherwise looks good, tnx 👍🏻
Co-authored-by: Brian Thomas Smith <brian@smith.berlin>
Co-authored-by: Brian Thomas Smith <brian@smith.berlin>
Co-authored-by: Brian Thomas Smith <brian@smith.berlin>
Co-authored-by: Brian Thomas Smith <brian@smith.berlin>
Co-authored-by: Brian Thomas Smith <brian@smith.berlin>
Co-authored-by: Brian Thomas Smith <brian@smith.berlin>
Co-authored-by: Brian Thomas Smith <brian@smith.berlin>
} else { | ||
document.hasStorageAccess().then((hasAccess) => { | ||
async function handleCookieAccess() { | ||
if (document.hasStorageAccess === null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@chrisdavidmills I am not quite sure this code is correct. If this API does not exist then this value would be undefined
and not null
. Am I missing something here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sidenote, I encountered this as we're trying to implement this API for Keycloak (keycloak/keycloak#19835).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ooops, yes you are right. I am just attempting to show a basic member existence check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets correct it in the docs? Or maybe it is better to make check !("hasStorageAccess" in document)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And seems like storage-access
permission is not supported in Safari. @jonkoops can you confirm it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, I forgot to fix this problem. See #27718
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@teleginzhenya this is correct, the following code will throw an exception in Safari:
const permission = await navigator.permissions.query({
name: "storage-access",
});
We recently had to fix this in Keycloak (keycloak/keycloak#21325). This also seems to affect older versions of Firefox. This should be wrapped in a try
block.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Description
This PR updates the documentation for the Storage Access API, which is being shipped in Chrome 113. In particular I:
See my research document for the overall details of what this project entails.
Motivation
Additional details
Related issues and pull requests