Make WordPress Core

Opened 9 months ago

Closed 9 months ago

Last modified 8 months ago

#57787 closed defect (bug) (fixed)

WP_HTML_Tag_Processor::seek() off by 1 byte when seeking bookmarks set on a closing tag

Reported by: zieladam's profile zieladam Owned by: hellofromtonya's profile hellofromTonya
Milestone: 6.2 Priority: normal
Severity: normal Version: 6.2
Component: HTML API Keywords: gutenberg-merge has-patch has-unit-tests commit
Focuses: Cc:

Description

The HTML API was introduced in #57575. The seek() functionality is off by 1 position when seeking to find bookmarks set on a closing tag.

Setting a bookmark on a tag should set its "start" position before the opening "<", e.g.:

<div> Testing a <b>Bookmark</b>
----------------^

The current calculation assumes this is always one byte to the left from $tag_name_starts_at.

However, in tag closers that index points to a solidus symbol "/":

<div> Testing a <b>Bookmark</b>
----------------------------^

The bookmark should therefore start two bytes before the tag name:

<div> Testing a <b>Bookmark</b>
---------------------------^

Change History (12)

This ticket was mentioned in PR #4115 on WordPress/wordpress-develop by @zieladam.


9 months ago
#1

  • Keywords has-patch has-unit-tests added; needs-patch removed

## Description

With this PR seek() correctly finds bookmarks set on tag closers. It:

  • Uses the correct starting index for tag closer bookmarks
  • Adds array( 'tag_closers' => 'visit' ) in seek()

### About bookmark start indices:

Setting a bookmark on a tag should set its "start" position before the opening "<", e.g.:

<div> Testing a <b>Bookmark</b>
----------------^

The current calculation assumes this is always one byte to the left from $tag_name_starts_at.

However, in tag closers that index points to a solidus symbol "/":

<div> Testing a <b>Bookmark</b>
----------------------------^

The bookmark should therefore start two bytes before the tag name:

<div> Testing a <b>Bookmark</b>
---------------------------^

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

cc @ockham @dmsnell @gziolo

#2 @hellofromTonya
9 months ago

  • Owner set to hellofromTonya
  • Status changed from new to reviewing

Self-assigning for testing and commit review.

#3 @hellofromTonya
9 months ago

  • Reporter changed from @… to zieladam

#4 @hellofromTonya
9 months ago

  • Keywords commit added

Patch: https://github.com/WordPress/wordpress-develop/pull/4115

  • Before applying WP_HTML_Tag_Processor changes, the test fails 🔴
  • After applying the WP_HTML_Tag_Processor changes, the test passes 🟢

The changes work as described ✅ Ready for commit 👍

#5 @hellofromTonya
9 months ago

@zieladam I don't see an upstream issue or PR applying this same change in Gutenberg. Are you planning to backport these changes to Gutenberg to keep both in sync?

#6 @hellofromTonya
9 months ago

With additional PR approvals from @flixos90 and @dmsnell, I'm prepping the commit now.

#8 follow-up: @zieladam
9 months ago

@hellofromTonya yes I am. We're still working out a specific workflow for backporting these patches – it could be a single PR per patch or a batch PR every two weeks. Let's keep this issue open for now.

#9 @hellofromTonya
9 months ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

In 55407:

HTML API: Fix finding bookmarks set on closing tag WP_HTML_Tag_Processor.

Setting a bookmark on a tag should set its "start" position before the opening "<", e.g.:

<div> Testing a <b>Bookmark</b>
----------------^

The previous calculation assumed this is always one byte to the left from $tag_name_starts_at.

However, in a closing tag that index points to a solidus symbol "/":

<div> Testing a <b>Bookmark</b>
----------------------------^

The bookmark should therefore start two bytes before the tag name:

<div> Testing a <b>Bookmark</b>
---------------------------^

This changeset achieves this by:

  • Using the correct starting index for closing tag bookmarks.
  • Adding array( 'tag_closers' => 'visit' ) in WP_HTML_Tag_Processor::seek().

Follow-up to [55203].

Props zieladam, dmsnell, flixos90.
Fixes #57787.
See #57575.

#10 in reply to: ↑ 8 @hellofromTonya
9 months ago

Replying to zieladam:

@hellofromTonya yes I am. We're still working out a specific workflow for backporting these patches – it could be a single PR per patch or a batch PR every two weeks. Let's keep this issue open for now.

Care must be given to if it's a bug fix for 6.2 and when the patch is introduced in the cycle. Beta 4 is next week and then a week later is RC1.

This particular ticket is targeted at one specific fix. Thinking [55407] resolves it. That said, if a follow-up PR is needed to fix something related to bookmark closing tag for 6.2, then please reopen the ticket; else, let's open a new Trac ticket.

#12 @SergeyBiryukov
8 months ago

  • Component changed from Editor to HTML API
Note: See TracTickets for help on using tickets.