fix(combobox): dispatch model-value-changed properly on unselect

.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.

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`
+    <lion-form>
+      <form>
+        <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>
+          <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.

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",

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 =

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`
+          <lion-combobox name="foo" .validators=${[new Required()]}>
+            <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>
+        `));
+
+        // 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');
+      });
     });
   });
 

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 <select>
-      // @ts-expect-error multipleChoice is not directly available but only as side effect
+      if (this._repropagationConditionFails(target)) {
-      if (this._repropagationRole === 'choice-group' && !this.multipleChoice && !target.checked) {
         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
+     */
+    _repropagationConditionFails(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

packages/form-integrations/docs/15-features-overview.md

@@ -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>

packages/listbox/src/ListboxMixin.js

@@ -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} */
@@ -670,10 +673,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;
     }
 
     /**