HTML Tag Processor: Add WP 6.3 compat layer

[ockham]

What?

Part of #44410.

Copy lib/compat/wordpress-6.2/html-api/class-wp-html-tag-processor.php to lib/compat/wordpress-6.3/html-api/class-wp-html-tag-processor.php, and rename the class to Gutenberg_HTML_Tag_Processor_6_3.

Why?

So we can continue developing the Tag Processor while retaining an exact copy of WP_HTML_Tag_Processor (as was merged into Core) in the 6.2 compat layer.

How?

By copying the file from the 6.2 compat layer, and renaming the class to Gutenberg_HTML_Tag_Processor_6_3 (and having it extend WP_HTML_Tag_Processor). This is consistent with established practices.

Note that since we're deriving from WP_HTML_Tag_Processor, we wouldn't really need to copy the entire class definition from the 6.2 layer. However, with PRs such as #47559 (which is modifying a private method), we kind of do after all 😬

Testing Instructions

Currently, the best test is to diff lib/compat/wordpress-6.2/html-api/class-wp-html-tag-processor.php and lib/compat/wordpress-6.3/html-api/class-wp-html-tag-processor.php and to verify that only the class WP_HTML_Tag_Processor line was changed to class Gutenberg_HTML_Tag_Processor_6_3 extends WP_HTML_Tag_Processor.

[github-actions]

Flaky tests detected in 1d39251d152cb55855b9f4543397cf572b221adb.
Some tests passed with failed attempts. The failures may not be related to this commit but are still reported for visibility. See the documentation for more information.

🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4226314867
📝 Reported issues:

• [Flaky Test] should always expand single line selection #39787 in specs/editor/various/multi-block-selection.test.js
• [Flaky Test] should handle Enter across blocks #39763 in specs/editor/various/multi-block-selection.test.js
• [Flaky Test] should leave a scheduled page scheduled if canceled #47499 in specs/editor/various/switch-to-draft.test.js
• [Flaky Test] should revert a scheduled page to a draft if confirmed #47500 in specs/editor/various/switch-to-draft.test.js

[adamziel]

@ockham Couldn't we use the same Gutenberg class for 6.2 and 6.3 compat?

[ockham]

@ockham Couldn't we use the same Gutenberg class for 6.2 and 6.3 compat?

It depends 😬 We could in theory remove the WP_ version (that's identical to the one we merged into Core) if we update all call sites to use the Gutenberg_..._6_3 version instead, and change that class to not even extend WP_HTML_Tag_Processor, since it's a full copy anyway.

The textbook example where I'd definitely opt to keep both classes would be if the 6.3 one could actually inherit from the 6.2 one, but since we're modifying a private method here, that point is kind of moot.

There's another aspect: By having callers use the WP_ version, we:

• State that they only need the API and behavior as found in that class.
• Allow removal of Gutenberg's copy of WP_HTML_Tag_Processor from the 6.2 compat layer once WP 6.4 is released (since GB will only need to support WP 6.4 and 6.3 at that point).

However, this might also be a bit academic, since we'll have to retain the Gutenberg_..._6_3 version until WP 6.5 is released anyway.

I don't have any strong preference either way. Curious to hear y'all's thoughts 😊

[adamziel]

@ockham here's what Gutenberg needs to work with different WP versions:

• WP <= 6.1 – no tag processor is shipped – Gutenberg must provide a shim
• WP >= 6.2 – WP_HTML_Tag_Processor is shipped – Gutenberg needs its own version

How about we only ship Gutenberg_HTML_Tag_Processor in the plugin AND the following polyfill?

if(!class_exist('WP_HTML_Tag_Processor')) {
    class WP_HTML_Tag_Processor extends Gutenberg_HTML_Tag_Processor {}
}

Advantages:

• No duplicate code
• Compat with all WP versions

Disadvantages:

• ...?

[ockham]

I'm a bit apprehensive about deriving WP_HTML_Tag_Processor from Gutenberg_HTML_Tag_Processor, since WP_HTML_Tag_Processor is what's also in Core, so the API and behavior for both versions (Core and GB) should be identical IMO to avoid any nasty surprises.

It makes more sense to me to have Gutenberg_HTML_Tag_Processor(_6_3) extend WP_HTML_Tag_Processor, since it's fine for the Gutenberg_ prefixed version of the class to have a different API and behavior.

[adamziel]

the API and behavior for both versions (Core and GB) should be identical IMO to avoid any nasty surprises.

That's exactly why I propose to derive the WP class from the GB class. A change in the behavior of a released public API is a BC break – I'd rather detect it surprises before 6.3 core merge.

[ockham]

the API and behavior for both versions (Core and GB) should be identical IMO to avoid any nasty surprises.

That's exactly why I propose to derive the WP class from the GB class. A change in the behavior of a released public API is a BC break – I'd rather detect it surprises before 6.3 core merge.

Not sure I see how that would work 🤔 For reference, I'll try to describe "my" scenario (Gutenberg_HTML_Tag_Processor_6_3 extends WP_HTML_Tag_Processor -- BTW it seems to me that that's more "established practice"):

• WP_HTML_Tag_Processor remains unchanged.
• We add e.g. bookmark invalidation (Tag Processor: Add bookmark invalidation logic #47559) to Gutenberg_HTML_Tag_Processor_6_3 (including a public has_bookmark method).
• Most of our blocks continue to use WP_HTML_Tag_Processor, since they don't need any of the new features.
• We add or update some code to use Gutenberg_HTML_Tag_Processor_6_3 instead, to make use of the new features.

When preparing for the 6.3 Core merge, we should discover that Gutenberg_HTML_Tag_Processor_6_3 is used in a few places, and backport the features we have added to it to Core's WP_HTML_Tag_Processor (with @since PHPDocs etc.)

Can you describe "your" scenario? 😊

[adamziel]

@ockham here's my scenario:

• WP_HTML_Tag_Processor is an empty class inheriting from Gutenberg_HTML_Tag_Processor
• We add e.g. bookmark invalidation (Tag Processor: Add bookmark invalidation logic #47559) to Gutenberg_HTML_Tag_Processor (including a public has_bookmark method).
• Most of our blocks continue to use WP_HTML_Tag_Processor, since they don't need any of the new features.
• If any of them fail, we spot that well before WordPress 6.3
• If any users report issues related to Gutenberg_HTML_Tag_Processor, it's great – we just avoided merging a bug to WP 6.3

When preparing for the 6.3 Core merge, we should discover that Gutenberg_HTML_Tag_Processor is used in a few places, and backport the features we have added to it to Core's WP_HTML_Tag_Processor (with @SInCE PHPDocs etc.)

It's similar

[dmsnell]

Alternative idea: what about moving all of the further development of the HTML API into core directly? I was going to start by suggesting we not allow any Gutenberg code to use the new version until it's in Core, but then if we do things there I think we can avoid all of this work.

Right now the Tag Processor resolves its motivating need; future evolution is less about fixing a decades-old problem and more about opening up new possibilities nobody has used yet.

The unit tests are in Core, the code is in Core, maybe we let this be and shift over there?

[ockham]

Thank you @adamziel for elaborating your scenario! I've thought a bit more about it, and I think I'd be really uncomfortable to have Gutenberg declare a WP_HTML_Tag_Processor that's not identical to the one in Core 😅 IMO, there's too much potential for confusion if we start using the same name (whose WP_ prefix also indicates it's "owned" by Core) and have Gutenberg give it different meaning (however subtle).

The unit tests are in Core, the code is in Core, maybe we let this be and shift over there?

@dmsnell If it was just about developing the HTML (Tag) Processor in isolation, I'd agree 100% and would keep development inside of wordpress-develop. However, since my (our? 😅) main driver is still implementing the directives, I'd like to have new features available more quickly/easily, which is why I'd like to continue adding them to GB. (Our block-hydration-experiments repo is relying on the presence of the GB plugin already; eventually, I expect most of that codebase to be absorbed into GB.)

[ockham]

If the above reasoning more or less 😬 works for y'all @adamziel @dmsnell -- would you be okay if we merged this PR? It's becoming a bit of a blocker to the more interesting stuff (see).

(I'm happy to tweak it later if we see fit -- I don't think it's locking us into this particular approach too badly.)

[adamziel]

@dmsnell I like developing in Gutenberg because of the testing and feedback we get along the way – Gutenberg is installed on hundreds of thousands of sites. You may recall this has already helped solve a few critical bugs that we might have otherwise not found in time.

IMO, there's too much potential for confusion if we start using the same name (whose WP_ prefix also indicates it's "owned" by Core) and have Gutenberg give it different meaning (however subtle).

@ockham I agree some confusion would likely follow, but I think it would be worth it. Nevertheless, I don't insist. Let's move forward with this PR.

[adamziel]

Why Gutenberg_HTML_Tag_Processor_6_3 and not just Gutenberg_HTML_Tag_Processor? Including 6_3 in the name means all the consumers of this API must specifically opt-in to this version – I don't think that's useful. For once, we wouldn't notice any E2E failures breaking the existing consumers.

[dmsnell]

would you be okay if we merged this PR?

based on the merge process we went through and the persistent complaints I hear from folks working on split-codebases I feel quite reluctant to merge this, having the impression we're setting ourselves up for more frustration later on.

cc: @azaozz

It's becoming a bit of a blocker to the more interesting stuff

because of the testing and feedback we get along the way – Gutenberg is installed on hundreds of thousands of sites.

developing within WordPress/WordPress-develop doesn't need to prevent us from using it in Gutenberg before a Core merge and release. maybe if we have something in development over there we can keep a copy here in Gutenberg and leave it hidden behind a if ( ! class_exists( … ) ) check.

I think something like that could resolve all of our needs, and as we've established, there's no reason we can't create our own vessel to work in while developing if the Core copy lags. as long as we're all coordinating and communicating and not rushing ahead of ourselves then we can still get what I think we're looking for by doing this work here.

I've become fond of git rebase --rebase-merges as it makes up for issues that were traditionally more difficult with maintaining long-running and inter-related branches.

in other words:

• we can develop in WordPress/WordPress-develop and pull updates back into Gutenberg as we make them so that they are available in Gutenberg but only as a compat module.
• we can work in spaghetti branches that keep abreast of multiple concurrent developments and maintain those with git rebase --rebase-merges

in fact as I think about it it seems much easier to do this in Core and pull directly into GB via compat updates. essentially working in reverse with "blessed" PRs into Gutenberg, but no need for mangling names or coordinating releases; just a long-running branch in Core.

[ockham]

Quick reply (going to read/think/reply more in-depth tomorrow): I'm fine with trying your suggestion @dmsnell. However...

...on a practical level, I see one obstacle: Core is now in Beta stage, so essentially, only bugfixes are allowed to be merged into trunk. This will only change with RC 1 (scheduled for March 7), when a dedicated 6.2 branch will be created, upon which trunk will be open for non-bugfix stuff again.

For the next three weeks, this could get somewhat in the way of what we hope to be an easier development flow 😕

[dmsnell]

Core is now in Beta stage, so essentially, only bugfixes are allowed to be merged into trunk.

yeah @ockham this is the part where a long-running branch comes into play. we can embrace git for what it is, and maintain the code outside of trunk until people are ready, but review and development and discussion could happen there in the meantime.

I don't know that this will solve the problem, but it does seem to eliminate the need to have all of the funny naming and hasty changes during Core merge.

[ockham]

I'll pull in a comment by @dmsnell from #47559 (comment), since it seems relevant to this PR:

Pending discussion on how to proceed, what I imagine we might do is end up getting compat/wordpress-6.3 in as a copy of what's in the long-running branch over at WordPress/WordPress-develop, and once this merges there, we can file a simpler update PR whose description is more like, "pull in updates from 6.3 HTML API"

TBH there's one part that I don't like about that strategy, and that's the fact that we'd only carry over updates from the long-running branch in wordpress-develop into GB once that branch is merged into wordpress-develop's trunk.

My priority has always been to implement the remaining directives in the Block Interactivity API, and to that end, to make the missing features of the HTML (Tag) Processor available to it as swiftly as possible (see). This has worked nicely as long as they were developed inside of Gutenberg, as we could simply require the latest Gutenberg version as a prerequisite.

The strategy you're proposing is kind of the diametrical opposite: Changes to the HTML (Tag) Processor would reach Gutenberg last rather than first 😅 Since Core will be in Beta Freeze until March 7, that would be the earliest date we'd be able to land the long-running branch, and it would make its way into Gutenberg no sooner than the first GB release after that.

So I'd rather avoid that TBH, as I worry it would add friction to implementing the directives, which are kind of a high priority to the team.

---

FWIW, I think we can avoid at least some of the issues of the Core merge process that we've seen during the 6.2 cycle. This time around, we can make sure we merge our code early during the upcoming 6.3 cycle into wordpress-develop -- basically as soon as trunk is open for business, come March 7.

We'll still need a compat layer in GB, and it should probably carry a funny name 😉 in order to tell it apart from Core's WP_HTML_Tag_Processor, but I don't think we could avoid that by developing in wordpress-develop and backporting it into Gutenberg 🤔

The one thing I'll be sorely missing in GB is wordpress-develop's full suite of unit tests (as we've discussed elsewhere). Since this also affects any other GB code that was merged into wordpress-develop alongside its tests, we could consider maybe running that suite inside of GB? (I've floated the idea before.)

Are there any other things we'd get from developing inside wordpress-develop? Maybe we can find a good-enough substitute for them as well 😊

[dmsnell]

Would have merged this @ockham but I wanted to give you the chance to review my changes.

Are you running an autoformatter? It seems like your patch undid style changes pushed in Core on the module.

I removed the extends because I don't know what it offers and because it introduces a real difference between this code and what's in Core. If this class remains a blind dump of updates in Core then I think we can just leave it identical except for adding the suffix, and I think that will ease updates since we won't have to start tracking a series of differences between Core's copy and Gutenberg's

git diff lib/compat/wordpress-6.3/html-api/class-wp-html-tag-processor-6.3.php ../wordpress-develop/src/wp-includes/html-api/class-wp-html-tag-processor.php

[gziolo]

I removed the extends because I don't know what it offers and because it introduces a real difference between this code and what's in Core.

Not a blocker. If you have discussed it already then feel free to ignore it, but it makes it possible now to simplify the polyfill for WP_HTML_Tag_Processor for older versions of WordPress to:

class WP_HTML_Tag_Processor extends Gutenberg_HTML_Tag_Processor_6_3 {}

This way you could release changes in the Gutenberg plugin without refactoring usage to Gutenberg_HTML_Tag_Processor_6_3 until WordPress 6.2 is out. It would continue to work with the latest code with WordPress 6.0 and 6.1, but once 6.2 is out it feels like the only way to use the latest API would be calling new Gutenberg_HTML_Tag_Processor_6_3() in the plugin.

[adamziel]

@gziolo we've had quite a discussion about this :-)

[azaozz]

• WP <= 6.1 – no tag processor is shipped – Gutenberg must provide a shim
• WP >= 6.2 – WP_HTML_Tag_Processor is shipped – Gutenberg needs its own version

Currently this is a bit more complex :)

• WP >= 6.3 – WP_HTML_Tag_Processor in core is newer than the plugin but may be missing "experimental" parts that were in Gutenberg_HTML_Tag_Processor_6_3. Ideally Gutenberg would still use its own version to avoid any chances of errors.

This case (older Gutenberg in newer WP) should actually be "doing it wrong" but don't think it is prevented for now. Just tried installing Gutenberg 11.8.3 in WP 6.2-beta2 and the plugin activated without problems. (I actually started a PR for fixing this, will revisit soon).

So imho Gutenberg should probably always use its own version. In WP <= 6.1 there's no expectation to have WP_HTML_Tag_Processor. Providing the (different) Gutenberg_HTML_Tag_Processor_6_3 as a shim would likely cause problems for developers rather than be helpful because of these differences.

[azaozz]

developing within WordPress/WordPress-develop doesn't need to prevent us from using it in Gutenberg before a Core merge and release.

can make sure we merge our code early during the upcoming 6.3 cycle into wordpress-develop

Frankly I think it doesn't make much difference which repo is "main" and which is "secondary" as long as the code is synced regularly. As WP_HTML_Tag_Processor is already in core, thinking that any (non-experimental) changes to it can be committed to WP 6.3-alpha immediately after they are merged to Gutenberg. That way the "pre-beta merge sprint+bottleneck" will be eliminated, and the new code will also be tested in WP 6.3 nightly builds (at least few large sites run these), which may help to discover new bugs/inconsistencies.

[ockham]

Thank you @dmsnell for fixing the array syntax (no idea where that came from); I'm also fine with removing the extends, since it doesn't add any value indeed.

Thank you @adamziel @gziolo and @azaozz for your reviews, suggestions, and feedback!

I'm gonna land this now 🎉🚢💥

[ockham]

I've rebased and re-run the Performance Tests a couple of times, and they're still failing with the same error:

Error: Command failed: npm run test:performance -- packages/e2e-tests/specs/performance/site-editor.test.js
FAIL packages/e2e-tests/specs/performance/site-editor.test.js (59.777 s)
  Site Editor Performance
    ✓ Loading (78[85](https://github.com/WordPress/gutenberg/actions/runs/4224201637/jobs/7336076145#step:6:86) ms)
    ✕ Typing (32013 ms)

  ● Site Editor Performance › Typing

    TimeoutError: waiting for selector `[data-type="core/post-content"] [data-type="core/paragraph"]` failed: timeout 30000ms exceeded

      144 |
      145 | 		// Measuring typing performance inside the post content.
    > 146 | 		await canvas().waitForSelector(
          | 		               ^
      147 | 			'[data-type="core/post-content"] [data-type="core/paragraph"]'
      148 | 		);
      149 | 		await enterEditMode();

      at new WaitTask (../../node_modules/puppeteer-core/src/common/DOMWorld.ts:813:28)
      at DOMWorld.waitForSelectorInPage (../../node_modules/puppeteer-core/src/common/DOMWorld.ts:656:22)
      at Object.internalHandler.waitFor (../../node_modules/puppeteer-core/src/common/QueryHandler.ts:78:19)
      at DOMWorld.waitForSelector (../../node_modules/puppeteer-core/src/common/DOMWorld.ts:511:25)
      at Frame.waitForSelector (../../node_modules/puppeteer-core/src/common/FrameManager.ts:1273:47)
      at Object.<anonymous> (specs/performance/site-editor.test.js:146:18)
          at runMicrotasks (<anonymous>)

Test Suites: 1 failed, 1 total
Tests:       1 failed, 1 passed, 2 total
Snapshots:   0 total
Time:        59.[89](https://github.com/WordPress/gutenberg/actions/runs/4224201637/jobs/7336076145#step:6:90)5 s

I'll look into this locally -- maybe there is a reproducible problem that results from loading the compat layer from lib/load.php (although upon first glance, I'd think that everything should be safe, since we're using an entirely different class name for the Tag Processor here 🤔 )

[ockham]

Alright, looks like Performance Tests have been failing for all of the most recently merged PRs. I guess I'll do what everyone else seems to be doing 🙈

Edit: Hold on, I seem to have lost @dmsnell's commit (see). I've now added it back. I'll let CI finish; might merge it only tomorrow, as I'm near the end of my day. (Unless you wanna go ahead @dmsnell -- feel free to!)

[ockham]

Looks like perf tests have been fixed in #48240 🎉

Going to rebase.