-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Fix compat notes for Firefox of permissions.request #9374
Fix compat notes for Firefox of permissions.request #9374
Conversation
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.
Thanks for this PR, @Rob--W! A couple of suggestions for you:
-
Structuring this as separate support entries feels kinda heavy weight. They don't really represent large departures of behavior, which is usually how we decide between new support statements or not. How about something like this, instead?
{ "version_added": "55", "notes": [ "Before version 75, permissions cannot be requested from popups (see bug…)." "Before version 61, permissions cannot be requested from options pages embedded in <code>about:addons</code> (see bug…).", ] }
-
For the "out-of-date claims about bugs in support on Firefox desktop" — did that happen recently? If so, we probably ought to keep it around (e.g., "Before version X…"). If it's quite old (or completely unknown), then we can ignore it.
Done.
"out-of-date" in the sense that a feature was documented to be unsupported while they were in fact supported. 75 is relatively recent, so I've added it. 61 is 3 year old, so if you'd like I could remove the note about that one. |
932e031
to
3311b7d
Compare
webextensions/api/permissions.json
Outdated
"notes": [ | ||
"It's not possible to request permissions from a sidebar document (<a href='https://bugzil.la/1493396'>bug 1493396</a>).", | ||
"Before version 75, permissions cannot be requested from popup panels (see <a href='https://bugzil.la/1432083'>bug 1432083</a>).", | ||
"Before version 61, permissions cannot be requested from options pages embedded in about:addons." |
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.
Let's keep the note and make it consistent with the others. Generally, I'm inclined to report things that are true and historic, if it's obvious that it's historic. If it's unclear (or unverifiable) and historic, then I usually drop it.
"Before version 61, permissions cannot be requested from options pages embedded in about:addons." | |
"Before version 61, permissions cannot be requested from options pages embedded in <code>about:addons</code> (see <a href='https://bugzil.la/1382953'>bug 1382953</a>." |
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.
Done. I updated the patch manually since your suggestion was missing a closing parenthesis.
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.
Good eye, thank you!
- Removed compat notes at Firefox for Android. The notes aren't relevant, e.g. sidebars were never supported on Android. - Fix out-of-date claims about bugs in support on Firefox desktop, permissions can be requested from a popup since Firefox 75.
3311b7d
to
1c5e948
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.
I'm really happy with the way this turned out. Thank you for the back and forth, @Rob--W! 🎉
References: