Make WordPress Core

Opened 12 months ago

Closed 12 months ago

Last modified 12 months ago

#58160 closed defect (bug) (fixed)

HTML API: Fix case where updates are overlooked when seeking to earlier locations.

Reported by: dmsnell's profile dmsnell Owned by: zieladam's profile zieladam
Milestone: 6.2.1 Priority: normal
Severity: normal Version: 6.2
Component: HTML API Keywords: has-patch has-unit-tests
Focuses: Cc:

Description

GitHub PR#4345

In certain cases when seeking to an earlier location in a document after changes have been enqueued, the tag processor will lose the earlier changes.

In this patch we're fixing the bug so that it properly retains the changes that were applied before the call to seek().

The problem was that when changes had already been applied and no more were enqueued, the tag processor was returning the updated HTML without updating its internal state to reflect that. This was probably an optimization for that case but I can't remember exactly why it doesn't update the internal pointers (@adamziel maybe you remember?). The fix is simple: mirror the second case and update the internal pointers.

cc: @ockham

Change History (10)

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


12 months ago
#1

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

Trac ticket #58160-trac

In certain cases when seeking to an earlier location in a document after changes have been enqueued, the tag processor will lose the earlier changes.

In this patch we're fixing the bug so that it properly retains the changes that were applied before the call to seek().

The problem was that when changes had already been applied and no more were enqueued, the tag processor was returning the updated HTML without updating its internal state to reflect that. This was probably an optimization for that case but I can't remember exactly why it doesn't update the internal pointers (@adamziel maybe you remember?). The fix is simple: mirror the second case and update the internal pointers.

This ticket was mentioned in PR #4355 on WordPress/wordpress-develop by @Bernhard Reiter.


12 months ago
#2

In https://github.com/WordPress/block-interactivity-experiments/pull/141, we found an issue with the Tag Processor that seems to be vaguely related to seeking after adding an attribute. We tried to distill this into a minimal test case for which @dmsnell found a fix in #4345.

Unfortunately, applying that fix to https://github.com/WordPress/block-interactivity-experiments/pull/141 proved insufficient -- the issue persisted :confused:

Part of the problem is that it’s fairly hard to reproduce issues that we’ve found in the wild (e.g. when running the Movies Demo against https://github.com/WordPress/block-interactivity-experiments/pull/141). I’ve been moderately successful at isolating a somewhat scaled-down test there, which -- while still not fully capturing all issues -- at least fails even with #4345 applied. Unfortunately, it involves an extra class with a newly introduced class method 😕

I’ve decided to carry this test over to this repo so we have at least a baseline to try our fixes out with, even though it’s far from minimal for now.

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

#3 @zieladam
12 months ago

  • Owner set to zieladam
  • Resolution set to fixed
  • Status changed from new to closed

In 55675:

HTML API: Fix a case where updates are overlooked when seeking to earlier locations.

This retains the WP_HTML_Tag_Processor attribute updates applied before calling seek() – they were erroneously erased in some cases.

Props dmsnell.
Fixes #58160.

@dmsnell commented on PR #4345:


12 months ago
#4

Merged in 55675

@Bernhard Reiter commented on PR #4355:


12 months ago
#5

Closing this, as it has served its purpose. @dmsnell managed to distill this into a minimal test case, and fixed the underlying issue in https://github.com/WordPress/wordpress-develop/pull/4371 🎉

#6 @Bernhard Reiter
12 months ago

  • Keywords fixed-major added
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening for consideration for 6.2.

While this is mostly rendered obsolete by r58179, I'll backport it to 6.2 anyway for consistency with trunk, and to avoid merge conflicts when backporting r58179.

#7 @Bernhard Reiter
12 months ago

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

In 55707:

HTML API: Fix a case where updates are overlooked when seeking to earlier locations.

This retains the WP_HTML_Tag_Processor attribute updates applied before calling seek() – they were erroneously erased in some cases.

Props dmsnell, zieladam.
Merges [55675] to the 6.2 branch.
Fixes #58160.

@Bernhard Reiter commented on PR #4345:


12 months ago
#8

Backported to the 6.2 branch in `r55707`.

#9 @Bernhard Reiter
12 months ago

  • Keywords fixed-major removed

#10 @swissspidy
12 months ago

  • Milestone changed from Awaiting Review to 6.2.1
Note: See TracTickets for help on using tickets.