Interactivity API: Allow more directives suffixes #65803
Conversation
sirreal
added
[Type] Enhancement
| const classFinder = new RegExp( | ||
| `(^|\\s)${ className }(\\s|$)`, | ||
| 'g' | ||
| ); |
There was a problem hiding this comment.
Independent of the other changes, this is likely a good change. Building a regex effectively from an input string like was done here is risky.
There was a problem hiding this comment.
This change is necessary here because there are number of special characters that would need special processing in the Regexp like [].
sirreal
requested review from
ntwb,
nerrad,
ajitbohra,
luisherranz and
DAreRodz
as code owners
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
Size Change: +37 B (0%) Total Size: 1.77 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Nice work, just had one comment in #65803 (comment)
Otherwise this looks good to go 🙂
|
This one is tricky! I see using tailwindcss classes with arbitrary values with I don't want to push back this PR, but maybe we need to revisit @sirreal, @michalczaplinski, do you agree? Any comments or ideas? |
|
Flaky tests detected in c5b8aed. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/11221440574
|
|
@DAreRodz Thanks for bringing this up! I completely agree. We gotta think it through. Perhaps a new syntax can be added that supports those characters. In any case, let's discuss before moving forward with the PR 🙂 . |
|
The HTML standard on data- attributes says:
According to that, the implementation was wrong before because it allowed uppercase letters. It's likely more interesting to look at what browers actually support. HTML parsing rules about attributes are more relaxed, I believe it's closer to "data attribute names cannot contain the characters |
|
Indeed, browsers might support those characters in the attribute names. On the other hand, we faced a very similar question regarding directive naming Two alternatives came up to the current PR:
The authoring experience of 1. (the object syntax) could be better (as @sirreal pointed out). It's true, but AI tooling makes the syntax question a bit less important in the future. AI-enabled editors can autocomplete with the right syntax without the user having to think about it. That said, I am slightly in favor of allowing the special characters in suffixes (the current PR) if they turn out to be supported by all major browsers. |
This aligns with server-side behavior
|
This gets more interesting the more time I spend on it. The most important thing is that the client and server agree, behavior should be indistinguishable. For me that's an essential principle of this API. Because the HTML is read and injected from the attribute name (where HTML character references are not processed) and these HTML encodings can be part of attribute names, and because HTML character references are processed in attribute values, HTML character references mean that many things can be used as class names. The dev ex starts to get horrible here, but this is all supported by the server right now. Did you know we can set multiple class values in a single directive on the server right now? I just realized it 🙂 The server handles this: <div
data-wp-class--class-with-space-FOO BAR="context.val"
></div>Setting the class value to Here's another interesting example using HTML character references: <div
data-wp-class--att="value"="context.val"
></div>The class set by the server here decodes to These character references also give us access to capital letters in class names, for example this gets the class name <div
data-wp-class--FOO="context.val"
></div> |
|
Obviously the developer experience becomes awful when you're using a directive like I simply want to point out that this behavior should either be supported or not supported, but the client and server should not support different things. |
There was a discussion back in the days of the Block Hydration Experiments. Here's the link: |
|
@sirreal, those examples you shared are really interesting. 🤔 Do you know if that syntax works in browsers as well? Asking because I did a quick test, and the attribute name seems to keep the escape characters. E.g., let's take the following example: <div
data-wp-class--class-with-space-FOO BAR="context.val bar"
></div>You can see the escape characters remain in the $0.attributes[0].nodeName
// 'data-wp-class--class-with-space-foo bar'The same characters in the attribute seem to be parsed, though. $0.attributes[0].nodeValue
// 'context.val bar' |
|
Yes, all of those examples already worked on the server and work on the client as of this PR (matching server behavior). Tested across latest Chrome, Firefox, and Safari. HTML character references are not processed in HTML attribute names but they are in values. The HTML API backing the server implementation gets this right and its why we have this interesting behavior now. |
|
Here's some basic demo code to play with: <div data-wp-interactive="demo" data-wp-context='{"val": true}' id="demo-6213">
<style >
#demo-6213 {
div { min-width: 10px; min-height: 10px; background: cyan; }
div::before {
content: 'class name: ' attr(class);
}
.att\=\"value\" { border-top: 2px solid yellow; }
.class-with-space-FOO { border-right: 2px solid yellow; }
.BAR { border-bottom: 2px solid yellow; }
}
</style>
<div data-wp-class--default="context.val"></div>
<div
data-wp-class--FOO="context.val"
></div>
<div
data-wp-class--att="value"="context.val"
></div>
<button type="button" data-wp-on--click="handleTest">toggle val</button>
</div>import * as I from '@wordpress/interactivity';
const store = I.createStore("demo", {
handleTest() {
I.getContext().val = ! I.getContext().val;
},
}); |
|
@sirreal, I have just tested the HTML you provided in #65803 (comment) with the less privileged user – author. Unfortunately, data attributes with special characters like |
|
https://core.trac.wordpress.org/ticket/61501 and WordPress/wordpress-develop#6429 propose allowing more data attribute names through kses. With a reasonable motivation we may be able to move those forward. Not everything goes through kses. Things like post content and blocks will, but plugins may have custom HTML that doesn't pass through kses but is processed by the Interactivity API. I'm not sure what that means for these changes. Should the Interactivity API inherit limitations and restrictions from other systems? |
That’s an perfect question. It looks like you need to be an user with special permissions to save the advanced Tailwind syntax to the database. That also applies to Interactivity API which might be a good middle ground. It's mostly the question whether we should relax the kses implementation to support more common use cases discussed in this PR. |
|
Since this conversation, I'm opposed to introducing APIs that promote the use of invalid HTML in WordPress, even if browsers support them. I think it's important for WordPress to remain spec-compliant when there are API alternatives that ensure it will work in all cases, like this one from @Potherca:
So, my proposal is not to add support for attributes that have characters other than hyphens, underscores or dots (disregarded because of lack of JSX support). I guess we should probably fix that in the server. For the specific case of classes with unusual characters, we can leverage the suffix "default" for My idea is that passing an object to Instead of: <div
data-wp-class--some-class="state.isSomeClass"
data-wp-class---some-tailwaind-[-24rem]="state.isTailwindClass"
></div>You would use: <div data-wp-class="state.someClasses"></div>And then define store('...', {
state: {
someClasses: {
'some-class': true,
'some-tailwind-[-24rem]': false,
},
},
});And toggle the object properties: const { state } = store('...', {
actions: {
toggleTailwindClass() {
state.someClasses['some-tailwaind-[-24rem]'] =
!state.someClasses['some-tailwaind-[-24rem]'];
},
},
});This API wouldn't be only a way to solve this particular edge case since its uses go beyond just this (like aggregating a bunch of classes into a single directive), but it would help solve this edge case (and others, like uppercase class names So the mental model for
|
|
That seems like reasonable opposition to supporting these non-standard data attributes. In that case this should absolutely be addressed on the server so that the behaviors match. I've created a trac ticket for that: https://core.trac.wordpress.org/ticket/62426 If this PR doesn't move ahead, it's likely a good idea to extract this change to avoid relying on a fragile regex: https://github.com/WordPress/gutenberg/pull/65803/files#r1783979043 |
|
WordPress/wordpress-develop#8048 restricts server-side directives. |
What?
https://core.trac.wordpress.org/ticket/62131 describes a case where the Interactivity API client does not correctly handle the class name part of a class directive. This class directive is correctly handled on the server, but will be ignored by the more restrictive client:
Alternative
If we do not want to relax this restriction, the server should apply the same restriction on directive attributes (they should match the same Regexp). This would avoid a mismatch where the server sets a class the client would not remove.
Why?
The "suffix" can support more characters technically. It seems like some unusual class names may be reasonable.
A use case mentioned on the ticket is Tailwind, a popular CSS framework:
<div class="top-[3px]"><div class="top-4 hover:top-6"><div class="top-4 md:top-6"><div class="grid grid-flow-col auto-cols-[minmax(0,_2fr)]">How?
Relax some client-side constraints on the directive names.
Testing Instructions
Tests are included.