Merge bdd77cf into f43c1bc

Author: adamjohnson

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;
     };

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

@@ -49,9 +49,24 @@ export abstract class ATFocusController<Item extends HTMLElement> {
 
   set atFocusedItemIndex(index: number) {
     const previousIndex = this.#atFocusedItemIndex;
-    const direction = index > previousIndex ? 1 : -1;
     const { items, atFocusableItems } = this;
+    // - Home (index=0): always search forward to find first focusable item
+    // - End (index=last): always search backward to find last focusable item
+    // - Other cases: use comparison to determine direction
+    const direction =
+      index === 0 ?
+        1
+      : index >= items.length - 1 ?
+        -1
+      : index > previousIndex ?
+        1
+      : -1;
     const itemsIndexOfLastATFocusableItem = items.indexOf(this.atFocusableItems.at(-1)!);
+    // Wrap to first focusable item (e.g. skip disabled placeholder at 0) so cycling works after selection.
+    const itemsIndexOfFirstATFocusableItem =
+      atFocusableItems.length ?
+        items.indexOf(this.atFocusableItems.at(0)!)
+      : 0;
     let itemToGainFocus = items.at(index);
     let itemToGainFocusIsFocusable = atFocusableItems.includes(itemToGainFocus!);
     if (atFocusableItems.length) {
@@ -60,7 +75,14 @@ export abstract class ATFocusController<Item extends HTMLElement> {
         if (index < 0) {
           index = itemsIndexOfLastATFocusableItem;
         } else if (index >= itemsIndexOfLastATFocusableItem) {
-          index = 0;
+          index = itemsIndexOfFirstATFocusableItem;
+        } else if (index < itemsIndexOfFirstATFocusableItem) {
+          // Before first focusable (index 0 when e.g. placeholder is not focusable).
+          // Home/End are handled in onKeydown by passing first/last focusable index, so the only
+          // time we see 0 here is Up from first focusable → wrap to last.
+          index = previousIndex === itemsIndexOfFirstATFocusableItem ?
+            itemsIndexOfLastATFocusableItem
+          : itemsIndexOfFirstATFocusableItem;
         } else {
           index = index + direction;
         }
@@ -113,6 +135,14 @@ export abstract class ATFocusController<Item extends HTMLElement> {
     this.itemsContainerElement ??= this.#initContainer();
   }
 
+  /**
+   * Refresh items from the getItems option. Call this when the list of items
+   * has changed (e.g. when a parent controller sets items).
+   */
+  refreshItems(): void {
+    this.initItems();
+  }
+
   hostConnected(): void {
     this.hostUpdate();
   }
@@ -183,24 +213,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;
     }

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

@@ -159,7 +159,7 @@ export class ComboboxController<
   Item extends HTMLElement
 > implements ReactiveController {
   public static of<T extends HTMLElement>(
-    host: ReactiveControllerHost,
+    host: ReactiveControllerHost & HTMLElement,
     options: ComboboxControllerOptions<T>,
   ): ComboboxController<T> {
     return new ComboboxController(host, options);
@@ -237,11 +237,13 @@ export class ComboboxController<
 
   #lb: ListboxController<Item>;
   #fc?: ATFocusController<Item>;
+  #initializing = false;
   #preventListboxGainingFocus = false;
   #input: HTMLElement | null = null;
   #button: HTMLElement | null = null;
   #listbox: HTMLElement | null = null;
   #buttonInitialRole: string | null = null;
+  #buttonHasMouseDown = false;
   #mo = new MutationObserver(() => this.#initItems());
   #microcopy = new Map<string, Record<Lang, string>>(Object.entries({
     dimmed: {
@@ -280,6 +282,7 @@ export class ComboboxController<
 
   set items(value: Item[]) {
     this.#lb.items = value;
+    this.#fc?.refreshItems?.();
   }
 
   /** Whether the combobox is disabled */
@@ -326,7 +329,7 @@ export class ComboboxController<
   }
 
   private constructor(
-    public host: ReactiveControllerHost,
+    public host: ReactiveControllerHost & HTMLElement,
     options: ComboboxControllerOptions<Item>,
   ) {
     host.addController(this);
@@ -362,7 +365,7 @@ export class ComboboxController<
   }
 
   hostUpdated(): void {
-    if (!this.#fc) {
+    if (!this.#fc && !this.#initializing) {
       this.#init();
     }
     const expanded = this.options.isExpanded();
@@ -380,7 +383,7 @@ export class ComboboxController<
     ComboboxController.hosts.delete(this.host);
   }
 
-  async _onFocusoutElement(): Promise<void> {
+  private async _onFocusoutElement(): Promise<void> {
     if (this.#hasTextInput && this.options.isExpanded()) {
       const root = this.#element?.getRootNode();
       await new Promise(requestAnimationFrame);
@@ -397,13 +400,15 @@ export class ComboboxController<
    * Order of operations is important
    */
   async #init() {
+    this.#initializing = true;
     await this.host.updateComplete;
     this.#initListbox();
     this.#initItems();
     this.#initButton();
     this.#initInput();
     this.#initLabels();
     this.#initController();
+    this.#initializing = false;
   }
 
   #initListbox() {
@@ -425,6 +430,8 @@ export class ComboboxController<
   #initButton() {
     this.#button?.removeEventListener('click', this.#onClickButton);
     this.#button?.removeEventListener('keydown', this.#onKeydownButton);
+    this.#button?.removeEventListener('mousedown', this.#onMousedownButton);
+    this.#button?.removeEventListener('mouseup', this.#onMouseupButton);
     this.#button = this.options.getToggleButton();
     if (!this.#button) {
       throw new Error('ComboboxController getToggleButton() option must return an element');
@@ -434,6 +441,8 @@ export class ComboboxController<
     this.#button.setAttribute('aria-controls', this.#listbox?.id ?? '');
     this.#button.addEventListener('click', this.#onClickButton);
     this.#button.addEventListener('keydown', this.#onKeydownButton);
+    this.#button.addEventListener('mousedown', this.#onMousedownButton);
+    this.#button.addEventListener('mouseup', this.#onMouseupButton);
   }
 
   #initInput() {
@@ -504,6 +513,8 @@ export class ComboboxController<
   }
 
   async #show(): Promise<void> {
+    // Re-read items on open so slotted/dynamically added options are included:
+    this.#initItems();
     const success = await this.options.requestShowListbox();
     this.#filterItems();
     if (success !== false && !this.#hasTextInput) {
@@ -531,26 +542,32 @@ export class ComboboxController<
     return strings?.[lang] ?? key;
   }
 
-  // TODO(bennypowers): perhaps move this to ActivedescendantController
-  #announce(item: Item) {
+  /**
+   * Announces the focused item to a live region (e.g. for Safari VoiceOver).
+   * @param item - The listbox option item to announce.
+   * TODO(bennypowers): perhaps move this to ActivedescendantController
+ */
+  #announce(item: Item): void {
     const value = this.options.getItemValue(item);
     ComboboxController.#alert?.remove();
     const fragment = ComboboxController.#alertTemplate.content.cloneNode(true) as DocumentFragment;
     ComboboxController.#alert = fragment.firstElementChild as HTMLElement;
     let text = value;
     const lang = deepClosest(this.#listbox, '[lang]')?.getAttribute('lang') ?? 'en';
-    const langKey = lang?.match(ComboboxController.langsRE)?.at(0) as Lang ?? 'en';
+    const langKey = (lang?.match(ComboboxController.langsRE)?.at(0) as Lang) ?? 'en';
     if (this.options.isItemDisabled(item)) {
       text += ` (${this.#translate('dimmed', langKey)})`;
     }
     if (this.#lb.isSelected(item)) {
       text += `, (${this.#translate('selected', langKey)})`;
     }
-    if (item.hasAttribute('aria-setsize') && item.hasAttribute('aria-posinset')) {
+    const posInSet = InternalsController.getAriaPosInSet(item);
+    const setSize = InternalsController.getAriaSetSize(item);
+    if (posInSet != null && setSize != null) {
       if (langKey === 'ja') {
-        text += `, (${item.getAttribute('aria-setsize')} 件中 ${item.getAttribute('aria-posinset')} 件目)`;
+        text += `, (${setSize} 件中 ${posInSet} 件目)`;
       } else {
-        text += `, (${item.getAttribute('aria-posinset')} ${this.#translate('of', langKey)} ${item.getAttribute('aria-setsize')})`;
+        text += `, (${posInSet} ${this.#translate('of', langKey)} ${setSize})`;
       }
     }
     ComboboxController.#alert.lang = lang;
@@ -580,6 +597,17 @@ export class ComboboxController<
     }
   };
 
+  /**
+   * Distinguish click-to-toggle vs Tab/Shift+Tab
+  */
+  #onMousedownButton = () => {
+    this.#buttonHasMouseDown = true;
+  };
+
+  #onMouseupButton = () => {
+    this.#buttonHasMouseDown = false;
+  };
+
   #onClickListbox = (event: MouseEvent) => {
     if (!this.multi && event.composedPath().some(this.options.isItem)) {
       this.#hide();
@@ -735,9 +763,14 @@ export class ComboboxController<
   #onFocusoutListbox = (event: FocusEvent) => {
     if (!this.#hasTextInput && this.options.isExpanded()) {
       const root = this.#element?.getRootNode();
+      // Check if focus moved to the toggle button via mouse click
+      // If so, let the click handler manage toggle (prevents double-toggle)
+      // But if focus moved via Shift+Tab (no mousedown), we should still hide
+      const isClickOnToggleButton =
+          event.relatedTarget === this.#button && this.#buttonHasMouseDown;
       if ((root instanceof ShadowRoot || root instanceof Document)
           && !this.items.includes(event.relatedTarget as Item)
-      ) {
+          && !isClickOnToggleButton) {
         this.#hide();
       }
     }