KSES: Allow autofocus within dialog elements#12255
Conversation
|
Hi there! 👋 Thank you for your contribution to WordPress! 💖 It looks like this is your first pull request to No one monitors this repository for new pull requests. Pull requests must be attached to a Trac ticket to be considered for inclusion in WordPress Core. To attach a pull request to a Trac ticket, please include the ticket's full URL in your pull request description. Pull requests are never merged on GitHub. The WordPress codebase continues to be managed through the SVN repository that this GitHub repository mirrors. Please feel free to open pull requests to work on any contribution you are making. More information about how GitHub pull requests can be used to contribute to WordPress can be found in the Core Handbook. Please include automated tests. Including tests in your pull request is one way to help your patch be considered faster. To learn about WordPress' test suites, visit the Automated Testing page in the handbook. If you have not had a chance, please review the Contribute with Code page in the WordPress Core Handbook. The Developer Hub also documents the various coding standards that are followed:
Thank you, |
Test using WordPress PlaygroundThe changes in this pull request can previewed and tested using a WordPress Playground instance. WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser. Some things to be aware of
For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation. |
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the Core Committers: Use this line as a base for the props when committing in SVN: To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
|
||
| return wp_kses_split( $content, $allowed_html, $allowed_protocols ); | ||
| $filter_dialog_autofocus = ( | ||
| 'post' === $allowed_html && |
There was a problem hiding this comment.
This only takes effect for the 'post' context. wp_kses( $content, 'data' ), custom allow-lists that include , and other contexts will silently strip autofocus. If that's intentional for 7.1, let's state it explicitly in the docblock so it's a documented limitation rather than a surprise. A test asserting the non-post behavior would lock it in.
There was a problem hiding this comment.
Added docblock note and non-post test
| $allowed_html[ $tag ]['autofocus'] = true; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Hardcoding autofocus/dialog-specific logic into the generic low-level wp_kses() is a concern. This function underpins every sanitization context, and baking one HTML feature's special case inline sets a precedent (the next person wants popover, inert, etc.). Can we either (a) move this behind a documented filter / a general "context-aware attribute" helper, or (b) at minimum add a code comment explaining why it must live here?
cc @westonruter since you raised the "only within a dialog" approach in #64576.
There was a problem hiding this comment.
Added explanatory comment, kept minimal scoped fix.
| } | ||
|
|
||
| /** | ||
| * Removes autofocus attributes unless the element appears within a dialog element. |
There was a problem hiding this comment.
The function strips autofocus from the element itself (not just keeping it on descendants) — the test bakes this in and it's the right behavior, but it's non-obvious from the name. Please note it in the docblock so future readers don't assume dialogs are exempt entirely.
| public function test_wp_kses_post_allows_autofocus_only_inside_dialog() { | ||
| $this->assertEqualHTML( | ||
| '<dialog><button autofocus>Close</button></dialog><button>Outside</button>', | ||
| wp_kses_post( '<dialog autofocus><button autofocus>Close</button></dialog><button autofocus>Outside</button>' ) | ||
| ); | ||
| } |
There was a problem hiding this comment.
The depth-counting explicitly handles nesting but there's no test for it. Please add cases for:
- nested dialogs, e.g. …
- the valued attribute form autofocus="autofocus"
| public function test_wp_kses_post_removes_autofocus_outside_dialog_after_unsupported_html() { | ||
| $this->assertStringNotContainsString( | ||
| 'autofocus', | ||
| wp_kses_post( '<table>text</table><button autofocus>Outside</button>' ) | ||
| ); | ||
| } |
There was a problem hiding this comment.
Please also add a test for a non-post context (ties to the 'post' === $allowed_html comment above) to lock in and document that behavior.
2 participants
Trac ticket: https://core.trac.wordpress.org/ticket/65491
This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.