Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -2388,7 +2388,9 @@ moduleFor(
{{check-attr}}
{{#check-attr}}{{/check-attr}}`);

equalsElement(assert, this.$('button')[0], 'button', { name: 'false' }, '');
// Bare `{{false}}` removes the attribute (issue #21344). Concat form
// (`name="{{(has-block)}}"`) still renders the literal string.
equalsElement(assert, this.$('button')[0], 'button', {}, '');
equalsElement(assert, this.$('button')[1], 'button', { name: 'true' }, '');

this.assertStableRerender();
Expand All @@ -2407,7 +2409,7 @@ moduleFor(
{{#check-attr}}{{/check-attr}}
{{#check-attr}}{{else}}{{/check-attr}}`);

equalsElement(assert, this.$('button')[0], 'button', { name: 'false' }, '');
equalsElement(assert, this.$('button')[0], 'button', {}, '');
equalsElement(assert, this.$('button')[1], 'button', { name: 'true' }, '');

this.assertStableRerender();
Expand All @@ -2426,7 +2428,7 @@ moduleFor(
{{#check-attr}}{{/check-attr}}
{{#check-attr as |something|}}{{/check-attr}}`);

equalsElement(assert, this.$('button')[0], 'button', { name: 'false' }, '');
equalsElement(assert, this.$('button')[0], 'button', {}, '');
equalsElement(assert, this.$('button')[1], 'button', { name: 'true' }, '');

this.assertStableRerender();
Expand All @@ -2445,8 +2447,8 @@ moduleFor(
{{#check-attr}}{{/check-attr}}
{{#check-attr as |something|}}{{/check-attr}}`);

equalsElement(assert, this.$('button')[0], 'button', { name: 'false' }, '');
equalsElement(assert, this.$('button')[1], 'button', { name: 'false' }, '');
equalsElement(assert, this.$('button')[0], 'button', {}, '');
equalsElement(assert, this.$('button')[1], 'button', {}, '');

this.assertStableRerender();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@ export class HasBlockParamsHelperSuite extends RenderTest {
else: 'else here',
});

this.assertComponent('<button name="false"></button>');
this.assertComponent('<button></button>');
this.assertStableRerender();
}

Expand All @@ -151,7 +151,7 @@ export class HasBlockParamsHelperSuite extends RenderTest {
template: 'block here',
});

this.assertComponent('<button name="false"></button>');
this.assertComponent('<button></button>');
this.assertStableRerender();
}

Expand All @@ -174,7 +174,7 @@ export class HasBlockParamsHelperSuite extends RenderTest {
template: 'block here',
});

this.assertComponent('<button name="false"></button>');
this.assertComponent('<button></button>');
this.assertStableRerender();
}

Expand All @@ -184,7 +184,7 @@ export class HasBlockParamsHelperSuite extends RenderTest {
layout: '<button name={{has-block-params}}></button>',
});

this.assertComponent('<button name="false"></button>');
this.assertComponent('<button></button>');
this.assertStableRerender();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ export class HasBlockSuite extends RenderTest {
template: 'block here',
});

this.assertComponent('<button name="false"></button>');
this.assertComponent('<button></button>');
this.assertStableRerender();
}

Expand All @@ -132,7 +132,7 @@ export class HasBlockSuite extends RenderTest {
layout: '<button name={{has-block}}></button>',
});

this.assertComponent('<button name="false"></button>');
this.assertComponent('<button></button>');
this.assertStableRerender();
}

Expand Down
149 changes: 149 additions & 0 deletions packages/@glimmer-workspace/integration-tests/test/attributes-test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -573,6 +573,155 @@ export class AttributesTests extends RenderTest {
this.assertHTML('<svg viewBox="0 0 100 100" />');
this.assertStableNodes();
}

// https://github.com/emberjs/ember.js/issues/21344
//
// Bare `attr={{false}}` previously dispatched through code paths that
// either coerced `false` to the literal string `"false"` (string-valued
// IDL attributes like autocomplete, name, popover) or ran it through the
// sanitizer's `String()` coercion (URL attributes like href, src). It now
// shares the same "remove the attribute" semantics as `{{null}}` and
// `{{undefined}}` across every dispatch path.

@test
'bare {{false}} removes string-valued IDL property attributes (autocomplete)'() {
this.render('<input autocomplete={{this.value}} />', { value: false });
this.assertHTML('<input />');
this.assertStableRerender();

this.rerender({ value: 'on' });
this.assertHTML('<input autocomplete="on" />');
this.assertStableNodes();

this.rerender({ value: false });
this.assertHTML('<input />');
this.assertStableNodes();

this.rerender({ value: null });
this.assertHTML('<input />');
this.assertStableNodes();
}

@test
'bare {{false}} removes string-valued IDL property attributes (name)'() {
this.render('<input name={{this.value}} />', { value: false });
this.assertHTML('<input />');
this.assertStableRerender();

this.rerender({ value: 'first' });
this.assertHTML('<input name="first" />');
this.assertStableNodes();

this.rerender({ value: false });
this.assertHTML('<input />');
this.assertStableNodes();
}

@test
'bare {{false}} removes the popover property attribute'() {
this.render('<div popover={{this.value}}></div>', { value: false });
this.assertHTML('<div></div>');
this.assertStableRerender();

this.rerender({ value: 'auto' });
this.assertHTML('<div popover="auto"></div>');
this.assertStableNodes();

this.rerender({ value: false });
this.assertHTML('<div></div>');
this.assertStableNodes();
}

@test
'bare {{false}} clears <input value> rather than setting it to "false"'() {
this.render('<input value={{this.value}} />', { value: false });
this.assert.strictEqual(this.readDOMAttr('value'), '');
this.assertStableRerender();

this.rerender({ value: 'hello' });
this.assert.strictEqual(this.readDOMAttr('value'), 'hello');
this.assertStableNodes();

this.rerender({ value: false });
this.assert.strictEqual(this.readDOMAttr('value'), '');
this.assertStableNodes();
}

@test
'bare {{false}} removes sanitized URL attributes (a[href])'() {
this.render('<a href={{this.value}}></a>', { value: false });
this.assertHTML('<a></a>');
this.assertStableRerender();

this.rerender({ value: 'http://example.com' });
this.assertHTML('<a href="http://example.com"></a>');
this.assertStableNodes();

this.rerender({ value: false });
this.assertHTML('<a></a>');
this.assertStableNodes();
}

@test
'bare {{false}} removes sanitized URL attributes (img[src])'() {
this.render('<img src={{this.value}} />', { value: false });
this.assertHTML('<img />');
this.assertStableRerender();

this.rerender({ value: 'http://example.com/x.png' });
this.assertHTML('<img src="http://example.com/x.png" />');
this.assertStableNodes();

this.rerender({ value: false });
this.assertHTML('<img />');
this.assertStableNodes();
}

@test
'bare {{false}} continues to clear boolean attributes (hidden)'() {
this.render('<div hidden={{this.value}}></div>', { value: true });
this.assertHTML('<div hidden></div>');
this.assertStableRerender();

this.rerender({ value: false });
this.assertHTML('<div></div>');
this.assertStableNodes();

this.rerender({ value: true });
this.assertHTML('<div hidden></div>');
this.assertStableNodes();
}

@test
'bare {{false}} on aria-* attributes continues to omit the attribute'() {
// aria-* names are not DOM properties (no `aria-hidden` slot on Element),
// so they take the SimpleDynamicAttribute path. This already handled
// false correctly — the test guards against future regressions.
this.render('<div aria-hidden={{this.value}}></div>', { value: false });
this.assertHTML('<div></div>');
this.assertStableRerender();

this.rerender({ value: 'true' });
this.assertHTML('<div aria-hidden="true"></div>');
this.assertStableNodes();

this.rerender({ value: false });
this.assertHTML('<div></div>');
this.assertStableNodes();
}

@test
'concat {{"{{false}}"}} keeps stringification semantics (existing behavior)'() {
// Concat form is intentionally NOT changed — `attr="{{value}}"` always
// produces a string. This guards the contract for #21344.
this.render('<div data-foo="{{this.value}}"></div>', { value: false });
this.assertHTML('<div data-foo="false"></div>');
this.assertStableRerender();

this.rerender({ value: 'bar' });
this.assertHTML('<div data-foo="bar"></div>');
this.assertStableNodes();
}
}

jitSuite(AttributesTests);
Expand Down
6 changes: 5 additions & 1 deletion packages/@glimmer/runtime/lib/dom/sanitized-values.ts
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,11 @@ export function sanitizeAttributeValue(
attribute: string,
value: unknown
): unknown {
if (value === null || value === undefined) {
// `false` is treated as a removal sentinel (matching `null`/`undefined`)
// rather than being stringified to `"false"` by `normalizeStringValue` below.
// The downstream attribute/property classifier then omits the attribute.
// See https://github.com/emberjs/ember.js/issues/21344.
if (value === null || value === undefined || value === false) {
return value;
}

Expand Down
33 changes: 28 additions & 5 deletions packages/@glimmer/runtime/lib/vm/attributes/dynamic.ts
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@ export class DefaultDynamicProperty extends DynamicAttribute {

value: unknown;
set(dom: TreeBuilder, value: unknown, _env: Environment): void {
if (value !== null && value !== undefined) {
if (!isAttrRemoval(value)) {
this.value = value;
dom.__setProperty(this.normalizedName, value);
}
Expand All @@ -126,10 +126,16 @@ export class DefaultDynamicProperty extends DynamicAttribute {
const { element } = this.attribute;

if (this.value !== value) {
// Assign the property first so reactive properties (e.g. `input.checked`,
// `input.value`) are reset to the framework-supplied state before we
// remove the reflected attribute. For string-valued IDL attributes (e.g.
// `autocomplete`, `name`, `popover`), assigning `false` would coerce to
// the string `"false"` — `removeAttribute` below resets the property
// back to its default via reflection.
(element as unknown as Element)[this.normalizedName as MutableKey<Element>] = this.value =
value as never;

if (value === null || value === undefined) {
if (isAttrRemoval(value)) {
this.removeAttribute();
}
}
Expand Down Expand Up @@ -178,12 +184,17 @@ export class SafeDynamicAttribute extends SimpleDynamicAttribute {

export class InputValueDynamicAttribute extends DefaultDynamicProperty {
override set(dom: TreeBuilder, value: unknown) {
const normalized = normalizeStringValue(value);
// Treat `false` like `null`/`undefined` — clearing the value rather than
// letting `String(false)` set the input to the literal string `"false"`.
// See https://github.com/emberjs/ember.js/issues/21344.
const normalized = isAttrRemoval(value) ? '' : normalizeStringValue(value);
dom.__setProperty('value', normalized);

// GH#19219: Browsers don't reflect `input.value = ''` as a value attribute when
// type is later changed to "radio"/"checkbox". Explicitly set the attribute for <input>.
// Not needed for <textarea> (no value attribute).
// Not needed for <textarea> (no value attribute). The attribute is only added when
// the user explicitly passed `''` — implicit empties (`null`, `undefined`, `false`)
// continue to render as `<input>` without a value attribute.
if (value === '' && this.attribute.element.tagName === 'INPUT') {
dom.__setAttribute('value', '', null);
}
Expand All @@ -192,7 +203,7 @@ export class InputValueDynamicAttribute extends DefaultDynamicProperty {
override update(value: unknown) {
const input = castToBrowser(this.attribute.element, ['input', 'textarea']);
const currentValue = input.value;
const normalizedValue = normalizeStringValue(value);
const normalizedValue = isAttrRemoval(value) ? '' : normalizeStringValue(value);
if (currentValue !== normalizedValue) {
input.value = normalizedValue;
}
Expand Down Expand Up @@ -225,6 +236,18 @@ function isUserInputValue(tagName: string, attribute: string) {
return (tagName === 'INPUT' || tagName === 'TEXTAREA') && attribute === 'value';
}

/**
* Returns `true` for the sentinel values that signal "this attribute is not
* present" in a bare attribute expression (`attr={{value}}`). Keeping these
* three values in lockstep across every dispatch path (plain attribute, DOM
* property, sanitized attribute, input value, …) is what makes
* `<input autocomplete={{false}}>` and `<input autocomplete={{null}}>` behave
* the same way. See https://github.com/emberjs/ember.js/issues/21344.
*/
function isAttrRemoval(value: unknown): boolean {
return value === null || value === undefined || value === false;
}

function normalizeValue(value: unknown): Nullable<string> {
if (
value === false ||
Expand Down
Loading