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

Update Storage Access API data #21336

Merged
merged 12 commits into from
Nov 28, 2023

Conversation

tunetheweb
Copy link
Contributor

@tunetheweb tunetheweb commented Nov 23, 2023

Summary

Updates Storage Access API:

Removed first party sets data based on #21292 (comment) as it became supported in that case afterwards.

Test results and supporting details

Linked above

Related issues

Last updated in #20094 where it was noted that it had restricted support but that is no longer the case

@github-actions github-actions bot added the data:api 🐇 Compat data for Web APIs. https://developer.mozilla.org/docs/Web/API label Nov 23, 2023
@queengooborg queengooborg self-requested a review November 24, 2023 00:11
api/Document.json Outdated Show resolved Hide resolved
api/Document.json Outdated Show resolved Hide resolved
queengooborg added a commit to openwebdocs/mdn-bcd-collector that referenced this pull request Nov 24, 2023
@tunetheweb
Copy link
Contributor Author

tunetheweb commented Nov 24, 2023

Saw a comment about Firefox support which I think has been since delated. I did have this comment to cover that:

access-storage permission supported in Firefox from 117 (not in release notes but narrowed it down by testing)

Edit: since was made aware of the link: https://bugzilla.mozilla.org/show_bug.cgi?id=1805860

but here's some more evidence:

Works in 117:
image

Doesn't work in 116:

image

@tunetheweb
Copy link
Contributor Author

FYI @chrisdavidmills as hear that you might also be working on this.

@johannhof
Copy link

Note that the name of the permission is storage-access. :)

Yes, Firefox has added permission API support in 117, see https://bugzilla.mozilla.org/show_bug.cgi?id=1805860

@tunetheweb
Copy link
Contributor Author

Well that'll explain why I couldn't find it :-) Updated first comment with a link.

@chrisdavidmills
Copy link
Collaborator

FYI @chrisdavidmills as hear that you might also be working on this.

@tunetheweb This is superb; I was actually about to do this stuff, but you beat me to it ;-)

I am currently working on a update to the docs that explains all this stuff, and also includes content on RWS. Not sure if this is something you want to do in this PR, but I was intending to add a specific sub-data point for RWS inside requestStorageAccess. Rationale being that RWS is basically a subfeature of SAA, but there is not really a better place to put it, and I want to provide RWS-specific data on the RWS page.

This would include: supported from Chrome 113, but it was called FPS up until Chrome 117.

@tunetheweb
Copy link
Contributor Author

Ha ha! Currently doing some docs on developer.chrome.com on Chrome's implementation and asking myself the same question as to whether RWS and requestStorageAccessFor belongs in a Storage Access API post or not!

Let's land this BCD change, as think it's good to go, and then open a separate one for any extra changes you want for that as might be more discussion on whether that belongs or not.

api/Permissions.json Outdated Show resolved Hide resolved
api/Document.json Outdated Show resolved Hide resolved
api/Document.json Outdated Show resolved Hide resolved
@queengooborg
Copy link
Collaborator

Would you be down to split this into three separate PRs, one for each browser engine? I think that Chrome and Firefox are good to go, but Safari isn't quite ready, and it would be great to get the other two engine's changes landed right away!

@tunetheweb
Copy link
Contributor Author

Do we need three or can I just remove the Safari changes from here and then merge this?

@queengooborg
Copy link
Collaborator

I'd say that so long as the Safari changes are separated, two PRs is sufficient!

@tunetheweb
Copy link
Contributor Author

Reverted the Safari changes so think this is good to merge.

I'm happy to leave Safari as is. It was a minor note anyway and think @chrisdavidmills is planning on updating the doc page on it anyway (though that won't feed through to any consumers like caniuse like the note suggestion would have).

@queengooborg
Copy link
Collaborator

Thank you -- actually, could we revert the Firefox changes as well? Using the collector, I just confirmed that Firefox 65 (well, technically, Firefox 72 or earlier) is accurate.

api/Document.json Outdated Show resolved Hide resolved
@tunetheweb
Copy link
Contributor Author

tunetheweb commented Nov 26, 2023

Thank you -- actually, could we revert the Firefox changes as well? Using the collector, I just confirmed that Firefox 65 (well, technically, Firefox 72 or earlier) is accurate.

Thats fair for the hasStorageAccess() and requestSttorageAccess() methods since the earlier version was supported since then so I’ve reverted that.

However, I’ve left the Firefox Permisson Policy change in from 117 as that was changed from then as per evidence above and previously it said it wasn’t supported at all so that does need updating.

Copy link
Collaborator

@queengooborg queengooborg left a comment

Choose a reason for hiding this comment

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

Thank you, this is LGTM now!

@queengooborg queengooborg merged commit 31e6bba into mdn:main Nov 28, 2023
4 checks passed
@tunetheweb
Copy link
Contributor Author

Thanks for the thorough review and helping catch a lot of things @queengooborg !

@tunetheweb tunetheweb deleted the update-storage-access-api-versions branch November 28, 2023 16:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
data:api 🐇 Compat data for Web APIs. https://developer.mozilla.org/docs/Web/API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants