Skip to content
This repository has been archived by the owner on Jul 28, 2023. It is now read-only.

SSR: Handle closing tags whose openers had an attribute directive #144

Merged
merged 1 commit into from
Feb 15, 2023

Conversation

ockham
Copy link
Collaborator

@ockham ockham commented Feb 1, 2023

Some attribute directives (such as wp-context, see #143), need to be run not only on an opening tag (e.g. <div wp-context="..."> but also on the matching closing one. (For wp-context, this is required to reset the context to the value that corresponds to the enclosing scope's context.)

This needs somewhat complex logic to keep track of nested tags. Consider e.g. the following example (used in this PR's unit test):

<div>Example: 
	<div foo-test="abc">
		<img>
		<span>This is a test></span>
		<div>Here is a nested div</div>
	</div>
</div>

Here, we want to run the foo-test attribute directive processor on the closing </div> that matches the opening <div foo-test="abc"> -- but not on any of the other two closing </div>s.

Rather than requiring individual directive processors to implement the required tracking of nested tags, we implement it in the wp_process_directives function.

To that end, this PR also moves it into a separate file and changes its signature a bit for better testability. Finally, we're adding a unit test to verify the behavior described above.

@ockham ockham self-assigned this Feb 1, 2023
@ockham ockham force-pushed the update/wp-process-directives branch from aacd1e1 to 3d4a576 Compare February 1, 2023 15:29
@ockham ockham force-pushed the update/wp-process-directives branch 2 times, most recently from 83d037a to b2c9da5 Compare February 13, 2023 13:32
@ockham ockham force-pushed the update/wp-process-directives branch from b2c9da5 to f9eb75c Compare February 13, 2023 13:57
@ockham ockham requested a review from a team February 13, 2023 14:08
@ockham ockham marked this pull request as ready for review February 13, 2023 14:13
Copy link
Member

@luisherranz luisherranz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The code looks fine to me 🙂

Although I thought the closing tag logic was going to be delegated to the HTML (Tag) Processor. Is it not the case anymore, or is this a temporary solution until something like this is finished?

@ockham
Copy link
Collaborator Author

ockham commented Feb 15, 2023

The code looks fine to me 🙂

Thank you! 🥳

Although I thought the closing tag logic was going to be delegated to the HTML (Tag) Processor. Is it not the case anymore, or is this a temporary solution until something like this is finished?

I'm not sure at this point. We still need logic in the HTML (Tag) Processor to handle closing tags, in order to implement methods such as set_content_within_balanced_tags (which will also involve moving back and forth inside the HTML). (Note that OTOH, this current PR only ever moves forward.)

WordPress/gutenberg#47573 has logic that's somewhat similar to what we're doing here (stack of opening tags). However, I didn't find it directly applicable to what we're doing here, and so I opted to implement it separately. I wouldn't rule out that eventually, we will have logic in the HTML (Tag) Processor that can serve both purposes, but they seemed different enough that I didn't see any harm in implementing this one separately already (rather than waiting for the HTML (Tag) Processor).

It's also possible that the purposes are different enough that we'll continue to handle things differently. (After all, we can optimize the logic here a bit to e.g only track opening tags of a given type 🤷‍♂️ )

cc/ @dmsnell 😊

@dmsnell
Copy link
Member

dmsnell commented Feb 15, 2023

set_content_within_balanced_tags

the thing that's been stewing lately and I hope to still get into soon is that I think we can avoid creating this and instead only expose the expected function, set_inner_html().

this requires taking some time to understand the adoption agency algorithm, but from what I can tell it's mostly adding a few tag-type lists, maintaining a small tag stack (not unlike here and in our other HTML processing PRs), and crossing off all the cases of format boundaries.

this isn't to speak to this PR; go crazy here in your own code. for the HTML API I think that given that it does actually seem possible to do things 100% I'm tempted to avoid introducing things that are like the 100% but aren't.

@ockham ockham merged commit b3df431 into main-wp-directives-plugin Feb 15, 2023
@ockham ockham deleted the update/wp-process-directives branch February 15, 2023 17:08
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants