feat: merge CustomChoiceGroupMixin with ChoiceGroupMixin; enhance code readibility

Author: tlouisse

.changeset/thirty-frogs-refuse.md

@@ -0,0 +1,5 @@
+---
+'@lion/ui': patch
+---
+
+merge CustomChoiceGroupMixin functionality into ChoiceGroupMixin

docs/components/combobox/use-cases.md

@@ -26,22 +26,6 @@ import { loadDefaultFeedbackMessages } from '@lion/ui/validate-messages.js';
 loadDefaultFeedbackMessages();
 ```
 
-## Require option match
-
-By default `requireOptionMatch` is set to true, which means that the listbox is leading. The textbox is a helping aid to quickly select an option/options. Unmatching input values become Unparseable, with the `MatchesOption` set as a default validator.
-
-When `requireOptionMatch` is set to false the textbox is leading, with the listbox as an aid to supply suggestions, e.g. a search input. This means that all input values are allowed.
-
-```js preview-story
-export const optionMatch = () => html`
-  <lion-combobox name="search" label="Search" .requireOptionMatch=${false}>
-    ${lazyRender(
-      listboxData.map(entry => html` <lion-option .choiceValue="${entry}">${entry}</lion-option> `),
-    )}
-  </lion-combobox>
-`;
-```
-
 ## Autocomplete
 
 Below you will find an overview of all possible `autocomplete` behaviors and how they correspond
@@ -257,7 +241,7 @@ Alternatively, the multi-choice flag can be combined with .requireMultipleMatch=
 
 ```js preview-story
 export const multipleCustomizableChoice = () => html`
-  <lion-combobox name="combo" label="Multiple" .requireOptionMatch=${false} multiple-choice>
+  <lion-combobox name="combo" label="Multiple" allow-custom-choice multiple-choice>
     <demo-selection-display
       slot="selection-display"
       style="display: contents;"
@@ -272,11 +256,25 @@ export const multipleCustomizableChoice = () => html`
 `;
 ```
 
-## Validation
+## Allow custom choice
+
+By default `allow-custom-choice` is set to false.
+This means that the textbox value must correspond with one of the options in the listbox.
+When set to true, the textbox value will be leading.
+
+```js preview-story
+export const optionMatch = () => html`
+  <lion-combobox name="search" label="Search" allow-custom-choice>
+    ${lazyRender(
+      listboxData.map(entry => html` <lion-option .choiceValue="${entry}">${entry}</lion-option> `),
+    )}
+  </lion-combobox>
+`;
+```
 
-The combobox works with a `Required` validator to check if it is empty.
+## Validation
 
-By default the a check is made which makes sure the value matches an option. This only works if `requireOptionMatch` is set to true.
+The combobox works with a `Required` validator to check if it is empty. By default a check is made which makes sure the value matches an option.
 
 ```js preview-story
 export const validation = () => html`

packages/ui/components/combobox/src/LionCombobox.js

@@ -5,10 +5,11 @@ import { LocalizeMixin } from '@lion/ui/localize-no-side-effects.js';
 import { OverlayMixin, withDropdownConfig } from '@lion/ui/overlays.js';
 import { css, html } from 'lit';
 import { makeMatchingTextBold, unmakeMatchingTextBold } from './utils/makeMatchingTextBold.js';
+import {
+  fixOptionA11yForSafari,
+  cleanupOptionA11yForSafari,
+} from './utils/fixOptionA11yForSafari.js';
 import { MatchesOption } from './validators.js';
-import { CustomChoiceGroupMixin } from '../../form-core/src/choice-group/CustomChoiceGroupMixin.js';
-
-const matchA11ySpanReverseFns = new WeakMap();
 
 // TODO: make ListboxOverlayMixin that is shared between SelectRich and Combobox
 // TODO: extract option matching based on 'typed character cache' and share that logic
@@ -28,29 +29,17 @@ const matchA11ySpanReverseFns = new WeakMap();
  * LionCombobox: implements the wai-aria combobox design pattern and integrates it as a Lion
  * FormControl
  */
-export class LionCombobox extends LocalizeMixin(OverlayMixin(CustomChoiceGroupMixin(LionListbox))) {
+export class LionCombobox extends LocalizeMixin(OverlayMixin(LionListbox)) {
   /** @type {any} */
-  static get properties() {
-    return {
-      autocomplete: { type: String, reflect: true },
-      matchMode: {
-        type: String,
-        attribute: 'match-mode',
-      },
-      showAllOnEmpty: {
-        type: Boolean,
-        attribute: 'show-all-on-empty',
-      },
-      requireOptionMatch: {
-        type: Boolean,
-      },
-      allowCustomChoice: {
-        type: Boolean,
-        attribute: 'allow-custom-choice',
-      },
-      __shouldAutocompleteNextUpdate: Boolean,
-    };
-  }
+  static properties = {
+    autocomplete: { type: String, reflect: true },
+    matchMode: { type: String, attribute: 'match-mode' },
+    showAllOnEmpty: { type: Boolean, attribute: 'show-all-on-empty' },
+    // N.B.: deprecated: use allowCustomChoice instead
+    requireOptionMatch: { type: Boolean },
+    allowCustomChoice: { type: Boolean, attribute: 'allow-custom-choice' },
+    __shouldAutocompleteNextUpdate: { type: Boolean, state: true },
+  };
 
   static get styles() {
     return [
@@ -358,7 +347,7 @@ export class LionCombobox extends LocalizeMixin(OverlayMixin(CustomChoiceGroupMi
    */
   get _listboxNode() {
     return /** @type {LionOptions} */ (
-      (this._overlayCtrl && this._overlayCtrl.contentNode) ||
+      this._overlayCtrl?.contentNode ||
         Array.from(this.children).find(child => child.slot === 'listbox')
     );
   }
@@ -372,7 +361,8 @@ export class LionCombobox extends LocalizeMixin(OverlayMixin(CustomChoiceGroupMi
   }
 
   /**
-   * @returns {boolean}
+   * @type {boolean}
+   * @deprecated
    */
   get requireOptionMatch() {
     return !this.allowCustomChoice;
@@ -407,11 +397,6 @@ export class LionCombobox extends LocalizeMixin(OverlayMixin(CustomChoiceGroupMi
      * By default, the listbox closes on empty, similar to wai-aria example and <datalist>
      */
     this.showAllOnEmpty = false;
-    /**
-     * If set to false, the value is allowed to not match any of the options.
-     * We set the default to true for backwards compatibility
-     */
-    this.requireOptionMatch = true;
     /**
      * @configure ListboxMixin: the wai-aria pattern and <datalist> rotate
      */
@@ -490,7 +475,7 @@ export class LionCombobox extends LocalizeMixin(OverlayMixin(CustomChoiceGroupMi
     }
     if (name === 'modelValue' && this.modelValue && this.modelValue !== oldValue) {
       if (this._syncToTextboxCondition(this.modelValue, this._oldModelValue)) {
-        if (!this.multipleChoice) {
+        if (this._isSingleChoice) {
           const textboxValue = this._getTextboxValueFromOption(
             this.formElements[/** @type {number} */ (this.checkedIndex)],
           );
@@ -507,13 +492,13 @@ export class LionCombobox extends LocalizeMixin(OverlayMixin(CustomChoiceGroupMi
 
   /**
    * Converts viewValue to modelValue
-   * @override CustomChoiceGroupMixin
+   * @override ChoiceGroupMixin
    * @param {string|string[]} value - viewValue: the formatted value inside <input>
-   * @returns {*} modelValue
+   * @returns {any} modelValue
    */
   parser(value) {
     if (
-      this.requireOptionMatch &&
+      !this.allowCustomChoice &&
       this.checkedIndex === -1 &&
       value !== '' &&
       !Array.isArray(value)
@@ -525,7 +510,7 @@ export class LionCombobox extends LocalizeMixin(OverlayMixin(CustomChoiceGroupMi
 
   /**
    * When textbox value doesn't match checkedIndex anymore, update accordingly...
-   * @protected
+   * @private
    */
   __unsyncCheckedIndexOnInputChange() {
     const autoselect = this._autoSelectCondition();
@@ -782,25 +767,7 @@ export class LionCombobox extends LocalizeMixin(OverlayMixin(CustomChoiceGroupMi
   // eslint-disable-next-line class-methods-use-this
   _highlightMatchedOption(option, matchingString) {
     makeMatchingTextBold(option, matchingString);
-
-    // For Safari, we need to add a label to the element
-    if (option.textContent) {
-      const a11ySpan = document.createElement('span');
-      a11ySpan.setAttribute('aria-label', option.textContent.replace(/\s+/g, ' '));
-      Array.from(option.childNodes).forEach(childNode => {
-        a11ySpan.appendChild(childNode);
-      });
-      option.appendChild(a11ySpan);
-
-      matchA11ySpanReverseFns.set(option, () => {
-        Array.from(a11ySpan.childNodes).forEach(childNode => {
-          option.appendChild(childNode);
-        });
-        if (option.contains(a11ySpan)) {
-          option.removeChild(a11ySpan);
-        }
-      });
-    }
+    fixOptionA11yForSafari(option);
   }
 
   /**
@@ -826,10 +793,7 @@ export class LionCombobox extends LocalizeMixin(OverlayMixin(CustomChoiceGroupMi
   // eslint-disable-next-line class-methods-use-this
   _unhighlightMatchedOption(option) {
     unmakeMatchingTextBold(option);
-
-    if (matchA11ySpanReverseFns.has(option)) {
-      matchA11ySpanReverseFns.get(option)();
-    }
+    cleanupOptionA11yForSafari(option);
   }
   /* eslint-enable no-param-reassign */
 
@@ -1095,7 +1059,7 @@ export class LionCombobox extends LocalizeMixin(OverlayMixin(CustomChoiceGroupMi
         break;
       case 'Backspace':
       case 'Delete':
-        if (this.requireOptionMatch) {
+        if (!this.allowCustomChoice) {
           super._listboxOnKeyDown(ev);
         } else {
           this.opened = false;
@@ -1107,7 +1071,7 @@ export class LionCombobox extends LocalizeMixin(OverlayMixin(CustomChoiceGroupMi
         }
 
         if (
-          !this.requireOptionMatch &&
+          this.allowCustomChoice &&
           this.multipleChoice &&
           (!this.formElements[this.activeIndex] ||
             this.formElements[this.activeIndex].hasAttribute('aria-hidden') ||
@@ -1158,14 +1122,15 @@ export class LionCombobox extends LocalizeMixin(OverlayMixin(CustomChoiceGroupMi
    */
   // eslint-disable-next-line no-unused-vars
   _syncToTextboxMultiple(modelValue, oldModelValue = []) {
-    if (this.requireOptionMatch) {
-      const diff = modelValue.filter(x => !oldModelValue.includes(x));
-      const newValue = this.formElements
-        .filter(option => diff.includes(option.choiceValue))
-        .map(option => this._getTextboxValueFromOption(option))
-        .join(' ');
-      this._setTextboxValue(newValue); // or last selected value?
+    if (this.allowCustomChoice) {
+      return;
     }
+    const diff = modelValue.filter(x => !oldModelValue.includes(x));
+    const newValue = this.formElements
+      .filter(option => diff.includes(option.choiceValue))
+      .map(option => this._getTextboxValueFromOption(option))
+      .join(' ');
+    this._setTextboxValue(newValue); // or last selected value?
   }
 
   /**

packages/ui/components/combobox/src/utils/fixOptionA11yForSafari.js

@@ -0,0 +1,49 @@
+/**
+ * @typedef {import('@lion/ui/listbox.js').LionOption} LionOption
+ */
+
+const matchA11ySpanReverseFns = new WeakMap();
+
+/**
+ * For Safari, we need to add a label to the element
+ * Create a wrapping span: => `<my-option><b>my</b> text</my-option>`
+ * becomes `<my-option><span aria-label="my text"><b>my</b> text</span></my-option>`
+ * @param {Element} option
+ */
+export function fixOptionA11yForSafari(option) {
+  if (!option.textContent) {
+    return;
+  }
+
+  // [1] Wrap the content in a span with an aria-label
+  // `<my-option><b>my</b> text</my-option>` =>
+  // `<my-option><span aria-label="my text"><b>my</b> text</span></my-option>`
+  const a11ySpan = document.createElement('span');
+  a11ySpan.setAttribute('aria-label', option.textContent.replace(/\s+/g, ' '));
+  for (const childNode of Array.from(option.childNodes)) {
+    a11ySpan.appendChild(childNode);
+  }
+  option.appendChild(a11ySpan);
+
+  // [2] Store a function to call later, that does:
+  // `<my-option><span aria-label="my text"><b>my</b> text</span></my-option>` =>
+  // `<my-option><b>my</b> text</my-option>`
+  matchA11ySpanReverseFns.set(option, () => {
+    for (const childNode of Array.from(a11ySpan.childNodes)) {
+      option.appendChild(childNode);
+    }
+    if (option.contains(a11ySpan)) {
+      option.removeChild(a11ySpan);
+    }
+  });
+}
+
+/**
+ * @param {Element} option
+ */
+export function cleanupOptionA11yForSafari(option) {
+  if (matchA11ySpanReverseFns.has(option)) {
+    matchA11ySpanReverseFns.get(option)();
+  }
+  matchA11ySpanReverseFns.delete(option);
+}

packages/ui/components/combobox/test/lion-combobox-integrations.test.js

@@ -1,9 +1,9 @@
 import { runListboxMixinSuite } from '@lion/ui/listbox-test-suites.js';
+import { runChoiceGroupMixinSuite } from '@lion/ui/form-core-test-suites.js';
 import '@lion/ui/define/lion-combobox.js';
-import { runCustomChoiceGroupMixinSuite } from '../../form-core/test-suites/choice-group/CustomChoiceGroupMixin.suite.js';
 
 runListboxMixinSuite({ tagString: 'lion-combobox' });
-runCustomChoiceGroupMixinSuite({
+runChoiceGroupMixinSuite({
   parentTagString: 'lion-combobox',
   childTagString: 'lion-option',
 });