HTML API: Tag bookmark length is incorrect

[jonsurrell]

Opening tag tokens in the HTML API are incorrect. I'd expect that substr( $html, $token_start, $token_length ); to produce the entire tag. Currently, this produces the tags up to (but not including) the final >. A tag like <div> has a length 4, so the result would be <div.

This is largely internal, although classes extending the tag processor observe this bug through bookmarks.

It seems like the open tags have incorrect length in the tag processor. I'd expect that substr( $html, $token_start, $token_length ); to produce the entire tag. This patch fixes that, before it would exclude the final > on tags.

This is mostly internal and it seems to have adjustments in several places to fix it, but I'd like to have a consistent internal representation. Other types of tokens behave as I'd expect (with substr( $html, $start, $length )).

@dmsnell thoughts?

<details>

<summary>I'd expect something like this to hold:</summary>

<?php

$t = '<div class="target">text</div>trailing text';
$start = 0;
$token_length = 20;
$text_start = $token_length - $start;
$text_length = 4;

$end_start = $text_start + $text_length;
$end_length = 6;

var_dump(
    substr( $t, $start, $token_length ),
    substr( $t, $text_start, $text_length ),
    substr( $t, $end_start, $end_length ),
);

</details>

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

The test failure in ​#6651 makes me nervous.

@sirreal can you look into the failure in the linked PR? I think it's related to seeking with updates and getting the wrong $this->bytes_already_processed

The test failure in ​https://github.com/WordPress/wordpress-develop/pull/6651 makes me nervous.
Can you look into the failure in the linked PR? I think it's related to seeking with updates and getting the wrong $this->bytes_already_processed

68a3251d307c39467ab85bf739578d15da378d75 was missing from that branch. There was a test that used the broken offset. The offset adjustment was removed in this branch, I pushed to #6651.

@westonruter This is a bugfix for incorrect token lengths in tag bookmarks in the Tag Processor. Subclasses might see and be affected by the bug (and the fix) if they ever work directly with tag bookmarks lengths. I believe you have some plugins that may be doing advanced things with the Tag Processor so wanted to make sure you were aware.

Below is an example fix from this PR, plugins would be impacted in the same way and would need to remove the off-by-one correction on the bookmark lengths:

src/wp-includes/interactivity-api/class-wp-interactivity-api-directives-processor.php

@@ -107 +107 @@
 
                 $bookmark = 'append_content_after_template_tag_closer';
                 $this->set_bookmark( $bookmark );
-                $after_closing_tag = $this->bookmarks[ $bookmark ]->start + $this->bookmarks[ $bookmark ]->length + 1;
+                $after_closing_tag = $this->bookmarks[ $bookmark ]->start + $this->bookmarks[ $bookmark ]->length;
                 $this->release_bookmark( $bookmark );
 
                 // Appends the new content.
@@ -140 +140 @@
                 }
                 list( $opener_tag, $closer_tag ) = $bookmarks;
 
-                $after_opener_tag  = $this->bookmarks[ $opener_tag ]->start + $this->bookmarks[ $opener_tag ]->length + 1;
+                $after_opener_tag  = $this->bookmarks[ $opener_tag ]->start + $this->bookmarks[ $opener_tag ]->length;
                 $before_closer_tag = $this->bookmarks[ $closer_tag ]->start;
 
                 if ( $rewind ) {

[gziolo]

It will need a small section in the miscellaneous dev note for developers that extend the WP_HTML_Tag_Processor.

@sirreal See the following method in ​this subclass of `WP_HTML_Tag_Processor`:

/**
         * Appends HTML to the provided bookmark.
         *
         * @param string $bookmark Bookmark.
         * @param string $html     HTML to inject.
         * @return bool Whether the HTML was appended.
         */
        public function append_html( string $bookmark, string $html ): bool {
                if ( ! $this->has_bookmark( $bookmark ) ) {
                        return false;
                }

                $start = $this->bookmarks[ $bookmark ]->start;

                $this->lexical_updates[] = new WP_HTML_Text_Replacement(
                        $start,
                        $this->old_text_replacement_signature_needed ? $start : 0,
                        $html
                );
                return true;
        }

So since it's not dealing with length then is a patch needed?

So since it's not dealing with length then is a patch needed?

this should be correct, @westonruter

[jonsurrell]

I don't think this needs a full dev note but should definitely have a mention in the release like the following:

A bug was fixed in the WP_HTML_Tag_Processor where tag token bookmarks. This bug affected classes that extend the WP_HTML_Tag_Processor or WP_HTML_Processor classes and access tag bookmark or token lengths directly. Affected classes would have needed to correct the token length by adding 1. The tag length has been fixed and this correction should be removed in WordPress 6.6.

[dmsnell]

In 58233:

HTML API: Fix token length bug in Tag Processor.

The Tag Processor stores the byte-offsets into its HTML document where
the current token starts and ends, and also for every bookmark. In some
cases for tags, the end offset has been off by one.

In this patch the offset is fixed so that a bookmark always properly
refers to the full span of the token it's bookmarking. Also the current
token byte offsets are properly recorded.

While this is a defect in the Tag Processor, it hasn't been exposed
through the public interface and has not affected any of the working
of the processor. Only subclasses which rely on the length of a bookmark
have been potentially affected, and these are not supported environments
in the ongoing work.

This fix is important for future work and for ensuring that subclasses
performing custom behaviors remain as reliable as the public interface.

Developed in ​https://github.com/WordPress/wordpress-develop/pull/6625
Discussed in https://core.trac.wordpress.org/ticket/61301

Props dmsnell, gziolo, jonsurrell, westonruter.
Fixes #61301.

Merged in [58233]
​https://github.com/wordpress/wordpress-develop/commit/6ca5bdc3ac193f3aa196b188baab10ec8857c11e

[dmsnell]

Dev note incorporated into the ​Updates to the HTML API in 6.6 post