Fix attribute rendering for boolean values (take 2)

.changeset/small-ties-sort.md

@@ -0,0 +1,50 @@
+---
+'astro': major
+---
+
+Fixes attribute rendering for non-[boolean HTML attributes](https://developer.mozilla.org/en-US/docs/Glossary/Boolean/HTML) with boolean values to match proper attribute handling in browsers.
+
+Previously, non-boolean attributes may not have included their values when rendered to HTML. In Astro v5.0, the values are now explicitly rendered as `="true"` or `="false"`
+
+In the following `.astro` examples, only `allowfullscreen` is a boolean attribute:
+
+```astro
+<!-- src/pages/index.astro -->
+<!-- `allowfullscreen` is a boolean attribute -->
+<p allowfullscreen={true}></p>
+<p allowfullscreen={false}></p>
+
+<!-- `inherit` is *not* a boolean attribute -->
+<p inherit={true}></p>
+<p inherit={false}></p>
+
+<!-- `data-*` attributes are not boolean attributes -->
+<p data-light={true}></p>
+<p data-light={false}></p>
+```
+
+Astro v5.0 now preserves the full data attribute with its value when rendering the HTML of non-boolean attributes:
+
+```diff
+  <p allowfullscreen></p>
+  <p></p>
+
+  <p inherit="true"></p>
+- <p inherit></p>
++ <p inherit="false"></p>
+
+- <p data-light></p>
++ <p data-light="true"></p>
+- <p></p>
++ <p data-light="false"></p>
+```
+
+If you rely on attribute values, for example to locate elements or to conditionally render, update your code to match the new non-boolean attribute values:
+
+```diff
+- el.getAttribute('inherit') === ''
++ el.getAttribute('inherit') === 'false'
+
+- el.hasAttribute('data-light')
++ el.dataset.light === 'true'
+```

packages/astro/src/runtime/server/render/util.ts

@@ -8,9 +8,6 @@ export const voidElementNames =
 	/^(area|base|br|col|command|embed|hr|img|input|keygen|link|meta|param|source|track|wbr)$/i;
 const htmlBooleanAttributes =
 	/^(?:allowfullscreen|async|autofocus|autoplay|controls|default|defer|disabled|disablepictureinpicture|disableremoteplayback|formnovalidate|hidden|loop|nomodule|novalidate|open|playsinline|readonly|required|reversed|scoped|seamless|itemscope)$/i;
-const htmlEnumAttributes = /^(?:contenteditable|draggable|spellcheck|value)$/i;
-// Note: SVG is case-sensitive!
-const svgEnumAttributes = /^(?:autoReverse|externalResourcesRequired|focusable|preserveAlpha)$/i;
 
 const AMPERSAND_REGEX = /&/g;
 const DOUBLE_QUOTE_REGEX = /"/g;
@@ -67,13 +64,6 @@ export function addAttribute(value: any, key: string, shouldEscape = true) {
 		return '';
 	}
 
-	if (value === false) {
-		if (htmlEnumAttributes.test(key) || svgEnumAttributes.test(key)) {
-			return markHTMLString(` ${key}="false"`);
-		}
-		return '';
-	}
-
 	// compiler directives cannot be applied dynamically, log a warning and ignore.
 	if (STATIC_DIRECTIVES.has(key)) {
 		// eslint-disable-next-line no-console
@@ -115,11 +105,16 @@ Make sure to use the static attribute syntax (\`${key}={value}\`) instead of the
 	}
 
 	// Boolean values only need the key
-	if (value === true && (key.startsWith('data-') || htmlBooleanAttributes.test(key))) {
+	if (htmlBooleanAttributes.test(key)) {
+		return markHTMLString(value ? ` ${key}` : '');
+	}
+
+	// Other attributes with an empty string value can omit rendering the value
+	if (value === '') {
 		return markHTMLString(` ${key}`);
-	} else {
-		return markHTMLString(` ${key}="${toAttributeString(value, shouldEscape)}"`);
 	}
+
+	return markHTMLString(` ${key}="${toAttributeString(value, shouldEscape)}"`);
 }
 
 // Adds support for `<Component {...value} />

packages/astro/test/astro-attrs.test.js

@@ -16,21 +16,41 @@ describe('Attributes', async () => {
 		const $ = cheerio.load(html);
 
 		const attrs = {
-			'false-str': { attribute: 'attr', value: 'false' },
-			'true-str': { attribute: 'attr', value: 'true' },
-			false: { attribute: 'attr', value: undefined },
-			true: { attribute: 'attr', value: 'true' },
-			empty: { attribute: 'attr', value: '' },
+			'boolean-attr-true': { attribute: 'allowfullscreen', value: '' },
+			'boolean-attr-false': { attribute: 'allowfullscreen', value: undefined },
+			'boolean-attr-string-truthy': { attribute: 'allowfullscreen', value: '' },
+			'boolean-attr-string-falsy': { attribute: 'allowfullscreen', value: undefined },
+			'boolean-attr-number-truthy': { attribute: 'allowfullscreen', value: '' },
+			'boolean-attr-number-falsy': { attribute: 'allowfullscreen', value: undefined },
+			'data-attr-true': { attribute: 'data-foobar', value: 'true' },
+			'data-attr-false': { attribute: 'data-foobar', value: 'false' },
+			'data-attr-string-truthy': { attribute: 'data-foobar', value: 'foo' },
+			'data-attr-string-falsy': { attribute: 'data-foobar', value: '' },
+			'data-attr-number-truthy': { attribute: 'data-foobar', value: '1' },
+			'data-attr-number-falsy': { attribute: 'data-foobar', value: '0' },
+			'normal-attr-true': { attribute: 'foobar', value: 'true' },
+			'normal-attr-false': { attribute: 'foobar', value: 'false' },
+			'normal-attr-string-truthy': { attribute: 'foobar', value: 'foo' },
+			'normal-attr-string-falsy': { attribute: 'foobar', value: '' },
+			'normal-attr-number-truthy': { attribute: 'foobar', value: '1' },
+			'normal-attr-number-falsy': { attribute: 'foobar', value: '0' },
 			null: { attribute: 'attr', value: undefined },
 			undefined: { attribute: 'attr', value: undefined },
-			'html-boolean': { attribute: 'async', value: 'async' },
-			'html-boolean-true': { attribute: 'async', value: 'async' },
-			'html-boolean-false': { attribute: 'async', value: undefined },
 			'html-enum': { attribute: 'draggable', value: 'true' },
 			'html-enum-true': { attribute: 'draggable', value: 'true' },
 			'html-enum-false': { attribute: 'draggable', value: 'false' },
 		};
 
+		assert.ok(!/allowfullscreen=/.test(html), 'boolean attributes should not have values');
+		assert.ok(
+			!/id="data-attr-string-falsy"\s+data-foobar=/.test(html),
+			"data attributes should not have values if it's an empty string"
+		);
+		assert.ok(
+			!/id="normal-attr-string-falsy"\s+data-foobar=/.test(html),
+			"normal attributes should not have values if it's an empty string"
+		);
+
 		// cheerio will unescape the values, so checking that the url rendered unescaped to begin with has to be done manually
 		assert.equal(
 			html.includes('https://example.com/api/og?title=hello&description=somedescription'),
@@ -46,7 +66,7 @@ describe('Attributes', async () => {
 		for (const id of Object.keys(attrs)) {
 			const { attribute, value } = attrs[id];
 			const attr = $(`#${id}`).attr(attribute);
-			assert.equal(attr, value);
+			assert.equal(attr, value, `Expected ${attribute} to be ${value} for #${id}`);
 		}
 	});
 

packages/astro/test/fixtures/astro-attrs/src/pages/index.astro

@@ -1,19 +1,30 @@
-<span id="false-str" attr="false" />
-<span id="true-str" attr="true" />
-<span id="true" attr={true} />
-<span id="false" attr={false} />
-<span id="empty" attr="" />
+<span id="boolean-attr-true" allowfullscreen={true} />
+<span id="boolean-attr-false" allowfullscreen={false} />
+<span id="boolean-attr-string-truthy" allowfullscreen={'foo'} />
+<span id="boolean-attr-string-falsy" allowfullscreen={''} />
+<span id="boolean-attr-number-truthy" allowfullscreen={1} />
+<span id="boolean-attr-number-falsy" allowfullscreen={0} />
+
+<span id="data-attr-true" data-foobar={true} />
+<span id="data-attr-false" data-foobar={false} />
+<span id="data-attr-string-truthy" data-foobar={'foo'} />
+<span id="data-attr-string-falsy" data-foobar={''} />
+<span id="data-attr-number-truthy" data-foobar={1} />
+<span id="data-attr-number-falsy" data-foobar={0} />
+
+<span id="normal-attr-true" foobar={true} />
+<span id="normal-attr-false" foobar={false} />
+<span id="normal-attr-string-truthy" foobar={'foo'} />
+<span id="normal-attr-string-falsy" foobar={''} />
+<span id="normal-attr-number-truthy" foobar={1} />
+<span id="normal-attr-number-falsy" foobar={0} />
+
 <span id="null" attr={null} />
 <span id="undefined" attr={undefined} />
+
 <span id="url" attr={"https://example.com/api/og?title=hello&description=somedescription"}/>
 <span id="code" attr={"cmd: echo \"foo\" && echo \"bar\" > /tmp/hello.txt"} />
-<!--
-    Per HTML spec, some attributes should be treated as booleans
-    These should always render <span async /> or <span /> (without a string value)
--->
-<span id='html-boolean' async />
-<span id='html-boolean-true' async={true} />
-<span id='html-boolean-false' async={false} />
+
 <!--
     Other attributes should be treated as string enums
     These should always render <span draggable="true" /> or <span draggable="false" />

packages/integrations/mdx/test/mdx-vite-env-vars.test.js

@@ -57,8 +57,8 @@ describe('MDX - Vite env vars', () => {
 		const dataAttrDump = document.querySelector('[data-env-dump]');
 		assert.notEqual(dataAttrDump, null);
 
-		assert.notEqual(dataAttrDump.getAttribute('data-env-prod'), null);
-		assert.equal(dataAttrDump.getAttribute('data-env-dev'), null);
+		assert.equal(dataAttrDump.getAttribute('data-env-prod'), 'true');
+		assert.equal(dataAttrDump.getAttribute('data-env-dev'), 'false');
 		assert.equal(dataAttrDump.getAttribute('data-env-base-url'), '/');
 		assert.equal(dataAttrDump.getAttribute('data-env-mode'), 'production');
 	});