Console error on Listbox validation

[michielpauw]

Expected behavior

    <lion-form>
      <form>
        <lion-listbox name="listbox" .validators=${[new Required()]} label="Default">
          <lion-option .choiceValue=${'Apple'}>Apple</lion-option>
          <lion-option .choiceValue=${'Artichoke'}>Artichoke</lion-option>
          <lion-option .choiceValue=${'Asparagus'}>Asparagus</lion-option>
          <lion-option .choiceValue=${'Banana'}>Banana</lion-option>
          <lion-option .choiceValue=${'Beets'}>Beets</lion-option>
        </lion-listbox>
        <div class="buttons">
          <lion-button-submit>Submit</lion-button-submit>
        </div>
      </form>
    </lion-form>

When nothing is selected, validation message appears, and focus is placed on Listbox component (or some other component).

Actual Behavior

Validation message does appear, but a console error is thrown:

LionForm.js:101 Uncaught TypeError: Cannot read properties of undefined (reading 'focus')
    at LionForm._setFocusOnFirstErroneousFormElement (LionForm.js:101:43)
    at LionForm._setFocusOnFirstErroneousFormElement (LionForm.js:99:12)
    at LionForm._submit (LionForm.js:66:12)
    at LionButtonSubmit.__clickDelegationHandler (LionButtonReset.js:111:41)

Additional context

I suspect this PR introduced the issue: 31e079d#diff-418f32cd1a1ebdd1843e3b43e66e105d1c946423f10086fd1d0d0830f1bbd903

At first, firstFormElWithError is Listbox, and I believe this should be the element to which the focus is set to. But because of this condition, the method gets called again:

firstFormElWithError.formElements?.length > 0 // 1

The children of Listbox (i.e. LionOptions) do not have an error, but still the first child becomes firstFormElWithError because of this:

const firstFormElWithError =
      element.formElements.find(child => child.hasFeedbackFor.includes('error')) || // 2
      element.formElements[0];

After this, the method tries to set the focus to the first Option, but this does not have a _focusableNode.

I'm not 100% certain what the intended behaviour is, but I would argue that the check done in (2) should in some way be combined with the check done in (1), before re-calling the method.

[tlouisse]

Hey, thanks for pointing this out. There seems to be a small bug in the current implementation indeed.

In the end it comes down to whether the formElement (be it an input/field or an option) is focusable or not. For a listbox(but also for select-rich and combobox that use listbox under the hood), individual options are not focusable, only their "host" (the listbox component, that contains a tabindex) is. This is because listboxes are not implemented with roving tabindexes, but via aria-activedescendant.

Bottom line: LionOption is not designed to receive focus, and therefore it has no _focusableNode and that explains the undefined error above.

We should be able to solve it like this:

- if (firstFormElWithError.formElements?.length > 0) {
+ if (hasFocusableChildren(firstFormElWithError)) {

function hasFocusableChildren(formEl) {
  // this implies all children have the same type (either all of them are focusable or none of them are)
  return formEl.formElements?.some(child => child._focusableNode);
}