Skip to content
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

Tag Processor: Add bookmark invalidation logic #47559

Merged
merged 8 commits into from
Feb 21, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -1401,6 +1401,7 @@ private function class_name_updates_to_attributes_updates() {
* Applies attribute updates to HTML document.
*
* @since 6.2.0
* @since 6.3.0 Invalidate any bookmarks whose targets are overwritten.
*
* @return void
*/
Expand Down Expand Up @@ -1431,7 +1432,7 @@ private function apply_attributes_updates() {
* Adjust bookmark locations to account for how the text
* replacements adjust offsets in the input document.
*/
foreach ( $this->bookmarks as $bookmark ) {
foreach ( $this->bookmarks as $bookmark_name => $bookmark ) {
/*
* Each lexical update which appears before the bookmark's endpoints
* might shift the offsets for those endpoints. Loop through each change
Expand All @@ -1442,20 +1443,22 @@ private function apply_attributes_updates() {
$tail_delta = 0;

foreach ( $this->lexical_updates as $diff ) {
$update_head = $bookmark->start >= $diff->start;
$update_tail = $bookmark->end >= $diff->start;

if ( ! $update_head && ! $update_tail ) {
if ( $bookmark->start < $diff->start && $bookmark->end < $diff->start ) {
Copy link
Contributor

@adamziel adamziel Feb 21, 2023

Choose a reason for hiding this comment

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

I still think this would read better as:

Suggested change
if ( $bookmark->start < $diff->start && $bookmark->end < $diff->start ) {
if ( $bookmark->end < $diff->start ) {

If $bookmark->start is ever after $bookmark->end, I'd rather make it fail in some way rather than try to salvage it with a defensive condition.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

While we did inline $update_head and $update_tail here, we didn't otherwise change the condition -- it existed like that prior to this PR. For that reason, I'd like to keep it as is 😬 I know it shouldn't make a difference outside of any pathological cases, but since this PR has been sitting for a while, I'd like to de-risk it as much as possible.


Aside: I kinda like that it's symmetrical to the condition right below:

if ( $bookmark->start >= $diff->start && $bookmark->end < $diff->end ) {

I feel it helps me visualize better that the bookmark is either fully past the diff, or enclosed by it.


I'm open to changing this in a follow-up though 😊

break;
}

if ( $bookmark->start >= $diff->start && $bookmark->end < $diff->end ) {
ockham marked this conversation as resolved.
Show resolved Hide resolved
$this->release_bookmark( $bookmark_name );
continue 2;
}

$delta = strlen( $diff->text ) - ( $diff->end - $diff->start );

if ( $update_head ) {
if ( $bookmark->start >= $diff->start ) {
ockham marked this conversation as resolved.
Show resolved Hide resolved
$head_delta += $delta;
}

if ( $update_tail ) {
if ( $bookmark->end >= $diff->end ) {
$tail_delta += $delta;
}
}
Expand All @@ -1467,6 +1470,18 @@ private function apply_attributes_updates() {
$this->lexical_updates = array();
}

/**
* Checks whether a bookmark with the given name exists.
*
* @since 6.3.0
*
* @param string $bookmark_name Name to identify a bookmark that potentially exists.
* @return bool Whether that bookmark exists.
*/
public function has_bookmark( $bookmark_name ) {
return array_key_exists( $bookmark_name, $this->bookmarks );
}

/**
* Move the internal cursor in the Tag Processor to a given bookmark's location.
*
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
<?php
/**
* Unit tests covering Gutenberg_HTML_Tag_Processor_6_3 functionality.
*
* @package WordPress
* @subpackage HTML
*/

require_once __DIR__ . '/../../lib/compat/wordpress-6.3/html-api/class-gutenberg-html-tag-processor-6-3.php';

/**
* @group html-api
*
* @coversDefaultClass Gutenberg_HTML_Tag_Processor_6_3
*/
class Gutenberg_HTML_Tag_Processor_6_3_Bookmark_Test extends WP_UnitTestCase {
/**
* @covers has_bookmark
*/
public function test_has_bookmark_returns_false_if_bookmark_does_not_exist() {
$p = new Gutenberg_HTML_Tag_Processor_6_3( '<div>Test</div>' );
$this->assertFalse( $p->has_bookmark( 'my-bookmark' ) );
}

/**
* @covers has_bookmark
*/
public function test_has_bookmark_returns_true_if_bookmark_exists() {
$p = new Gutenberg_HTML_Tag_Processor_6_3( '<div>Test</div>' );
$p->next_tag();
$p->set_bookmark( 'my-bookmark' );
$this->assertTrue( $p->has_bookmark( 'my-bookmark' ) );
}

/**
* @covers has_bookmark
*/
public function test_has_bookmark_returns_false_if_bookmark_has_been_released() {
$p = new Gutenberg_HTML_Tag_Processor_6_3( '<div>Test</div>' );
$p->next_tag();
$p->set_bookmark( 'my-bookmark' );
$p->release_bookmark( 'my-bookmark' );
$this->assertFalse( $p->has_bookmark( 'my-bookmark' ) );
}
}