HTML API: Track spans of text with (offset, length) instead of (start, end) #5721

dmsnell · 2023-11-30T19:54:49Z

This patch follows-up with earlier design questions around how to represent spans of strings inside the class. It's relevant now as preparation for #5683.

The mixture of (offset, length) and (start, end) coordinates becomes confusing at times and all final string operations are performed with the (offset, length) pair, since these feed into strlen().

In preparation for exposing all tokens within an HTML document this change:

Unifies the representation throughout the class.
It creates token_starts_at to track the start of the current token.
It replaces tag_ends_at with token_length for re-use with other token types.

There should be no functional or behavioral changes in this patch.

For the internal helper classes this patch introduces breaking changes, but those classes are marked private and should not be used outside of the HTML API itself.

Trac ticket:

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.

adamziel · 2023-12-09T15:36:30Z

src/wp-includes/html-api/class-wp-html-attribute-token.php

 	 * @var int
 	 */
 	public $start;

 	/**
-	 * The string offset after the attribute value or its name.
+	 * Byte length of the entire attribute name or name and value pair expression.


Readability nitpick:

Suggested change

* Byte length of the entire attribute name or name and value pair expression.

* A byte length of either:

* * The entire attribute name, or

* * A name and value pair expression

Thanks! I updated the wording in f2c715d to include different wording and now seeing this I've added f6d3a64

adamziel · 2023-12-09T15:38:12Z

src/wp-includes/html-api/class-wp-html-span.php

-	 * @since 6.2.0
+	 * @since {WP_VERSION}


Let's not remove 6.2.0

I think this is actually a defect of the diff view. I'm not actually removing the old comment, but removing the old property and its attached docblock. this is the new @since tag for the new $length property, and the @since tag for the now-removed $end property is gone.

at first I added it back in but I found that more confusing, and the class itself has a @since tag noting the removal of the attribute. thoughts?

adamziel · 2023-12-09T15:38:29Z

src/wp-includes/html-api/class-wp-html-span.php

-	 * @param int $start Byte offset into document where replacement span begins.
-	 * @param int $end   Byte offset into document where replacement span ends.
+	 * @param int $start  Byte offset into document where replacement span begins.
+	 * @param int $length Byte length of span.


Suggested change

* @param int $length Byte length of span.

* @param int $length Byte length of this span.

Added in 045b5c0

adamziel · 2023-12-09T15:45:32Z

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

+			$bookmark_start = $bookmark->start;
+			$bookmark_end   = $bookmark_start + $bookmark->length;


Could these allocations be removed?

$bookmark_end is hard to remove because we have to compute it, and I left $bookmark_start in for symmetry. it wasn't offering much otherwise so I took it out in 449ae5a

these aren't really allocations either, as they will be created locally on the stack.

adamziel

I left some notes, but it looks good to me otherwise!

adamziel · 2023-12-09T15:56:51Z

src/wp-includes/html-api/class-wp-html-attribute-token.php

@@ -15,6 +15,7 @@
 *
 * @access private
 * @since 6.2.0
+ * @since {WP_VERSION} Replaced `end` with `length` to more closely match `substr()`.


Just a note to remember to update it before merging

adamziel · 2023-12-10T10:18:55Z

@dmsnell The unit tests fail now. Once they're fixed and the @since annotations are adjusted, we should be ready to merge.


1) Tests_HtmlApi_WpHtmlProcessorBreadcrumbs::test_remains_stable_when_editing_attributes
Undefined variable $bookmark_start

/var/www/src/wp-includes/html-api/class-wp-html-tag-processor.php:1654
/var/www/src/wp-includes/html-api/class-wp-html-tag-processor.php:2331
/var/www/tests/phpunit/tests/html-api/wpHtmlProcessorBreadcrumbs.php:449
/var/www/vendor/bin/phpunit:122

dmsnell · 2023-12-10T11:21:31Z

@adamziel everything should be buttoned up and working properly now

…, end) This patch follows-up with earlier design questions around how to represent spans of strings inside the class. It's relevant now as preparation for WordPress#5683. The mixture of (offset, length) and (start, end) coordinates becomes confusing at times and all final string operations are performed with the (offset, length) pair, since these feed into `strlen()`. In preparation for exposing all tokens within an HTML document this change: - Unifies the representation throughout the class. - It creates `token_starts_at` to track the start of the current token. - It replaces `tag_ends_at` with `token_length` for re-use with other token types. There should be no functional or behavioral changes in this patch. For the internal helper classes this patch introduces breaking changes, but those classes are marked private and should not be used outside of the HTML API itself.

adamziel · 2023-12-10T13:18:22Z

Merged in https://core.trac.wordpress.org/changeset/57179.

…d Crash Previously the HTML API was using a mixture of approaches in its helper classes for describing spans of text. One was (start, end) and the other was (start, length). In WordPress/wordpress-develop#5721 these were all normalized to (start, length) but the Interactivity API was not updated in concert with that change. In this patch the `inner_html` methods are updated to use the appropriate formats. A following PR is necessary to update the compat layer so that crashes don't appear on sites running older versions of WordPress with this updated code.

Fix: Update Interactivity API Use of Private HTML API Classes to avoid Crash Previously the HTML API was using a mixture of approaches in its helper classes for describing spans of text. One was (start, end) and the other was (start, length). In WordPress/wordpress-develop#5721 these were all normalized to (start, length) but the Interactivity API was not updated in concert with that change. In this patch the `inner_html` methods are updated to use the appropriate formats. this PR also includes the necessary back port of the HTML API to provide the new methods so that older WP installs don’t crash

This was referenced Dec 8, 2023

HTML API: avoid processing incomplete tokens #5725

Closed

HTML API: Provide mechanism to scan all tokens in an HTML document, not only the tags. #5683

Closed

adamziel reviewed Dec 9, 2023

View reviewed changes

adamziel approved these changes Dec 9, 2023

View reviewed changes

adamziel reviewed Dec 9, 2023

View reviewed changes

dmsnell force-pushed the html-api/use-start-length-instead-of-start-end branch from b502273 to 279e43d Compare December 10, 2023 10:42

dmsnell force-pushed the html-api/use-start-length-instead-of-start-end branch from 279e43d to 0346ceb Compare December 10, 2023 11:51

adamziel closed this Dec 10, 2023

dmsnell deleted the html-api/use-start-length-instead-of-start-end branch December 10, 2023 13:33

dmsnell mentioned this pull request Dec 10, 2023

Fix: Update Interactivity API Use of Private HTML API Classes to avoid Crash WordPress/gutenberg#56928

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HTML API: Track spans of text with (offset, length) instead of (start, end) #5721

HTML API: Track spans of text with (offset, length) instead of (start, end) #5721

dmsnell commented Nov 30, 2023 •

edited

Loading

adamziel Dec 9, 2023 •

edited

Loading

dmsnell Dec 9, 2023

adamziel Dec 9, 2023

dmsnell Dec 9, 2023

adamziel Dec 9, 2023

dmsnell Dec 9, 2023

adamziel Dec 9, 2023

dmsnell Dec 9, 2023

dmsnell Dec 9, 2023

adamziel left a comment

adamziel Dec 9, 2023

adamziel commented Dec 10, 2023

dmsnell commented Dec 10, 2023

adamziel commented Dec 10, 2023

-	 * Byte length of the entire attribute name or name and value pair expression.
+	 * A byte length of either:
+	 * * The entire attribute name, or
+	 * * A name and value pair expression

	* @param int $length Byte length of span.
	* @param int $length Byte length of this span.

		$bookmark_start = $bookmark->start;
		$bookmark_end = $bookmark_start + $bookmark->length;

HTML API: Track spans of text with (offset, length) instead of (start, end) #5721

HTML API: Track spans of text with (offset, length) instead of (start, end) #5721

Conversation

dmsnell commented Nov 30, 2023 • edited Loading

adamziel Dec 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamziel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamziel commented Dec 10, 2023

dmsnell commented Dec 10, 2023

adamziel commented Dec 10, 2023

dmsnell commented Nov 30, 2023 •

edited

Loading

adamziel Dec 9, 2023 •

edited

Loading