Skip to content

Conversation

sirreal
Copy link
Member

@sirreal sirreal commented Jul 17, 2024

Trac ticket: Core-61576

Adds support for TEMPLATE elements in the HTML Processor.

- Tests: 1527, Assertions: 3166, Skipped: 323.
+ Tests: 1525, Assertions: 3277, Skipped: 233.

Copy link

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

  • The Plugin and Theme Directories cannot be accessed within Playground.
  • 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.

Copy link
Member

@dmsnell dmsnell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sirreal I've merged trunk into this and tried to resolve the conflicts accordingly.

was step_in_head() supposed to be in this PR? It's fine if it is, but I didn't know if that was intentional or an artifact of other development.

Copy link
Member

@dmsnell dmsnell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've given this a full review. Many new tests are passing that were previously skipped, but I had to update the html5lib test runner to generate the content label and indentation inside TEMPLATE elements.

In a few spots I made small adjustments for clarity, e.g. changing '-' === $tag_name[0] to $is_closer, and replacing array_push() with a single element with $array[] = $single_element.

On the logic side I added in a check for an empty text node. Perhaps that eventually belongs in step() itself, but for now I wanted it in there in case we get all-NULL text nodes.

Additionally I've incorporated as much as I could from step_in_head() from #6006. There's a check in the META for the document encoding which I didn't think was necessary here, so there are two if statements that lack a condition - I'll add that back in once #6006 is ready, unless it merges first.

Please let me know your thoughts @sirreal, also @gziolo or @ockham or @westonruter. Otherwise I'll see if we can't merge this tomorrow (Tuesday, July 30).

@sirreal sirreal marked this pull request as ready for review July 30, 2024 08:24
Copy link

github-actions bot commented Jul 30, 2024

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, westonruter.

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

@sirreal
Copy link
Member Author

sirreal commented Jul 30, 2024

was step_in_head() supposed to be in this PR?

TEMPLATE required parsing following the rules for "in head". I knew that there were other PRs implementing that, so I tried to do just enough for TEMPLATE.

@@ -1195,15 +1205,16 @@ private function step_in_head(): bool {

/*
* > A start tag whose tag name is "template"
*
* @todo Could the adjusted insertion location be anything other than the current location?
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good question. I've read the spec several times and I don't see how. I'm concerned that means we're missing something subtle 😕

I did check html5ever tree construction, and it does not seem to do any special adjustment which makes me more confident.

@@ -1168,6 +1176,8 @@ private function step_in_head(): bool {

/*
* > A start tag whose tag name is "script"
*
* @todo Could the adjusted insertion location be anything other than the current location?
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In this case, html5ever does account for it, but I still don't see how it behaves differently.

I don't think this should block the PR, but I do want to follow-up.

@sirreal
Copy link
Member Author

sirreal commented Jul 30, 2024

I've done a careful review. This looks good to me, thanks for your additions @dmsnell 👍

Copy link
Member

@westonruter westonruter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, with one observation.

Also, the title of this PR should be updated to be more broad, as it is definitely more than just TEMPLATE support.

* > tentative, then change the encoding to the resulting encoding.
*/
$charset = $this->get_attribute( 'charset' );
if ( is_string( $charset ) ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this only bail if $charset is not UTF-8?

Suggested change
if ( is_string( $charset ) ) {
if ( is_string( $charset ) && ! is_utf8_charset( $charset ) ) {

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah there's an interplay here with #6977 that's an artifact due to sharing code but not merging everything at once. the test here is whether we support inferring the character set from a META tag, and that only happens if the parser's charset confidence is tentative, but I didn't want to add this code in this PR.

alternatively, we could remove META tag support, but I hope that #6977 merges within the next couple of days, making this all moot

if (
is_string( $http_equiv ) &&
is_string( $content ) &&
0 === strcasecmp( $http_equiv, 'Content-Type' )
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto above. Shouldn't this only bail if the charset is not UTF-8?

I guess this bail is essentially a TODO.

@dmsnell dmsnell changed the title HTML API: Implement TEMPLATE support HTML API: Implement TEMPLATE and related support Jul 30, 2024
pento pushed a commit that referenced this pull request Jul 30, 2024
As part of work to add more spec support to the HTML API, this patch adds
support for the IN TEMPLATE and IN HEAD insertion modes. These changes are
primarily about adding support for TEMPLATE elements in the HTML Processor,
but include support for other tags commonly found in the document head, such
as LINK, META, SCRIPT, STYLE, and TITLE.

Developed in #7046
Discussed in https://core.trac.wordpress.org/ticket/61576

Props: dmsnell, jonsurrell, westonruter.
See #61576.


git-svn-id: https://develop.svn.wordpress.org/trunk@58833 602fd350-edb4-49c9-b593-d223f7449a82
@dmsnell
Copy link
Member

dmsnell commented Jul 30, 2024

Merged in [58833]
dfd1de0

@dmsnell dmsnell closed this Jul 30, 2024
@dmsnell dmsnell deleted the html-api/add-template-support branch July 30, 2024 18:46
markjaquith pushed a commit to markjaquith/WordPress that referenced this pull request Jul 30, 2024
As part of work to add more spec support to the HTML API, this patch adds
support for the IN TEMPLATE and IN HEAD insertion modes. These changes are
primarily about adding support for TEMPLATE elements in the HTML Processor,
but include support for other tags commonly found in the document head, such
as LINK, META, SCRIPT, STYLE, and TITLE.

Developed in WordPress/wordpress-develop#7046
Discussed in https://core.trac.wordpress.org/ticket/61576

Props: dmsnell, jonsurrell, westonruter.
See #61576.

Built from https://develop.svn.wordpress.org/trunk@58833


git-svn-id: http://core.svn.wordpress.org/trunk@58229 1a063a9b-81f0-0310-95a4-ce76da25c4cd
github-actions bot pushed a commit to platformsh/wordpress-performance that referenced this pull request Jul 30, 2024
As part of work to add more spec support to the HTML API, this patch adds
support for the IN TEMPLATE and IN HEAD insertion modes. These changes are
primarily about adding support for TEMPLATE elements in the HTML Processor,
but include support for other tags commonly found in the document head, such
as LINK, META, SCRIPT, STYLE, and TITLE.

Developed in WordPress/wordpress-develop#7046
Discussed in https://core.trac.wordpress.org/ticket/61576

Props: dmsnell, jonsurrell, westonruter.
See #61576.

Built from https://develop.svn.wordpress.org/trunk@58833


git-svn-id: https://core.svn.wordpress.org/trunk@58229 1a063a9b-81f0-0310-95a4-ce76da25c4cd
aslamdoctor pushed a commit to aslamdoctor/wordpress-develop that referenced this pull request Dec 28, 2024
As part of work to add more spec support to the HTML API, this patch adds
support for the IN TEMPLATE and IN HEAD insertion modes. These changes are
primarily about adding support for TEMPLATE elements in the HTML Processor,
but include support for other tags commonly found in the document head, such
as LINK, META, SCRIPT, STYLE, and TITLE.

Developed in WordPress#7046
Discussed in https://core.trac.wordpress.org/ticket/61576

Props: dmsnell, jonsurrell, westonruter.
See #61576.


git-svn-id: https://develop.svn.wordpress.org/trunk@58833 602fd350-edb4-49c9-b593-d223f7449a82
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants