diff --git a/docs/pages/resources/changelog.md b/docs/pages/resources/changelog.md index 4b711c1e..437af4e9 100644 --- a/docs/pages/resources/changelog.md +++ b/docs/pages/resources/changelog.md @@ -17,7 +17,8 @@ New versions of Shoelace are released as-needed and generally occur when a criti - Added Norwegian translations for Bokmål and Nynorsk [#2268] - Added Ukrainian translation [#2270] - Added support for Enter to `` to align with ARIA APG's [window splitter pattern](https://www.w3.org/WAI/ARIA/apg/patterns/windowsplitter/) [#2234] -- Fixed a bug in `` where it was using the wrong tag name,. [#2287] +- Fixed a bug in `` when setting the value property before the element connected. [#2255] +- Fixed a bug in `` where it was using the wrong tag name. [#2287] - Fixed a bug in `` that caused the navigation icons to be reversed - Fixed a bug in `` that prevented label changes in `` from updating the controller [#1971] - Fixed a bug in `` that caused a console warning in Firefox when typing [#2107] diff --git a/src/components/select/select.component.ts b/src/components/select/select.component.ts index 3af09dc4..6016ee84 100644 --- a/src/components/select/select.component.ts +++ b/src/components/select/select.component.ts @@ -1,6 +1,5 @@ import { animateTo, stopAnimations } from '../../internal/animate.js'; import { classMap } from 'lit/directives/class-map.js'; -import { defaultValue } from '../../internal/default-value.js'; import { FormControlController } from '../../internal/form.js'; import { getAnimation, setDefaultAnimation } from '../../utilities/animation-registry.js'; import { HasSlotController } from '../../internal/slot.js'; @@ -102,21 +101,35 @@ export default class SlSelect extends ShoelaceElement implements ShoelaceFormCon /** The name of the select, submitted as a name/value pair with form data. */ @property() name = ''; + private _value: string | string[] = ''; + + get value() { + return this._value; + } + /** * The current value of the select, submitted as a name/value pair with form data. When `multiple` is enabled, the * value attribute will be a space-delimited list of values based on the options selected, and the value property will * be an array. **For this reason, values must not contain spaces.** */ - @property({ - converter: { - fromAttribute: (value: string) => value.split(' '), - toAttribute: (value: string[]) => value.join(' ') + @state() + set value(val: string | string[]) { + if (this.multiple) { + val = Array.isArray(val) ? val : val.split(' '); + } else { + val = Array.isArray(val) ? val.join(' ') : val; } - }) - value: string | string[] = ''; + + if (this._value === val) { + return; + } + + this.valueHasChanged = true; + this._value = val; + } /** The default value of the form control. Primarily used for resetting the form control. */ - @defaultValue() defaultValue: string | string[] = ''; + @property({ attribute: 'value' }) defaultValue: string | string[] = ''; /** The select's size. */ @property({ reflect: true }) size: 'small' | 'medium' | 'large' = 'medium'; @@ -451,6 +464,8 @@ export default class SlSelect extends ShoelaceElement implements ShoelaceFormCon private handleClearClick(event: MouseEvent) { event.stopPropagation(); + this.valueHasChanged = true; + if (this.value !== '') { this.setSelectedOptions([]); this.displayInput.focus({ preventScroll: true }); @@ -522,6 +537,8 @@ export default class SlSelect extends ShoelaceElement implements ShoelaceFormCon private handleTagRemove(event: SlRemoveEvent, option: SlOption) { event.stopPropagation(); + this.valueHasChanged = true; + if (!this.disabled) { this.toggleOptionSelection(option, false); @@ -598,6 +615,9 @@ export default class SlSelect extends ShoelaceElement implements ShoelaceFormCon // Update selected options cache this.selectedOptions = options.filter(el => el.selected); + // Keep a reference to the previous `valueHasChanged`. Changes made here don't count has changing the value. + const cachedValueHasChanged = this.valueHasChanged; + // Update the value and display label if (this.multiple) { this.value = this.selectedOptions.map(el => el.value); @@ -613,12 +633,14 @@ export default class SlSelect extends ShoelaceElement implements ShoelaceFormCon this.value = selectedOption?.value ?? ''; this.displayLabel = selectedOption?.getTextLabel?.() ?? ''; } + this.valueHasChanged = cachedValueHasChanged; // Update validity this.updateComplete.then(() => { this.formControlController.updateValidity(); }); } + protected get tags() { return this.selectedOptions.map((option, index) => { if (index < this.maxOptionsVisible || this.maxOptionsVisible <= 0) { @@ -649,8 +671,29 @@ export default class SlSelect extends ShoelaceElement implements ShoelaceFormCon } } - @watch('value', { waitUntilFirstUpdate: true }) + attributeChangedCallback(name: string, oldVal: string | null, newVal: string | null) { + super.attributeChangedCallback(name, oldVal, newVal); + + /** This is a backwards compatibility call. In a new major version we should make a clean separation between "value" the attribute mapping to "defaultValue" property and "value" the property not reflecting. */ + if (name === 'value') { + const cachedValueHasChanged = this.valueHasChanged; + this.value = this.defaultValue; + + // Set it back to false since this isn't an interaction. + this.valueHasChanged = cachedValueHasChanged; + } + } + + @watch(['defaultValue', 'value'], { waitUntilFirstUpdate: true }) handleValueChange() { + if (!this.valueHasChanged) { + const cachedValueHasChanged = this.valueHasChanged; + this.value = this.defaultValue; + + // Set it back to false since this isn't an interaction. + this.valueHasChanged = cachedValueHasChanged; + } + const allOptions = this.getAllOptions(); const value = Array.isArray(this.value) ? this.value : [this.value]; diff --git a/src/components/select/select.test.ts b/src/components/select/select.test.ts index a9ca7a22..e08c1c6d 100644 --- a/src/components/select/select.test.ts +++ b/src/components/select/select.test.ts @@ -740,6 +740,52 @@ describe('', () => { expect(new FormData(form).getAll('select')).to.have.members(['foo', 'bar', 'baz']); }); }); + + /** + * @see {https://github.com/shoelace-style/shoelace/issues/2254} + */ + it('Should account for if `value` changed before connecting', async () => { + const select = await fixture(html` + + Foo + Bar + + `); + + // just for safe measure. + await aTimeout(10); + + expect(select.value).to.deep.equal(['foo', 'bar']); + }); + + /** + * @see {https://github.com/shoelace-style/shoelace/issues/2254} + */ + it('Should still work if using the value attribute', async () => { + const select = await fixture(html` + + Foo + Bar + + `); + + // just for safe measure. + await aTimeout(10); + + expect(select.value).to.deep.equal(['foo', 'bar']); + + await clickOnElement(select); + await select.updateComplete; + await clickOnElement(select.querySelector("[value='foo']")!); + + await select.updateComplete; + await aTimeout(10); + expect(select.value).to.deep.equal(['bar']); + + select.setAttribute('value', 'foo bar'); + await aTimeout(10); + expect(select.value).to.deep.equal(['foo', 'bar']); + }); }); runFormControlBaseTests('sl-select');