#58179 closed defect (bug) (fixed)
HTML API: Accumulate shift for internal parsing pointer.
Reported by: |
|
Owned by: |
|
---|---|---|---|
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.
22 months ago
#1
@zieladam commented on PR #4371:
22 months ago
#2
Instead of introducing a new concept of accumulated shift, could this be just another bookmark?
22 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
@
22 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:
22 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
.
#7
@
22 months ago
- Owner set to Bernhard Reiter
- Resolution set to fixed
- Status changed from new to closed
In 55706:
@Bernhard Reiter commented on PR #4371:
22 months ago
#8
Committed to Core trunk
in `r55706`.
#9
@
22 months ago
- Keywords fixed-major added; commit removed
- Resolution fixed deleted
- Status changed from closed to reopened
Reopening for consideration for 6.2.
@Bernhard Reiter commented on PR #4371:
22 months ago
#12
Committed to Core's 6.2
branch in `r55708`.
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 thenget_updated_html()
is called, the internal pointer transfers to the newly-updated content as if no updates had been applied since the previous call toget_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.