HTML API: Ensure final / in unquoted attribute values is not treated as self-closing

[sirreal]

<g attr=/> is not a self-closing tag. It is a tag with the [attr="/"] attribute.

The self-closing flag is largely irrelevant except in foreign content and this requires a very unusual unquoted attribute value ending in /.

Trac ticket: https://core.trac.wordpress.org/ticket/65372

Use of AI Tools

---

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.

[github-actions]

Hi there! 👋

Thank you for your contribution to WordPress! 💖

It looks like this is your first pull request to wordpress-develop. Here are a few things to be aware of that may help you out!

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:

• PHP Coding Standards
• CSS Coding Standards
• HTML Coding Standards
• JavaScript Coding Standards
• Accessibility Coding Standards
• Inline Documentation Standards

Thank you,
The WordPress Project

[github-actions]

Test using WordPress Playground

The 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

• All changes will be lost when closing a tab with a Playground instance.
• All changes will be lost when refreshing the page.
• A fresh instance is created each time the link below is clicked.
• Every time this pull request is updated, a new ZIP file containing all changes is created. If changes are not reflected in the Playground instance,
  it's possible that the most recent build failed, or has not completed. Check the list of workflow runs to be sure.

For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation.

Test this pull request with WordPress Playground.

[sirreal]

That's a tricky one!

[github-actions]

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 props-bot label.

Core Committers: Use this line as a base for the props when committing in SVN:

Props jonsurrell, dmsnell.

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

[dmsnell]

how about we leave it as it was, but add …before the end of the document, or before some other stop condition.?

the change is placing the emphasis on whether to continue rather than whether something was found.

[sirreal]

I'll roll this back, it doesn't need to land with the rest of this fix.

I changed this specifically because I was confused by what the return value means here. Specifically these lines:

wordpress-develop/src/wp-includes/html-api/class-wp-html-tag-processor.php

Lines 2215 to 2223 in aa4e8ec

	if ( $attribute_end >= $doc_length ) {
	$this->parser_state = self::STATE_INCOMPLETE_INPUT;
	return false;
	}
	if ( $this->is_closing_tag ) {
	return true;
	}

I was surprised because closing tags don't have attributes. I expected a false return in that case.

The return condition really indicates whether attribute parsing has reached a stop condition:

wordpress-develop/src/wp-includes/html-api/class-wp-html-tag-processor.php

Lines 998 to 1001 in aa4e8ec

	// Parse all of its attributes.
	while ( $this->parse_next_attribute() ) {
	continue;
	}

wordpress-develop/src/wp-includes/html-api/class-wp-html-tag-processor.php

Lines 1467 to 1469 in aa4e8ec

	while ( $this->parse_next_attribute() ) {
	continue;
	}

wordpress-develop/src/wp-includes/html-api/class-wp-html-tag-processor.php

Lines 1677 to 1679 in aa4e8ec

	while ( $this->parse_next_attribute() ) {
	continue;
	}

It's confusing because attributes are parsed in close tags, they're just discarded.

[dmsnell]

yeah but I think it’s good to focus on what the parser returns in the positive sense. we use it as a continuation flag because if we found an attribute, we want to continue looking for the next one.

it indicates whether to continue parsing, but a true doesn’t imply there are more attributes, only that it found an attribute. likewise, false only means that we found no attribute when attempting to parse one.

you found and fixed the existing bug in the docs: this could be the case when we finish parsing the document or it could be the case that there was some unexpected stop condition.

in other words, if we focus on the stop condition, we have an asymmetric return value: in the true sense it says “keep going” but doesn’t speak to whether we found an attribute. in harmony with all of the other boolean-returning methods, a false signifies that it found what it was looking for or not.

[dmsnell]

Left a note and a pre-approval.
There’s a case of removing an attribute and leaving an ambiguous trailing /> that came up in my fuzz-testing, but I may have fixed it already.

<div a b=3 /b>
<!-- remove attribute b -->
<div a />

something like this

[sirreal]

There’s a case of removing an attribute and leaving an ambiguous trailing /> that came up in my fuzz-testing

Nice one, I'll see if variant of that is reproducible and add some unit tests.

[sirreal]

I plan to land this and address that on its own. Attribute removal behavior with the self-closing flag are a different issue.

[sirreal]

Merged in r62595.

[sirreal]

There’s a case of removing an attribute and leaving an ambiguous trailing /> that came up in my fuzz-testing, but I may have fixed it already.

<div a b=3 /b>
<!-- remove attribute b -->
<div a />

something like this

Work-in-progress: #12368