ayo/lion
1
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

fix(combobox): update the code to when show and hide the overlay, to be more robust (#2301)

Browse source
This commit is contained in:
gerjanvangeest 2024-06-10 15:10:08 +02:00 committed by GitHub
parent 57597bb9ba
commit b50b960daa
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
3 changed files with 149 additions and 88 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

5
.changeset/wet-eggs-reply.md Normal file
View file

@ -0,0 +1,5 @@
---
'@lion/ui': patch
---
[combobox] update the code to when show and hide the overlay, to be more robust

57
packages/ui/components/combobox/src/LionCombobox.js
View file

@ -456,10 +456,14 @@ export class LionCombobox extends LocalizeMixin(OverlayMixin(CustomChoiceGroupMi
*/ */
this.__listboxContentChanged = false; this.__listboxContentChanged = false;
/** @type {EventListener}
* @protected
*/
this._onKeyUp = this._onKeyUp.bind(this);
/** @type {EventListener} /** @type {EventListener}
* @private * @private
*/ */
this.__requestShowOverlay = this.__requestShowOverlay.bind(this); this._textboxOnClick = this._textboxOnClick.bind(this);
/** @type {EventListener} /** @type {EventListener}
* @protected * @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 * that wraps the listbox with options
* *
* @example * @example
* _showOverlayCondition(options) {
* return this.focused || super._showOverlayCondition(options);
* }
*
* @example
* _showOverlayCondition({ lastKey }) { * _showOverlayCondition({ lastKey }) {
* return lastKey === 'ArrowDown'; * 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 // TODO: batch all pending condition triggers in __pendingShowTriggers, reducing race conditions
// eslint-disable-next-line class-methods-use-this // eslint-disable-next-line class-methods-use-this
_showOverlayCondition({ lastKey }) { _showOverlayCondition({ lastKey }) {
const hideOn = ['Tab', 'Escape', 'Enter']; const alwaysHideOn = ['Tab', 'Escape'];
if (lastKey && hideOn.includes(lastKey)) { const notMultipleChoiceHideOn = ['Enter'];
if (
lastKey &&
(alwaysHideOn.includes(lastKey) ||
(!this.multipleChoice && notMultipleChoiceHideOn.includes(lastKey)))
) {
return false; return false;
} }
if (this.filled || this.showAllOnEmpty) {
if (this.showAllOnEmpty && this.focused) {
return true; return true;
} }
// when no keyboard action involved (on focused change), return current opened state // when no keyboard action involved (on focused change), return current opened state
if (!lastKey) { return /** @type {boolean} */ (this.opened);
return /** @type {boolean} */ (this.opened);
}
return true;
} }
/** /**
@ -678,9 +675,14 @@ export class LionCombobox extends LocalizeMixin(OverlayMixin(CustomChoiceGroupMi
this.__listboxContentChanged = true; this.__listboxContentChanged = true;
} }
/**
* @param {Event} ev
* @protected
*/
// eslint-disable-next-line no-unused-vars // eslint-disable-next-line no-unused-vars
_textboxOnInput() { _textboxOnInput(ev) {
this.__shouldAutocompleteNextUpdate = true; this.__shouldAutocompleteNextUpdate = true;
this.opened = true;
} }
/** /**
@ -1068,7 +1070,8 @@ export class LionCombobox extends LocalizeMixin(OverlayMixin(CustomChoiceGroupMi
*/ */
_setupOpenCloseListeners() { _setupOpenCloseListeners() {
super._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() { _teardownOpenCloseListeners() {
super._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] * @param {KeyboardEvent} [ev]
* @private * @protected
*/ */
__requestShowOverlay(ev) { _onKeyUp(ev) {
const lastKey = ev && ev.key; const lastKey = ev && ev.key;
this.opened = this._showOverlayCondition({ this.opened = this._showOverlayCondition({
lastKey, 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() { clear() {
this.value = ''; this.value = '';
super.clear(); super.clear();

175
packages/ui/components/combobox/test/lion-combobox.test.js
View file

@ -93,11 +93,9 @@ describe('lion-combobox', () => {
`) `)
); );
const { _comboboxNode, _listboxNode } = getComboboxMembers(el); const { _listboxNode } = getComboboxMembers(el);
async function open() { async function open() {
// activate opened listbox
_comboboxNode.dispatchEvent(new Event('focusin', { bubbles: true, composed: true }));
mimicUserTyping(el, 'ch'); mimicUserTyping(el, 'ch');
return el.updateComplete; return el.updateComplete;
} }
@ -150,7 +148,7 @@ describe('lion-combobox', () => {
await performChecks(); await performChecks();
}); });
it('shows overlay on focusin', async () => { it('shows overlay on click', async () => {
const el = /** @type {LionCombobox} */ ( const el = /** @type {LionCombobox} */ (
await fixture(html` await fixture(html`
<lion-combobox name="foo" .showAllOnEmpty="${true}"> <lion-combobox name="foo" .showAllOnEmpty="${true}">
@ -161,15 +159,15 @@ describe('lion-combobox', () => {
</lion-combobox> </lion-combobox>
`) `)
); );
const { _comboboxNode } = getComboboxMembers(el); const { _inputNode } = getComboboxMembers(el);
expect(el.opened).to.be.false; 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; await el.updateComplete;
expect(el.opened).to.be.true; 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} */ ( const el = /** @type {LionCombobox} */ (
await fixture(html` await fixture(html`
<lion-combobox name="foo" .showAllOnEmpty="${true}"> <lion-combobox name="foo" .showAllOnEmpty="${true}">
@ -183,7 +181,7 @@ describe('lion-combobox', () => {
const { _comboboxNode, _inputNode } = getComboboxMembers(el); const { _comboboxNode, _inputNode } = getComboboxMembers(el);
expect(el.opened).to.be.false; 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; await el.updateComplete;
expect(el.opened).to.be.true; expect(el.opened).to.be.true;
_inputNode.dispatchEvent(new KeyboardEvent('keyup', { key: 'Escape' })); _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() { async function open() {
// activate opened listbox
_comboboxNode.dispatchEvent(new Event('focusin', { bubbles: true, composed: true }));
mimicUserTyping(el, 'ch'); mimicUserTyping(el, 'ch');
return el.updateComplete; return el.updateComplete;
} }
@ -870,10 +866,10 @@ describe('lion-combobox', () => {
}); });
describe('Overlay visibility', () => { describe('Overlay visibility', () => {
it('does not show overlay on focusin', async () => { it('does not show overlay on click', async () => {
const el = /** @type {LionCombobox} */ ( const el = /** @type {LionCombobox} */ (
await fixture(html` await fixture(html`
<lion-combobox name="foo" multiple-choice> <lion-combobox name="foo">
<lion-option .choiceValue="${'Artichoke'}">Artichoke</lion-option> <lion-option .choiceValue="${'Artichoke'}">Artichoke</lion-option>
<lion-option .choiceValue="${'Chard'}">Chard</lion-option> <lion-option .choiceValue="${'Chard'}">Chard</lion-option>
<lion-option .choiceValue="${'Chicory'}">Chicory</lion-option> <lion-option .choiceValue="${'Chicory'}">Chicory</lion-option>
@ -884,7 +880,26 @@ describe('lion-combobox', () => {
const { _comboboxNode } = getComboboxMembers(el); const { _comboboxNode } = getComboboxMembers(el);
expect(el.opened).to.equal(false); 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`
<lion-combobox name="foo">
<lion-option .choiceValue="${'Artichoke'}">Artichoke</lion-option>
<lion-option .choiceValue="${'Chard'}">Chard</lion-option>
<lion-option .choiceValue="${'Chicory'}">Chicory</lion-option>
<lion-option .choiceValue="${'Victoria Plum'}">Victoria Plum</lion-option>
</lion-combobox>
`)
);
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; await el.updateComplete;
expect(el.opened).to.equal(false); expect(el.opened).to.equal(false);
}); });
@ -914,7 +929,7 @@ describe('lion-combobox', () => {
expect(el.opened).to.equal(false); expect(el.opened).to.equal(false);
// step [1] // step [1]
_inputNode.dispatchEvent(new Event('focusin', { bubbles: true, composed: true })); _inputNode.dispatchEvent(new Event('click', { bubbles: true, composed: true }));
await el.updateComplete; await el.updateComplete;
expect(el.opened).to.equal(false); expect(el.opened).to.equal(false);
@ -948,10 +963,7 @@ describe('lion-combobox', () => {
`) `)
); );
const { _comboboxNode, _inputNode } = getComboboxMembers(el); const { _inputNode } = getComboboxMembers(el);
// open
_comboboxNode.dispatchEvent(new Event('focusin', { bubbles: true, composed: true }));
mimicUserTyping(el, 'art'); mimicUserTyping(el, 'art');
await el.updateComplete; await el.updateComplete;
@ -963,6 +975,32 @@ describe('lion-combobox', () => {
expect(_inputNode.value).to.equal(''); expect(_inputNode.value).to.equal('');
}); });
it('hides overlay on [Enter]', async () => {
const el = /** @type {LionCombobox} */ (
await fixture(html`
<lion-combobox name="foo">
<lion-option .choiceValue="${'Artichoke'}">Artichoke</lion-option>
<lion-option .choiceValue="${'Chard'}">Chard</lion-option>
<lion-option .choiceValue="${'Chicory'}">Chicory</lion-option>
<lion-option .choiceValue="${'Victoria Plum'}">Victoria Plum</lion-option>
</lion-combobox>
`)
);
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 () => { it('hides overlay on [Tab]', async () => {
const el = /** @type {LionCombobox} */ ( const el = /** @type {LionCombobox} */ (
await fixture(html` await fixture(html`
@ -975,10 +1013,7 @@ describe('lion-combobox', () => {
`) `)
); );
const { _comboboxNode, _inputNode } = getComboboxMembers(el); const { _inputNode } = getComboboxMembers(el);
// open
_comboboxNode.dispatchEvent(new Event('focusin', { bubbles: true, composed: true }));
mimicUserTyping(el, 'art'); mimicUserTyping(el, 'art');
await el.updateComplete; await el.updateComplete;
@ -992,6 +1027,32 @@ describe('lion-combobox', () => {
expect(_inputNode.value).to.equal('Artichoke'); expect(_inputNode.value).to.equal('Artichoke');
}); });
it('keeps overlay open on [Shift]', async () => {
const el = /** @type {LionCombobox} */ (
await fixture(html`
<lion-combobox name="foo">
<lion-option .choiceValue="${'Artichoke'}">Artichoke</lion-option>
<lion-option .choiceValue="${'Chard'}">Chard</lion-option>
<lion-option .choiceValue="${'Chicory'}">Chicory</lion-option>
<lion-option .choiceValue="${'Victoria Plum'}">Victoria Plum</lion-option>
</lion-combobox>
`)
);
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 () => { it('clears checkedIndex on empty text', async () => {
const el = /** @type {LionCombobox} */ ( const el = /** @type {LionCombobox} */ (
await fixture(html` await fixture(html`
@ -1004,10 +1065,7 @@ describe('lion-combobox', () => {
`) `)
); );
const { _comboboxNode, _inputNode } = getComboboxMembers(el); const { _inputNode } = getComboboxMembers(el);
// open
_comboboxNode.dispatchEvent(new Event('focusin', { bubbles: true, composed: true }));
mimicUserTyping(el, 'art'); mimicUserTyping(el, 'art');
await el.updateComplete; await el.updateComplete;
@ -1043,34 +1101,6 @@ describe('lion-combobox', () => {
// NB: If this becomes a suite, move to separate file // NB: If this becomes a suite, move to separate file
describe('Subclassers', () => { 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>
<lion-option .choiceValue="${'Artichoke'}">Artichoke</lion-option>
<lion-option .choiceValue="${'Chard'}">Chard</lion-option>
<lion-option .choiceValue="${'Chicory'}">Chicory</lion-option>
<lion-option .choiceValue="${'Victoria Plum'}">Victoria Plum</lion-option>
</${tag}>
`)
);
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 () => { it('allows to control overlay visibility via "_showOverlayCondition": should not display overlay if currentValue length condition is not fulfilled', async () => {
class ShowOverlayConditionCombobox extends LionCombobox { class ShowOverlayConditionCombobox extends LionCombobox {
/** @param {{ currentValue: string, lastKey:string }} options */ /** @param {{ currentValue: string, lastKey:string }} options */
@ -1164,11 +1194,8 @@ describe('lion-combobox', () => {
`) `)
); );
const options = el.formElements; const options = el.formElements;
const { _comboboxNode } = getComboboxMembers(el);
expect(el.opened).to.equal(false); expect(el.opened).to.equal(false);
_comboboxNode.dispatchEvent(new Event('focusin', { bubbles: true, composed: true }));
mimicUserTyping(el, 'art'); mimicUserTyping(el, 'art');
await el.updateComplete; await el.updateComplete;
expect(el.opened).to.equal(true); expect(el.opened).to.equal(true);
@ -1208,11 +1235,8 @@ describe('lion-combobox', () => {
`) `)
); );
const options = el.formElements; const options = el.formElements;
const { _comboboxNode } = getComboboxMembers(el);
expect(el.opened).to.equal(false); expect(el.opened).to.equal(false);
_comboboxNode.dispatchEvent(new Event('focusin', { bubbles: true, composed: true }));
mimicUserTyping(el, 'art'); mimicUserTyping(el, 'art');
expect(el.opened).to.equal(true); expect(el.opened).to.equal(true);
await el.updateComplete; await el.updateComplete;
@ -2948,7 +2972,7 @@ describe('lion-combobox', () => {
describe('Multiple Choice', () => { describe('Multiple Choice', () => {
// TODO: possibly later share test with select-rich if it officially supports multipleChoice // 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} */ ( const el = /** @type {LionCombobox} */ (
await fixture(html` await fixture(html`
<lion-combobox name="foo" multiple-choice> <lion-combobox name="foo" multiple-choice>
@ -2960,10 +2984,7 @@ describe('lion-combobox', () => {
`) `)
); );
const { _comboboxNode } = getComboboxMembers(el);
// activate opened listbox // activate opened listbox
_comboboxNode.dispatchEvent(new Event('focusin', { bubbles: true, composed: true }));
mimicUserTyping(el, 'ch'); mimicUserTyping(el, 'ch');
await el.updateComplete; await el.updateComplete;
@ -2971,7 +2992,29 @@ describe('lion-combobox', () => {
const visibleOptions = el.formElements.filter(o => o.style.display !== 'none'); const visibleOptions = el.formElements.filter(o => o.style.display !== 'none');
visibleOptions[0].click(); visibleOptions[0].click();
expect(el.opened).to.equal(true); 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`
<lion-combobox name="foo" multiple-choice>
<lion-option .choiceValue="${'Artichoke'}">Artichoke</lion-option>
<lion-option .choiceValue="${'Chard'}">Chard</lion-option>
<lion-option .choiceValue="${'Chicory'}">Chicory</lion-option>
<lion-option .choiceValue="${'Victoria Plum'}">Victoria Plum</lion-option>
</lion-combobox>
`)
);
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); expect(el.opened).to.equal(true);
}); });