From 365a5971725486ae99e8147c81c1c2a75fe0f1fe Mon Sep 17 00:00:00 2001 From: Luis Herranz Date: Wed, 23 Aug 2023 17:57:19 +0200 Subject: [PATCH 1/9] Add undefined and null to the removeAttribute This should be done in a separate PR. --- packages/interactivity/src/directives.js | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/packages/interactivity/src/directives.js b/packages/interactivity/src/directives.js index 1b7a82be38cfa..d9ae4343ad776 100644 --- a/packages/interactivity/src/directives.js +++ b/packages/interactivity/src/directives.js @@ -226,7 +226,12 @@ export default () => { // 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 ] !== '-' ) { + if ( + ( result === false || + result === undefined || + result === null ) && + attribute[ 4 ] !== '-' + ) { element.ref.current.removeAttribute( attribute ); } else { element.ref.current.setAttribute( From f37ca9ba491a77b2c49d4c5d6cd457540f67c5eb Mon Sep 17 00:00:00 2001 From: David Arenas Date: Mon, 28 Aug 2023 20:13:46 +0200 Subject: [PATCH 2/9] Adding first test cases (WIP) --- .../directive-bind/render.php | 32 ++++++++++ .../interactive-blocks/directive-bind/view.js | 10 +++ packages/interactivity/src/directives.js | 40 ++++++++---- .../interactivity/directive-bind.spec.ts | 61 +++++++++++++++++++ 4 files changed, 132 insertions(+), 11 deletions(-) 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..6ed3b85bda7c6 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,36 @@ > Some Text

+ + '{ "disabled": false }', + 'true' => '{ "disabled": true }', + 'string "false"' => '{ "disabled": "false" }', + 'string "true"' => '{ "disabled": "true" }', + 'null' => '{ "disabled": null }', + 'undefined' => '{ "other": "other" }', + 'empty string' => '{ "disabled": "" }', + 'any string' => '{ "disabled": "any" }', + ); + ?> + + $context ): ?> +
+ + +
+ 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..c89154685ea92 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,16 @@ state.show = ! state.show; state.width += foo.bar; }, + toggleDisabled: ( { context } ) => { + const prevDisabled = ( 'prevDisabled' in context ) + ? context.prevDisabled + // Any string works here; we just want to toggle the value + // to ensure Preact renders the same we are hydrating. + : 'disabled'; + + context.prevDisabled = context.disabled; + context.disabled = prevDisabled; + } }, } ); } )( window ); diff --git a/packages/interactivity/src/directives.js b/packages/interactivity/src/directives.js index d9ae4343ad776..02cd124a7b5c2 100644 --- a/packages/interactivity/src/directives.js +++ b/packages/interactivity/src/directives.js @@ -222,24 +222,42 @@ 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; + + 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; + // labelled break is 1b smaller here than a return statement (sorry) + 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 || - result === undefined || - result === null ) && - attribute[ 4 ] !== '-' + ( result !== null || result !== undefined ) && + ( result !== false || attribute[ 4 ] === '-' ) ) { - element.ref.current.removeAttribute( attribute ); + 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..0600cc428b7d7 100644 --- a/test/e2e/specs/interactivity/directive-bind.spec.ts +++ b/test/e2e/specs/interactivity/directive-bind.spec.ts @@ -93,4 +93,65 @@ test.describe( 'data-wp-bind', () => { await expect( el ).toHaveAttribute( 'aria-expanded', 'true' ); await expect( el ).toHaveAttribute( 'data-some-value', 'true' ); } ); + + // `width`: using the red-dot image (default value comes from image) + // `tabIndex`: can be a div (default value is -1) + // `hidden`: can be a div (values are treated as boolean) + // `value`: a text input (can be any string) + // `aria-disabled`: an input field + // `data-disabled`: the same input field + + test.describe( 'attribute hydration', () => { + const cases = [ + { type: 'false', attrValues: [ null, 'false' ] }, + { type: 'true', attrValues: [ '', 'true' ] }, + { type: 'string "false"', attrValues: [ '', 'false' ] }, + { type: 'string "true"', attrValues: [ '', 'true' ] }, + { type: 'null', attrValues: [ null, null ] }, + { type: 'undefined', attrValues: [ null, null ] }, + { type: 'empty string', attrValues: [ null, '' ] }, + { type: 'any string', attrValues: [ '', 'any' ] }, + ]; + + for ( const { + type, + attrValues: [ regularValue, ariaDataValue ], + } of cases ) { + test( `works for ${ type } values correctly`, async ( { + page, + } ) => { + const el = page.getByTestId( `hydrating ${ type }` ); + const input = el.getByTestId( 'input' ); + const toggle = el.getByTestId( 'toggle-prop' ); + + const initialValues = { + ariaDisabled: await input.getAttribute( 'aria-disabled' ), + dataDisabled: await input.getAttribute( 'data-disabled' ), + disabled: await input.getAttribute( 'disabled' ), + }; + + expect( initialValues.disabled ).toBe( regularValue ); + expect( initialValues.ariaDisabled ).toBe( ariaDataValue ); + expect( initialValues.dataDisabled ).toBe( ariaDataValue ); + + // Here we check that the hydrated values match the rendered + // values. + await toggle.click( { clickCount: 2, delay: 50 } ); + + const finalValues = { + ariaDisabled: await input.getAttribute( 'aria-disabled' ), + dataDisabled: await input.getAttribute( 'data-disabled' ), + disabled: await input.getAttribute( 'disabled' ), + }; + + expect( initialValues.disabled ).toBe( finalValues.disabled ); + expect( initialValues.ariaDisabled ).toBe( + finalValues.ariaDisabled + ); + expect( initialValues.dataDisabled ).toBe( + finalValues.dataDisabled + ); + } ); + } + } ); } ); From 73db3bfa19ad17019d9f2b145d5a2d37fac1fbab Mon Sep 17 00:00:00 2001 From: David Arenas Date: Mon, 28 Aug 2023 20:16:50 +0200 Subject: [PATCH 3/9] Add comment --- packages/interactivity/src/directives.js | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/packages/interactivity/src/directives.js b/packages/interactivity/src/directives.js index 02cd124a7b5c2..2a4c6d549bc59 100644 --- a/packages/interactivity/src/directives.js +++ b/packages/interactivity/src/directives.js @@ -224,6 +224,10 @@ export default () => { 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' && From 8ed56b2ce715b467317f90b8cb4ac99a6e249bd0 Mon Sep 17 00:00:00 2001 From: David Arenas Date: Mon, 28 Aug 2023 20:17:22 +0200 Subject: [PATCH 4/9] Remove unnecessary comment --- packages/interactivity/src/directives.js | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/interactivity/src/directives.js b/packages/interactivity/src/directives.js index 2a4c6d549bc59..f1b7f1c4d959c 100644 --- a/packages/interactivity/src/directives.js +++ b/packages/interactivity/src/directives.js @@ -247,7 +247,6 @@ export default () => { result === null || result === undefined ? '' : result; - // labelled break is 1b smaller here than a return statement (sorry) return; } catch ( err ) {} } From 0969006322ec31d67c50ab3abcb8e1dba12c09ae Mon Sep 17 00:00:00 2001 From: David Arenas Date: Tue, 29 Aug 2023 01:17:53 +0200 Subject: [PATCH 5/9] Update test cases --- .../directive-bind/render.php | 34 ++-- .../interactive-blocks/directive-bind/view.js | 15 +- .../interactivity/directive-bind.spec.ts | 174 ++++++++++++------ 3 files changed, 149 insertions(+), 74 deletions(-) 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 6ed3b85bda7c6..84fd969c900b2 100644 --- a/packages/e2e-tests/plugins/interactive-blocks/directive-bind/render.php +++ b/packages/e2e-tests/plugins/interactive-blocks/directive-bind/render.php @@ -59,14 +59,13 @@ '{ "disabled": false }', - 'true' => '{ "disabled": true }', - 'string "false"' => '{ "disabled": "false" }', - 'string "true"' => '{ "disabled": "true" }', - 'null' => '{ "disabled": null }', - 'undefined' => '{ "other": "other" }', - 'empty string' => '{ "disabled": "" }', - 'any string' => '{ "disabled": "any" }', + 'false' => '{ "value": false }', + 'true' => '{ "value": true }', + 'null' => '{ "value": null }', + 'undef' => '{ "__any": "any" }', + 'emptyString' => '{ "value": "" }', + 'anyString' => '{ "value": "any" }', + 'number' => '{ "value": 10 }' ); ?> @@ -75,16 +74,25 @@ data-testid='hydrating ' data-wp-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 c89154685ea92..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,15 +18,16 @@ state.show = ! state.show; state.width += foo.bar; }, - toggleDisabled: ( { context } ) => { - const prevDisabled = ( 'prevDisabled' in context ) - ? context.prevDisabled + 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. - : 'disabled'; + // to ensure Preact renders the same we are hydrating in the + // first place. + : 'tacocat'; - context.prevDisabled = context.disabled; - context.disabled = prevDisabled; + context.previousValue = context.value; + context.value = previousValue; } }, } ); diff --git a/test/e2e/specs/interactivity/directive-bind.spec.ts b/test/e2e/specs/interactivity/directive-bind.spec.ts index 0600cc428b7d7..d8cfe2cc583fe 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 } ) => { @@ -94,63 +94,129 @@ test.describe( 'data-wp-bind', () => { await expect( el ).toHaveAttribute( 'data-some-value', 'true' ); } ); - // `width`: using the red-dot image (default value comes from image) - // `tabIndex`: can be a div (default value is -1) - // `hidden`: can be a div (values are treated as boolean) - // `value`: a text input (can be any string) - // `aria-disabled`: an input field - // `data-disabled`: the same input field - test.describe( 'attribute hydration', () => { - const cases = [ - { type: 'false', attrValues: [ null, 'false' ] }, - { type: 'true', attrValues: [ '', 'true' ] }, - { type: 'string "false"', attrValues: [ '', 'false' ] }, - { type: 'string "true"', attrValues: [ '', 'true' ] }, - { type: 'null', attrValues: [ null, null ] }, - { type: 'undefined', attrValues: [ null, null ] }, - { type: 'empty string', attrValues: [ null, '' ] }, - { type: 'any string', attrValues: [ '', 'any' ] }, + const matrix = [ + { + 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 { - type, - attrValues: [ regularValue, ariaDataValue ], - } of cases ) { - test( `works for ${ type } values correctly`, async ( { + for ( const { testid, name, values } of matrix ) { + test( `${ name } is correctly hydrated for different values`, async ( { page, } ) => { - const el = page.getByTestId( `hydrating ${ type }` ); - const input = el.getByTestId( 'input' ); - const toggle = el.getByTestId( 'toggle-prop' ); - - const initialValues = { - ariaDisabled: await input.getAttribute( 'aria-disabled' ), - dataDisabled: await input.getAttribute( 'data-disabled' ), - disabled: await input.getAttribute( 'disabled' ), - }; - - expect( initialValues.disabled ).toBe( regularValue ); - expect( initialValues.ariaDisabled ).toBe( ariaDataValue ); - expect( initialValues.dataDisabled ).toBe( ariaDataValue ); - - // Here we check that the hydrated values match the rendered - // values. - await toggle.click( { clickCount: 2, delay: 50 } ); - - const finalValues = { - ariaDisabled: await input.getAttribute( 'aria-disabled' ), - dataDisabled: await input.getAttribute( 'data-disabled' ), - disabled: await input.getAttribute( 'disabled' ), - }; - - expect( initialValues.disabled ).toBe( finalValues.disabled ); - expect( initialValues.ariaDisabled ).toBe( - finalValues.ariaDisabled - ); - expect( initialValues.dataDisabled ).toBe( - finalValues.dataDisabled - ); + for ( const type in values ) { + const [ attrValue, propValue ] = ( values as any )[ 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, + ] ); + } } ); } } ); From 817461200b5ad7bb6c9ad5292dabbd92f6987494 Mon Sep 17 00:00:00 2001 From: David Arenas Date: Tue, 29 Aug 2023 01:18:25 +0200 Subject: [PATCH 6/9] Fix bind useEffect logic --- packages/interactivity/src/directives.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/interactivity/src/directives.js b/packages/interactivity/src/directives.js index f1b7f1c4d959c..0fd532debc775 100644 --- a/packages/interactivity/src/directives.js +++ b/packages/interactivity/src/directives.js @@ -255,7 +255,8 @@ export default () => { // 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 !== null || result !== undefined ) && + result !== null && + result !== undefined && ( result !== false || attribute[ 4 ] === '-' ) ) { el.setAttribute( attribute, result ); From 95f2ad1d0794d22f1a97e88be1c8e90723791d18 Mon Sep 17 00:00:00 2001 From: David Arenas Date: Tue, 29 Aug 2023 01:48:08 +0200 Subject: [PATCH 7/9] Update changelog --- packages/interactivity/CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) 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) From 23489d5c8c27583b006d034a49bbc904e614fa8e Mon Sep 17 00:00:00 2001 From: David Arenas Date: Tue, 29 Aug 2023 02:10:38 +0200 Subject: [PATCH 8/9] Add type and comments to matrix entries --- .../interactivity/directive-bind.spec.ts | 40 ++++++++++++++++++- 1 file changed, 38 insertions(+), 2 deletions(-) diff --git a/test/e2e/specs/interactivity/directive-bind.spec.ts b/test/e2e/specs/interactivity/directive-bind.spec.ts index d8cfe2cc583fe..401bbcd6b24dd 100644 --- a/test/e2e/specs/interactivity/directive-bind.spec.ts +++ b/test/e2e/specs/interactivity/directive-bind.spec.ts @@ -95,7 +95,43 @@ test.describe( 'data-wp-bind', () => { } ); test.describe( 'attribute hydration', () => { - const matrix = [ + /** + * 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', @@ -168,7 +204,7 @@ test.describe( 'data-wp-bind', () => { page, } ) => { for ( const type in values ) { - const [ attrValue, propValue ] = ( values as any )[ type ]; + const [ attrValue, propValue ] = values[ type ]; const container = page.getByTestId( `hydrating ${ type }` ); const el = container.getByTestId( testid ); From fddfad79fab554c9dc4d70974a442210a3056eac Mon Sep 17 00:00:00 2001 From: David Arenas Date: Tue, 29 Aug 2023 11:21:39 +0200 Subject: [PATCH 9/9] Fix phpcs errors --- .../directive-bind/render.php | 20 +++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) 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 84fd969c900b2..a94eb20bfa6d5 100644 --- a/packages/e2e-tests/plugins/interactive-blocks/directive-bind/render.php +++ b/packages/e2e-tests/plugins/interactive-blocks/directive-bind/render.php @@ -58,18 +58,18 @@

'{ "value": false }', - 'true' => '{ "value": true }', - 'null' => '{ "value": null }', - 'undef' => '{ "__any": "any" }', - 'emptyString' => '{ "value": "" }', - 'anyString' => '{ "value": "any" }', - 'number' => '{ "value": 10 }' - ); + $hydration_cases = array( + 'false' => '{ "value": false }', + 'true' => '{ "value": true }', + 'null' => '{ "value": null }', + 'undef' => '{ "__any": "any" }', + 'emptyString' => '{ "value": "" }', + 'anyString' => '{ "value": "any" }', + 'number' => '{ "value": 10 }', + ); ?> - $context ): ?> + $context ) : ?>