From 365a5971725486ae99e8147c81c1c2a75fe0f1fe Mon Sep 17 00:00:00 2001 From: Luis Herranz Date: Wed, 23 Aug 2023 17:57:19 +0200 Subject: [PATCH 01/14] 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 02/14] 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 03/14] 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 04/14] 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 05/14] 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 06/14] 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 07/14] 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 08/14] 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 09/14] 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 ) : ?>
Date: Fri, 25 Aug 2023 20:21:32 +0200 Subject: [PATCH 10/14] Copy current implementation --- packages/interactivity/src/directives.js | 52 ++++++++++++++++++++++++ packages/interactivity/src/slots.js | 38 +++++++++++++++++ 2 files changed, 90 insertions(+) create mode 100644 packages/interactivity/src/slots.js diff --git a/packages/interactivity/src/directives.js b/packages/interactivity/src/directives.js index 0fd532debc775..bc54c57b8d43b 100644 --- a/packages/interactivity/src/directives.js +++ b/packages/interactivity/src/directives.js @@ -10,6 +10,7 @@ import { deepSignal, peek } from 'deepsignal'; import { createPortal } from './portals'; import { useSignalEffect } from './utils'; import { directive } from './hooks'; +import { SlotProvider, Slot, Fill } from './slots'; const isObject = ( item ) => item && typeof item === 'object' && ! Array.isArray( item ); @@ -305,4 +306,55 @@ export default () => { } ); } ); + + // data-wp-slot + directive( + 'slot', + ( { + directives: { + slot: { default: slot, above, below }, + }, + props: { children }, + } ) => { + return ( + <> + { above && } + { slot ? ( + { children } + ) : ( + children + ) } + { below && } + + ); + }, + { priority: 4 } + ); + + // data-wp-fill + directive( + 'fill', + ( { + directives: { + fill: { default: fill }, + }, + props: { children }, + evaluate, + context, + } ) => { + const contextValue = useContext( context ); + const slot = evaluate( fill, { context: contextValue } ); + return { children }; + }, + { priority: 4 } + ); + + // data-wp-slot-provider + directive( + 'slot-provider', + ( { props: { children } } ) => ( + { children } + ), + { priority: 4 } + ); }; diff --git a/packages/interactivity/src/slots.js b/packages/interactivity/src/slots.js new file mode 100644 index 0000000000000..e8bc6ddfa368f --- /dev/null +++ b/packages/interactivity/src/slots.js @@ -0,0 +1,38 @@ +/** + * External dependencies + */ +import { createContext } from 'preact'; +import { useContext, useEffect } from 'preact/hooks'; +import { signal } from '@preact/signals'; + +const slotsContext = createContext(); + +export const Fill = ( { slot, children } ) => { + const slots = useContext( slotsContext ); + + useEffect( () => { + if ( slot ) { + slots.value = { ...slots.value, [ slot ]: children }; + return () => { + slots.value = { ...slots.value, [ slot ]: null }; + }; + } + }, [ slots, slot, children ] ); + + return !! slot ? null : children; +}; + +export const SlotProvider = ( { children } ) => { + return ( + // TODO: We can change this to use deepsignal once this PR is merged. + // https://github.com/luisherranz/deepsignal/pull/38 + + { children } + + ); +}; + +export const Slot = ( { name, children } ) => { + const slots = useContext( slotsContext ); + return slots.value[ name ] || children; +}; From 72eba45e9065f41aa879fae0dc650ad363b15474 Mon Sep 17 00:00:00 2001 From: David Arenas Date: Fri, 25 Aug 2023 22:19:19 +0200 Subject: [PATCH 11/14] Refactor data-wp-slot --- packages/interactivity/src/directives.js | 41 +++++++++++++++++------- 1 file changed, 29 insertions(+), 12 deletions(-) diff --git a/packages/interactivity/src/directives.js b/packages/interactivity/src/directives.js index bc54c57b8d43b..f0a3d7e32e09e 100644 --- a/packages/interactivity/src/directives.js +++ b/packages/interactivity/src/directives.js @@ -312,21 +312,38 @@ export default () => { 'slot', ( { directives: { - slot: { default: slot, above, below }, + slot: { default: slot }, }, props: { children }, + element, } ) => { - return ( - <> - { above && } - { slot ? ( - { children } - ) : ( - children - ) } - { below && } - - ); + const name = typeof slot === 'string' ? slot : slot.name; + const position = slot.position || 'children'; + + if ( position === 'before' ) { + return ( + <> + + { children } + + ); + } + if ( position === 'after' ) { + return ( + <> + { children } + + + ); + } + if ( position === 'replace' ) { + return { children }; + } + if ( position === 'children' ) { + element.props.children = ( + { element.props.children } + ); + } }, { priority: 4 } ); From 0f743296e464799711f80d7498865084e1463459 Mon Sep 17 00:00:00 2001 From: David Arenas Date: Fri, 25 Aug 2023 22:19:38 +0200 Subject: [PATCH 12/14] Add slot and fill tests --- .../directive-slots/block.json | 14 ++ .../directive-slots/render.php | 67 +++++++ .../directive-slots/view.js | 18 ++ .../interactivity/directive-slots.spec.ts | 186 ++++++++++++++++++ 4 files changed, 285 insertions(+) create mode 100644 packages/e2e-tests/plugins/interactive-blocks/directive-slots/block.json create mode 100644 packages/e2e-tests/plugins/interactive-blocks/directive-slots/render.php create mode 100644 packages/e2e-tests/plugins/interactive-blocks/directive-slots/view.js create mode 100644 test/e2e/specs/interactivity/directive-slots.spec.ts diff --git a/packages/e2e-tests/plugins/interactive-blocks/directive-slots/block.json b/packages/e2e-tests/plugins/interactive-blocks/directive-slots/block.json new file mode 100644 index 0000000000000..f79f89a6e81b8 --- /dev/null +++ b/packages/e2e-tests/plugins/interactive-blocks/directive-slots/block.json @@ -0,0 +1,14 @@ +{ + "apiVersion": 2, + "name": "test/directive-slots", + "title": "E2E Interactivity tests - directive slots", + "category": "text", + "icon": "heart", + "description": "", + "supports": { + "interactivity": true + }, + "textdomain": "e2e-interactivity", + "viewScript": "directive-slots-view", + "render": "file:./render.php" +} diff --git a/packages/e2e-tests/plugins/interactive-blocks/directive-slots/render.php b/packages/e2e-tests/plugins/interactive-blocks/directive-slots/render.php new file mode 100644 index 0000000000000..5c1558d35403d --- /dev/null +++ b/packages/e2e-tests/plugins/interactive-blocks/directive-slots/render.php @@ -0,0 +1,67 @@ + +
+
+
[1]
+
[2]
+
[3]
+
[4]
+
[5]
+
+ +
+ initial +
+ +
+ + + + + + +
+
diff --git a/packages/e2e-tests/plugins/interactive-blocks/directive-slots/view.js b/packages/e2e-tests/plugins/interactive-blocks/directive-slots/view.js new file mode 100644 index 0000000000000..ab5b39379f3a8 --- /dev/null +++ b/packages/e2e-tests/plugins/interactive-blocks/directive-slots/view.js @@ -0,0 +1,18 @@ +( ( { wp } ) => { + const { store } = wp.interactivity; + + store( { + state: { + slot: '' + }, + actions: { + changeSlot: ( { state, event } ) => { + state.slot = event.target.dataset.slot; + }, + updateSlotText: ( { context } ) => { + const n = context.text[1]; + context.text = `[${n} updated]`; + }, + }, + } ); +} )( window ); diff --git a/test/e2e/specs/interactivity/directive-slots.spec.ts b/test/e2e/specs/interactivity/directive-slots.spec.ts new file mode 100644 index 0000000000000..d93e50f767215 --- /dev/null +++ b/test/e2e/specs/interactivity/directive-slots.spec.ts @@ -0,0 +1,186 @@ +/** + * Internal dependencies + */ +import { test, expect } from './fixtures'; + +test.describe( 'data-wp-slot', () => { + test.beforeAll( async ( { interactivityUtils: utils } ) => { + await utils.activatePlugins(); + await utils.addPostWithBlock( 'test/directive-slots' ); + } ); + + test.beforeEach( async ( { interactivityUtils: utils, page } ) => { + await page.goto( utils.getLink( 'test/directive-slots' ) ); + } ); + + test.afterAll( async ( { interactivityUtils: utils } ) => { + await utils.deactivatePlugins(); + await utils.deleteAllPosts(); + } ); + + test( 'should render the fill in its children by default', async ( { + page, + } ) => { + const slot1 = page.getByTestId( 'slot-1' ); + const slots = page.getByTestId( 'slots' ); + const fillContainer = page.getByTestId( 'fill-container' ); + + await page.getByTestId( 'slot-1-button' ).click(); + + await expect( fillContainer ).toBeEmpty(); + await expect( slot1.getByTestId( 'fill' ) ).toBeVisible(); + await expect( slot1 ).toHaveText( 'fill inside slot 1' ); + await expect( slots.locator( 'css= > *' ) ).toHaveText( [ + 'fill inside slot 1', + '[2]', + '[3]', + '[4]', + '[5]', + ] ); + } ); + + test( 'should render the fill before if specified', async ( { page } ) => { + const slot2 = page.getByTestId( 'slot-2' ); + const slots = page.getByTestId( 'slots' ); + const fillContainer = page.getByTestId( 'fill-container' ); + + await page.getByTestId( 'slot-2-button' ).click(); + + await expect( fillContainer ).toBeEmpty(); + await expect( slot2 ).toHaveText( '[2]' ); + await expect( slots.getByTestId( 'fill' ) ).toBeVisible(); + await expect( slots.locator( 'css= > *' ) ).toHaveText( [ + '[1]', + 'fill inside slots', + '[2]', + '[3]', + '[4]', + '[5]', + ] ); + } ); + + test( 'should render the fill after if specified', async ( { page } ) => { + const slot3 = page.getByTestId( 'slot-3' ); + const slots = page.getByTestId( 'slots' ); + const fillContainer = page.getByTestId( 'fill-container' ); + + await page.getByTestId( 'slot-3-button' ).click(); + + await expect( fillContainer ).toBeEmpty(); + await expect( slot3 ).toHaveText( '[3]' ); + await expect( slots.getByTestId( 'fill' ) ).toBeVisible(); + await expect( slots.locator( 'css= > *' ) ).toHaveText( [ + '[1]', + '[2]', + '[3]', + 'fill inside slots', + '[4]', + '[5]', + ] ); + } ); + + test( 'should render the fill in its children if specified', async ( { + page, + } ) => { + const slot4 = page.getByTestId( 'slot-4' ); + const slots = page.getByTestId( 'slots' ); + const fillContainer = page.getByTestId( 'fill-container' ); + + await page.getByTestId( 'slot-4-button' ).click(); + + await expect( fillContainer ).toBeEmpty(); + await expect( slot4.getByTestId( 'fill' ) ).toBeVisible(); + await expect( slot4 ).toHaveText( 'fill inside slot 4' ); + await expect( slots.locator( 'css= > *' ) ).toHaveText( [ + '[1]', + '[2]', + '[3]', + 'fill inside slot 4', + '[5]', + ] ); + } ); + + test( 'should be replaced by the fill if specified', async ( { page } ) => { + const slot5 = page.getByTestId( 'slot-5' ); + const slots = page.getByTestId( 'slots' ); + const fillContainer = page.getByTestId( 'fill-container' ); + + await page.getByTestId( 'slot-5-button' ).click(); + + await expect( fillContainer ).toBeEmpty(); + await expect( slot5 ).not.toBeVisible(); + await expect( slots.getByTestId( 'fill' ) ).toBeVisible(); + await expect( slots.locator( 'css= > *' ) ).toHaveText( [ + '[1]', + '[2]', + '[3]', + '[4]', + 'fill inside slots', + ] ); + } ); + + test( 'should keep the fill in its original position if no slot matches', async ( { + page, + } ) => { + const fillContainer = page.getByTestId( 'fill-container' ); + await expect( fillContainer.getByTestId( 'fill' ) ).toBeVisible(); + + await page.getByTestId( 'slot-1-button' ).click(); + + await expect( fillContainer ).toBeEmpty(); + + await page.getByTestId( 'reset' ).click(); + + await expect( fillContainer.getByTestId( 'fill' ) ).toBeVisible(); + } ); + + test( 'should not be re-mounted when adding the fill before', async ( { + page, + } ) => { + const slot2 = page.getByTestId( 'slot-2' ); + const slots = page.getByTestId( 'slots' ); + + await expect( slot2 ).toHaveText( '[2]' ); + + await slot2.click(); + + await expect( slot2 ).toHaveText( '[2 updated]' ); + + await page.getByTestId( 'slot-2-button' ).click(); + + await expect( slots.getByTestId( 'fill' ) ).toBeVisible(); + await expect( slots.locator( 'css= > *' ) ).toHaveText( [ + '[1]', + 'fill inside slots', + '[2 updated]', + '[3]', + '[4]', + '[5]', + ] ); + } ); + + test( 'should not be re-mounted when adding the fill after', async ( { + page, + } ) => { + const slot3 = page.getByTestId( 'slot-3' ); + const slots = page.getByTestId( 'slots' ); + + await expect( slot3 ).toHaveText( '[3]' ); + + await slot3.click(); + + await expect( slot3 ).toHaveText( '[3 updated]' ); + + await page.getByTestId( 'slot-3-button' ).click(); + + await expect( slots.getByTestId( 'fill' ) ).toBeVisible(); + await expect( slots.locator( 'css= > *' ) ).toHaveText( [ + '[1]', + '[2]', + '[3 updated]', + 'fill inside slots', + '[4]', + '[5]', + ] ); + } ); +} ); From 0013244a54b54503b7d71d545e15dc53ec991315 Mon Sep 17 00:00:00 2001 From: David Arenas Date: Fri, 25 Aug 2023 22:41:55 +0200 Subject: [PATCH 13/14] Add changelog --- packages/interactivity/CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/interactivity/CHANGELOG.md b/packages/interactivity/CHANGELOG.md index 62515115484fa..9ce48303637d8 100644 --- a/packages/interactivity/CHANGELOG.md +++ b/packages/interactivity/CHANGELOG.md @@ -15,6 +15,8 @@ - Allow passing optional `afterLoad` callbacks to `store` calls. ([#53363](https://github.com/WordPress/gutenberg/pull/53363)) +- Add new directives that implement the Slot and Fill pattern: `data-wp-slot-provider`, `data-wp-slot` and `data-wp-fill`. ([#53958](https://github.com/WordPress/gutenberg/pull/53958)) + ### Bug Fix - Add support for underscores and leading dashes in the suffix part of the directive. ([#53337](https://github.com/WordPress/gutenberg/pull/53337)) From cb8eacb437e2346949c93b05d8695553f16c8952 Mon Sep 17 00:00:00 2001 From: David Arenas Date: Tue, 29 Aug 2023 01:52:28 +0200 Subject: [PATCH 14/14] Fix changelog --- packages/interactivity/CHANGELOG.md | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/packages/interactivity/CHANGELOG.md b/packages/interactivity/CHANGELOG.md index 9ce48303637d8..6a4565f3ad183 100644 --- a/packages/interactivity/CHANGELOG.md +++ b/packages/interactivity/CHANGELOG.md @@ -9,14 +9,16 @@ - 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)) +### New Features + +- Add new directives that implement the Slot and Fill pattern: `data-wp-slot-provider`, `data-wp-slot` and `data-wp-fill`. ([#53958](https://github.com/WordPress/gutenberg/pull/53958)) + ## 2.1.0 (2023-08-16) ### New Features - Allow passing optional `afterLoad` callbacks to `store` calls. ([#53363](https://github.com/WordPress/gutenberg/pull/53363)) -- Add new directives that implement the Slot and Fill pattern: `data-wp-slot-provider`, `data-wp-slot` and `data-wp-fill`. ([#53958](https://github.com/WordPress/gutenberg/pull/53958)) - ### Bug Fix - Add support for underscores and leading dashes in the suffix part of the directive. ([#53337](https://github.com/WordPress/gutenberg/pull/53337))