-
-
Notifications
You must be signed in to change notification settings - Fork 809
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
extension: Upgrade Firefox to Manifest V3 #16808
Conversation
ae9dc71
to
14fbfd3
Compare
I thought that with Firefox 127 the installation prompt will now include all permissions?
https://blog.mozilla.org/addons/2024/05/14/manifest-v3-updates/ |
Interesting. Surely we want to keep this working on older Firefox versions though, right? The onboarding page only appears if hasAllUrlsPermission returns false. |
Also the new permissionsButton popup is still nice because it handles the described situation where "MV3 extensions should still leverage the permissions API to ensure that the permissions required are already granted." |
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.
Thank you!
@@ -34,6 +34,7 @@ | |||
</div> | |||
</div> | |||
<div class="buttons-container"> | |||
<button class="hidden" id="permissions-button">Permanently enable for this site</button> |
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.
I wonder if we should highlight this button a bit more, so it's at least more prominent than the "open SWF player" button?
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.
This can be a follow-up. I'm not good with design. I did move this button to a separate buttons-container
div, so that it ends up on a separate line explicitly rather than just by a quirk of the CSS.
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.
I think this is fine to land as-is. Users running without <all_urls> are probably used to pain.
aafa0e6
to
4257916
Compare
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.
Looks ok. If we get complaints about reloading tabs we can still fix it.
1f4a9c4
to
32fee0a
Compare
This works.
The only reason it's a draft is because the onboarding page looks awful.Manifests have been merged, please test on Chrome as well. If you have Firefox 126 (one below stable), test there too, as it treats host_permissions differently.