-
Notifications
You must be signed in to change notification settings - Fork 3.1k
HTML API: Update customizable <select> parsing #9298
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
base: trunk
Are you sure you want to change the base?
HTML API: Update customizable <select> parsing #9298
Conversation
Remove SELECT case and update comments numbers. The SELECT case was removed from the algorithm in the standard.
This was removed from the HTML standard
These insertion modes are removed from the standard.
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. |
When SELECT > BUTTON > SELECTEDCONTENT is encountered, the selected option may need to be cloned into the SELECTEDCONTENT. The HTML processor does not support this action as it may require out of order processing.
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. |
The html5lib-tests updates were merged: html5lib/html5lib-tests#178 |
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.
It’s taken me some time but I reviewed the changes to the HTML Spec and compared against this and I appreciate the thoroughness with which you implemented them.
- I wanted before to add a reference to the spec version. This would be good to add here, so someone can cross-reference historically when the changes appeared. This can go in the module or class docblock I think.
- We can put this into 6.9, so
{WP_VERSION}
can be updated. - Let’s review and harmonize all of the deprecations, both in coordinating the docblock annotations with the call to
_deprecated_function()
and in emptying things like the select scope code.
Not yet have I reviewed this against the SELECTEDCONTENT
changes, but I think we will be able to largely ignore the semantics, since that appears at render time by the browser. It may be worth adding a note in the class docs indicating the level of support.
* | ||
* @param string $tag_name Name of tag to check. | ||
* @return bool Whether the given element is in SELECT scope. | ||
*/ | ||
public function has_element_in_select_scope( string $tag_name ): bool { | ||
_deprecated_function( __METHOD__, '{WP_VERSION}' ); |
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 still don’t know how the docblock tag and the function call interact. Do you feel like you have a strong understand? Are both expected/appropriate?
* | ||
* @param string $tag_name Name of tag to check. | ||
* @return bool Whether the given element is in SELECT scope. | ||
*/ | ||
public function has_element_in_select_scope( string $tag_name ): bool { | ||
_deprecated_function( __METHOD__, '{WP_VERSION}' ); |
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 we should also be able to remove the body from this function and always return false
since the actual select scope no longer exists
* > 1. Parse error. | ||
* > 2. Ignore the token. | ||
*/ | ||
if ( null !== $this->context_node && 'SELECT' === $this->context_node->node_name ) { |
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 appear to be inconsistent here, but I think I have tried to use isset( $this->context_node )
over null
checks.
* > An end tag whose tag name is "option" | ||
* | ||
* The "option" end tag is handled in the any other end tag section below. | ||
*/ |
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.
do we need this comment here? I am guessing it was added because it used to be in the code as a specific case
but now it isn’t? If so, I don’t see a reason to duplicate the description that it’s handled by the catch-all.
Trac ticket: #63736
Update HTML API parsing to account for changes in whatwg/html#10548.
Apply html5lib/html5lib-tests#178.
Todo:
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.