diff --git a/packages/e2e-tests/plugins/interactive-blocks/directive-bind/render.php b/packages/e2e-tests/plugins/interactive-blocks/directive-bind/render.php index e4580f8cf02fd..a94eb20bfa6d5 100644 --- a/packages/e2e-tests/plugins/interactive-blocks/directive-bind/render.php +++ b/packages/e2e-tests/plugins/interactive-blocks/directive-bind/render.php @@ -56,4 +56,44 @@ > Some Text

+ + '{ "value": false }', + 'true' => '{ "value": true }', + 'null' => '{ "value": null }', + 'undef' => '{ "__any": "any" }', + 'emptyString' => '{ "value": "" }', + 'anyString' => '{ "value": "any" }', + 'number' => '{ "value": 10 }', + ); + ?> + + $context ) : ?> +
+ Red dot + + +
+ diff --git a/packages/e2e-tests/plugins/interactive-blocks/directive-bind/view.js b/packages/e2e-tests/plugins/interactive-blocks/directive-bind/view.js index 0cd590f184cc8..cbe562f5e2549 100644 --- a/packages/e2e-tests/plugins/interactive-blocks/directive-bind/view.js +++ b/packages/e2e-tests/plugins/interactive-blocks/directive-bind/view.js @@ -18,6 +18,17 @@ state.show = ! state.show; state.width += foo.bar; }, + toggleValue: ( { context } ) => { + const previousValue = ( 'previousValue' in context ) + ? context.previousValue + // Any string works here; we just want to toggle the value + // to ensure Preact renders the same we are hydrating in the + // first place. + : 'tacocat'; + + context.previousValue = context.value; + context.value = previousValue; + } }, } ); } )( window ); diff --git a/packages/interactivity/CHANGELOG.md b/packages/interactivity/CHANGELOG.md index 63da342d030a5..62515115484fa 100644 --- a/packages/interactivity/CHANGELOG.md +++ b/packages/interactivity/CHANGELOG.md @@ -7,6 +7,7 @@ - Support keys using `data-wp-key`. ([#53844](https://github.com/WordPress/gutenberg/pull/53844)) - Merge new server-side rendered context on client-side navigation. ([#53853](https://github.com/WordPress/gutenberg/pull/53853)) - Support region-based client-side navigation. ([#53733](https://github.com/WordPress/gutenberg/pull/53733)) +- Improve `data-wp-bind` hydration to match Preact's logic. ([#54003](https://github.com/WordPress/gutenberg/pull/54003)) ## 2.1.0 (2023-08-16) diff --git a/packages/interactivity/src/directives.js b/packages/interactivity/src/directives.js index 1b7a82be38cfa..0fd532debc775 100644 --- a/packages/interactivity/src/directives.js +++ b/packages/interactivity/src/directives.js @@ -222,19 +222,46 @@ export default () => { // on the hydration, so we have to do it manually. It doesn't need // deps because it only needs to do it the first time. useEffect( () => { + const el = element.ref.current; + + // We set the value directly to the corresponding + // HTMLElement instance property excluding the following + // special cases. + // We follow Preact's logic: https://github.com/preactjs/preact/blob/ea49f7a0f9d1ff2c98c0bdd66aa0cbc583055246/src/diff/props.js#L110-L129 + if ( + attribute !== 'width' && + attribute !== 'height' && + attribute !== 'href' && + attribute !== 'list' && + attribute !== 'form' && + // Default value in browsers is `-1` and an empty string is + // cast to `0` instead + attribute !== 'tabIndex' && + attribute !== 'download' && + attribute !== 'rowSpan' && + attribute !== 'colSpan' && + attribute in el + ) { + try { + el[ attribute ] = + result === null || result === undefined + ? '' + : result; + return; + } catch ( err ) {} + } // aria- and data- attributes have no boolean representation. // A `false` value is different from the attribute not being // present, so we can't remove it. // We follow Preact's logic: https://github.com/preactjs/preact/blob/ea49f7a0f9d1ff2c98c0bdd66aa0cbc583055246/src/diff/props.js#L131C24-L136 - if ( result === false && attribute[ 4 ] !== '-' ) { - element.ref.current.removeAttribute( attribute ); + if ( + result !== null && + result !== undefined && + ( result !== false || attribute[ 4 ] === '-' ) + ) { + el.setAttribute( attribute, result ); } else { - element.ref.current.setAttribute( - attribute, - result === true && attribute[ 4 ] !== '-' - ? '' - : result - ); + el.removeAttribute( attribute ); } }, [] ); } ); diff --git a/test/e2e/specs/interactivity/directive-bind.spec.ts b/test/e2e/specs/interactivity/directive-bind.spec.ts index 67ee1232a6798..401bbcd6b24dd 100644 --- a/test/e2e/specs/interactivity/directive-bind.spec.ts +++ b/test/e2e/specs/interactivity/directive-bind.spec.ts @@ -37,12 +37,12 @@ test.describe( 'data-wp-bind', () => { test( 'add missing checked at hydration', async ( { page } ) => { const el = page.getByTestId( 'add missing checked at hydration' ); - await expect( el ).toHaveAttribute( 'checked', '' ); + await expect( el ).toBeChecked(); } ); test( 'remove existing checked at hydration', async ( { page } ) => { const el = page.getByTestId( 'remove existing checked at hydration' ); - await expect( el ).not.toHaveAttribute( 'checked', '' ); + await expect( el ).not.toBeChecked(); } ); test( 'update existing checked', async ( { page } ) => { @@ -93,4 +93,167 @@ test.describe( 'data-wp-bind', () => { await expect( el ).toHaveAttribute( 'aria-expanded', 'true' ); await expect( el ).toHaveAttribute( 'data-some-value', 'true' ); } ); + + test.describe( 'attribute hydration', () => { + /** + * Data structure to define a hydration test case. + */ + type MatrixEntry = { + /** + * Test ID of the element (the `data-testid` attr). + */ + testid: string; + /** + * Name of the attribute being hydrated. + */ + name: string; + /** + * Array of different values to test. + */ + values: Record< + /** + * The type of value we are hydrating. E.g., false is `false`, + * undef is `undefined`, emptyString is `''`, etc. + */ + string, + [ + /** + * Value that the attribute should contain after hydration. + * If the attribute is missing, this value is `null`. + */ + attributeValue: any, + /** + * Value that the HTMLElement instance property should + * contain after hydration. + */ + entityPropValue: any + ] + >; + }; + + const matrix: MatrixEntry[] = [ + { + testid: 'image', + name: 'width', + values: { + false: [ null, 5 ], + true: [ 'true', 5 ], + null: [ null, 5 ], + undef: [ null, 5 ], + emptyString: [ '', 5 ], + anyString: [ 'any', 5 ], + number: [ '10', 10 ], + }, + }, + { + testid: 'input', + name: 'name', + values: { + false: [ 'false', 'false' ], + true: [ 'true', 'true' ], + null: [ '', '' ], + undef: [ '', '' ], + emptyString: [ '', '' ], + anyString: [ 'any', 'any' ], + number: [ '10', '10' ], + }, + }, + { + testid: 'input', + name: 'value', + values: { + false: [ null, 'false' ], + true: [ null, 'true' ], + null: [ null, '' ], + undef: [ null, '' ], + emptyString: [ null, '' ], + anyString: [ null, 'any' ], + number: [ null, '10' ], + }, + }, + { + testid: 'input', + name: 'disabled', + values: { + false: [ null, false ], + true: [ '', true ], + null: [ null, false ], + undef: [ null, false ], + emptyString: [ null, false ], + anyString: [ '', true ], + number: [ '', true ], + }, + }, + { + testid: 'input', + name: 'aria-disabled', + values: { + false: [ 'false', undefined ], + true: [ 'true', undefined ], + null: [ null, undefined ], + undef: [ null, undefined ], + emptyString: [ '', undefined ], + anyString: [ 'any', undefined ], + number: [ '10', undefined ], + }, + }, + ]; + + for ( const { testid, name, values } of matrix ) { + test( `${ name } is correctly hydrated for different values`, async ( { + page, + } ) => { + for ( const type in values ) { + const [ attrValue, propValue ] = values[ type ]; + + const container = page.getByTestId( `hydrating ${ type }` ); + const el = container.getByTestId( testid ); + const toggle = container.getByTestId( 'toggle value' ); + + const hydratedAttr = await el.getAttribute( name ); + const hydratedProp = await el.evaluate( + ( node, propName ) => ( node as any )[ propName ], + name + ); + expect( [ type, hydratedAttr ] ).toEqual( [ + type, + attrValue, + ] ); + expect( [ type, hydratedProp ] ).toEqual( [ + type, + propValue, + ] ); + + // Only check the rendered value if the new value is not + // `undefined` and the attibute is neither `value` nor + // `disabled` because Preact doesn't update the attribute + // for those cases. + // See https://github.com/preactjs/preact/blob/099c38c6ef92055428afbc116d18a6b9e0c2ea2c/src/diff/index.js#L471-L494 + if ( + type === 'undef' && + ( name === 'value' || name === 'undefined' ) + ) { + return; + } + + await toggle.click( { clickCount: 2, delay: 50 } ); + + // Values should be the same as before. + const renderedAttr = await el.getAttribute( name ); + const renderedProp = await el.evaluate( + ( node, propName ) => ( node as any )[ propName ], + name + ); + expect( [ type, renderedAttr ] ).toEqual( [ + type, + attrValue, + ] ); + expect( [ type, renderedProp ] ).toEqual( [ + type, + propValue, + ] ); + } + } ); + } + } ); } );