From 9fcb67f0ec85cd325072c649cd02fb4543df1458 Mon Sep 17 00:00:00 2001 From: Joren Broekema Date: Mon, 12 Oct 2020 18:29:45 +0200 Subject: [PATCH 1/5] fix(combobox): dispatch model-value-changed properly on unselect --- .changeset/silent-waves-teach.md | 8 +++++ packages/combobox/README.md | 30 +++++++++++++++++++ packages/combobox/package.json | 3 ++ packages/combobox/src/LionCombobox.js | 23 ++++++++++++-- packages/combobox/test/lion-combobox.test.js | 26 ++++++++++++++++ packages/form-core/src/FormControlMixin.js | 21 +++++++++++-- .../docs/15-features-overview.md | 7 ++++- packages/listbox/src/ListboxMixin.js | 8 ++++- 8 files changed, 119 insertions(+), 7 deletions(-) create mode 100644 .changeset/silent-waves-teach.md diff --git a/.changeset/silent-waves-teach.md b/.changeset/silent-waves-teach.md new file mode 100644 index 000000000..7c0f7953a --- /dev/null +++ b/.changeset/silent-waves-teach.md @@ -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. diff --git a/packages/combobox/README.md b/packages/combobox/README.md index b5210901b..56736707f 100644 --- a/packages/combobox/README.md +++ b/packages/combobox/README.md @@ -263,6 +263,36 @@ export const invokerButton = () => html` `; ``` +## Validation + +Validation can be used as normal, below is an example of a combobox with a `Required` validator. + +```js story +export const validation = () => { + loadDefaultFeedbackMessages(); + Required.getMessage = () => 'Please enter a value'; + return html` + +
+ + Rocky + Rocky II + Rocky III + Rocky IV + Rocky V + Rocky Balboa + +
+ + `; +}; +``` + ## Listbox compatibility All configurations that can be applied to `lion-listbox`, can be applied to `lion-combobox` as well. diff --git a/packages/combobox/package.json b/packages/combobox/package.json index 96f1dd0e6..dbcf07604 100644 --- a/packages/combobox/package.json +++ b/packages/combobox/package.json @@ -46,6 +46,9 @@ "@lion/listbox": "0.2.0", "@lion/overlays": "0.21.0" }, + "devDependencies": { + "@lion/validate-messages": "0.3.1" + }, "keywords": [ "combobox", "form", diff --git a/packages/combobox/src/LionCombobox.js b/packages/combobox/src/LionCombobox.js index d66e33750..02566c574 100644 --- a/packages/combobox/src/LionCombobox.js +++ b/packages/combobox/src/LionCombobox.js @@ -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 + */ + _repropagationConditionFails(target) { + return super._repropagationConditionFails(target) && this.formElements?.some(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,12 +468,14 @@ 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 = diff --git a/packages/combobox/test/lion-combobox.test.js b/packages/combobox/test/lion-combobox.test.js index ca065073e..6995e569f 100644 --- a/packages/combobox/test/lion-combobox.test.js +++ b/packages/combobox/test/lion-combobox.test.js @@ -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 @@ -409,6 +410,31 @@ describe('lion-combobox', () => { expect(o.getAttribute('aria-hidden')).to.equal('true'); }); }); + + it('works with validation', async () => { + const el = /** @type {LionCombobox} */ (await fixture(html` + + Artichoke + Chard + Chicory + Victoria Plum + + `)); + + // open + el._comboboxNode.dispatchEvent(new Event('focusin', { bubbles: true, composed: true })); + + mimicUserTyping(el, 'art'); + await el.updateComplete; + expect(el.checkedIndex).to.equal(0); + + mimicUserTyping(el, ''); + await el.updateComplete; + expect(el.checkedIndex).to.equal(-1); + await el.feedbackComplete; + expect(el.hasFeedbackFor).to.include('error'); + expect(el.showsFeedbackFor).to.include('error'); + }); }); }); diff --git a/packages/form-core/src/FormControlMixin.js b/packages/form-core/src/FormControlMixin.js index 03470e943..68e7af09d 100644 --- a/packages/form-core/src/FormControlMixin.js +++ b/packages/form-core/src/FormControlMixin.js @@ -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 - if (this._repropagationConditionFails(target)) { + if (!this._repropagationCondition(target)) { return; } @@ -828,8 +828,8 @@ const FormControlMixinImplementation = superclass => * This will fix the types and reduce the need for ignores/expect-errors * @param {EventTarget & import('../types/choice-group/ChoiceInputMixinTypes').ChoiceInputHost} target */ - _repropagationConditionFails(target) { - return ( + _repropagationCondition(target) { + return !( this._repropagationRole === 'choice-group' && // @ts-expect-error multipleChoice is not directly available but only as side effect !this.multipleChoice && diff --git a/packages/form-core/src/FormatMixin.js b/packages/form-core/src/FormatMixin.js index ac2d6bbdc..136132dc8 100644 --- a/packages/form-core/src/FormatMixin.js +++ b/packages/form-core/src/FormatMixin.js @@ -57,6 +57,7 @@ import { ValidateMixin } from './validate/ValidateMixin.js'; * @param {import('@open-wc/dedupe-mixin').Constructor} 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 { diff --git a/packages/form-core/src/InteractionStateMixin.js b/packages/form-core/src/InteractionStateMixin.js index 403cbbb05..e7fb9e590 100644 --- a/packages/form-core/src/InteractionStateMixin.js +++ b/packages/form-core/src/InteractionStateMixin.js @@ -18,6 +18,7 @@ import { FormControlMixin } from './FormControlMixin.js'; * @param {import('@open-wc/dedupe-mixin').Constructor} 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 { diff --git a/packages/form-core/src/choice-group/ChoiceGroupMixin.js b/packages/form-core/src/choice-group/ChoiceGroupMixin.js index f165833d0..6d4a50f6b 100644 --- a/packages/form-core/src/choice-group/ChoiceGroupMixin.js +++ b/packages/form-core/src/choice-group/ChoiceGroupMixin.js @@ -15,6 +15,7 @@ import { InteractionStateMixin } from '../InteractionStateMixin.js'; * @param {import('@open-wc/dedupe-mixin').Constructor} 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 { diff --git a/packages/form-core/src/choice-group/ChoiceInputMixin.js b/packages/form-core/src/choice-group/ChoiceInputMixin.js index 931ded641..599848825 100644 --- a/packages/form-core/src/choice-group/ChoiceInputMixin.js +++ b/packages/form-core/src/choice-group/ChoiceInputMixin.js @@ -21,6 +21,7 @@ const hasChanged = (nw, old = {}) => nw.value !== old.value || nw.checked !== ol * @param {import('@open-wc/dedupe-mixin').Constructor} 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 { diff --git a/packages/form-core/src/form-group/FormGroupMixin.js b/packages/form-core/src/form-group/FormGroupMixin.js index 41f1a05ed..8c0bebd8f 100644 --- a/packages/form-core/src/form-group/FormGroupMixin.js +++ b/packages/form-core/src/form-group/FormGroupMixin.js @@ -29,6 +29,7 @@ import { FormElementsHaveNoError } from './FormElementsHaveNoError.js'; * @param {import('@open-wc/dedupe-mixin').Constructor} 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)))), ) { diff --git a/packages/form-core/src/validate/ValidateMixin.js b/packages/form-core/src/validate/ValidateMixin.js index f33c67268..ac40528c0 100644 --- a/packages/form-core/src/validate/ValidateMixin.js +++ b/packages/form-core/src/validate/ValidateMixin.js @@ -32,6 +32,7 @@ function arrayDiff(array1 = [], array2 = []) { * @param {import('@open-wc/dedupe-mixin').Constructor} 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)))), ) { diff --git a/packages/form-core/types/FormControlMixinTypes.d.ts b/packages/form-core/types/FormControlMixinTypes.d.ts index 0a6095597..5acfac293 100644 --- a/packages/form-core/types/FormControlMixinTypes.d.ts +++ b/packages/form-core/types/FormControlMixinTypes.d.ts @@ -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