-
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
Document changes to action.openPopup API #22267
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.
We found quite some interesting behaviors and edge cases during the development of this feature. Did you already check and rule out their relevance for the documentation?
files/en-us/mozilla/add-ons/webextensions/api/action/openpopup/index.md
Outdated
Show resolved
Hide resolved
files/en-us/mozilla/add-ons/webextensions/api/action/openpopup/index.md
Outdated
Show resolved
Hide resolved
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.
Changes plus suggested changes, good with me. Thanks @oliverdunk
Preview URLs
(comment last updated: 2023-08-29 17:52:39) |
Pinging @oliverdunk there are unaddressed reviews. |
Sorry for the delay on this. I haven't had a chance to look over the feedback yet but will try to do it soon. |
@oliverdunk let me know if you don't have time to complete this, I'd be happy to finish it up |
Thanks for the ping! I've tried to address the feedback - feel free to also make any changes on the branch that make sense.
We could probably add a lot more of the different inconsistencies as mentioned in w3c/webextensions#160 (comment). I've left that for now since it seems like a rabbit hole to try and document (and hopefully we'll fix some of them) but happy for anyone else to add on :) |
@Rob--W I can't merge this while it still has your request for changes on it. The specific items you mentioned have been addressed. As this is now nine months old, perhaps we can address any need for documentation of the nuances separately? |
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.
Fine by me!
Description
Updates the documentation for the
action.openPopup
API following changes across browsers, including the Firefox changes in https://phabricator.services.mozilla.com/D139796.I've removed the note about user gesture requirements from the main body since this does not apply in any other browser (see https://docs.google.com/document/d/1xzuw1dMuf5Lub8UPME2zjxyp3nCKXk-3jtSY2kdI-FQ/edit?usp=sharing for Chrome) or Safari. Instead I've added it as a compatibility note in mdn/browser-compat-data#18202.
All browsers now also support a
windowId
parameter so I've added documentation for that.Motivation
I've been working on this API recently and want to make sure the documentation is an up to date point of reference.
Additional details
Related issues and pull requests
Depends on: mdn/browser-compat-data#18202