fix(core): improve ATFocus, Combobox, Internals, Listbox, and RovingTabIndex controllers

.changeset/brave-pens-study.md

@@ -0,0 +1,9 @@
+---
+"@patternfly/pfe-core": patch
+---
+`ATFocusController`: fix keyboard focus wrapping and dynamic item support
+
+- Fix arrow up/down focus wrapping when a non-focusable placeholder occupies
+  index 0 (e.g. a disabled "Select a value" option)
+- Fix dynamically added options not receiving keyboard focus until the listbox
+  is reopened

.changeset/warm-dogs-laugh.md

@@ -0,0 +1,4 @@
+---
+"@patternfly/elements": patch
+---
+`<pf-select>`: don't steal browser focus on page load, and improve keyboard accessibility of the listbox.

core/pfe-core/controllers/activedescendant-controller.ts

@@ -134,7 +134,10 @@ export class ActivedescendantController<
     super.atFocusedItemIndex = index;
     const item = this._items.at(this.atFocusedItemIndex);
     for (const _item of this.items) {
-      this.options.setItemActive?.(_item, _item === item);
+      const isActive = _item === item;
+      // Map clone back to original item for setItemActive callback
+      const originalItem = this.#shadowToLightMap.get(_item) ?? _item;
+      this.options.setItemActive?.(originalItem, isActive);
     }
     const container = this.options.getActiveDescendantContainer();
     if (!ActivedescendantController.supportsCrossRootActiveDescendant) {
@@ -150,6 +153,12 @@ export class ActivedescendantController<
   }
 
   protected set controlsElements(elements: HTMLElement[]) {
+    // Avoid removing/re-adding listeners if elements haven't changed
+    // This prevents breaking event listeners during active event dispatch
+    if (elements.length === this.#controlsElements.length
+        && elements.every((el, i) => el === this.#controlsElements[i])) {
+      return;
+    }
     for (const old of this.#controlsElements) {
       old?.removeEventListener('keydown', this.onKeydown);
     }
@@ -159,6 +168,22 @@ export class ActivedescendantController<
     }
   }
 
+  /**
+   * Check the source item's focusable state, not the clone's.
+   * This is needed because filtering sets `hidden` on the light DOM item,
+   * and the MutationObserver sync to clones is asynchronous.
+   */
+  override get atFocusableItems(): Item[] {
+    return this._items.filter(item => {
+      // Map clone to source item to check actual hidden state
+      const sourceItem = this.#shadowToLightMap.get(item) ?? item;
+      return !!sourceItem
+          && sourceItem.ariaHidden !== 'true'
+          && !sourceItem.hasAttribute('inert')
+          && !sourceItem.hasAttribute('hidden');
+    });
+  }
+
   /** All items */
   get items() {
     return this._items;
@@ -195,6 +220,11 @@ export class ActivedescendantController<
           this.#shadowToLightMap.set(item, item);
           return item;
         } else {
+          // Reuse existing clone if available to maintain stable IDs
+          const existingClone = this.#lightToShadowMap.get(item);
+          if (existingClone) {
+            return existingClone;
+          }
           const clone = item.cloneNode(true) as Item;
           clone.id = getRandomId();
           this.#lightToShadowMap.set(item, clone);
@@ -214,6 +244,7 @@ export class ActivedescendantController<
     protected options: ActivedescendantControllerOptions<Item>,
   ) {
     super(host, options);
+    this.initItems();
     this.options.getItemValue ??= function(this: Item) {
       return (this as unknown as HTMLOptionElement).value;
     };
@@ -236,7 +267,8 @@ export class ActivedescendantController<
     }
   };
 
-  protected override initItems(): void {
+  /** @internal */
+  override initItems(): void {
     this.#attrMO.disconnect();
     super.initItems();
     this.controlsElements = this.options.getControlsElements?.() ?? [];

core/pfe-core/controllers/at-focus-controller.ts

@@ -47,31 +47,51 @@ export abstract class ATFocusController<Item extends HTMLElement> {
     return this.#atFocusedItemIndex;
   }
 
-  set atFocusedItemIndex(index: number) {
-    const previousIndex = this.#atFocusedItemIndex;
-    const direction = index > previousIndex ? 1 : -1;
+  set atFocusedItemIndex(requestedIndex: number) {
     const { items, atFocusableItems } = this;
-    const itemsIndexOfLastATFocusableItem = items.indexOf(this.atFocusableItems.at(-1)!);
-    let itemToGainFocus = items.at(index);
-    let itemToGainFocusIsFocusable = atFocusableItems.includes(itemToGainFocus!);
-    if (atFocusableItems.length) {
-      let count = 0;
-      while (!itemToGainFocus || !itemToGainFocusIsFocusable && count++ <= 1000) {
-        if (index < 0) {
-          index = itemsIndexOfLastATFocusableItem;
-        } else if (index >= itemsIndexOfLastATFocusableItem) {
-          index = 0;
-        } else {
-          index = index + direction;
-        }
-        itemToGainFocus = items.at(index);
-        itemToGainFocusIsFocusable = atFocusableItems.includes(itemToGainFocus!);
-      }
-      if (count >= 1000) {
-        throw new Error('Could not atFocusedItemIndex');
-      }
+
+    if (!atFocusableItems.length) {
+      this.#atFocusedItemIndex = requestedIndex;
+      return;
+    }
+
+    // Fast path: requested item is already focusable
+    if (requestedIndex >= 0
+      && requestedIndex < items.length
+      && atFocusableItems.includes(items[requestedIndex])) {
+      this.#atFocusedItemIndex = requestedIndex;
+      return;
+    }
+
+    const lastFocusableIndex = items.indexOf(atFocusableItems.at(-1)!);
+
+    // Navigated before start → wrap to last focusable
+    if (requestedIndex < 0) {
+      this.#atFocusedItemIndex = lastFocusableIndex;
+      return;
+    }
+
+    const firstFocusableIndex = items.indexOf(atFocusableItems[0]);
+
+    // Navigated past end or past last focusable → wrap to first focusable
+    if (requestedIndex >= items.length
+      || requestedIndex > lastFocusableIndex) {
+      this.#atFocusedItemIndex = firstFocusableIndex;
+      return;
     }
-    this.#atFocusedItemIndex = index;
+
+    // Before first focusable (e.g. disabled placeholder at index 0).
+    // ArrowUp from first focusable → wrap to last; otherwise snap to first.
+    if (requestedIndex < firstFocusableIndex) {
+      this.#atFocusedItemIndex =
+          this.#atFocusedItemIndex === firstFocusableIndex ? lastFocusableIndex
+        : firstFocusableIndex;
+      return;
+    }
+
+    // Mid-list non-focusable item: find nearest focusable in the navigation direction
+    this.#atFocusedItemIndex =
+      items.indexOf(this.#getNextFocusableItem(requestedIndex));
   }
 
   /** Elements which control the items container e.g. a combobox input */
@@ -106,9 +126,12 @@ export abstract class ATFocusController<Item extends HTMLElement> {
   }
 
   /**
-   * Initialize the items and itemsContainerElement fields
+   * Initialize the items and itemsContainerElement fields.
+   * Call this when the list of items has changed
+   * (e.g. when a parent controller sets items).
+   * @internal not for use by element authors
    */
-  protected initItems(): void {
+  initItems(): void {
     this.items = this.options.getItems();
     this.itemsContainerElement ??= this.#initContainer();
   }
@@ -130,6 +153,22 @@ export abstract class ATFocusController<Item extends HTMLElement> {
       ?? (!isServer && this.host instanceof HTMLElement ? this.host : null);
   }
 
+  /**
+   * When setting atFocusedItemIndex, and the current focus is on a
+   * mid-list non-focusable item: find nearest focusable in the navigation direction.
+   * disabled items will be skipped, and out of bounds items will be wrapped
+   *
+   * @param requestedIndex the desired index
+   */
+  #getNextFocusableItem(requestedIndex: number) {
+    const { items, atFocusableItems } = this;
+    if (requestedIndex > this.#atFocusedItemIndex) {
+      return atFocusableItems.find(item => items.indexOf(item) > requestedIndex)!;
+    } else {
+      return atFocusableItems.findLast(item => items.indexOf(item) < requestedIndex)!;
+    }
+  }
+
   /**
    * Override and conditionally call `super.onKeydown` to filter out keyboard events
    * which should not result in a focus change. Ensure that subclass' method is bound
@@ -183,24 +222,30 @@ export abstract class ATFocusController<Item extends HTMLElement> {
         event.stopPropagation();
         event.preventDefault();
         break;
-      case 'Home':
+      case 'Home': {
         if (!(event.target instanceof HTMLElement
             && (event.target.hasAttribute('aria-activedescendant')
              || event.target.ariaActiveDescendantElement))) {
-          this.atFocusedItemIndex = 0;
+          // Use first focusable index so the setter doesn't see 0 (reserved for Up-from-first wrap).
+          const first = this.atFocusableItems.at(0);
+          this.atFocusedItemIndex = first != null ? this.items.indexOf(first) : 0;
           event.stopPropagation();
           event.preventDefault();
         }
         break;
-      case 'End':
+      }
+      case 'End': {
         if (!(event.target instanceof HTMLElement
             && (event.target.hasAttribute('aria-activedescendant')
              || event.target.ariaActiveDescendantElement))) {
-          this.atFocusedItemIndex = this.items.length - 1;
+          // Use last focusable index for consistency with lists that have non-focusable items.
+          const last = this.atFocusableItems.at(-1);
+          this.atFocusedItemIndex = last != null ? this.items.indexOf(last) : this.items.length - 1;
           event.stopPropagation();
           event.preventDefault();
         }
         break;
+      }
       default:
         break;
     }