Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WP_HTML_Processor: Add set_content_inside_balanced_tags #47036

Closed

Conversation

ockham
Copy link
Contributor

@ockham ockham commented Jan 10, 2023

What?

Part of #44410.

Work in progress. Branch based on @dmsnell's #46345, which introduces the get_content_inside_balanced_tags counterpart.

Why?

This is going to be a handy multi-purpose method for HTML manipulation. Needed for the Block Interactivity API, see WordPress/block-interactivity-experiments#125.

How?

By hijacking WP_HTML_Tag_Processor's established $attribute_updates mechanism. This seems to work well enough at a basic level but will of course require some changes.

What's interesting here is that while $attribute_updates is keyed by ("comparable", i.e. lowercase) attribute name, those keys are only relevant for set_attribute() (to check if an attribute of the same name already exists). They are however entirely ignored by apply_attributes_updates -- which is why we can use that mechanism for set_content_inside_balanced_tags without much modification 🎉

TODO

Testing Instructions

Locally:

npm run test:unit:php -- --group=html-processor

@github-actions
Copy link

github-actions bot commented Jan 10, 2023

Flaky tests detected in 6850a65.
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/3933179913
📝 Reported issues:

@dmsnell
Copy link
Member

dmsnell commented Jan 10, 2023

I almost proposed a lexical_updates rename yesterday but deferred it so we can sync all our open PRs/branches to that rename.

On finding the balanced tag we also probably need to heed void and self-closing tags (void tags are the only thing currently supported by my HTML processor, but I plan to add support for self-closing tags (of which only foreign elements can be self-closing; HTML elements with a self-closing flag are errors and the self-closer is ignored).

I found this while working on the CSS child selector, as I was inappropriately skipping past the first element following a void element before taking that into account.

@ockham
Copy link
Contributor Author

ockham commented Jan 11, 2023

I almost proposed a lexical_updates rename yesterday but deferred it so we can sync all our open PRs/branches to that rename.

FWIW, I filed one now: #47053 😊

(I hope I read you correctly -- you did mean a separate PR that we can land and then rebase our open PRs on?)

@dmsnell
Copy link
Member

dmsnell commented Jan 11, 2023

you did mean a separate PR that we can land and then rebase our open PRs on?

This, plus a good place to pause/merge other development to avoid needless conflict resolution.

@ockham ockham force-pushed the add/get-set-content-inside-balanced-tags branch from da249d0 to a843d89 Compare January 12, 2023 13:08
@ockham ockham force-pushed the add/get-set-content-inside-balanced-tags branch from a843d89 to 61e9308 Compare January 12, 2023 13:10
@ockham ockham force-pushed the add/get-set-content-inside-balanced-tags branch from 61e9308 to a8d798b Compare January 12, 2023 13:11
@ockham
Copy link
Contributor Author

ockham commented Jan 12, 2023

Now implementing add_lexical_update by cherry-picking 435987b from #47053, and bc92001 from #47068.

@ockham
Copy link
Contributor Author

ockham commented Jan 16, 2023

test_set_content_inside_balanced_tags_preceded_by_set_attribute_works (a989f71) is failing. For reference, here is what it does:

public function test_set_content_inside_balanced_tags_preceded_by_set_attribute_works() {
	// For reference: self::HTML === '<div>outside</div><section><div><img>inside</div></section>';
	$tags = new WP_HTML_Processor( self::HTML );

	$tags->next_tag( 'section' );
	$tags->set_attribute( 'id', 'thesection' );
	$tags->set_content_inside_balanced_tags( 'This is the new section content.' );
	$this->assertSame( '<div>outside</div><section id="thesection">This is the new section content.</section>', $tags->get_updated_html() );
}
1) WP_HTML_Processor_Test::test_set_content_inside_balanced_tags_preceded_by_set_attribute_works
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-'<div>outside</div><section id="thesection">This is the new section content.</section>'
+'<div>outside</div><section id="thesection"><div><img>insideThis is the new section content.</section>'

I think I've tracked down why, and how to fix. Stay tuned...

@getdave
Copy link
Contributor

getdave commented Jan 30, 2023

Will this PR allow for the following use case?:

  • filter on replace_block
  • find a <button> in the DOM of the block markup
  • replace an SVG tag within that button with a new SVG I have.
  • commit those changes back to the block markup

I was looking for this and I realised Tag Processor doesn't currently allow and @adamziel pointed me at this PR.

@dmsnell
Copy link
Member

dmsnell commented Jan 30, 2023

Will this PR allow for the following use case?:

@getdave the short answer is that it probably will, but probably not this PR. look for WP_HTML_Processor->replace_inner_markup() and WP_HTML_Processor->replace_outer_markup() which uses the tag processor to provide tree-level HTML operations. it may take a while to get there.

@ockham
Copy link
Contributor Author

ockham commented Mar 6, 2023

Closing, for the same reasons as outlined in #47573 (comment):

Closing. Per recent discussions, we're going to unblock the work in the BHE repo by implementing missing features in a subclass of WP_HTML_Tag_Processor right there (e.g.).

Development of the HTML Processor API is expected to progress more cautiously/slowly, to take into account more exotic HTML constructs; it will happen in (forks of) the wordpress-develop repo (e.g.).

@ockham ockham closed this Mar 6, 2023
@ockham ockham deleted the add/get-set-content-inside-balanced-tags branch March 6, 2023 10:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Block API API that allows to express the block paradigm. [Type] Experimental Experimental feature or API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants