WordPress / gutenberg

The Block Editor project for WordPress and beyond. Plugin is available from the official repository.
https://wordpress.org/gutenberg/
Other
10.5k stars 4.2k forks source link

Interactivity API: Context not updating properly from radio button click with nested contexts #65623

Open danielpost opened 1 month ago

danielpost commented 1 month ago

Description

This is a bit of a weird one, but hopefully the minimal repro example below makes sense. I'm trying to update the context of a parent from a radio button change. In the example below, you will see that when changing a radio button, context.trigger will not update to radio, but context.filters.radioValue will update correctly. However, when doing the exact same action from the button, context.trigger updates correctly.

If you remove the dummy context (the one with "somethingNew" => "example") it suddenly works fine for both the radio and the button. However, in my real world usage I do need the values in that context, so deleting it is not an option.

Step-by-step reproduction instructions

view.js

import { store, getContext, getElement } from '@wordpress/interactivity';

store( 'playground', {
    state: {
        get isRadioChecked() {
            const context = getContext();
            const { filters, value } = context;
            return value === filters.radioValue;
        },
    },
    actions: {
        onRadioChange( event ) {
            const context = getContext();
            context.trigger = 'radio';
            context.filters.radioValue = event.target.value;
        },
        onButtonClick() {
            const context = getContext();
            context.trigger = 'reset';
            context.filters.radioValue = '';
        },
    },
    callbacks: {
        *logRadioValue() {
            const context = getContext();
            const { trigger, filters } = context;
            console.log( trigger );
            console.log( filters.radioValue );
        },
    },
} );

render.php

<?php
$context = [
    'trigger' => 'page-load',
    'filters' => [
        'radioValue' => 'two',
    ],
];

$radios = [
    [
        'value' => 'one',
        'label' => 'One',
    ],
    [
        'value' => 'two',
        'label' => 'Two',
    ],
    [
        'value' => 'three',
        'label' => 'Three',
    ],
];
?>

<div
    data-wp-interactive="playground"
    data-wp-watch="callbacks.logRadioValue"
    <?php echo wp_interactivity_data_wp_context( $context, 'playground' ); // phpcs:ignore WordPress.Security.EscapeOutput.OutputNotEscaped ?>
    <?php echo get_block_wrapper_attributes(); // phpcs:ignore WordPress.Security.EscapeOutput.OutputNotEscaped ?>
>
    <div <?php echo wp_interactivity_data_wp_context( [ 'somethingNew' => 'example' ], 'playground' ); // phpcs:ignore WordPress.Security.EscapeOutput.OutputNotEscaped ?>>
        <?php foreach ( $radios as $radio ) : ?>
            <label>
                <input
                    type="radio"
                    name="radio"
                    value="<?php echo esc_attr( $radio['value'] ); ?>"
                    data-wp-bind--checked="state.isRadioChecked"
                    data-wp-on-async--click="actions.onRadioChange"
                    <?php
                    echo wp_interactivity_data_wp_context(
                        [
                            'value' => $radio['value'],
                            'playground',
                        ]
                    ); // phpcs:ignore WordPress.Security.EscapeOutput.OutputNotEscaped
                    ?>
                >
                <?php echo esc_html( $radio['label'] ); ?>
            </label>
        <?php endforeach; ?>

        <button data-wp-on-async--click="actions.onButtonClick">Reset</button>
    </div>
</div>

Screenshots, screen recording, code snippet

No response

Environment info

Please confirm that you have searched existing issues in the repo.

Please confirm that you have tested with all plugins deactivated except Gutenberg.

michalczaplinski commented 19 hours ago

I can reproduce the issue and it appears to be a problem with data-wp-context. Here's the code that I used:

render.php

<?php
$context = array(
    'trigger' => 'page-load',
    'radios' => array(
        array(
        'value' => 'one',
        'label' => 'One',
    ),
    array(
        'value' => 'two',
        'label' => 'Two',
    ),
    array(
        'value' => 'three',
        'label' => 'Three',
    )));
?>

<div
    data-wp-interactive="playground"
    data-wp-watch="callbacks.logRadioValue"
    <?php echo wp_interactivity_data_wp_context($context); ?>
    <?php echo get_block_wrapper_attributes(); ?>
>
    <div <?php echo wp_interactivity_data_wp_context(
        array( 'somethingNew' => array( 'example' => 'example' ) )
    );?>>
        <template data-wp-each="context.radios">
            <label>
                <input
                    type="radio"
                    name="radio"
                    data-wp-bind--value="context.item.value"
                    data-wp-bind--checked="state.isRadioChecked"
                    data-wp-on--click="actions.onRadioChange"
                >
                  <span data-wp-text="context.item.label"></span>
            </label>
        </template>
        <div>
            <div><strong>Selected value:</strong> 
                <span data-wp-text="state.radioValue"></span>
            </div>
            <div>
                <strong>Trigger:</strong> 
                <span data-wp-text="context.trigger"></span>
            </div>
        </div>
        <button data-wp-on--click="actions.onButtonClick">Reset</button>
    </div>
</div>

view.js

import { store, getContext, getElement } from "@wordpress/interactivity";

const { state } = store("playground", {
  state: {
    get isRadioChecked() {
      const { ref } = getElement();
      return state.radioValue === ref.value;
    },
  },
  actions: {
    onRadioChange(event) {
      const context = getContext();
      context.trigger = "radio";
      state.radioValue = event.target.value;
    },
    onButtonClick() {
      const context = getContext();
      context.trigger = "reset";
      state.radioValue = "";
    },
  },
});

Removing the inner context (the one containing somethingNew) resolves the problem. Otherwise, the context does not update. Some further observations:

https://github.com/user-attachments/assets/8f3f9489-bfdb-440e-b3c9-d09ea3113eef

michalczaplinski commented 16 hours ago

@DAreRodz Does this ring any bells? It looks like an issue with context merging.

DAreRodz commented 1 hour ago

@DAreRodz Does this ring any bells? It looks like an issue with context merging.

Not really. I'm taking a look at the issue right now.

DAreRodz commented 17 minutes ago

I've also reproduced the issue, and I think I've found the origin.

It happens when context.trigger is set to "radio" here:

onRadioChange(event) {
  const context = getContext();
  context.trigger = "radio";
  state.radioValue = event.target.value;
},

When the [[Set]] internal method is intercepted inside the context proxy handlers, the execution reaches the following line:

https://github.com/WordPress/gutenberg/blob/636710ba98514345009d7aab2de1dc5471c69a2c/packages/interactivity/src/proxies/context.ts#L27

And the problem arises when the key in fallback expression is evaluated.

At that moment, given that fallback contains the proxy for the parent context, the expression is supposed to return true, but it returns false. This happens because the parent context actually doesn't have that property: it's inherited from another context.

I think the solution is to define a trap for the [[HasProperty]] internal method, returning true for inherited properties. We already have a trap for [[OwnPropertyKeys]] that returns the list of "own keys," which includes the inherited properties as well.

DAreRodz commented 5 minutes ago

Proposed fix:

diff --git a/packages/interactivity/src/proxies/context.ts b/packages/interactivity/src/proxies/context.ts
index 64517c91a69..8d5bc54a8b8 100644
--- a/packages/interactivity/src/proxies/context.ts
+++ b/packages/interactivity/src/proxies/context.ts
@@ -38,6 +38,9 @@ const contextHandlers: ProxyHandler< object > = {
    getOwnPropertyDescriptor: ( target, key ) =>
        descriptor( target, key ) ||
        descriptor( contextObjectToFallback.get( target ), key ),
+   has: ( target, key ) =>
+       Reflect.has( target, key ) ||
+       Reflect.has( contextObjectToFallback.get( target ), key ),
 };

 /**

Tested, and it seems to work fine:

https://github.com/user-attachments/assets/2b5ef784-2ff2-4188-a87e-7ea834bb7cf6