diff --git a/src/plugins/props/README.md b/src/plugins/props/README.md index b8a34659..5ed36bb8 100644 --- a/src/plugins/props/README.md +++ b/src/plugins/props/README.md @@ -163,3 +163,5 @@ The `reflect` property takes the following values: - `to`: If `true`, reflect to the attribute with the same name as the prop. If a string, reflect to the attribute with the given name. By default, `reflect` is `true` **unless** `get` is also specified, in which case it defaults to `false`. + +**Defaults are not reflected to attributes** — only user-set values are. Restoring the default (via `el.prop = undefined` or `removeAttribute`) clears any previously-reflected attribute. diff --git a/src/plugins/props/util/Prop.js b/src/plugins/props/util/Prop.js index 2e072d7a..c4626c86 100644 --- a/src/plugins/props/util/Prop.js +++ b/src/plugins/props/util/Prop.js @@ -104,21 +104,9 @@ let Self = class Prop { #onComputedChange (element, source, newValue, oldValue) { element.props[this.name] = newValue; - // Reflect to attribute if this prop opts in - if (this.toAttribute) { - let attributeValue = this.stringify(newValue); - let oldAttributeValue = element.getAttribute(this.toAttribute); - if (oldAttributeValue !== attributeValue) { - element.ignoredAttributes.add(this.toAttribute); - if (attributeValue === null) { - element.removeAttribute(this.toAttribute); - } - else { - element.setAttribute(this.toAttribute, attributeValue); - } - element.ignoredAttributes.delete(this.toAttribute); - } - } + // Defaults don't synthesize attributes (would clobber pre-mount writes; see #105); + // passing `null` clears any stale attribute left from a prior user write. + this.#reflect(element, source === "default" ? null : newValue); this.changed(element, { element, @@ -128,6 +116,30 @@ let Self = class Prop { }); } + /** + * The single reflection site, called from both user writes (`prop.set`) and + * dep-driven recomputes (`#onComputedChange`). Mirrors pre-signals' shape + * where `set()` was the one place attribute reflection happened. + * + * @param {HTMLElement} element + * @param {*} value - Post-convert value to reflect; `null` removes the attribute. + */ + #reflect (element, value) { + if (!this.toAttribute) { + return; + } + + let attributeName = this.toAttribute; + let attributeValue = this.stringify(value); + if (element.getAttribute(attributeName) === attributeValue) { + return; + } + + element.ignoredAttributes.add(attributeName); + this.applyChange(element, { source: "attribute", attributeName, attributeValue }); + element.ignoredAttributes.delete(attributeName); + } + /** * Get or lazily create the Signal for this prop on a given element. * - Computed props (spec.get): Computed that auto-tracks dependencies @@ -157,7 +169,7 @@ let Self = class Prop { let rawSignal = new Signal(undefined, options); this.#rawSignals.set(element, rawSignal); - let source = this.spec.convert ? "convert" : "default"; + let source = this.spec.convert ? "convert" : "property"; signal = new Computed(() => { let value = rawSignal.value; if (value === undefined && this.spec.default !== undefined) { @@ -181,7 +193,8 @@ let Self = class Prop { return value; }, options); signal.subscribe((newValue, oldValue) => { - this.#onComputedChange(element, source, newValue, oldValue); + let newSource = rawSignal.value === undefined ? "default" : source; + this.#onComputedChange(element, newSource, newValue, oldValue); }); } else { @@ -271,15 +284,37 @@ let Self = class Prop { return; } + // removeAttribute() arrives as null; collapse to undefined so the prop + // reverts to its natural empty state (default, if any, otherwise just + // undefined). Property writes of null remain a legitimate user value. + if (source === "attribute" && parsedValue === null) { + parsedValue = undefined; + } + if (this.equals(parsedValue, oldInternalValue)) { return; } if (rawSignal) { // Computed-backed: write to the raw signal. The Computed recomputes, - // and its subscriber (#onComputedChange) handles element.props, - // reflection, and events. + // and its subscriber (#onComputedChange) handles element.props and events. rawSignal.value = parsedValue; + + // Why reflect here too, when the subscriber already calls `#reflect`? + // - The Computed dedupes recomputes whose new value equals its + // cached value. `el.v = 5` when the default resolves to 5 produces + // no subscriber fire, so reflection from `#onComputedChange` is + // skipped (#105). + // - We can't read `signal.value` to get the post-convert form: that + // would force a sync recompute, fire subscribers immediately, and + // split multi-prop dep cascades across two propchange drains. + // So apply `spec.convert` manually and call `#reflect` directly. + if (source === "property" && parsedValue !== undefined) { + this.#reflect( + element, + this.spec.convert ? this.spec.convert.call(element, parsedValue) : parsedValue, + ); + } } else { // For plain props: update signal, element.props, reflect, and fire events @@ -330,7 +365,9 @@ let Self = class Prop { if (element.setAttribute) { let attributeName = change.attributeName ?? this.toAttribute; let attributeValue = - change.attributeValue ?? change.element.getAttribute(attributeName); + change.attributeValue !== undefined + ? change.attributeValue + : change.element.getAttribute(attributeName); if (attributeValue === null) { element.removeAttribute(attributeName); diff --git a/src/signals.js b/src/signals.js index 38dd9579..474a573c 100644 --- a/src/signals.js +++ b/src/signals.js @@ -187,17 +187,7 @@ export class Computed extends Signal { })); } - // Check if value actually changed, and if so, notify subscribers. - // Temporarily suspend tracking so the internal read doesn't - // register this Computed as a dependency of an outer Computed. - let prev2 = tracking; - tracking = null; - let old = super.value; - tracking = prev2; - - if (!this.equals(value, old)) { - // Use Signal.prototype.value setter directly (bypasses no-op Computed setter) - Object.getOwnPropertyDescriptor(Signal.prototype, "value").set.call(this, value); - } + // Bypass Computed's read-only setter; Signal#set handles the equals dedupe. + Object.getOwnPropertyDescriptor(Signal.prototype, "value").set.call(this, value); } } diff --git a/test/Prop.js b/test/Prop.js index 0b70ee48..31020249 100644 --- a/test/Prop.js +++ b/test/Prop.js @@ -303,7 +303,7 @@ export default { expect: [ "src/default", "mirror/default", - "src/default", + "src/property", "mirror/default", ], }, @@ -372,8 +372,8 @@ export default { "computed/get", "fnDefault/default", - // Update — Computed-backed: source stays construction-time - "plain/default", + // Update + "plain/property", "computed/get", "fnDefault/default", ], @@ -561,6 +561,18 @@ export default { }, expect: 42, }, + { + name: "removeAttribute restores default", + arg: { + props: { v: { type: Number, reflect: true, default: 5 } }, + actions: [ + el => el.setAttribute("v", "6"), + el => el.removeAttribute("v"), + ], + read: "v", + }, + expect: 5, + }, ], }, { @@ -580,15 +592,15 @@ export default { expect: "42", }, { - name: "Default + reflect reflects on mount (plain)", + name: "Default does NOT reflect on mount (plain)", arg: { props: { plain: { type: Number, default: 7, reflect: true } }, attr: "plain", }, - expect: "7", + expect: null, }, { - name: "Default + convert + reflect reflects on mount", + name: "Default does NOT reflect on mount (with convert)", arg: { props: { val: { @@ -602,7 +614,40 @@ export default { }, attr: "val", }, - expect: "10", + expect: null, + }, + { + name: "removeAttribute clears the reflected attribute", + arg: { + props: { v: { type: Number, reflect: true, default: 5 } }, + actions: [ + el => el.setAttribute("v", "6"), + el => el.removeAttribute("v"), + ], + attr: "v", + }, + expect: null, + }, + { + name: "Explicit write equal to default still reflects", + arg: { + props: { v: { type: Number, reflect: true, default: 5 } }, + actions: [el => (el.v = 5)], + attr: "v", + }, + expect: "5", + }, + { + name: "Restoring the default clears the previously-reflected attribute", + arg: { + props: { v: { type: Number, reflect: true, default: 5 } }, + actions: [ + el => (el.v = 6), + el => (el.v = undefined), + ], + attr: "v", + }, + expect: null, }, ], }, diff --git a/test/util/FakeElement.js b/test/util/FakeElement.js index a3e3f34e..fc8630a7 100644 --- a/test/util/FakeElement.js +++ b/test/util/FakeElement.js @@ -16,11 +16,15 @@ export default class FakeElement extends EventTarget { } setAttribute (name, value) { + let oldValue = this.#attrs.get(name) ?? null; this.#attrs.set(name, String(value)); + this.constructor.props?.attributeChanged(this, name, oldValue); } removeAttribute (name) { + let oldValue = this.#attrs.get(name) ?? null; this.#attrs.delete(name); + this.constructor.props?.attributeChanged(this, name, oldValue); } mount () {