diff --git a/.changeset/wet-eggs-reply.md b/.changeset/wet-eggs-reply.md new file mode 100644 index 000000000..f7ef3e0b5 --- /dev/null +++ b/.changeset/wet-eggs-reply.md @@ -0,0 +1,5 @@ +--- +'@lion/ui': patch +--- + +[combobox] update the code to when show and hide the overlay, to be more robust diff --git a/packages/ui/components/combobox/src/LionCombobox.js b/packages/ui/components/combobox/src/LionCombobox.js index 1febaa4aa..ce1b74ebf 100644 --- a/packages/ui/components/combobox/src/LionCombobox.js +++ b/packages/ui/components/combobox/src/LionCombobox.js @@ -456,10 +456,14 @@ export class LionCombobox extends LocalizeMixin(OverlayMixin(CustomChoiceGroupMi */ this.__listboxContentChanged = false; + /** @type {EventListener} + * @protected + */ + this._onKeyUp = this._onKeyUp.bind(this); /** @type {EventListener} * @private */ - this.__requestShowOverlay = this.__requestShowOverlay.bind(this); + this._textboxOnClick = this._textboxOnClick.bind(this); /** @type {EventListener} * @protected */ @@ -500,9 +504,6 @@ export class LionCombobox extends LocalizeMixin(OverlayMixin(CustomChoiceGroupMi } } } - if (name === 'focused' && this.focused) { - this.__requestShowOverlay(); - } } /** @@ -614,11 +615,6 @@ export class LionCombobox extends LocalizeMixin(OverlayMixin(CustomChoiceGroupMi * that wraps the listbox with options * * @example - * _showOverlayCondition(options) { - * return this.focused || super._showOverlayCondition(options); - * } - * - * @example * _showOverlayCondition({ lastKey }) { * return lastKey === 'ArrowDown'; * } @@ -635,19 +631,20 @@ export class LionCombobox extends LocalizeMixin(OverlayMixin(CustomChoiceGroupMi // TODO: batch all pending condition triggers in __pendingShowTriggers, reducing race conditions // eslint-disable-next-line class-methods-use-this _showOverlayCondition({ lastKey }) { - const hideOn = ['Tab', 'Escape', 'Enter']; - if (lastKey && hideOn.includes(lastKey)) { + const alwaysHideOn = ['Tab', 'Escape']; + const notMultipleChoiceHideOn = ['Enter']; + if ( + lastKey && + (alwaysHideOn.includes(lastKey) || + (!this.multipleChoice && notMultipleChoiceHideOn.includes(lastKey))) + ) { return false; } - - if (this.showAllOnEmpty && this.focused) { + if (this.filled || this.showAllOnEmpty) { return true; } // when no keyboard action involved (on focused change), return current opened state - if (!lastKey) { - return /** @type {boolean} */ (this.opened); - } - return true; + return /** @type {boolean} */ (this.opened); } /** @@ -678,9 +675,14 @@ export class LionCombobox extends LocalizeMixin(OverlayMixin(CustomChoiceGroupMi this.__listboxContentChanged = true; } + /** + * @param {Event} ev + * @protected + */ // eslint-disable-next-line no-unused-vars - _textboxOnInput() { + _textboxOnInput(ev) { this.__shouldAutocompleteNextUpdate = true; + this.opened = true; } /** @@ -1068,7 +1070,8 @@ export class LionCombobox extends LocalizeMixin(OverlayMixin(CustomChoiceGroupMi */ _setupOpenCloseListeners() { super._setupOpenCloseListeners(); - this._inputNode.addEventListener('keyup', this.__requestShowOverlay); + this._inputNode.addEventListener('keyup', this._onKeyUp); + this._inputNode.addEventListener('click', this._textboxOnClick); } /** @@ -1077,7 +1080,8 @@ export class LionCombobox extends LocalizeMixin(OverlayMixin(CustomChoiceGroupMi */ _teardownOpenCloseListeners() { super._teardownOpenCloseListeners(); - this._inputNode.removeEventListener('keyup', this.__requestShowOverlay); + this._inputNode.removeEventListener('keyup', this._onKeyUp); + this._inputNode.removeEventListener('click', this._textboxOnClick); } /** @@ -1225,9 +1229,9 @@ export class LionCombobox extends LocalizeMixin(OverlayMixin(CustomChoiceGroupMi /** * @param {KeyboardEvent} [ev] - * @private + * @protected */ - __requestShowOverlay(ev) { + _onKeyUp(ev) { const lastKey = ev && ev.key; this.opened = this._showOverlayCondition({ lastKey, @@ -1235,6 +1239,15 @@ export class LionCombobox extends LocalizeMixin(OverlayMixin(CustomChoiceGroupMi }); } + /** + * @param {FocusEvent} [ev] + * @protected + */ + // eslint-disable-next-line no-unused-vars + _textboxOnClick(ev) { + this.opened = this._showOverlayCondition({}); + } + clear() { this.value = ''; super.clear(); diff --git a/packages/ui/components/combobox/test/lion-combobox.test.js b/packages/ui/components/combobox/test/lion-combobox.test.js index d56e9e4ed..0e316c1af 100644 --- a/packages/ui/components/combobox/test/lion-combobox.test.js +++ b/packages/ui/components/combobox/test/lion-combobox.test.js @@ -93,11 +93,9 @@ describe('lion-combobox', () => { `) ); - const { _comboboxNode, _listboxNode } = getComboboxMembers(el); + const { _listboxNode } = getComboboxMembers(el); async function open() { - // activate opened listbox - _comboboxNode.dispatchEvent(new Event('focusin', { bubbles: true, composed: true })); mimicUserTyping(el, 'ch'); return el.updateComplete; } @@ -150,7 +148,7 @@ describe('lion-combobox', () => { await performChecks(); }); - it('shows overlay on focusin', async () => { + it('shows overlay on click', async () => { const el = /** @type {LionCombobox} */ ( await fixture(html` @@ -161,15 +159,15 @@ describe('lion-combobox', () => { `) ); - const { _comboboxNode } = getComboboxMembers(el); + const { _inputNode } = getComboboxMembers(el); expect(el.opened).to.be.false; - _comboboxNode.dispatchEvent(new Event('focusin', { bubbles: true, composed: true })); + _inputNode.dispatchEvent(new Event('click', { bubbles: true, composed: true })); await el.updateComplete; expect(el.opened).to.be.true; }); - it('hides overlay on focusin after [Escape]', async () => { + it('hides overlay on [Escape] after being opened', async () => { const el = /** @type {LionCombobox} */ ( await fixture(html` @@ -183,7 +181,7 @@ describe('lion-combobox', () => { const { _comboboxNode, _inputNode } = getComboboxMembers(el); expect(el.opened).to.be.false; - _comboboxNode.dispatchEvent(new Event('focusin', { bubbles: true, composed: true })); + _comboboxNode.dispatchEvent(new Event('click', { bubbles: true, composed: true })); await el.updateComplete; expect(el.opened).to.be.true; _inputNode.dispatchEvent(new KeyboardEvent('keyup', { key: 'Escape' })); @@ -203,11 +201,9 @@ describe('lion-combobox', () => { `) ); - const { _comboboxNode, _listboxNode, _inputNode } = getComboboxMembers(el); + const { _listboxNode, _inputNode } = getComboboxMembers(el); async function open() { - // activate opened listbox - _comboboxNode.dispatchEvent(new Event('focusin', { bubbles: true, composed: true })); mimicUserTyping(el, 'ch'); return el.updateComplete; } @@ -870,10 +866,10 @@ describe('lion-combobox', () => { }); describe('Overlay visibility', () => { - it('does not show overlay on focusin', async () => { + it('does not show overlay on click', async () => { const el = /** @type {LionCombobox} */ ( await fixture(html` - + Artichoke Chard Chicory @@ -884,7 +880,26 @@ describe('lion-combobox', () => { const { _comboboxNode } = getComboboxMembers(el); expect(el.opened).to.equal(false); - _comboboxNode.dispatchEvent(new Event('focusin', { bubbles: true, composed: true })); + _comboboxNode.dispatchEvent(new Event('click', { bubbles: true, composed: true })); + await el.updateComplete; + expect(el.opened).to.equal(false); + }); + + it('shows overlay on click when filled', async () => { + const el = /** @type {LionCombobox} */ ( + await fixture(html` + + Artichoke + Chard + Chicory + Victoria Plum + + `) + ); + const { _comboboxNode } = getComboboxMembers(el); + el.modelValue = 'Art'; + expect(el.opened).to.equal(false); + _comboboxNode.dispatchEvent(new Event('click', { bubbles: true, composed: true })); await el.updateComplete; expect(el.opened).to.equal(false); }); @@ -914,7 +929,7 @@ describe('lion-combobox', () => { expect(el.opened).to.equal(false); // step [1] - _inputNode.dispatchEvent(new Event('focusin', { bubbles: true, composed: true })); + _inputNode.dispatchEvent(new Event('click', { bubbles: true, composed: true })); await el.updateComplete; expect(el.opened).to.equal(false); @@ -948,10 +963,7 @@ describe('lion-combobox', () => { `) ); - const { _comboboxNode, _inputNode } = getComboboxMembers(el); - - // open - _comboboxNode.dispatchEvent(new Event('focusin', { bubbles: true, composed: true })); + const { _inputNode } = getComboboxMembers(el); mimicUserTyping(el, 'art'); await el.updateComplete; @@ -963,6 +975,32 @@ describe('lion-combobox', () => { expect(_inputNode.value).to.equal(''); }); + it('hides overlay on [Enter]', async () => { + const el = /** @type {LionCombobox} */ ( + await fixture(html` + + Artichoke + Chard + Chicory + Victoria Plum + + `) + ); + + const { _inputNode } = getComboboxMembers(el); + + mimicUserTyping(el, 'art'); + await el.updateComplete; + expect(el.opened).to.equal(true); + expect(_inputNode.value).to.equal('Artichoke'); + + // 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(false); + expect(_inputNode.value).to.equal('Artichoke'); + }); + it('hides overlay on [Tab]', async () => { const el = /** @type {LionCombobox} */ ( await fixture(html` @@ -975,10 +1013,7 @@ describe('lion-combobox', () => { `) ); - const { _comboboxNode, _inputNode } = getComboboxMembers(el); - - // open - _comboboxNode.dispatchEvent(new Event('focusin', { bubbles: true, composed: true })); + const { _inputNode } = getComboboxMembers(el); mimicUserTyping(el, 'art'); await el.updateComplete; @@ -992,6 +1027,32 @@ describe('lion-combobox', () => { expect(_inputNode.value).to.equal('Artichoke'); }); + it('keeps overlay open on [Shift]', async () => { + const el = /** @type {LionCombobox} */ ( + await fixture(html` + + Artichoke + Chard + Chicory + Victoria Plum + + `) + ); + + const { _inputNode } = getComboboxMembers(el); + + mimicUserTyping(el, 'art'); + await el.updateComplete; + expect(el.opened).to.equal(true); + expect(_inputNode.value).to.equal('Artichoke'); + + // 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: 'Shift' })); + expect(el.opened).to.equal(true); + expect(_inputNode.value).to.equal('Artichoke'); + }); + it('clears checkedIndex on empty text', async () => { const el = /** @type {LionCombobox} */ ( await fixture(html` @@ -1004,10 +1065,7 @@ describe('lion-combobox', () => { `) ); - const { _comboboxNode, _inputNode } = getComboboxMembers(el); - - // open - _comboboxNode.dispatchEvent(new Event('focusin', { bubbles: true, composed: true })); + const { _inputNode } = getComboboxMembers(el); mimicUserTyping(el, 'art'); await el.updateComplete; @@ -1043,34 +1101,6 @@ describe('lion-combobox', () => { // NB: If this becomes a suite, move to separate file describe('Subclassers', () => { - it('allows to control overlay visibility via "_showOverlayCondition"', async () => { - class ShowOverlayConditionCombobox extends LionCombobox { - /** @param {{ currentValue: string, lastKey:string }} options */ - _showOverlayCondition(options) { - return this.focused || super._showOverlayCondition(options); - } - } - const tagName = defineCE(ShowOverlayConditionCombobox); - const tag = unsafeStatic(tagName); - - const el = /** @type {LionCombobox} */ ( - await fixture(html` - <${tag} name="foo" multiple-choice> - Artichoke - Chard - Chicory - Victoria Plum - - `) - ); - const { _comboboxNode } = getComboboxMembers(el); - - expect(el.opened).to.equal(false); - _comboboxNode.dispatchEvent(new Event('focusin', { bubbles: true, composed: true })); - await el.updateComplete; - expect(el.opened).to.equal(true); - }); - it('allows to control overlay visibility via "_showOverlayCondition": should not display overlay if currentValue length condition is not fulfilled', async () => { class ShowOverlayConditionCombobox extends LionCombobox { /** @param {{ currentValue: string, lastKey:string }} options */ @@ -1164,11 +1194,8 @@ describe('lion-combobox', () => { `) ); const options = el.formElements; - const { _comboboxNode } = getComboboxMembers(el); expect(el.opened).to.equal(false); - _comboboxNode.dispatchEvent(new Event('focusin', { bubbles: true, composed: true })); - mimicUserTyping(el, 'art'); await el.updateComplete; expect(el.opened).to.equal(true); @@ -1208,11 +1235,8 @@ describe('lion-combobox', () => { `) ); const options = el.formElements; - const { _comboboxNode } = getComboboxMembers(el); expect(el.opened).to.equal(false); - _comboboxNode.dispatchEvent(new Event('focusin', { bubbles: true, composed: true })); - mimicUserTyping(el, 'art'); expect(el.opened).to.equal(true); await el.updateComplete; @@ -2948,7 +2972,7 @@ describe('lion-combobox', () => { describe('Multiple Choice', () => { // TODO: possibly later share test with select-rich if it officially supports multipleChoice - it('does not close listbox on click/enter', async () => { + it('does not close listbox on click', async () => { const el = /** @type {LionCombobox} */ ( await fixture(html` @@ -2960,10 +2984,7 @@ describe('lion-combobox', () => { `) ); - const { _comboboxNode } = getComboboxMembers(el); - // activate opened listbox - _comboboxNode.dispatchEvent(new Event('focusin', { bubbles: true, composed: true })); mimicUserTyping(el, 'ch'); await el.updateComplete; @@ -2971,7 +2992,29 @@ describe('lion-combobox', () => { const visibleOptions = el.formElements.filter(o => o.style.display !== 'none'); visibleOptions[0].click(); expect(el.opened).to.equal(true); - mimicKeyPress(visibleOptions[1], 'Enter'); + }); + + it('does not close listbox on [Enter]', async () => { + const el = /** @type {LionCombobox} */ ( + await fixture(html` + + Artichoke + Chard + Chicory + Victoria Plum + + `) + ); + + const { _inputNode } = getComboboxMembers(el); + + mimicUserTyping(el, 'art'); + await el.updateComplete; + expect(el.opened).to.equal(true); + + // 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); });