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

Merge pull request #1026 from ing-bank/combobox/model-value-changed

Browse source 
fix(combobox): dispatch model-value-changed properly on unselect
This commit is contained in:
Joren Broekema 2020-10-15 13:49:57 +02:00 committed by GitHub
commit e5a9f45f1e
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
18 changed files with 294 additions and 30 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

8
.changeset/silent-waves-teach.md Normal file
View file

@ -0,0 +1,8 @@
---
'@lion/combobox': patch
'@lion/form-core': patch
'@lion/form-integrations': patch
'@lion/listbox': patch
---
Allow flexibility for extending the repropagation prevention conditions, which is needed for combobox, so that a model-value-changed event is propagated when no option matches after an input change. This allows validation to work properly e.g. for Required.

29
packages/combobox/README.md
View file

@ -263,6 +263,35 @@ export const invokerButton = () => html`
`;
```
## Validation
Validation can be used as normal, below is an example of a combobox with a `Required` validator.
```js preview-story
export const validation = () => {
loadDefaultFeedbackMessages();
Required.getMessage = () => 'Please enter a value';
return html`
<lion-form>
<form>
<lion-combobox
.validators="${[new Required()]}"
name="favoriteMovie"
label="Favorite movie"
>
<lion-option checked .choiceValue=${'Rocky'}>Rocky</lion-option>
<lion-option .choiceValue=${'Rocky II'}>Rocky II</lion-option>
<lion-option .choiceValue=${'Rocky III'}>Rocky III</lion-option>
<lion-option .choiceValue=${'Rocky IV'}>Rocky IV</lion-option>
<lion-option .choiceValue=${'Rocky V'}>Rocky V</lion-option>
<lion-option .choiceValue=${'Rocky Balboa'}>Rocky Balboa</lion-option>
</lion-combobox>
</form>
</lion-form>
`;
};
```
## Listbox compatibility
All configurations that can be applied to `lion-listbox`, can be applied to `lion-combobox` as well.

3
packages/combobox/package.json
View file

@ -46,6 +46,9 @@
"@lion/listbox": "0.2.0",
"@lion/overlays": "0.21.0"
},
"devDependencies": {
"@lion/validate-messages": "0.3.1"
},
"keywords": [
"combobox",
"form",

25
packages/combobox/src/LionCombobox.js
View file

@ -420,13 +420,30 @@ export class LionCombobox extends OverlayMixin(LionListbox) {
}
}
/* eslint-disable no-param-reassign, class-methods-use-this */
/**
* We need to extend the repropagation prevention conditions here.
* Usually form groups with single choice will not repropagate model-value-changed of an option upwards
* if this option itself is not the checked one. We want to prevent duplicates. However, for combobox
* it is reasonable that an option can become unchecked without another one becoming checked, because
* users can enter any text they want, whether it matches an option or not.
*
* Therefore, extend the condition to fail by checking if there is any elements checked. If so, then we
* should indeed not repropagate as normally. If there is no elements checked, this will be the only
* model-value-changed event that gets received, and we should repropagate it.
*
* @param {EventTarget & import('../types/choice-group/ChoiceInputMixinTypes').ChoiceInputHost} target
*/
_repropagationCondition(target) {
return super._repropagationCondition(target) || this.formElements.every(el => !el.checked);
}
/* eslint-disable no-param-reassign */
/**
* @overridable
* @param {LionOption & {__originalInnerHTML?:string}} option
* @param {string} matchingString
*/
// eslint-disable-next-line class-methods-use-this
_onFilterMatch(option, matchingString) {
const { innerHTML } = option;
option.__originalInnerHTML = innerHTML;
@ -443,7 +460,7 @@ export class LionCombobox extends OverlayMixin(LionListbox) {
* @param {string} [curValue]
* @param {string} [prevValue]
*/
// eslint-disable-next-line no-unused-vars
// eslint-disable-next-line no-unused-vars, class-methods-use-this
_onFilterUnmatch(option, curValue, prevValue) {
if (option.__originalInnerHTML) {
option.innerHTML = option.__originalInnerHTML;
@ -451,18 +468,21 @@ export class LionCombobox extends OverlayMixin(LionListbox) {
// Alternatively, an extension can add an animation here
option.style.display = 'none';
}
/* eslint-enable no-param-reassign */
/**
* Computes whether a user intends to autofill (inline autocomplete textbox)
* @overridable
* @param {{ prevValue:string, curValue:string }} config
*/
// eslint-disable-next-line class-methods-use-this
_computeUserIntendsAutoFill({ prevValue, curValue }) {
const userIsAddingChars = prevValue.length < curValue.length;
const userStartsNewWord =
prevValue.length &&
curValue.length &&
prevValue[0].toLowerCase() !== curValue[0].toLowerCase();
return userIsAddingChars || userStartsNewWord;
}
@ -629,7 +649,6 @@ export class LionCombobox extends OverlayMixin(LionListbox) {
this.opened = false;
this.__shouldAutocompleteNextUpdate = true;
this._setTextboxValue('');
// this.checkedIndex = -1;
break;
case 'Enter':
if (!this.formElements[this.activeIndex]) {

64
packages/combobox/test/lion-combobox.test.js
View file

@ -4,6 +4,7 @@ import sinon from 'sinon';
import '../lion-combobox.js';
import { LionOptions } from '@lion/listbox/src/LionOptions.js';
import { browserDetection, LitElement } from '@lion/core';
import { Required } from '@lion/form-core';
/**
* @typedef {import('../src/LionCombobox.js').LionCombobox} LionCombobox
@ -207,6 +208,42 @@ describe('lion-combobox', () => {
await el.updateComplete;
expect(el._inputNode.value).to.equal('20');
});
it('sets modelValue to empty string if no option is selected', async () => {
const el = /** @type {LionCombobox} */ (await fixture(html`
<lion-combobox name="foo" .modelValue="${'Artichoke'}">
<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>
`));
expect(el.modelValue).to.equal('Artichoke');
expect(el.formElements[0].checked).to.be.true;
el.checkedIndex = -1;
await el.updateComplete;
expect(el.modelValue).to.equal('');
expect(el.formElements[0].checked).to.be.false;
});
it('sets modelValue to empty array if no option is selected for multiple choice', async () => {
const el = /** @type {LionCombobox} */ (await fixture(html`
<lion-combobox name="foo" multiple-choice .modelValue="${['Artichoke']}">
<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>
`));
expect(el.modelValue).to.eql(['Artichoke']);
expect(el.formElements[0].checked).to.be.true;
el.checkedIndex = [];
await el.updateComplete;
expect(el.modelValue).to.eql([]);
expect(el.formElements[0].checked).to.be.false;
});
});
describe('Listbox visibility', () => {
@ -409,6 +446,32 @@ describe('lion-combobox', () => {
expect(o.getAttribute('aria-hidden')).to.equal('true');
});
});
it('works with validation', async () => {
const el = /** @type {LionCombobox} */ (await fixture(html`
<lion-combobox name="foo" .validators=${[new Required()]}>
<lion-option checked .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>
`));
expect(el.checkedIndex).to.equal(0);
// Simulate backspace deleting the char at the end of the string
el._inputNode.dispatchEvent(new KeyboardEvent('keydown', { key: 'Backspace' }));
el._inputNode.dispatchEvent(new Event('input'));
const arr = el._inputNode.value.split('');
arr.splice(el._inputNode.value.length - 1, 1);
el._inputNode.value = arr.join('');
await el.updateComplete;
el.dispatchEvent(new Event('blur'));
expect(el.checkedIndex).to.equal(-1);
await el.feedbackComplete;
expect(el.hasFeedbackFor).to.include('error');
expect(el.showsFeedbackFor).to.include('error');
});
});
});
@ -483,7 +546,6 @@ describe('lion-combobox', () => {
<lion-option .choiceValue="${'10'}" checked>Item 1</lion-option>
</lion-combobox>
`));
// @ts-expect-error sinon not typed correctly?
const spy = sinon.spy(el._selectionDisplayNode, 'onComboboxElementUpdated');
el.requestUpdate('modelValue');
await el.updateComplete;

1
packages/form-core/src/FocusMixin.js
View file

@ -6,6 +6,7 @@ import { FormControlMixin } from './FormControlMixin.js';
* @param {import('@open-wc/dedupe-mixin').Constructor<import('@lion/core').LitElement>} superclass
*/
const FocusMixinImplementation = superclass =>
// @ts-expect-error false positive for incompatible static get properties. Lit-element merges super properties already for you.
// eslint-disable-next-line no-unused-vars, max-len, no-shadow
class FocusMixin extends FormControlMixin(superclass) {
static get properties() {

21
packages/form-core/src/FormControlMixin.js
View file

@ -794,13 +794,14 @@ const FormControlMixinImplementation = superclass =>
return;
}
// B2. Are we a single choice choice-group? If so, halt when unchecked
// B2. Are we a single choice choice-group? If so, halt when target unchecked
// and something else is checked, meaning we will get
// another model-value-changed dispatch for the checked target
//
// We only send the checked changed up (not the unchecked). In this way a choice group
// (radio-group, checkbox-group, select/listbox) acts as an 'endpoint' (a single Field)
// just like the native <select>
// @ts-expect-error multipleChoice is not directly available but only as side effect
if (this._repropagationRole === 'choice-group' && !this.multipleChoice && !target.checked) {
if (!this._repropagationCondition(target)) {
return;
}
@ -822,6 +823,20 @@ const FormControlMixinImplementation = superclass =>
);
}
/**
* TODO: Extend this in choice group so that target is always a choice input and multipleChoice exists.
* This will fix the types and reduce the need for ignores/expect-errors
* @param {EventTarget & import('../types/choice-group/ChoiceInputMixinTypes').ChoiceInputHost} target
*/
_repropagationCondition(target) {
return !(
this._repropagationRole === 'choice-group' &&
// @ts-expect-error multipleChoice is not directly available but only as side effect
!this.multipleChoice &&
!target.checked
);
}
/**
* @overridable
* A Subclasser should only override this method if the interactive element

1
packages/form-core/src/FormatMixin.js
View file

@ -57,6 +57,7 @@ import { ValidateMixin } from './validate/ValidateMixin.js';
* @param {import('@open-wc/dedupe-mixin').Constructor<import('@lion/core').LitElement>} superclass
*/
const FormatMixinImplementation = superclass =>
// @ts-expect-error false positive for incompatible static get properties. Lit-element merges super properties already for you.
class FormatMixin extends ValidateMixin(FormControlMixin(superclass)) {
static get properties() {
return {

1
packages/form-core/src/InteractionStateMixin.js
View file

@ -18,6 +18,7 @@ import { FormControlMixin } from './FormControlMixin.js';
* @param {import('@open-wc/dedupe-mixin').Constructor<import('@lion/core').LitElement>} superclass
*/
const InteractionStateMixinImplementation = superclass =>
// @ts-expect-error false positive for incompatible static get properties. Lit-element merges super properties already for you.
class InteractionStateMixin extends FormControlMixin(superclass) {
static get properties() {
return {

3
packages/form-core/src/choice-group/ChoiceGroupMixin.js
View file

@ -15,6 +15,7 @@ import { InteractionStateMixin } from '../InteractionStateMixin.js';
* @param {import('@open-wc/dedupe-mixin').Constructor<import('@lion/core').LitElement>} superclass
*/
const ChoiceGroupMixinImplementation = superclass =>
// @ts-expect-error false positive for incompatible static get properties. Lit-element merges super properties already for you.
class ChoiceGroupMixin extends FormRegistrarMixin(InteractionStateMixin(superclass)) {
static get properties() {
return {
@ -49,8 +50,8 @@ const ChoiceGroupMixinImplementation = superclass =>
this.__isInitialModelValue = false;
this.registrationComplete.then(() => {
this._setCheckedElements(value, checkCondition);
this.requestUpdate('modelValue');
});
this.requestUpdate('modelValue');
} else {
this._setCheckedElements(value, checkCondition);
this.requestUpdate('modelValue');

1
packages/form-core/src/choice-group/ChoiceInputMixin.js
View file

@ -21,6 +21,7 @@ const hasChanged = (nw, old = {}) => nw.value !== old.value || nw.checked !== ol
* @param {import('@open-wc/dedupe-mixin').Constructor<import('@lion/core').LitElement>} superclass
*/
const ChoiceInputMixinImplementation = superclass =>
// @ts-expect-error false positive for incompatible static get properties. Lit-element merges super properties already for you.
class ChoiceInputMixin extends FormatMixin(superclass) {
static get properties() {
return {

1
packages/form-core/src/form-group/FormGroupMixin.js
View file

@ -29,6 +29,7 @@ import { FormElementsHaveNoError } from './FormElementsHaveNoError.js';
* @param {import('@open-wc/dedupe-mixin').Constructor<import('@lion/core').LitElement>} superclass
*/
const FormGroupMixinImplementation = superclass =>
// @ts-expect-error false positive for incompatible static get properties. Lit-element merges super properties already for you.
class FormGroupMixin extends FormRegistrarMixin(
FormControlMixin(ValidateMixin(DisabledMixin(SlotMixin(superclass)))),
) {

1
packages/form-core/src/validate/ValidateMixin.js
View file

@ -32,6 +32,7 @@ function arrayDiff(array1 = [], array2 = []) {
* @param {import('@open-wc/dedupe-mixin').Constructor<import('@lion/core').LitElement>} superclass
*/
export const ValidateMixinImplementation = superclass =>
// @ts-expect-error false positive for incompatible static get properties. Lit-element merges super properties already for you.
class extends FormControlMixin(
SyncUpdatableMixin(DisabledMixin(SlotMixin(ScopedElementsMixin(superclass)))),
) {

21
packages/form-core/types/FormControlMixinTypes.d.ts vendored
View file

@ -12,6 +12,27 @@ declare interface HTMLElementWithValue extends HTMLElement {
export declare class FormControlHost {
static get styles(): CSSResult | CSSResult[];
static get properties(): {
name: {
type: StringConstructor;
reflect: boolean;
};
readOnly: {
type: BooleanConstructor;
attribute: string;
reflect: boolean;
};
label: StringConstructor;
helpText: {
type: StringConstructor;
attribute: string;
};
modelValue: { attribute: boolean };
_ariaLabelledNodes: { attribute: boolean };
_ariaDescribedNodes: { attribute: boolean };
_repropagationRole: { attribute: boolean };
_isRepropagationEndpoint: { attribute: boolean };
};
/**
* A Boolean attribute which, if present, indicates that the user should not be able to edit
* the value of the input. The difference between disabled and readonly is that read-only

7
packages/form-integrations/docs/15-features-overview.md
View file

@ -101,7 +101,12 @@ export const main = () => {
<lion-option checked .choiceValue=${'Banana'}>Banana</lion-option>
<lion-option .choiceValue=${'Mango'}>Mango</lion-option>
</lion-listbox>
<lion-combobox name="favoriteMovie" label="Favorite movie" autocomplete="both">
<lion-combobox
.validators="${[new Required()]}"
name="favoriteMovie"
label="Favorite movie"
autocomplete="both"
>
<lion-option checked .choiceValue=${'Rocky'}>Rocky</lion-option>
<lion-option .choiceValue=${'Rocky II'}>Rocky II</lion-option>
<lion-option .choiceValue=${'Rocky III'}>Rocky III</lion-option>

46
packages/listbox/src/ListboxMixin.js
View file

@ -223,7 +223,7 @@ const ListboxMixinImplementation = superclass =>
* @deprecated
* This setter exists for backwards compatibility of single choice groups.
* A setter api would be confusing for a multipleChoice group. Use `setCheckedIndex` instead.
* @param {number} index
* @param {number|number[]} index
*/
set checkedIndex(index) {
this.setCheckedIndex(index);
@ -267,6 +267,9 @@ const ListboxMixinImplementation = superclass =>
*/
this._listboxReceivesNoFocus = false;
/** @type {string | string[] | undefined} */
this.__oldModelValue = undefined;
/** @type {EventListener} */
this._listboxOnKeyDown = this._listboxOnKeyDown.bind(this);
/** @type {EventListener} */
@ -335,23 +338,29 @@ const ListboxMixinImplementation = superclass =>
}
/**
* When `multipleChoice` is false, will toggle, else will check provided index
* @param {number} index
* @param {'set'|'unset'|'toggle'} multiMode
* If an array is passed for multiple-choice, it will check the indexes in array, and uncheck the rest
* If a number is passed, the item with the passed index is checked without unchecking others
* For single choice, __onChildCheckedChanged we ensure that we uncheck siblings
* @param {number|number[]} index
*/
setCheckedIndex(index, multiMode = 'toggle') {
if (this.formElements[index]) {
if (!this.multipleChoice) {
this.formElements[index].checked = true;
// In __onChildCheckedChanged, which also responds to programmatic (model)value changes
// of children, we do the rest (uncheck siblings)
} else if (multiMode === 'toggle') {
this.formElements[index].checked = !this.formElements[index].checked;
} else {
this.formElements[index].checked = multiMode === 'set';
setCheckedIndex(index) {
if (this.multipleChoice && Array.isArray(index)) {
this._uncheckChildren(this.formElements.filter(i => i === index));
index.forEach(i => {
if (this.formElements[i]) {
this.formElements[i].checked = true;
}
});
return;
}
if (typeof index === 'number') {
if (index === -1) {
this._uncheckChildren();
}
if (this.formElements[index]) {
this.formElements[index].checked = true;
}
} else if (!this.multipleChoice) {
this._uncheckChildren();
}
}
@ -670,10 +679,13 @@ const ListboxMixinImplementation = superclass =>
ev.stopPropagation();
}
this.__onChildCheckedChanged(ev);
this.requestUpdate('modelValue', this.modelValue);
// don't send this.modelValue as oldValue, since it will take modelValue getter which takes it from child elements, which is already the updated value
this.requestUpdate('modelValue', this.__oldModelValue);
this.dispatchEvent(
new CustomEvent('model-value-changed', { detail: { element: ev.target } }),
);
this.__oldModelValue = this.modelValue;
}
/**

87
packages/listbox/test-suites/ListboxMixin.suite.js
View file

@ -1,4 +1,5 @@
import { Required } from '@lion/form-core';
import sinon from 'sinon';
import { expect, html, fixture as _fixture, unsafeStatic } from '@open-wc/testing';
import { LionOptions } from '@lion/listbox';
import '@lion/listbox/lion-option.js';
@ -109,6 +110,88 @@ export function runListboxMixinSuite(customConfig = {}) {
expect(el.formElements[2].checked).to.be.true;
});
it('requests update for modelValue when checkedIndex changes', async () => {
const el = await fixture(html`
<${tag} name="gender" .modelValue=${'other'}>
<${optionTag} .choiceValue=${'male'}></${optionTag}>
<${optionTag} .choiceValue=${'female'}></${optionTag}>
<${optionTag} .choiceValue=${'other'}></${optionTag}>
</${tag}>
`);
expect(el.checkedIndex).to.equal(2);
const requestSpy = sinon.spy(el, 'requestUpdate');
const updatedSpy = sinon.spy(el, 'updated');
el.setCheckedIndex(1);
await el.updateComplete;
expect(requestSpy).to.have.been.calledWith('modelValue', 'other');
expect(updatedSpy).to.have.been.calledWith(
sinon.match.map.contains(new Map([['modelValue', 'other']])),
);
});
it('requests update for modelValue after click', async () => {
const el = await fixture(html`
<${tag} name="gender" .modelValue=${'other'}>
<${optionTag} .choiceValue=${'male'}></${optionTag}>
<${optionTag} .choiceValue=${'female'}></${optionTag}>
<${optionTag} .choiceValue=${'other'}></${optionTag}>
</${tag}>
`);
expect(el.checkedIndex).to.equal(2);
const requestSpy = sinon.spy(el, 'requestUpdate');
const updatedSpy = sinon.spy(el, 'updated');
el.formElements[0].click();
await el.updateComplete;
expect(requestSpy).to.have.been.calledWith('modelValue', 'other');
expect(updatedSpy).to.have.been.calledWith(
sinon.match.map.contains(new Map([['modelValue', 'other']])),
);
});
it('requests update for modelValue when checkedIndex changes for multiple choice', async () => {
const el = await fixture(html`
<${tag} name="gender" multiple-choice .modelValue=${['other']}>
<${optionTag} .choiceValue=${'male'}></${optionTag}>
<${optionTag} .choiceValue=${'female'}></${optionTag}>
<${optionTag} .choiceValue=${'other'}></${optionTag}>
</${tag}>
`);
expect(el.checkedIndex).to.eql([2]);
const requestSpy = sinon.spy(el, 'requestUpdate');
const updatedSpy = sinon.spy(el, 'updated');
el.setCheckedIndex(1);
await el.updateComplete;
expect(requestSpy).to.have.been.calledWith(
'modelValue',
sinon.match.array.deepEquals(['other']),
);
expect(updatedSpy).to.have.been.calledOnce;
// reference values vs real values suck :( had to do it like this, sinon matchers did not match because 'other' is inside an array so it's not a "real" match
expect([...updatedSpy.args[0][0].entries()]).to.deep.include(['modelValue', ['other']]);
});
it('requests update for modelValue after click for multiple choice', async () => {
const el = await fixture(html`
<${tag} name="gender" multiple-choice .modelValue=${['other']}>
<${optionTag} .choiceValue=${'male'}></${optionTag}>
<${optionTag} .choiceValue=${'female'}></${optionTag}>
<${optionTag} .choiceValue=${'other'}></${optionTag}>
</${tag}>
`);
expect(el.checkedIndex).to.eql([2]);
const requestSpy = sinon.spy(el, 'requestUpdate');
const updatedSpy = sinon.spy(el, 'updated');
el.formElements[0].click();
await el.updateComplete;
expect(requestSpy).to.have.been.calledWith(
'modelValue',
sinon.match.array.deepEquals(['other']),
);
expect(updatedSpy).to.have.been.calledOnce;
// reference values vs real values suck :( had to do it like this, sinon matchers did not match because 'other' is inside an array so it's not a "real" match
expect([...updatedSpy.args[0][0].entries()]).to.deep.include(['modelValue', ['other']]);
});
it(`has a fieldName based on the label`, async () => {
const el1 = await fixture(html`
<${tag} label="foo"></${tag}>
@ -356,8 +439,8 @@ export function runListboxMixinSuite(customConfig = {}) {
<${optionTag} .choiceValue=${'30'} checked>Item 3</${optionTag}>
</${tag}>
`);
el.setCheckedIndex(2);
expect(el.modelValue).to.deep.equal(['20']);
el.setCheckedIndex(0);
expect(el.modelValue).to.deep.equal(['10', '20', '30']);
el.reset();
expect(el.modelValue).to.deep.equal(['20', '30']);
});

4
packages/listbox/types/ListboxMixinTypes.d.ts vendored
View file

@ -39,7 +39,7 @@ export declare class ListboxHost {
public formElements: LionOption[];
public setCheckedIndex(index: number): void;
public setCheckedIndex(index: number | number[]): void;
/** Reset interaction states and modelValue */
public reset(): void;
@ -79,7 +79,7 @@ export declare function ListboxImplementation<T extends Constructor<LitElement>>
superclass: T,
): T &
Constructor<ListboxHost> &
ListboxHost &
typeof ListboxHost &
Constructor<ChoiceGroupHost> &
typeof ChoiceGroupHost &
Constructor<SlotHost> &