Avoid iterating and adjusting the bookmarks at the same time

As we loop through $this->attribute_updates, we keep comparing $position->start and $position->end to $diff->start. We can't change it and still expect the correct result, so this commit accumulates the deltas separately and applies them all at once after the loop.
WordPress · Nov 26, 2022 · 152556b · 152556b
1 parent b2a0e39
commit 152556b
Show file tree

Hide file tree

Showing 2 changed files with 46 additions and 7 deletions.
diff --git a/lib/experimental/html/class-wp-html-tag-processor.php b/lib/experimental/html/class-wp-html-tag-processor.php
@@ -1150,28 +1150,42 @@ private function apply_attributes_updates() {
 		usort( $this->attribute_updates, array( self::class, 'sort_start_ascending' ) );
 
 		foreach ( $this->attribute_updates as $diff ) {
-			$this->updated_html .= substr( $this->html, $this->updated_bytes, $diff->start - $this->updated_bytes );
-			$this->updated_html .= $diff->text;
+			$this->updated_html  .= substr( $this->html, $this->updated_bytes, $diff->start - $this->updated_bytes );
+			$this->updated_html  .= $diff->text;
 			$this->updated_bytes = $diff->end;
+		}
+
+		foreach ( $this->bookmarks as $position ) {
+			/**
+			 * As we loop through $this->attribute_updates, we keep comparing
+			 * $position->start and $position->end to $diff->start. We can't
+			 * change it and still expect the correct result, so let's accumulate
+			 * the deltas separately and apply them all at once after the loop.
+			 */
+			$head_delta = 0;
+			$tail_delta = 0;
 
-			foreach ( $this->bookmarks as &$position ) {
+			foreach ( $this->attribute_updates as $diff ) {
 				$update_head = $position->start >= $diff->start;
 				$update_tail = $position->end >= $diff->start;
 
 				if ( ! $update_head && ! $update_tail ) {
-					continue;
+					break;
 				}
 
 				$delta = strlen( $diff->text ) - ( $diff->end - $diff->start );
 
 				if ( $update_head ) {
-					$position->start += $delta;
+					$head_delta += $delta;
 				}
 
 				if ( $update_tail ) {
-					$position->end += $delta;
+					$tail_delta += $delta;
 				}
 			}
+
+			$position->start += $head_delta;
+			$position->end   += $tail_delta;
 		}
 
 		$this->attribute_updates = array();

diff --git a/phpunit/html/wp-html-tag-processor-bookmark-test.php b/phpunit/html/wp-html-tag-processor-bookmark-test.php
@@ -37,7 +37,32 @@ public function test_bookmark() {
 		);
 	}
 
-	public function test_bookmark_complex(  ) {
+	public function test_removing_long_attributes_doesnt_break_seek(  ) {
+		$input = <<<HTML
+		<button class="....................................................." data-details-container></button>
+		<button></button>
+HTML;
+		$p = new WP_HTML_Tag_Processor( $input );
+		$p->next_tag( 'button' );
+		$p->set_bookmark( 'first' );
+		$p->next_tag( 'button' );
+		$p->set_bookmark( 'second' );
+
+		$this->assertTrue(
+			$p->seek( 'first' ),
+			'Seek() to the first button has failed'
+		);
+		$p->remove_attribute('class');
+		$p->remove_attribute('data-details-container');
+
+		$this->assertTrue(
+			$p->seek( 'second' ),
+			'Seek() to the second button has failed'
+		);
+	}
+
+
+	public function test_bookmark_complex_use_case(  ) {
 		$input = <<<HTML
 <div selected class="merge-message" checked>
 	<div class="select-menu d-inline-block">