Skip to content

Commit

Permalink
Fix attribute rendering for boolean values (#11369)
Browse files Browse the repository at this point in the history
  • Loading branch information
@bluwy
bluwy authored Jul 1, 2024
1 parent d1ad389 commit e6de11f
Show file tree
Hide file tree
Showing 5 changed files with 67 additions and 36 deletions.
5 changes: 5 additions & 0 deletions .changeset/small-ties-sort.md
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'astro': patch
---

Fixes attribute rendering for non-boolean attributes with boolean values
21 changes: 8 additions & 13 deletions packages/astro/src/runtime/server/render/util.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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} />
Expand Down
38 changes: 29 additions & 9 deletions packages/astro/test/astro-attrs.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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'),
Expand All @@ -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}`);
}
});

Expand Down
35 changes: 23 additions & 12 deletions packages/astro/test/fixtures/astro-attrs/src/pages/index.astro
View file
Original file line number Diff line number Diff line change
@@ -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" />
Expand Down
4 changes: 2 additions & 2 deletions packages/integrations/mdx/test/mdx-vite-env-vars.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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');
});
Expand Down

0 comments on commit e6de11f

Please sign in to comment.