Make WordPress Core

Opened 12 months ago

Closed 12 months ago

Last modified 12 months ago

#58179 closed defect (bug) (fixed)

HTML API: Accumulate shift for internal parsing pointer.

Reported by: dmsnell's profile dmsnell Owned by: bernhard-reiter's profile Bernhard Reiter
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 #4371

While working on WordPress/block-interactivity-experiments#141 @ockham discovered that the Tag Processor was making a small mistake. After making some updates the parser wasn't returning to the start of the affected tag. A test case was created in WordPress/wordpress-develop#4355.

In few words, the Tag Processor has not been treating its own internal pointer bytes_already_parsed the same way it treats its bookmarks. That is, when updates are applied to the input document and then get_updated_html() is called, the internal pointer transfers to the newly-updated content as if no updates had been applied since the previous call to get_updated_html().

In this patch we're creating a new "shift accumulator" to account for all of the updates that accrue before calling get_updated_html(). This accumulated shift will be applied when swapping the input document with the output buffer, which should result in the pointer pointing to the same logical spot in the document it did before the udpate.

In effect this patch adds a single workaround for treating the internal pointer like a bookmark, plus a temporary pointer which points to the beginning of the current tag when calling get_updated_html(). This will preserve the assumption that updating a document doesn't move that pointer, or shift which tag is currently matched.

Change History (13)

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


12 months ago
#1

Trac #58179-ticket

While working on WordPress/block-interactivity-experiments#141 @ockham discovered that the Tag Processor was making a small mistake. After making some updates the parser wasn't returning to the start of the affected tag. A test case was created in #4355.

In few words, the Tag Processor has not been treating its own internal pointer bytes_already_parsed the same way it treats its bookmarks. That is, when updates are applied to the input document and then get_updated_html() is called, the internal pointer transfers to the newly-updated content as if no updates had been applied since the previous call to get_updated_html().

In this patch we're creating a new "shift accumulator" to account for all of the updates that accrue before calling get_updated_html(). This accumulated shift will be applied when swapping the input document with the output buffer, which should result in the pointer pointing to the same logical spot in the document it did before the udpate.

In effect this patch adds a single workaround for treating the internal pointer like a bookmark, plus a temporary pointer which points to the beginning of the current tag when calling get_updated_html(). This will preserve the assumption that updating a document doesn't move that pointer, or shift which tag is currently matched.

@zieladam commented on PR #4371:


12 months ago
#2

Instead of introducing a new concept of accumulated shift, could this be just another bookmark?

@dmsnell commented on PR #4371:


12 months ago
#3

@adamziel removed the "duality" of $this->output_buffer and $this->bytes_already_copied. thanks for pushing for that.

more tests pass now, including those that aren't part of this PR, and there are fewer code paths and fewer _things_ in the processor.

#4 @dmsnell
12 months ago

The description is now out of date with the description on the GitHub PR. I cannot edit it, so please refer to the GitHub PR instead of the top of this ticket for information about the patch.

@Bernhard Reiter commented on PR #4371:


12 months ago
#5

Thank you very much for working on this @dmsnell !

I've tested this against https://github.com/WordPress/block-interactivity-experiments/pull/141, and I'm happy to confirm that it fixes all failing unit tests that I added there while attempting to pin-point the issue! 🎉

Outside of unit tests, in markup produced by the Movies Demo, there is one extraneous <template > wrapper that I get when using this branch that I don't encounter when using trunk:

{{{html

<template data-wp-show="selectors.wpmovies.isVideosTab"><template ><div aria-hidden

role="tabpanel"


data-wp-bind.aria-hidden="selectors.wpmovies.isImagesTab"
aria-labelledby="wpmovies-videos-tab"

<div class="...

}}}

Interestingly, that is the same behavior that I get when using Core trunk with https://github.com/WordPress/block-interactivity-experiments/pull/141, plus applying my earlier fix (https://github.com/WordPress/block-interactivity-experiments/pull/215/commits/9ccf375dff2fc32fc105ca2cbe61e2731c5a47c9) from https://github.com/WordPress/block-interactivity-experiments/pull/215. It seems like for the purposes of wp-show, that fix is equivalent to this PR -- AFAICT, the markup generated by both is the same.

This includes another instance of three nested extraneous <template >s:

{{{html
<template data-wp-show="selectors.wpmovies.isPlaying"><template ><template ><template ><div id="wp-movies-video-player" class="wpmovies-video-player wp-block-wpmovies-video-player">

<div class="wpmovies-video-wrapper">

}}}

... that I originally thought were due to repeated processing of inner blocks and thus could be fixed by applying my other commit (https://github.com/WordPress/block-interactivity-experiments/pull/215/commits/a8cadaca5aceb74433bbb6c367dbfea26d303c0f, later extracted into https://github.com/WordPress/block-interactivity-experiments/pull/227). _However,_ I discovered that that purported fix introduced another issue :facepalm:

Anyway, I don't think the latter is a blocker for this PR. I also don't think we need to spend much more time on investigating that latter issue; I believe I've found a blunt-yet-effective workaround for it: https://github.com/WordPress/block-interactivity-experiments/pull/141/commits/3dbff3f2696b9f2e00cacd5dafb9bd40ba298c29 (pushed to https://github.com/WordPress/block-interactivity-experiments/pull/141). Since testing fixes in Core against the Movies Demo with https://github.com/WordPress/block-interactivity-experiments/pull/141 is also kinda finicky and time-consuming (I ended up destroying my local wp-env cache a couple of times for good measure to make sure no stale copies of Core were used), I'd be happy to proceed with landing this current PR plus https://github.com/WordPress/block-interactivity-experiments/pull/141 with my workaround. Together, they seem to fix all issues I've seen with wp-show.

#6 @Bernhard Reiter
12 months ago

  • Keywords commit added

#7 @Bernhard Reiter
12 months ago

  • Owner set to Bernhard Reiter
  • Resolution set to fixed
  • Status changed from new to closed

In 55706:

HTML API: Accumulate shift for internal parsing pointer.

A bug was discovered where where the parser wasn't returning to the
start of the affected tag after making some updates.

In few words, the Tag Processor has not been treating its own internal
pointer bytes_already_parsed the same way it treats its bookmarks.
That is, when updates are applied to the input document and then
get_updated_html() is called, the internal pointer transfers to
the newly-updated content as if no updates had been applied since
the previous call to get_updated_html().

In this patch we're creating a new "shift accumulator" to account for
all of the updates that accrue before calling get_updated_html().
This accumulated shift will be applied when swapping the input document
with the output buffer, which should result in the pointer pointing to
the same logical spot in the document it did before the udpate.

In effect this patch adds a single workaround for treating the
internal pointer like a bookmark, plus a temporary pointer which points
to the beginning of the current tag when calling get_updated_html().
This will preserve the assumption that updating a document doesn't
move that pointer, or shift which tag is currently matched.

Props dmsnell, zieladam.
Fixes #58179.

@Bernhard Reiter commented on PR #4371:


12 months ago
#8

Committed to Core trunk in `r55706`.

#9 @Bernhard Reiter
12 months ago

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

Reopening for consideration for 6.2.

#10 @Bernhard Reiter
12 months ago

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

In 55708:

HTML API: Accumulate shift for internal parsing pointer.

A bug was discovered where where the parser wasn't returning to the
start of the affected tag after making some updates.

In few words, the Tag Processor has not been treating its own internal
pointer bytes_already_parsed the same way it treats its bookmarks.
That is, when updates are applied to the input document and then
get_updated_html() is called, the internal pointer transfers to
the newly-updated content as if no updates had been applied since
the previous call to get_updated_html().

In this patch we're creating a new "shift accumulator" to account for
all of the updates that accrue before calling get_updated_html().
This accumulated shift will be applied when swapping the input document
with the output buffer, which should result in the pointer pointing to
the same logical spot in the document it did before the udpate.

In effect this patch adds a single workaround for treating the
internal pointer like a bookmark, plus a temporary pointer which points
to the beginning of the current tag when calling get_updated_html().
This will preserve the assumption that updating a document doesn't
move that pointer, or shift which tag is currently matched.

Props dmsnell, zieladam.
Merges [55706] to the 6.2 branch.
Fixes #58179.

#11 @Bernhard Reiter
12 months ago

  • Keywords fixed-major removed

@Bernhard Reiter commented on PR #4371:


12 months ago
#12

Committed to Core's 6.2 branch in `r55708`.

#13 @swissspidy
12 months ago

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