From 41ba67275cef7f48eeb1c3b6d1c6b3aaf56a30ce Mon Sep 17 00:00:00 2001 From: Konnor Rogers Date: Fri, 20 Sep 2024 12:54:19 -0400 Subject: [PATCH] fix attributes and properties turning to null from 'morphing' (#2177) * fix attributes and properties turning to null from 'morphing' * add morphing tests * prettier * remove unnecessary console.log from button group test --- docs/pages/resources/changelog.md | 1 + .../button-group/button-group.test.ts | 1 - src/components/button/button.test.ts | 57 +++++++++++++++++++ src/internal/shoelace-element.ts | 36 ++++++++++++ 4 files changed, 94 insertions(+), 1 deletion(-) diff --git a/docs/pages/resources/changelog.md b/docs/pages/resources/changelog.md index 2c531b893e..6a7e7ea877 100644 --- a/docs/pages/resources/changelog.md +++ b/docs/pages/resources/changelog.md @@ -17,6 +17,7 @@ New versions of Shoelace are released as-needed and generally occur when a criti - Scroll buttons for `` auto hide when they are not clickable. The `fixed-scroll-controls` attribute can be included to prevent this behavior. [#2128] - Added support for using `` in `` default slot [#2015] - Added the `countdown` attribute to `` to show a visual indicator before the toast disappears [#1899] +- Fixed a bug with morphing and DOM diffing that would cause elements with reflected initial attributes to not reset. [#2177] - Fixed a bug that caused errors to show in the console when components disconnect before before `firstUpdated()` executes [#2127] - Fixed a bug that made pagination work incorrectly in `` [#2155] - Fixed a bug in `` that caused the active tab indicator to be the wrong size when the tab's content changes [#2164] diff --git a/src/components/button-group/button-group.test.ts b/src/components/button-group/button-group.test.ts index 44c6bd60cb..07de7d24dd 100644 --- a/src/components/button-group/button-group.test.ts +++ b/src/components/button-group/button-group.test.ts @@ -89,7 +89,6 @@ describe('', () => { allButtons[0].dispatchEvent(new MouseEvent('mouseout', { bubbles: true })); await elementUpdated(allButtons[0]); - console.log(allButtons[0]); expect(allButtons[0]).to.not.have.attribute('data-sl-button-group__button--hover'); }); }); diff --git a/src/components/button/button.test.ts b/src/components/button/button.test.ts index b71ed43367..5e475ef0d0 100644 --- a/src/components/button/button.test.ts +++ b/src/components/button/button.test.ts @@ -15,6 +15,63 @@ describe('', () => { }); }); }); + describe('when an attribute is removed', () => { + it("should return to 'default' when attribute removed with no initial attribute", async () => { + const el = await fixture(html`Button label`); + + expect(el.variant).to.equal('default'); + expect(el.getAttribute('variant')).to.equal('default'); + + el.removeAttribute('variant'); + await el.updateComplete; + + expect(el.variant).to.equal('default'); + expect(el.getAttribute('variant')).to.equal('default'); + }); + + it("should return to 'default' when attribute removed with an initial attribute", async () => { + const el = await fixture(html`Button label`); + + expect(el.variant).to.equal('primary'); + expect(el.getAttribute('variant')).to.equal('primary'); + + el.removeAttribute('variant'); + await el.updateComplete; + + expect(el.variant).to.equal('default'); + expect(el.getAttribute('variant')).to.equal('default'); + }); + }); + + describe('when a property is set to null', () => { + it("should return to 'default' when property set to null with no initial attribute", async () => { + const el = await fixture(html`Button label`); + + expect(el.variant).to.equal('default'); + expect(el.getAttribute('variant')).to.equal('default'); + + // @ts-expect-error Its a test. Stop. + el.variant = null; + await el.updateComplete; + + expect(el.variant).to.equal('default'); + expect(el.getAttribute('variant')).to.equal('default'); + }); + + it("should return to 'default' when property set to null with an initial attribute", async () => { + const el = await fixture(html`Button label`); + + expect(el.variant).to.equal('primary'); + expect(el.getAttribute('variant')).to.equal('primary'); + + // @ts-expect-error Its a test. Stop. + el.variant = null; + await el.updateComplete; + + expect(el.variant).to.equal('default'); + expect(el.getAttribute('variant')).to.equal('default'); + }); + }); describe('when provided no parameters', () => { it('passes accessibility test', async () => { diff --git a/src/internal/shoelace-element.ts b/src/internal/shoelace-element.ts index 219ebdac70..b92d3286a8 100644 --- a/src/internal/shoelace-element.ts +++ b/src/internal/shoelace-element.ts @@ -146,6 +146,42 @@ export default class ShoelaceElement extends LitElement { (this.constructor as typeof ShoelaceElement).define(name, component); }); } + + #hasRecordedInitialProperties = false; + + // Store the constructor value of all `static properties = {}` + initialReflectedProperties: Map = new Map(); + + attributeChangedCallback(name: string, oldValue: string | null, newValue: string | null) { + if (!this.#hasRecordedInitialProperties) { + (this.constructor as typeof ShoelaceElement).elementProperties.forEach( + (obj, prop: keyof typeof this & string) => { + // eslint-disable-next-line + if (obj.reflect && this[prop] != null) { + this.initialReflectedProperties.set(prop, this[prop]); + } + } + ); + + this.#hasRecordedInitialProperties = true; + } + + super.attributeChangedCallback(name, oldValue, newValue); + } + + protected willUpdate(changedProperties: Parameters[0]): void { + super.willUpdate(changedProperties); + + // Run the morph fixing *after* willUpdate. + this.initialReflectedProperties.forEach((value, prop: string & keyof typeof this) => { + // If a prop changes to `null`, we assume this happens via an attribute changing to `null`. + // eslint-disable-next-line + if (changedProperties.has(prop) && this[prop] == null) { + // Silly type gymnastics to appease the compiler. + (this as Record)[prop] = value; + } + }); + } } export interface ShoelaceFormControl extends ShoelaceElement {