Skip to content

Interactivity API: Fix SSR context included in void tags shouldn't propagate to following elements #6267

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.

Sign up for GitHub

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

Closed
SantosGuillamot wants to merge 12 commits into from
Closed

Interactivity API: Fix SSR context included in void tags shouldn't propagate to following elements #6267

Changes from 1 commit
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
5613ba6
Add test to cover void tags context
SantosGuillamot Mar 13, 2024
cb5ba25
Add potential solution to void tags
SantosGuillamot Mar 13, 2024
4122ba4
Use more general approach
SantosGuillamot Mar 13, 2024
19a9afe
Change cover annotation
SantosGuillamot Mar 13, 2024
34ee1dc
Revert "Use more general approach"
SantosGuillamot Mar 14, 2024
09cc147
Only process directives when is not a closing tag
SantosGuillamot Mar 14, 2024
ba79dec
Revert "Only process directives when is not a closing tag"
SantosGuillamot Mar 14, 2024
f2e3336
Revert "Revert "Use more general approach""
SantosGuillamot Mar 14, 2024
01ae241
Use two different modes "enter" and "exit"
SantosGuillamot Mar 14, 2024
86bf0fb
Add test for namespace in void tags
SantosGuillamot Mar 14, 2024
8c90ece
Add string mode annotation in params
SantosGuillamot Mar 14, 2024
3975be1
Change `data-wp-interactive` comment
SantosGuillamot Mar 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Revert "Use more general approach" 
This reverts commit 4122ba4.
  • Loading branch information
@SantosGuillamot
SantosGuillamot committed Mar 14, 2024
commit 34ee1dc2e65a021fa5a802ffbcca56fd31b60f77
97 changes: 40 additions & 57 deletions src/wp-includes/interactivity-api/class-wp-interactivity-api.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -299,42 +299,33 @@ private function process_directives_args( string $html, array &$context_stack, a

/*
* Sorts the attributes by the order of the `directives_processor` array
* and checks what directives are present in this element.
* and checks what directives are present in this element. The processing
* order is reversed for tag closers.
*/
$existing_directives_prefixes = array_intersect(
$directive_processor_prefixes,
$directives_prefixes = array_intersect(
$p->is_tag_closer()
? $directive_processor_prefixes_reversed
: $directive_processor_prefixes,
$directives_prefixes
);
$existing_directives_prefixes_reversed = array_intersect(
$directive_processor_prefixes_reversed,
$directives_prefixes
);
// If it is not a tag closer, process directives in normal order.
if ( ! $p->is_tag_closer() ) {
foreach ( $existing_directives_prefixes as $directive_prefix ) {
$func = is_array( self::$directive_processors[ $directive_prefix ] )
? self::$directive_processors[ $directive_prefix ]
: array( $this, self::$directive_processors[ $directive_prefix ] );

call_user_func_array(
$func,
array( $p, false, &$context_stack, &$namespace_stack, &$tag_stack )
);
}

// Executes the directive processors present in this element.
foreach ( $directives_prefixes as $directive_prefix ) {
$func = is_array( self::$directive_processors[ $directive_prefix ] )
? self::$directive_processors[ $directive_prefix ]
: array( $this, self::$directive_processors[ $directive_prefix ] );
call_user_func_array(
$func,
array( $p, &$context_stack, &$namespace_stack, &$tag_stack )
);
}

// If it is a tag closer, or it doesn't visit the closer tag, process directives in reversed order.
// For cases where it doesn't visit closer tags, it needs to run in both orders.
if ( $p->is_tag_closer() || ! $p->has_and_visits_its_closer_tag() ) {
foreach ( $existing_directives_prefixes_reversed as $directive_prefix ) {
$func = is_array( self::$directive_processors[ $directive_prefix ] )
? self::$directive_processors[ $directive_prefix ]
: array( $this, self::$directive_processors[ $directive_prefix ] );

call_user_func_array(
$func,
array( $p, true, &$context_stack, &$namespace_stack, &$tag_stack )
);
// Remove context from stack if it is a void tag.
if ( WP_HTML_Processor::is_void( $tag_name ) ) {
foreach ( $directives_prefixes as $directive_prefix ) {
if ( 'data-wp-context' === $directive_prefix ) {
array_pop( $context_stack );
}
Copy link
Member

@dmsnell dmsnell Mar 13, 2024

Choose a reason for hiding this comment

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

I believe that this is a surprise way to handle this based on how the directive handlers work. that is, they all handle their own cleanup, except now the specific context directive is coupled into the general directive dispatch.

cc: @ockham

what I think could work more generally is to do two things:

  • when matched on void or special elements (elements without a closing tag) we should run the handlers in reverse as is done above in line 307.
  • update the handlers so that instead of checking is_closing_tag() and using HTML semantics, they check something different, such as 'enter' or 'exit', representing whether the code is entering or existing a directive's scope

this implies of course that this function would then pass in that action to the directive handlers.

if ( ! $this->has_and_visits_its_closing_tag() ) {
  call_user_func_array(
	  $func,
	  array( $p, 'enter', &$context_stack, &$namespace_stack, &$tag_stack )
  );
  call_user_func_array(
	  $func,
	  array( $p, 'exit', &$context_stack, &$namespace_stack, &$tag_stack )
  );
} else {
  call_user_func_array(
	  $func,
	  array( $p, $p->is_tag_closer() ? 'exit' : 'enter', &$context_stack, &$namespace_stack, &$tag_stack )
  );
}

Copy link
Author

@SantosGuillamot SantosGuillamot Mar 13, 2024

Choose a reason for hiding this comment

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

I believe it makes sense to use a more general mechanism and not something hardcoded for the context.

I implemented a different solution based on your feedback in this commit. For void tags or special elements, it runs the directives in both orders.

It seems to be working fine, but I'd love to know if that's what you had in mind.

By the way, I am not convinced about the variable name $is_exiting_tag. I can easily changed that if we pursue this approach.

Copy link
Member

@luisherranz luisherranz Mar 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I agree this is a better solution, but I wouldn't mind to include this fix in 6.5 as it is, and work on this improvement for a future version because everything in this logic is still private and subject to change 🙂

Up to you!

Copy link
Member

@gziolo gziolo Mar 14, 2024

Choose a reason for hiding this comment

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

01ae241 - exercises enter vs exit mode.

}
}
}
Expand Down Expand Up @@ -488,13 +479,12 @@ function ( $matches ) {
* @since 6.5.0
*
* @param WP_Interactivity_API_Directives_Processor $p The directives processor instance.
* @param boolean $is_exiting_tag Whether the current tag is currently exiting or not.
* @param array $context_stack The reference to the context stack.
* @param array $namespace_stack The reference to the store namespace stack.
*/
private function data_wp_interactive_processor( WP_Interactivity_API_Directives_Processor $p, $is_exiting_tag, array &$context_stack, array &$namespace_stack ) {
private function data_wp_interactive_processor( WP_Interactivity_API_Directives_Processor $p, array &$context_stack, array &$namespace_stack ) {
// In closing tags, it removes the last namespace from the stack.
if ( $is_exiting_tag ) {
if ( $p->is_tag_closer() ) {
array_pop( $namespace_stack );
return;
}
Expand Down Expand Up @@ -533,13 +523,12 @@ private function data_wp_interactive_processor( WP_Interactivity_API_Directives_
* @since 6.5.0
*
* @param WP_Interactivity_API_Directives_Processor $p The directives processor instance.
* @param boolean $is_exiting_tag Whether the current tag is currently exiting or not.
* @param array $context_stack The reference to the context stack.
* @param array $namespace_stack The reference to the store namespace stack.
*/
private function data_wp_context_processor( WP_Interactivity_API_Directives_Processor $p, $is_exiting_tag, array &$context_stack, array &$namespace_stack ) {
// When exiting tags, it removes the last context from the stack.
if ( $is_exiting_tag ) {
private function data_wp_context_processor( WP_Interactivity_API_Directives_Processor $p, array &$context_stack, array &$namespace_stack ) {
// In closing tags, it removes the last context from the stack.
if ( $p->is_tag_closer() ) {
array_pop( $context_stack );
return;
}
Expand Down Expand Up @@ -580,12 +569,11 @@ private function data_wp_context_processor( WP_Interactivity_API_Directives_Proc
* @since 6.5.0
*
* @param WP_Interactivity_API_Directives_Processor $p The directives processor instance.
* @param boolean $is_exiting_tag Whether the current tag is currently exiting or not.
* @param array $context_stack The reference to the context stack.
* @param array $namespace_stack The reference to the store namespace stack.
*/
private function data_wp_bind_processor( WP_Interactivity_API_Directives_Processor $p, $is_exiting_tag, array &$context_stack, array &$namespace_stack ) {
if ( ! $is_exiting_tag ) {
private function data_wp_bind_processor( WP_Interactivity_API_Directives_Processor $p, array &$context_stack, array &$namespace_stack ) {
if ( ! $p->is_tag_closer() ) {
$all_bind_directives = $p->get_attribute_names_with_prefix( 'data-wp-bind--' );

foreach ( $all_bind_directives as $attribute_name ) {
Expand Down Expand Up @@ -625,12 +613,11 @@ private function data_wp_bind_processor( WP_Interactivity_API_Directives_Process
* @since 6.5.0
*
* @param WP_Interactivity_API_Directives_Processor $p The directives processor instance.
* @param boolean $is_exiting_tag Whether the current tag is currently exiting or not.
* @param array $context_stack The reference to the context stack.
* @param array $namespace_stack The reference to the store namespace stack.
*/
private function data_wp_class_processor( WP_Interactivity_API_Directives_Processor $p, $is_exiting_tag, array &$context_stack, array &$namespace_stack ) {
if ( ! $is_exiting_tag ) {
private function data_wp_class_processor( WP_Interactivity_API_Directives_Processor $p, array &$context_stack, array &$namespace_stack ) {
if ( ! $p->is_tag_closer() ) {
$all_class_directives = $p->get_attribute_names_with_prefix( 'data-wp-class--' );

foreach ( $all_class_directives as $attribute_name ) {
Expand Down Expand Up @@ -660,12 +647,11 @@ private function data_wp_class_processor( WP_Interactivity_API_Directives_Proces
* @since 6.5.0
*
* @param WP_Interactivity_API_Directives_Processor $p The directives processor instance.
* @param boolean $is_exiting_tag Whether the current tag is currently exiting or not.
* @param array $context_stack The reference to the context stack.
* @param array $namespace_stack The reference to the store namespace stack.
*/
private function data_wp_style_processor( WP_Interactivity_API_Directives_Processor $p, $is_exiting_tag, array &$context_stack, array &$namespace_stack ) {
if ( ! $is_exiting_tag ) {
private function data_wp_style_processor( WP_Interactivity_API_Directives_Processor $p, array &$context_stack, array &$namespace_stack ) {
if ( ! $p->is_tag_closer() ) {
$all_style_attributes = $p->get_attribute_names_with_prefix( 'data-wp-style--' );

foreach ( $all_style_attributes as $attribute_name ) {
Expand Down Expand Up @@ -753,12 +739,11 @@ private function merge_style_property( string $style_attribute_value, string $st
* @since 6.5.0
*
* @param WP_Interactivity_API_Directives_Processor $p The directives processor instance.
* @param boolean $is_exiting_tag Whether the current tag is currently exiting or not.
* @param array $context_stack The reference to the context stack.
* @param array $namespace_stack The reference to the store namespace stack.
*/
private function data_wp_text_processor( WP_Interactivity_API_Directives_Processor $p, $is_exiting_tag, array &$context_stack, array &$namespace_stack ) {
if ( ! $is_exiting_tag ) {
private function data_wp_text_processor( WP_Interactivity_API_Directives_Processor $p, array &$context_stack, array &$namespace_stack ) {
if ( ! $p->is_tag_closer() ) {
$attribute_value = $p->get_attribute( 'data-wp-text' );
$result = $this->evaluate( $attribute_value, end( $namespace_stack ), end( $context_stack ) );

Expand Down Expand Up @@ -851,11 +836,10 @@ class="screen-reader-text"
*
* @since 6.5.0
*
* @param WP_Interactivity_API_Directives_Processor $p The directives processor instance.
* @param boolean $is_exiting_tag Whether the current tag is currently exiting or not.
* @param WP_Interactivity_API_Directives_Processor $p The directives processor instance.
*/
private function data_wp_router_region_processor( WP_Interactivity_API_Directives_Processor $p, $is_exiting_tag ) {
if ( ! $is_exiting_tag && ! $this->has_processed_router_region ) {
private function data_wp_router_region_processor( WP_Interactivity_API_Directives_Processor $p ) {
if ( ! $p->is_tag_closer() && ! $this->has_processed_router_region ) {
$this->has_processed_router_region = true;

// Initialize the `core/router` store.
Expand Down Expand Up @@ -891,13 +875,12 @@ private function data_wp_router_region_processor( WP_Interactivity_API_Directive
* @since 6.5.0
*
* @param WP_Interactivity_API_Directives_Processor $p The directives processor instance.
* @param boolean $is_exiting_tag Whether the current tag is currently exiting or not.
* @param array $context_stack The reference to the context stack.
* @param array $namespace_stack The reference to the store namespace stack.
* @param array $tag_stack The reference to the tag stack.
*/
private function data_wp_each_processor( WP_Interactivity_API_Directives_Processor $p, $is_exiting_tag, array &$context_stack, array &$namespace_stack, array &$tag_stack ) {
if ( ! $is_exiting_tag && 'TEMPLATE' === $p->get_tag() ) {
private function data_wp_each_processor( WP_Interactivity_API_Directives_Processor $p, array &$context_stack, array &$namespace_stack, array &$tag_stack ) {
if ( ! $p->is_tag_closer() && 'TEMPLATE' === $p->get_tag() ) {
$attribute_name = $p->get_attribute_names_with_prefix( 'data-wp-each' )[0];
$extracted_suffix = $this->extract_prefix_and_suffix( $attribute_name );
$item_name = isset( $extracted_suffix[1] ) ? $this->kebab_to_camel_case( $extracted_suffix[1] ) : 'item';
Expand Down