Skip to content

Commit 902a300

Browse files
DAreRodzluisherranz
DAreRodz
and
luisherranz
authored
Interactivity API: Update data-wp-bind directive logic (#54003)
* Add undefined and null to the removeAttribute This should be done in a separate PR. * Adding first test cases (WIP) * Add comment * Remove unnecessary comment * Update test cases * Fix bind useEffect logic * Update changelog * Add type and comments to matrix entries * Fix phpcs errors --------- Co-authored-by: Luis Herranz <[email protected]>
1 parent 58f5e9a commit 902a300

File tree

5 files changed

+252
-10
lines changed

5 files changed

+252
-10
lines changed

‎packages/e2e-tests/plugins/interactive-blocks/directive-bind/render.php

+40
Original file line numberDiff line numberDiff line change
@@ -56,4 +56,44 @@
5656
>
5757
Some Text
5858
</p>
59+
60+
<?php
61+
$hydration_cases = array(
62+
'false' => '{ "value": false }',
63+
'true' => '{ "value": true }',
64+
'null' => '{ "value": null }',
65+
'undef' => '{ "__any": "any" }',
66+
'emptyString' => '{ "value": "" }',
67+
'anyString' => '{ "value": "any" }',
68+
'number' => '{ "value": 10 }',
69+
);
70+
?>
71+
72+
<?php foreach ( $hydration_cases as $type => $context ) : ?>
73+
<div
74+
data-testid='hydrating <?php echo $type; ?>'
75+
data-wp-context='<?php echo $context; ?>'
76+
>
77+
<img
78+
alt="Red dot"
79+
data-testid="image"
80+
data-wp-bind--width="context.value"
81+
src="data:image/png;base64, iVBORw0KGgoAAAANSUhEUgAAAAUA
82+
AAAFCAYAAACNbyblAAAAHElEQVQI12P4//8/w38GIAXDIBKE0DHxgljNBAAO
83+
9TXL0Y4OHwAAAABJRU5ErkJggg=="
84+
>
85+
<input
86+
type="text"
87+
data-testid="input"
88+
data-wp-bind--name="context.value"
89+
data-wp-bind--value="context.value"
90+
data-wp-bind--disabled="context.value"
91+
data-wp-bind--aria-disabled="context.value"
92+
>
93+
<button
94+
data-testid="toggle value"
95+
data-wp-on--click="actions.toggleValue"
96+
>Toggle</button>
97+
</div>
98+
<?php endforeach; ?>
5999
</div>

‎packages/e2e-tests/plugins/interactive-blocks/directive-bind/view.js

+11
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,17 @@
1818
state.show = ! state.show;
1919
state.width += foo.bar;
2020
},
21+
toggleValue: ( { context } ) => {
22+
const previousValue = ( 'previousValue' in context )
23+
? context.previousValue
24+
// Any string works here; we just want to toggle the value
25+
// to ensure Preact renders the same we are hydrating in the
26+
// first place.
27+
: 'tacocat';
28+
29+
context.previousValue = context.value;
30+
context.value = previousValue;
31+
}
2132
},
2233
} );
2334
} )( window );

‎packages/interactivity/CHANGELOG.md

+1
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
- Support keys using `data-wp-key`. ([#53844](https://github.com/WordPress/gutenberg/pull/53844))
88
- Merge new server-side rendered context on client-side navigation. ([#53853](https://github.com/WordPress/gutenberg/pull/53853))
99
- Support region-based client-side navigation. ([#53733](https://github.com/WordPress/gutenberg/pull/53733))
10+
- Improve `data-wp-bind` hydration to match Preact's logic. ([#54003](https://github.com/WordPress/gutenberg/pull/54003))
1011

1112
## 2.1.0 (2023-08-16)
1213

‎packages/interactivity/src/directives.js

+35-8
Original file line numberDiff line numberDiff line change
@@ -222,19 +222,46 @@ export default () => {
222222
// on the hydration, so we have to do it manually. It doesn't need
223223
// deps because it only needs to do it the first time.
224224
useEffect( () => {
225+
const el = element.ref.current;
226+
227+
// We set the value directly to the corresponding
228+
// HTMLElement instance property excluding the following
229+
// special cases.
230+
// We follow Preact's logic: https://github.com/preactjs/preact/blob/ea49f7a0f9d1ff2c98c0bdd66aa0cbc583055246/src/diff/props.js#L110-L129
231+
if (
232+
attribute !== 'width' &&
233+
attribute !== 'height' &&
234+
attribute !== 'href' &&
235+
attribute !== 'list' &&
236+
attribute !== 'form' &&
237+
// Default value in browsers is `-1` and an empty string is
238+
// cast to `0` instead
239+
attribute !== 'tabIndex' &&
240+
attribute !== 'download' &&
241+
attribute !== 'rowSpan' &&
242+
attribute !== 'colSpan' &&
243+
attribute in el
244+
) {
245+
try {
246+
el[ attribute ] =
247+
result === null || result === undefined
248+
? ''
249+
: result;
250+
return;
251+
} catch ( err ) {}
252+
}
225253
// aria- and data- attributes have no boolean representation.
226254
// A `false` value is different from the attribute not being
227255
// present, so we can't remove it.
228256
// We follow Preact's logic: https://github.com/preactjs/preact/blob/ea49f7a0f9d1ff2c98c0bdd66aa0cbc583055246/src/diff/props.js#L131C24-L136
229-
if ( result === false && attribute[ 4 ] !== '-' ) {
230-
element.ref.current.removeAttribute( attribute );
257+
if (
258+
result !== null &&
259+
result !== undefined &&
260+
( result !== false || attribute[ 4 ] === '-' )
261+
) {
262+
el.setAttribute( attribute, result );
231263
} else {
232-
element.ref.current.setAttribute(
233-
attribute,
234-
result === true && attribute[ 4 ] !== '-'
235-
? ''
236-
: result
237-
);
264+
el.removeAttribute( attribute );
238265
}
239266
}, [] );
240267
} );

‎test/e2e/specs/interactivity/directive-bind.spec.ts

+165-2
Original file line numberDiff line numberDiff line change
@@ -37,12 +37,12 @@ test.describe( 'data-wp-bind', () => {
3737

3838
test( 'add missing checked at hydration', async ( { page } ) => {
3939
const el = page.getByTestId( 'add missing checked at hydration' );
40-
await expect( el ).toHaveAttribute( 'checked', '' );
40+
await expect( el ).toBeChecked();
4141
} );
4242

4343
test( 'remove existing checked at hydration', async ( { page } ) => {
4444
const el = page.getByTestId( 'remove existing checked at hydration' );
45-
await expect( el ).not.toHaveAttribute( 'checked', '' );
45+
await expect( el ).not.toBeChecked();
4646
} );
4747

4848
test( 'update existing checked', async ( { page } ) => {
@@ -93,4 +93,167 @@ test.describe( 'data-wp-bind', () => {
9393
await expect( el ).toHaveAttribute( 'aria-expanded', 'true' );
9494
await expect( el ).toHaveAttribute( 'data-some-value', 'true' );
9595
} );
96+
97+
test.describe( 'attribute hydration', () => {
98+
/**
99+
* Data structure to define a hydration test case.
100+
*/
101+
type MatrixEntry = {
102+
/**
103+
* Test ID of the element (the `data-testid` attr).
104+
*/
105+
testid: string;
106+
/**
107+
* Name of the attribute being hydrated.
108+
*/
109+
name: string;
110+
/**
111+
* Array of different values to test.
112+
*/
113+
values: Record<
114+
/**
115+
* The type of value we are hydrating. E.g., false is `false`,
116+
* undef is `undefined`, emptyString is `''`, etc.
117+
*/
118+
string,
119+
[
120+
/**
121+
* Value that the attribute should contain after hydration.
122+
* If the attribute is missing, this value is `null`.
123+
*/
124+
attributeValue: any,
125+
/**
126+
* Value that the HTMLElement instance property should
127+
* contain after hydration.
128+
*/
129+
entityPropValue: any
130+
]
131+
>;
132+
};
133+
134+
const matrix: MatrixEntry[] = [
135+
{
136+
testid: 'image',
137+
name: 'width',
138+
values: {
139+
false: [ null, 5 ],
140+
true: [ 'true', 5 ],
141+
null: [ null, 5 ],
142+
undef: [ null, 5 ],
143+
emptyString: [ '', 5 ],
144+
anyString: [ 'any', 5 ],
145+
number: [ '10', 10 ],
146+
},
147+
},
148+
{
149+
testid: 'input',
150+
name: 'name',
151+
values: {
152+
false: [ 'false', 'false' ],
153+
true: [ 'true', 'true' ],
154+
null: [ '', '' ],
155+
undef: [ '', '' ],
156+
emptyString: [ '', '' ],
157+
anyString: [ 'any', 'any' ],
158+
number: [ '10', '10' ],
159+
},
160+
},
161+
{
162+
testid: 'input',
163+
name: 'value',
164+
values: {
165+
false: [ null, 'false' ],
166+
true: [ null, 'true' ],
167+
null: [ null, '' ],
168+
undef: [ null, '' ],
169+
emptyString: [ null, '' ],
170+
anyString: [ null, 'any' ],
171+
number: [ null, '10' ],
172+
},
173+
},
174+
{
175+
testid: 'input',
176+
name: 'disabled',
177+
values: {
178+
false: [ null, false ],
179+
true: [ '', true ],
180+
null: [ null, false ],
181+
undef: [ null, false ],
182+
emptyString: [ null, false ],
183+
anyString: [ '', true ],
184+
number: [ '', true ],
185+
},
186+
},
187+
{
188+
testid: 'input',
189+
name: 'aria-disabled',
190+
values: {
191+
false: [ 'false', undefined ],
192+
true: [ 'true', undefined ],
193+
null: [ null, undefined ],
194+
undef: [ null, undefined ],
195+
emptyString: [ '', undefined ],
196+
anyString: [ 'any', undefined ],
197+
number: [ '10', undefined ],
198+
},
199+
},
200+
];
201+
202+
for ( const { testid, name, values } of matrix ) {
203+
test( `${ name } is correctly hydrated for different values`, async ( {
204+
page,
205+
} ) => {
206+
for ( const type in values ) {
207+
const [ attrValue, propValue ] = values[ type ];
208+
209+
const container = page.getByTestId( `hydrating ${ type }` );
210+
const el = container.getByTestId( testid );
211+
const toggle = container.getByTestId( 'toggle value' );
212+
213+
const hydratedAttr = await el.getAttribute( name );
214+
const hydratedProp = await el.evaluate(
215+
( node, propName ) => ( node as any )[ propName ],
216+
name
217+
);
218+
expect( [ type, hydratedAttr ] ).toEqual( [
219+
type,
220+
attrValue,
221+
] );
222+
expect( [ type, hydratedProp ] ).toEqual( [
223+
type,
224+
propValue,
225+
] );
226+
227+
// Only check the rendered value if the new value is not
228+
// `undefined` and the attibute is neither `value` nor
229+
// `disabled` because Preact doesn't update the attribute
230+
// for those cases.
231+
// See https://github.com/preactjs/preact/blob/099c38c6ef92055428afbc116d18a6b9e0c2ea2c/src/diff/index.js#L471-L494
232+
if (
233+
type === 'undef' &&
234+
( name === 'value' || name === 'undefined' )
235+
) {
236+
return;
237+
}
238+
239+
await toggle.click( { clickCount: 2, delay: 50 } );
240+
241+
// Values should be the same as before.
242+
const renderedAttr = await el.getAttribute( name );
243+
const renderedProp = await el.evaluate(
244+
( node, propName ) => ( node as any )[ propName ],
245+
name
246+
);
247+
expect( [ type, renderedAttr ] ).toEqual( [
248+
type,
249+
attrValue,
250+
] );
251+
expect( [ type, renderedProp ] ).toEqual( [
252+
type,
253+
propValue,
254+
] );
255+
}
256+
} );
257+
}
258+
} );
96259
} );

0 commit comments

Comments
 (0)