Skip to content
This repository has been archived by the owner on Jul 28, 2023. It is now read-only.

Should we follow the HTML specification to the letter? #132

Closed
domtra opened this issue Jan 18, 2023 · 49 comments · Fixed by #239
Closed

Should we follow the HTML specification to the letter? #132

domtra opened this issue Jan 18, 2023 · 49 comments · Fixed by #239

Comments

@domtra
Copy link

domtra commented Jan 18, 2023

Hi everyone,

I am wondering if you have considered the fact, that the directives introduced here are not valid html when they are used as an attribute?

On regular, built in html elements you are not allowed to define arbitrary attributes. In these cases, you have to use a data- attribute to keep the markup valid.

For custom elements you are basically free to use whatever you want.

Although most JS frameworks use custom attributes when coding, will compile them away / remove these attributes when rendering to the DOM or on the server.

If you are aware of this issue, was it a conscious choice to still use those directives as attributes? If so, what was the reasoning? While there is an ongoing discussion about how important valid markup is, I think this will be a sensible topic in the WordPress ecosystem.

If you do not intend to use the directives as attributes, I apologise for raising this issue. It is still not so easy to get all of the details right for this project from the outside.

@luisherranz
Copy link
Member

Thanks for opening an issue about this topic.

I am wondering if you have considered the fact, that the directives introduced here are not valid html

If I recall correctly, we briefly discussed it with Riad and Miguel (Gutenberg contributors).

It's true wp- attributes don't follow the spec and that is a shame because it is more of a theoretical problem than a real one, as the chances of the HTML spec introducing some sort of wp- attributes that would conflict with ours are basically zero.

In terms of DX, I would personally prefer to keep it as wp- because it's cleaner and faster to write. But I reckon there may be strong opposition in the community because it goes against the spec.

We also thought about replacing all the wp- attributes with data-wp- using PHP right before sending the HTML to the client, but we didn't like the idea because it may cause bugs and some confusion.

So right now, my personal opinion would be to present them using wp-, acknowledge the problem and its possible consequences, and listen to people's opinions. But I'm not the only one contributing to this proposal, so that doesn't mean it's what's going to be done.

I think this will be a sensible topic in the WordPress ecosystem

Yes, I (a bit sadly) agree. And I think the chances of having to resort to data-wp- attributes are high. But although I would prefer not to do so for DX and aesthetic reasons, it won't be the end of the world 🙂🤷‍♂️


Anyway, thanks again for opening this issue. Let's use it to start collecting feedback!! 🙂

@luisherranz
Copy link
Member

@dmsnell shared on this issue that he would go with data-wp-.

@WordPress/frontend-dx what's your take? Go with data-wp- and respect the spec or try to convince the WordPress community that the likelihood of HTML introducing wp- attributes is close to zero?

@DAreRodz
Copy link
Collaborator

If we go with the wp- prefix, we wouldn't be the only ones using custom attributes outside the spec, and it would be a bit nicer and more comfortable to write. 🤔

On the other hand, regarding implementation, attributes with the data-wp- prefix could make hydration a bit faster, as toVdom could ignore most of the attributes of an element and iterate over the dataset instead.

@domtra
Copy link
Author

domtra commented Jan 27, 2023

Thanks for the updates. I personally also would like the wp- fix better, just because of development experience.
But for a project like WordPress, sticking to the spec will probably be a wiser choice. You do not want 40% of the web to suddenly have invalid markup, i guess.

@dmsnell
Copy link
Member

dmsnell commented Jan 27, 2023

I'll repeat here for completeness, what I mentioned over there.

I think the spec issue is probably overblown wording it this way. There's likely no system in the world that would break by adding custom attributes without the data- prefix. As @DAreRodz pointed out there are some intrinsic benefits to using data- attributes (and access via node.dataset wasn't even mentioned. while it's a small convenience, for attribute restructuring it can be useful).

So ultimately the choice is likely best made on other reasons than the spec; whereas the spec can be seen as a tie-breaker or a subtle nudge one way or the other.

There's no way that adding wp-show="…" attributes will break 40% of the web. Even if it's technically invalid, it's not going to break any parsers or any system that handles HTML. At worst it probably means that in some extremely rare cases we'd never heard about an external system that processes a post's HTML and hands it off to some other rare script will skip over these attributes and so they won't make it to the final processing system.

What would be a much bigger problem for validity is choosing to break HTML syntax, as if we were writing <div wp-show={"when": …}>, or to use a name that has a reasonable chance of some day entering HTML's vocabulary, such as <div show-if="…"> or <div when="…">.

IMO the only real risk I've seen in these discussions is using wp- as the prefix vs. something else, anything else, due to the practical consideration of detecting quickly if these attribute directives might exist inside a post. Even then it's not the end of the world if we go that way; it would simply be a decision in which we accept a known avoidable performance optimization in favor of a language aesthetic.

@luisherranz luisherranz changed the title Validity of directives as attributes Directive attributes: wp- or data-wp-? Jan 30, 2023
@luisherranz
Copy link
Member

@SantosGuillamot and I have just realized that the : (colon) character is also invalid HTML. The only valid characters on attribute names are . (dot) and _ (underscore). There may be other valid ones like · (middle dot) but they are not easy to type in most keyboards.

So I guess that if we want to ship valid HTML, we'd better change all our : with ., like this:

<div data-wp-on.click="actions.someAction" data-wp-class.red="selectors.isRed">
  <img data-wp-bind.src="state.someImageURL" />
</div>

@adamziel, @dmsnell: can you confirm this?

@dmsnell
Copy link
Member

dmsnell commented Feb 21, 2023

@luisherranz according to the spec both : and . are expressly allowable as attribute names, as attribute names are quite permissive. however, given that : indicates a namespaced attribute in XML documents (including XHTML documents) it would be advisable to avoid using that here.

The only real guidance is this sentence:

Authors must not use elements, attributes, or attribute values that are not permitted by this specification or other applicable specifications, as doing so makes it significantly harder for the language to be extended in the future.

Obviously this is broken all the time and it's a single mention in a huge document and the WWW works fine. My interpretation is that the spirit of the rule is to avoid reinterpreting well-defined behaviors and to avoid with due diligence anything that could become part of the spec in the future. To me this rules out the use of : since there's a reasonable expectation for that to be used for namespacing, unless we decided to use wpx:show='{"if": "…"}' with the use as namespacing.

@luisherranz
Copy link
Member

Sorry, I meant colons (:) in data-* attributes, not in regular ones. I.e., data-wp-on:click.

The spec says the names should be XML-compatible and XML names can't have colons:

Attribute names are said to be XML-compatible if they match the Name production defined in XML and they contain no U+003A COLON characters (:).

@luisherranz
Copy link
Member

I guess the question we have to answer here is whether we want to propose an API that follows HTML specifications to the letter or not (even if we know that not following it will be fine and won't cause any issues).

In this case, it's just about using data-wp-on.click or wp-on:click, but there may be other cases in the future that will be quickly resolved if we have decided the general position about this.

I'm going to change the title of the issue to reflect that.

@luisherranz luisherranz changed the title Directive attributes: wp- or data-wp-? Should we follow the HTML specification to the letter? Feb 28, 2023
@dmsnell
Copy link
Member

dmsnell commented Feb 28, 2023

I guess the question we have to answer here is whether we want to propose an API that follows HTML specifications to the letter

Well I'd encourage us not to frame it this way, as I don't think "following it to the letter" is something that's meaningful, possible, or practical. The spec covers a number of use cases and provides a spectrum of conformance.

Rules are spread out and suggest internal contradiction. For instance, in the attribute name parsing rules, there's nothing wrong with the : appearing, and while it's a parse error to include a namespace in an attribute name, there's no consequence to that error.

So I prefer to ask about what the specification allows and what implementations will handle. Keep in mind that everything being discussed with directives breaks the HTML specification, because we aren't allowed to add these attributes if they aren't data- attributes, and the tags <wp-show> and all are also not allowed.

What I prefer to follow is how our choices will impact the web and how the web impacts the results of our choices. According to the HTML5 specification we cannot use <div /> to indicate an empty div element, because that will break the rest of the page render. We're also not allowed to use a self-closing <img /> but that error is ignored and benign.

Again, I don't think it's possible to follow the spec to the letter, and if we even could theoretically do that, I'm not sure it would be desirable.

@SantosGuillamot
Copy link
Contributor

SantosGuillamot commented Mar 9, 2023

Trying to resume this conversation, it seems to me we are talking about three different things:

  • Should we follow the HTML specification to the letter?
  • Should we use wp-text or data-wp-text? Keep in mind that we may remove directive tags (<wp-text>) from the proposal. If we decide to follow HTML specification, we would have to go with data-wp-text, if I am not mistaken.
  • Should we use colons (:) or dots (.) for directives. data-wp-on:click vs data-wp-on.click. If we decide to follow HTML specification, we would have to go with data-wp-on.click.

If we end up deciding not to follow the HTML specification to the letter, we may want to open separate discussion for the other questions.

@dmsnell
Copy link
Member

dmsnell commented Mar 9, 2023

Should we follow the HTML specification to the letter?

I still think this is a weird question, or at least a weird phrasing.

My vote is 100% against wp-text for reasons repeated multiple times. I wish we considered that a non-starter; it's an easily preventable foot-gun. To that end I'll leave this as my vote for data-wp-text.

Also my vote is for the :, so data-wp-on:click with no reasoning but stylistic preference.

@luisherranz
Copy link
Member

My vote between wp- and data-wp also goes for data-wp-.

For : or ., I don't see any meaningful difference between both.

  • For aesthetic reasons, I would also choose :. Maybe because it's the norm in other frameworks (Angular, Svelte, Alpine…) so I got used to it.
  • But using : will trigger errors in HTML validation.

So even though the chances that someone is validating their HTML output are practically zero, I think my vote goes for . to avoid causing any trouble.

@adamziel
Copy link

adamziel commented Mar 10, 2023

+1 for data-wp from me.

I prefer : over . because it's a better visual distinction.

But using : will trigger errors in HTML validation.

@luisherranz does it have any practical consequences for parsing, SEO, or anything else other than setting ourselves up for hearing this feedback over and over again? I can't think of anything.

@SantosGuillamot
Copy link
Contributor

I also vote for data-wp.

Regarding : or ., if the only reason to choose : is because it looks better, I would go with . if that could prevent issues with HTML Validators. Although I'm influenced by the fact that I don't know the consequences of not having valid HTML. Not sure how important that is.

@DAreRodz
Copy link
Collaborator

DAreRodz commented Mar 10, 2023

Let's go for data-wp. 👍

I prefer : over . because it's a better visual distinction.

For some reason, my JS brain makes it easier to detect . visually, especially when it joins two words together (without spaces in between). 🤔 🤷

Edit: I add some examples so you can compare.

<button
  data-wp-text="state.myBlock.buttonText"
  data-wp-on:click="actions.myBlock.clickHandler"
  data-wp-class:active="state.myBlock.isActive"
  data-wp-bind:disabled="state.myBlock.isDisabled"
/>

<button
  data-wp-text="state.myBlock.buttonText"
  data-wp-on.click="actions.myBlock.clickHandler"
  data-wp-class.active="state.myBlock.isActive"
  data-wp-bind.disabled="state.myBlock.isDisabled"
/>

@luisherranz
Copy link
Member

@luisherranz does it have any practical consequences for parsing, SEO, or anything else other than setting ourselves up for hearing this feedback over and over again? I can't think of anything.

I don't think there are any practical consequences, but neither for wp- 🤷‍♂️

@luisherranz
Copy link
Member

So yeah, being super honest, we would be basically optimizing for not having to argue that there are no practical consequences over and over 😆

@dmsnell
Copy link
Member

dmsnell commented Mar 10, 2023

the consequences of not having valid HTML

if HTML validation mattered then planes would fall out of the sky, mars would dive into the sun, zebras would stop painting striped camouflage on their bodies, and the web as we know it would crash.

Gopher and Gemini would of course still be fine, as would Xanadu.

I jest of course. Zebras will never stop painting stripes on their bodies.

@michalczaplinski
Copy link
Collaborator

wp-* vs. data-wp-*

I prefer wp-*, but there are some practical considerations to sticking to valid HTML. For example, if you want to sell WordPress themes on ThemeForest (one of the biggest theme marketplaces), your theme will be soft-rejected if it doesn't pass HTML5 validation.

People have reported that it prevented them from using Alpine.js in their themes: alpinejs/alpine#433 (comment) because Alpine uses the x-* attributes like x-data, x-text, etc.

Given that, I lean slightly towards wp-data-* but let's make the prefix user-configurable 🙂 .

. vs. :

Not a very meaningful difference to me. If . is better because it's valid HTML, then let's go with that for the same reason as stated above.


@dmsnell I'm confused about your position here. You said:

My vote is 100% against wp-text for reasons repeated multiple times. I wish we considered that a non-starter; it's an easily preventable foot-gun

But you previously stated:

IMO the only real risk I've seen in these discussions is using wp- as the prefix vs. something else, anything else, due to the practical consideration of detecting quickly if these attribute directives might exist inside a post. Even then it's not the end of the world if we go that way; (emphasis mine)

Could you clarify?

@Potherca
Copy link

Regarding HTML validity (from an accessibility perspective) and working for (semi-)government agencies...

Although the EN 301 549 en WCAG do not specify anything regarding "valid" HTML, there are still government agencies that require HTML output to pass validation.

In this respect, it might be ill-advised to use : in attributes names (as that would trigger validation errors) if valid alternatives are available.

@adamziel
Copy link

The . character sounds like a winner to me. It keeps the HTML validators happy, and invalid HTML is a source of problems, even if these problems are not technical in nature. There isn't any clear advantage of using : – some folks find it more readable, and others don't.

@luisherranz
Copy link
Member

Thanks all for the conversation, and special thanks to @Potherca for chiming in and confirming that validation errors can cause problems for some sites.

It's decided then, let's go with data-wp- and .. Closing this now as resolved.

@dmsnell
Copy link
Member

dmsnell commented Mar 16, 2023

Could you clarify?

@michalczaplinski the point is that all this processing is likely going to add a significant overhead and a quick bail-out could be incredibly important if we can know quickly if a post or block cannot contain a directive.

One way we can do that is to use a prefix for attributes whose literal value is unlikely to occur as part of legitimate expected existing HTML. In the case of wp- we expect a lot of blocks to produce CSS classes like wp-full-width and the like. That rules out the ability to search for this…

function html_possibly_contains_directives( $html ) {
	return false !== strpos( $html, 'wp-' );
}

On the other hand if we choose a prefix like data-wp- or wpx- then it's relatively unlikely that a post contains that substring if it doesn't include directives. It may still happen, but those cases should represent a small subset of real posts.

The goal is to find a way to quickly assess if we can know beyond a doubt that a given snippet of HTML cannot have directives and then avoid doing all the more costly processing in those cases. If our prefix doesn't fit the bill we can find one that does (as appears to be with what @luisherranz chose)

@westonruter
Copy link
Member

tl;dr: +1

Just wanted to share some related experience with AMP here. In AMP, there is something similar to (data-)wp-bind where you can bind some attribute value to some state. It's called, unsurprisingly, amp-bind. Since browser parsers are so forgiving, a syntax was devised that used square brackets around bound attributes. For example, see the [hidden] attribute:

<amp-state id="myVisibility">
  <script type="application/json">
    false
  </script>
</amp-state>
   
<button type="button" on="tap:AMP.setState({myVisibility: !myVisibility})">
  Toggle Visibility
</button>

<p id="visible-via-state" hidden [hidden]="!myVisibility">
  This is now shown!
</p>

This ended up being a big challenge when implementing support for amp-bind in the AMP plugin for WordPress because PHP's DOM absolutely does not like these attributes (even when parsing as HTML). In part because of such cases, amp-bind has an alternate XML/React-compatible syntax which uses an data-amp-bind- prefix (e.g. data-amp-bind-hidden) instead of [hidden]. So in the AMP plugin before parsing we have to do a bunch of regex to transform any bracketed syntax attributes with the data- prefixed version.

I know that PHP DOM is not involved here since HTML_Tag_Processor is being employed. But even so, other plugins (e.g. optimizer plugins) may output buffer the output of the page and load it into a DOMDocument, so it's important that no syntax be used which could fail to be parsed. That being said, attribute names with or without data- and with either . or : are all being parsed just fine. Still, ensuring attribute names all conform to strict HTML5 validity will prevent any surprises with other tools in the ecosystem.

@SantosGuillamot
Copy link
Contributor

Reopening this issue because it seems the dot notation we are using for directives like data-wp-bind.hidden is not valid JSX 😠

This means that, to support serializing directives to the database using JSX, we need to change the syntax again. At this moment, we can't use the following code in static blocks:

export const Save = () => {
	const blockProps = useBlockProps.save();

	return (
		<div data-wp-context='{ "hidden": true }' {...blockProps}>
			<button data-wp-on.click="actions.toggle">Show Text</button>
			<div data-wp-bind.hidden="context.hidden">
				<span>Some Text!</span>
			</div>
		</div>
	);
};

We could go back to the : syntax. However, apart from not being valid HTML, it needs the "throwIfNamespace": false option, which is not straightforward to configure.

With those considerations, these are the alternatives that come to our mind:

  • data-wp-bind--hidden: Using two hyphens (--) for the suffix.
  • data-wp-bind__hidden: Using two underscores (__) for the suffix.
  • data-wp-bind_hidden: Using one underscore (_) for the suffix.

All of them are valid HTML and valid JSX, and the first two possibilities are inspired by BEM (Block, Element, Modifier) methodology, which is also used for CSS WordPress coding guidelines.

Any opinions about which one is better? Any other option that has not been considered?

@dmsnell
Copy link
Member

dmsnell commented Apr 18, 2023

there's still the option of abstracting this, which might help down the line if we have to adjust again: create a helper function which lets us focus on the behaviors we want to add and not the html.

return (
	<div {...wpBehaviors({context: {hidden: true}})} {...blockProps}>
		<button {...wpBehaviors({on: {click: 'actions.toggle'}})}>Show Text</button>
		<div {...wpBehaviors({bind: {hidden: 'context.hidden'}})}>
			<span>Some Text!</span>
		</div>
	</div>
);

this lets us keep the syntax we prefer, being it . or : or anything else, lets us continue to write whatever we want in PHP, and provides some convenience on escaping and encoding.

const wpBehaviors = behaviors => {
	const attributes = {};

	foreach ( const [ key, value ] of Object.entries( behaviors ) ) {
		// extract relevant info from values
		// e.g. {on: {click: 'actions.toggle'}} into "data-wp-on.click='actions.toggle'"
	}

	return attributes;
}

@felixarntz
Copy link
Member

felixarntz commented Apr 18, 2023

@dmsnell I like your suggestion of abstracting the syntax in JSX. This keeps the syntax centralized in a single utility function for JSX, while sticking with the syntax originally decided on which is already supported by the HTML standard.

We could take this idea even further and also implement a PHP utility function of the same kind, encouraged over direct output of e.g. data-wp-bind.hidden, let's say in a server-side rendered block. Something like:

return '<div' . wp_behaviors( array( 'bind' => array( 'hidden' => 'context.hidden' ) ) ) . '><span>Some Text!</span></div>';

And the function like:

// $behaviors would be an associative array - could also be an object, but those aren't great to use in PHP.
function wp_behaviors( array $behaviors ) {
	$attributes = '';
	foreach ( $behaviors as $key => $value ) {
		foreach ( $value as $k => $v ) {
			$attributes .= ' data-wp-' . $key . '.' . $k . '="' . $v . '"';
		}
	}
	return $attributes;
}

@adamziel
Copy link

adamziel commented Apr 19, 2023

@felixarntz Nit: in PHP we now have WP_HTML_Tag_Processor:

$p = new WP_HTML_Tag_Processor('<div><span>Some Text!</span></div>');
$p->next_tag();
foreach ( $behaviors as $key => $value ) {
	foreach ( $value as $k => $v ) {
		$p->set_attribute( "data-wp-$key-$k", $v );
	}
}
$html = $p.'';

@adamziel
Copy link

adamziel commented Apr 19, 2023

What about a dollar character $? As in <span data-wp-on$click={1} ></span>?

JSX attributes consist of IdentifierPart which consists of IdentifierPartChar which itself can contain the $ characters and any Unicode code point with the Unicode property “ID_Continue.

I checked these using the following python script:

import unicodedata

for code_point in range(0x110000):
    char = chr(code_point)
    if unicodedata.category(char)[0] == "L" or unicodedata.category(char) == "Nd" or unicodedata.category(char) == "Pc":
        print(char, end="")

None of these unicode symbol stood out to me as international and easy to type enough, but the dollar sign could work.

@westonruter
Copy link
Member

IMO, the $ is harder to read as a separator. I'd say --, __, or _ are better in that regard.

@luisherranz
Copy link
Member

I also prefer --, __ or _.

Regarding the proposed syntax abstraction, one of the main disadvantages of the Interactivity API is that it introduces a third templating language in WordPress. I wouldn't want that to be split into yet another three slightly different ways of writing it (JSX, PHP and HTML). In my opinion, the closer we get to being able to copy/paste the code between JSX, PHP and HTML, the better and simpler the DX will be. So if this is something that can be solved by simply switching to --, __ or _, I would do that over any syntax abstraction.

@SantosGuillamot
Copy link
Contributor

I also feel the $ character is a bit harder to read as a separator. Additionally, I believe it is not strictly valid HTML, so I guess we would face similar issues to the ones with :.

I would go with --, __, or _ as well. Between those, I'm more inclined to __ or --, but I don't have a strong opinion.

@dmsnell
Copy link
Member

dmsnell commented Apr 20, 2023

did any of us consider data-wp-bind-dashy-mc-dash-face-src=…?

joking aside, thanks for the open attitude all; finding a solution that works may be "ugly" to some but likely whatever we choose is actually quite fine both technically and socially, as long as we don't change it afterwards and break existing code.

one piece of value in having the JS and PHP abstractions is that (while it remains close to the language in the HTML) it won't suffer from encouraging un-encoded and un-serialized attribute use. if we use the Tag Processor to add them this won't be a problem, but in most of the code examples and discussions I've followed we're mostly demonstrating a normative way of serializing that encourages skipping the safety step.

once we get the "funky comment" support in the Tag Processor it's my intention to quickly build up the templating engine, which I think can alleviate some of this though. I'm sharing some personal worry here, but it makes me nervous demonstrating unescaped JSON data in an attribute, regardless of how the attribute is named.

</tangent>

@westonruter
Copy link
Member

I would go with --, __, or _ as well. Between those, I'm more inclined to __ or --, but I don't have a strong opinion.

Upon considering this a bit further, I'd like to exclude __ from consideration since from casual viewer it isn't always obvious that this is one character or two (though granted there is no such thing as an "em underscore" [but TIL there is a TWO-EM DASH and THREE-EM DASH!]).

This is not a problem, however, for double hyphens (--). Use of double hyphens also used in BEM, as noted above.

Use of a single underscore (_) would perhaps be easier to read than double-hyphen (--), when the single hyphen (-) is already being used. (And _ is one fewer byte.)

All this being said, couldn't we solve this problem and just go with a single hyphen? The hyphen could just be the separator. Considering these cases:

data-wp-on:click
data-wp-on.click
data-wp-on--click
data-wp-on__click
data-wp-on_click

There doesn't seem to be an ambiguity problem to just go with:

data-wp-on-click

or

data-wp-bind-hidden

If an attribute begins with on or wp-bind, we know that the next segment afterward will always be the event name or attribute name, respectively. The only possible complication I see here being if namespaces are used in the data attributes, like <div data-wp-context.myPlugin.videoId="123"> in #198 (comment). If data-wp-context-myPlugin-videoId were used, then the dataset key would be wpContextMypluginVideoid which could introduce ambiguities (e.g. namespaces of myPluginVideo.id vs myPlugin.videoId). The way around that would be to grab the attributes via element.attributes instead of element.dataset, which would require a bit more iteration.

@luisherranz
Copy link
Member

I like the easiness of a single hyphen.

If we finally promote the use of namespaces in custom directives to avoid naming collisions between plugins, maybe we can allow single underscores in the directive names part: data-wp-pluginname_directivename-aria-label.

@gziolo
Copy link
Member

gziolo commented Apr 21, 2023

If we finally promote the use of namespaces in custom directives to avoid naming collisions between plugins, maybe we can allow single underscores in the directive names part: data-wp-pluginname_directivename-aria-label.

It probably would also work to have data-wp-plugin_name_directive_name-aria-label. I know that, in theory, you could have foo/boo-bar vs foo-boo/bar, but it's very very very unlikely to happen. We also didn't take that into account for block names when automatically converting them to CSS class names but nobody complained so far.

@luisherranz
Copy link
Member

I know that, in theory, you could have foo/boo-bar vs foo-boo/bar, but it's very very very unlikely to happen

I've always wondered about that 😄

@luisherranz
Copy link
Member

There's no point in delaying this more, so I think we should make a decision.

My vote goes for @westonruter's proposal of a single hyphen. I think it's the most elegant solution.

The only core directive that may have more than one word and need an underscore is if we add dangerous to data-wp-html: data-wp-dangerous_html. I think that's fine, since that directive should be used with care (or just use dangerous as the name of the directive and keep data-wp-dangerous-html 😄).

Also, if we go with that and we want to avoid underscores on custom directives, we could use a prefix to indicate that it's custom and includes a namespace. maybe x (for eXtensibility?):

data-wp-x-myplugin-bind-hidden (as opposed to data-wp-myplugin_bind-hidden)

But that's not a decision we need to make now.

@ockham
Copy link
Collaborator

ockham commented May 22, 2023

dashy-mc-dash-face works for me.

The only core directive that may have more than one word and need an underscore is if we add dangerous to data-wp-html: data-wp-dangerous_html. I think that's fine, since that directive should be used with care (or just use dangerous as the name of the directive and keep data-wp-dangerous-html 😄).

Yeah, I'd prefer data-wp-dangerous-html over data-wp-dangerous_html. A single Core directive using an underscore like that is a recipe for confusion (and a ton of ensuing StackOverflow questions) IMO 😬

@SantosGuillamot
Copy link
Contributor

I agree using a single hyphen seems the cleanest solution. I'm a bit afraid of the syntax of custom directives, and, from a newbie, it could feel a bit more confusing to differentiate between the directive and the attributes: (In data-wp-bind-hidden feels less intuitive than in data-wp-bind__hidden or data-wp-bind--hidden).

Anyway, it seems none of them is perfect so I'm happy to go with a single hyphen.

@DAreRodz
Copy link
Collaborator

I don't really like any of the options 😞, but if I had to choose one, I would go for hyphens instead of underscores.

A single hyphen would force devs to use a single word, at least for the namespace and directive name (not needed for the directive attribute). It would be a good idea if you have in mind that the whole directive identifier would be shorter. However, you don't have any visual clue to differentiate between any of the identifier parts, so it would be harder to read.
It would also be harder to explain how the whole directive identifier is interpreted.

<div
  data-wp-bind-aria-label="..."
  data-wp-x-plugin-directive-attribute="..."
></div>

On the other hand, a double hyphen would allow devs to use multiple words for each identifier part. That would make them easier to read and differentiate, although it would lead to longer identifiers. This option could also be easier to explain.

<div
  data-wp--bind--aria-label="..."
  data-wp-x-my-plugin--my-directive-name--some-attribute="..."
></div>

I think I prefer the double hyphen, as it could have more advantages (just my point of view). I don't really like it anyway, though. 😅

@luisherranz
Copy link
Member

SantosGuillamot: it could feel a bit more confusing to differentiate between the directive and the attributes

DAreRodz: you don't have any visual clue to differentiate between any of the identifier parts, so it would be harder to read

After you said so, I started using the single hyphen in my code, and I think you're right.

Locating the class name here seems hard: data-wp-class-is-open="context.isOpen"
It seems mentally faster with something like this: data-wp-class_is-open="context.isOpen"
Or with something like this: data-wp-class--is-open="context.isOpen"

At least in my current mental subjective experience 🤷 😓

If I step away from the computer screen and look at it, the third one is the easiest to spot to me.

<button
    data-wp-on-click="actions.toggle"
    data-wp-bind-aria-expanded="context.isOpen"
    data-wp-class-is-open="context.isOpen"
  >
<button
    data-wp-on_click="actions.toggle"
    data-wp-bind_aria-expanded="context.isOpen"
    data-wp-class_is-open="context.isOpen"
  >
<button
    data-wp-on--click="actions.toggle"
    data-wp-bind--aria-expanded="context.isOpen"
    data-wp-class--is-open="context.isOpen"
  >

I didn't consider __ because of what @westonruter said.

@SantosGuillamot
Copy link
Contributor

At least in my current mental subjective experience 🤷 😓

From my subjective perspective, they single hyphen as well 😄

As mentioned in the comment where we started this part of the discussion, using something like -- feels more in line with BEM (Block, Element, Modifier) methodology, which is also used for CSS WordPress coding guidelines.

@acketon
Copy link

acketon commented May 24, 2023

As an outside observer just reading up on the Interactivity API, if I had a vote I think the double hyphen -- as a separator is the easiest to mentally process in the examples shown and the clearest when there are potentially double word directive names or multi word class names, etc on the right side of the separator.

@luisherranz
Copy link
Member

luisherranz commented May 25, 2023

if I had a vote I think the double hyphen -- as a separator is the easiest to mentally process

Thanks for sharing that. Everybody's opinion is as valid as anyone else's here 🙂

Should we go with -- then? I'm also happy with it.

@SantosGuillamot
Copy link
Contributor

Should we go with -- then? I'm also happy with it.

-- seems the best viable option to me 👍

@gziolo
Copy link
Member

gziolo commented May 25, 2023

-- is one of the options proposed, so it works for me.

@luisherranz
Copy link
Member

Ok, decided then. Let's see if this is finally the final syntax 😄

Even though we are already migrating things to Gutenberg, I think it makes sense to make the change here as well so people coming to this repository see the new syntax.


And if someone thinks there may be any potential issue with the double hyphen -- either now or in the future, please say so.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.