From 02a9427a7de789d3f4ca5191d51b44374afaef97 Mon Sep 17 00:00:00 2001 From: gerjanvangeest Date: Tue, 27 Aug 2024 10:48:12 +0200 Subject: [PATCH] fix(combobox): when multiple choice reset all listbox options on select (#2332) * fix(combobox): when multiple choice reset all listbox options on select * Apply suggestions from code review Co-authored-by: Thijs Louisse * chore: clear options also on click * chore: adopt code to show list when only when showAllOnEmpty is true --------- Co-authored-by: Thijs Louisse --- .changeset/brave-plums-drum.md | 5 + docs/components/combobox/use-cases.md | 4 +- .../components/combobox/src/LionCombobox.js | 42 ++++-- .../combobox/test/lion-combobox.test.js | 141 +++++++++++++++++- 4 files changed, 174 insertions(+), 18 deletions(-) create mode 100644 .changeset/brave-plums-drum.md diff --git a/.changeset/brave-plums-drum.md b/.changeset/brave-plums-drum.md new file mode 100644 index 000000000..305c28cf4 --- /dev/null +++ b/.changeset/brave-plums-drum.md @@ -0,0 +1,5 @@ +--- +'@lion/ui': patch +--- + +[combobox] reset listbox options on click/enter for multiple-choice diff --git a/docs/components/combobox/use-cases.md b/docs/components/combobox/use-cases.md index 36bc75d29..5c24790c6 100644 --- a/docs/components/combobox/use-cases.md +++ b/docs/components/combobox/use-cases.md @@ -243,7 +243,7 @@ export const multipleChoice = () => html` ${lazyRender( listboxData.map( (entry, i) => html` - + ${entry} `, ), )} @@ -266,7 +266,7 @@ export const multipleCustomizableChoice = () => html` ${lazyRender( listboxData.map( (entry, i) => html` - + ${entry} `, ), )} diff --git a/packages/ui/components/combobox/src/LionCombobox.js b/packages/ui/components/combobox/src/LionCombobox.js index ce1b74ebf..20e145493 100644 --- a/packages/ui/components/combobox/src/LionCombobox.js +++ b/packages/ui/components/combobox/src/LionCombobox.js @@ -22,6 +22,7 @@ const matchA11ySpanReverseFns = new WeakMap(); * @typedef {import('@lion/ui/types/form-core.js').ChoiceInputHost} ChoiceInputHost * @typedef {import('@lion/ui/types/form-core.js').FormControlHost} FormControlHost * @typedef {import('../types/SelectionDisplay.js').SelectionDisplay} SelectionDisplay + * @typedef {LionOption & { onFilterUnmatch?:function; onFilterMatch?:function }} OptionWithFilterFn */ /** @@ -201,8 +202,30 @@ export class LionCombobox extends LocalizeMixin(OverlayMixin(CustomChoiceGroupMi reset() { super.reset(); - // @ts-ignore _initialModelValue comes from ListboxMixin - this.value = this._initialModelValue; + if (!this.multipleChoice) { + // @ts-ignore _initialModelValue comes from ListboxMixin + this.value = this._initialModelValue; + } + this._resetListboxOptions(); + } + + /** + * @protected + */ + _resetListboxOptions() { + this.formElements.forEach((/** @type {OptionWithFilterFn} */ option, idx) => { + this._unhighlightMatchedOption(option); + if (!this.showAllOnEmpty || !this.opened) { + // eslint-disable-next-line no-param-reassign + option.style.display = 'none'; + } else { + // eslint-disable-next-line no-param-reassign + option.style.display = ''; + option.setAttribute('aria-posinset', `${idx + 1}`); + option.setAttribute('aria-setsize', `${this.formElements.length}`); + option.removeAttribute('aria-hidden'); + } + }); } /** @@ -704,11 +727,13 @@ export class LionCombobox extends LocalizeMixin(OverlayMixin(CustomChoiceGroupMi */ _listboxOnClick(ev) { super._listboxOnClick(ev); - this._inputNode.focus(); if (!this.multipleChoice) { this.activeIndex = -1; this.opened = false; + } else { + this._inputNode.value = ''; + this._resetListboxOptions(); } } @@ -897,7 +922,6 @@ export class LionCombobox extends LocalizeMixin(OverlayMixin(CustomChoiceGroupMi const autoselect = this._autoSelectCondition(); const noFilter = this.autocomplete === 'inline' || this.autocomplete === 'none'; - /** @typedef {LionOption & { onFilterUnmatch?:function, onFilterMatch?:function }} OptionWithFilterFn */ this.formElements.forEach((/** @type {OptionWithFilterFn} */ option, i) => { // [1]. Decide whether option should be shown const matches = this.matchCondition(option, curValue); @@ -983,7 +1007,7 @@ export class LionCombobox extends LocalizeMixin(OverlayMixin(CustomChoiceGroupMi // [8]. These values will help computing autofill intentions next autocomplete cycle this.__prevCboxValueNonSelected = curValue; - // See test 'computation of "user intends autofill" works correctly afer autofill' + // See test 'computation of "user intends autofill" works correctly after autofill' this.__prevCboxValue = this._inputNode.value; this.__hadSelectionLastAutofill = this._inputNode.value.length !== this._inputNode.selectionStart; @@ -1120,16 +1144,16 @@ export class LionCombobox extends LocalizeMixin(OverlayMixin(CustomChoiceGroupMi ev.preventDefault(); this.modelValue = this.parser([...this.modelValue, this._inputNode.value]); - this._inputNode.value = ''; this.opened = false; } else { super._listboxOnKeyDown(ev); - // TODO: should we clear the input value here when allowCustomChoice is false? - // For now, we don't... + this._resetListboxOptions(); } if (!this.multipleChoice) { this.opened = false; + } else { + this._inputNode.value = ''; } break; default: { @@ -1168,7 +1192,7 @@ export class LionCombobox extends LocalizeMixin(OverlayMixin(CustomChoiceGroupMi .filter(option => diff.includes(option.choiceValue)) .map(option => this._getTextboxValueFromOption(option)) .join(' '); - this._setTextboxValue(newValue); // or last selected value? + this._setTextboxValue(newValue); } } diff --git a/packages/ui/components/combobox/test/lion-combobox.test.js b/packages/ui/components/combobox/test/lion-combobox.test.js index 0e316c1af..da1dab28c 100644 --- a/packages/ui/components/combobox/test/lion-combobox.test.js +++ b/packages/ui/components/combobox/test/lion-combobox.test.js @@ -81,6 +81,54 @@ describe('lion-combobox', () => { await performChecks(); }); + it('hides all options on reset()', async () => { + const el = /** @type {LionCombobox} */ ( + await fixture(html` + + Artichoke + Chard + Chicory + Victoria Plum + + `) + ); + + const options = el.formElements; + const visibleOptions = () => options.filter(o => o.style.display !== 'none'); + + mimicUserTyping(el, 'cha'); + await el.updateComplete; + expect(visibleOptions().length).to.equal(1); + el.reset(); + await el.updateComplete; + expect(visibleOptions().length).to.equal(0); + }); + + it('shows all options on reset() when showAllOnEmpty is set to true and overlay was open', async () => { + const el = /** @type {LionCombobox} */ ( + await fixture(html` + + Artichoke + Chard + Chicory + Victoria Plum + + `) + ); + + const options = el.formElements; + const visibleOptions = () => options.filter(o => o.getAttribute('aria-hidden') !== 'true'); + + mimicUserTyping(el, 'cha'); + await el.updateComplete; + expect(visibleOptions().length).to.equal(1); + expect(el.opened).to.be.true; + + el.reset(); + await el.updateComplete; + expect(visibleOptions().length).to.equal(4); + }); + it('hides listbox on click/enter (when multiple-choice is false)', async () => { const el = /** @type {LionCombobox} */ ( await fixture(html` @@ -467,13 +515,12 @@ describe('lion-combobox', () => { mimicUserTyping(el, 'a'); await el.updateComplete; - const visibleOptions = options.filter(o => o.style.display !== 'none'); - expect(visibleOptions.length).to.equal(3, 'after input'); + const visibleOptions = () => options.filter(o => o.style.display !== 'none'); + expect(visibleOptions().length).to.equal(3, 'after input'); el.clear(); await el.updateComplete; - const visibleOptions2 = options.filter(o => o.style.display !== 'none'); - expect(visibleOptions2.length).to.equal(0, 'after clear'); + expect(visibleOptions().length).to.equal(0, 'after clear'); }); it('resets modelValue and textbox value on reset()', async () => { @@ -508,7 +555,7 @@ describe('lion-combobox', () => { el2.reset(); expect(el2.modelValue).to.deep.equal(['Artichoke']); // @ts-ignore [allow-protected] in test - expect(el2._inputNode.value).to.equal('Artichoke'); + expect(el2._inputNode.value).to.equal(''); }); it('syncs textbox to modelValue', async () => { @@ -735,7 +782,7 @@ describe('lion-combobox', () => { await el.updateComplete; }); - it('allows manyu custom selections when multi-choice when requireOptionMatch is false', async () => { + it('allows many custom selections when multi-choice when requireOptionMatch is false', async () => { const el = /** @type {LionCombobox} */ ( await fixture(html` { }); }); - it('highlights first occcurence of matching option', async () => { + it('highlights first occurrence of matching option', async () => { const el = /** @type {LionCombobox} */ ( await fixture(html` @@ -2170,6 +2217,28 @@ describe('lion-combobox', () => { ); }); + it('resets removes highlights on reset()', async () => { + const el = /** @type {LionCombobox} */ ( + await fixture(html` + + Artichoke + Chard + Chicory + Victoria Plum + + `) + ); + const options = el.formElements; + + mimicUserTyping(/** @type {LionCombobox} */ (el), 'c'); + + await el.updateComplete; + expect(options[0]).lightDom.to.equal(`Artichoke`); + el.reset(); + await el.updateComplete; + expect(options[0]).lightDom.to.equal(`Artichoke`); + }); + it('synchronizes textbox when autocomplete is "inline" or "both"', async () => { const el = /** @type {LionCombobox} */ ( await fixture(html` @@ -3018,6 +3087,64 @@ describe('lion-combobox', () => { expect(el.opened).to.equal(true); }); + it('clears textbox after selection of a new item on [enter]', async () => { + const el = /** @type {LionCombobox} */ ( + await fixture(html` + + Artichoke + Chard + Chicory + Victoria Plum + + `) + ); + + const { _inputNode } = getComboboxMembers(el); + const options = el.formElements; + + mimicUserTyping(el, 'art'); + await el.updateComplete; + + expect(el.opened).to.equal(true); + const visibleOptions = () => options.filter(o => o.style.display !== 'none'); + expect(visibleOptions().length).to.equal(1); + + // N.B. we do only trigger keydown here (and not mimicKeypress (both keyup and down)), + // because this closely mimics what happens in the browser + _inputNode.dispatchEvent(new KeyboardEvent('keydown', { key: 'Enter' })); + expect(el.opened).to.equal(true); + expect(visibleOptions().length).to.equal(0); + expect(_inputNode.value).to.equal(''); + }); + + it('clears textbox after selection of a new item on click', async () => { + const el = /** @type {LionCombobox} */ ( + await fixture(html` + + Artichoke + Chard + Chicory + Victoria Plum + + `) + ); + + const { _inputNode } = getComboboxMembers(el); + const options = el.formElements; + + mimicUserTyping(el, 'art'); + await el.updateComplete; + + expect(el.opened).to.equal(true); + const visibleOptions = () => options.filter(o => o.style.display !== 'none'); + expect(visibleOptions().length).to.equal(1); + + visibleOptions()[0].click(); + expect(el.opened).to.equal(true); + expect(visibleOptions().length).to.equal(0); + expect(_inputNode.value).to.equal(''); + }); + it('submits form on [Enter] when listbox is closed', async () => { const submitSpy = sinon.spy(e => e.preventDefault()); const el = /** @type {HTMLFormElement} */ (