fix(core)!: add getItems and getItemsContainer to RTI constructor (#2678)

* fix(core)!: add getItems and getItemsContainer to RTI constructor

Closes #2644

* fix(core): update initialization of RTI in TabsController

* fix: keyboard a11y for select et al

---------

Co-authored-by: Steven Spriggs <[email protected]>

Author: bennypowers

.changeset/empty-otters-join.md

@@ -0,0 +1,22 @@
+---
+"@patternfly/pfe-core": major
+---
+
+`RovingTabindexController`: deprecate the `initItems` method and add `getItems` and `getItemContainer` instead
+
+BEFORE:
+```ts
+#tabindex = new RovingTabindexController(this);
+constructor() {
+  super();
+  this.#tabindex.initItems(this.#items);
+}
+```
+
+AFTER:
+```ts
+#tabindex = new RovingTabindexController(this, {
+  getItems: () => this.#items,
+});
+```
+

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

@@ -177,7 +177,10 @@ export class ListboxController<Item extends HTMLElement> implements ReactiveCont
 
   private internals: InternalsController;
 
-  private tabindex = new RovingTabindexController<Item>(this.host);
+  private tabindex = new RovingTabindexController<Item>(this.host, {
+    getElement: () => this.controllerOptions.getHTMLElement?.() ?? this.element,
+    getItems: () => this.visibleOptions,
+  });
 
   public static of<Item extends HTMLElement>(
     host: ReactiveControllerHost,
@@ -283,7 +286,7 @@ export class ListboxController<Item extends HTMLElement> implements ReactiveCont
     const oldValue = this.value;
 
     if (this.#updateFocus) {
-      this.tabindex.updateItems(this.visibleOptions);
+      this.tabindex.updateItems();
       this.#updateFocus = false;
     } else {
       this.tabindex.initItems(this.visibleOptions);

core/pfe-core/controllers/roving-tabindex-controller.ts

@@ -6,6 +6,12 @@ const isFocusableElement = (el: Element): el is HTMLElement =>
   !el.ariaHidden &&
   !el.hasAttribute('hidden');
 
+export interface RovingTabindexControllerOptions<Item extends HTMLElement> {
+  getElement?: () => Element;
+  getItems?: () => Item[];
+  getItemContainer?: () => HTMLElement;
+}
+
 /**
  * Implements roving tabindex, as described in WAI-ARIA practices, [Managing Focus Within
  * Components Using a Roving
@@ -30,8 +36,6 @@ export class RovingTabindexController<
   /** array of all focusable elements */
   #items: Item[] = [];
 
-  #getElement: () => Element | null;
-
   /**
    * finds focusable items from a group of items
    */
@@ -121,16 +125,30 @@ export class RovingTabindexController<
     return this.#caseSensitive;
   }
 
-  constructor(public host: ReactiveControllerHost, options?: {
-    getElement: () => Element;
-  }) {
-    this.#getElement = options?.getElement ?? (() => host instanceof Element ? host : null);
+  #options: {
+    getElement(): Element | null;
+    getItems?(): Item[];
+    getItemContainer?(): HTMLElement;
+  };
+
+  constructor(
+    public host: ReactiveControllerHost,
+    options?: RovingTabindexControllerOptions<Item>,
+  ) {
+    this.#options = {
+      getElement: options?.getElement ?? (() => host instanceof HTMLElement ? host : null),
+      getItems: options?.getItems,
+      getItemContainer: options?.getItemContainer,
+    };
     const instance = RovingTabindexController.hosts.get(host);
     if (instance) {
       return instance as RovingTabindexController<Item>;
     }
     RovingTabindexController.hosts.set(host, this);
     this.host.addController(this);
+    if (typeof this.#options?.getItems === 'function') {
+      this.initItems(this.#options.getItems(), this.#options.getItemContainer?.());
+    }
   }
 
   #nextMatchingItem(key: string) {
@@ -157,7 +175,7 @@ export class RovingTabindexController<
       return;
     }
 
-    const orientation = this.#getElement()?.getAttribute('aria-orientation');
+    const orientation = this.#options.getElement()?.getAttribute('aria-orientation');
 
     const item = this.activeItem;
     let shouldPreventDefault = false;
@@ -252,8 +270,8 @@ export class RovingTabindexController<
   /**
    * Focuses next focusable item
    */
-  updateItems(items: Item[]) {
-    const hasActive = document.activeElement && this.#getElement()?.contains(document.activeElement);
+  updateItems(items: Item[] = this.#options.getItems?.() ?? []) {
+    const hasActive = document.activeElement && this.#options.getElement()?.contains(document.activeElement);
     const sequence = [...items.slice(this.#itemIndex - 1), ...items.slice(0, this.#itemIndex - 1)];
     const first = sequence.find(item => this.#focusableItems.includes(item));
     this.#items = items ?? [];
@@ -268,6 +286,7 @@ export class RovingTabindexController<
 
   /**
    * from array of HTML items, and sets active items
+   * @deprecated: use getItems and getItemContainer option functions
    */
   initItems(items: Item[], itemsContainer?: Element) {
     this.#items = items ?? [];
@@ -276,12 +295,12 @@ export class RovingTabindexController<
     this.#activeItem = focusableItem;
     this.#updateTabindex();
 
-    itemsContainer ??= this.#getElement() ?? undefined;
+    itemsContainer ??= this.#options.getElement() ?? undefined;
 
     /**
      * removes listener on previous contained and applies it to new container
      */
-    if (!this.#itemsContainer || itemsContainer !== this.#itemsContainer) {
+    if (itemsContainer !== this.#itemsContainer) {
       this.#itemsContainer?.removeEventListener('keydown', this.#onKeydown);
       this.#itemsContainer = itemsContainer;
       this.hostConnected();

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

@@ -157,7 +157,9 @@ export class TabsController implements ReactiveController {
     this.#logger = new Logger(host);
     if (host instanceof HTMLElement) {
       this.#element = host;
-      this.#tabindex = new RovingTabindexController(host);
+      this.#tabindex = new RovingTabindexController(host, {
+        getItems: () => this._tabs,
+      });
     } else {
       const element = options.getHTMLElement?.();
       if (!element) {
@@ -223,8 +225,7 @@ export class TabsController implements ReactiveController {
 
     if (this._tabs.length > 0) {
       this.#updateAccessibility();
-      // TODO(bennypowers): adjust to fit, in or after #2570
-      this.#tabindex.initItems(this._tabs, this.#element);
+      this.#tabindex.updateItems();
       this.#setActiveTab();
     }
 

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

@@ -419,7 +419,6 @@ export class ToggleController implements ReactiveController {
       if (focus) {
         this.#popupElement?.focus();
       }
-      this.#getToggleElement()?.dispatchEvent(new CustomEvent('open'));
     }
   }
 
@@ -451,7 +450,6 @@ export class ToggleController implements ReactiveController {
       if (focused) {
         (el as HTMLElement)?.focus();
       }
-      el?.dispatchEvent(new CustomEvent('close'));
     }
   }
 }

elements/pf-accordion/BaseAccordion.ts

@@ -46,7 +46,9 @@ export abstract class BaseAccordion extends LitElement {
     return event instanceof AccordionHeaderChangeEvent;
   }
 
-  #headerIndex = new RovingTabindexController<BaseAccordionHeader>(this);
+  #headerIndex = new RovingTabindexController<BaseAccordionHeader>(this, {
+    getItems: () => this.headers,
+  });
 
   #expandedIndex: number[] = [];
 
@@ -146,14 +148,13 @@ export abstract class BaseAccordion extends LitElement {
    */
   async #init() {
     this.#initialized ||= !!await this.updateComplete;
-    this.#headerIndex.initItems(this.headers);
     // Event listener to the accordion header after the accordion has been initialized to add the roving tabindex
     this.addEventListener('focusin', this.#updateActiveHeader);
     this.updateAccessibility();
   }
 
   #updateActiveHeader() {
-    if (this.#activeHeader) {
+    if (this.#activeHeader !== this.#headerIndex.activeItem) {
       this.#headerIndex.updateActiveItem(this.#activeHeader);
     }
   }
@@ -231,6 +232,7 @@ export abstract class BaseAccordion extends LitElement {
   }
 
   public updateAccessibility() {
+    this.#headerIndex.updateItems();
     const { headers } = this;
 
     // For each header in the accordion, attach the aria connections

elements/pf-chip/pf-chip-group.ts

@@ -93,9 +93,9 @@ export class PfChipGroup extends LitElement {
 
   #buttons: HTMLElement[] = [];
 
-  #itemsInit = false;
-
-  #tabindex = new RovingTabindexController(this);
+  #tabindex = new RovingTabindexController(this, {
+    getItems: () => this.#buttons,
+  });
 
   constructor() {
     super();
@@ -188,13 +188,8 @@ export class PfChipGroup extends LitElement {
       ].filter((x): x is PfChip => !!x);
       if (oldButtons.length !== this.#buttons.length ||
           !oldButtons.every((element, index) => element === this.#buttons[index])) {
-        if (this.#itemsInit) {
-          this.#tabindex.updateItems(this.#buttons);
-        } else {
-          this.#tabindex.initItems(this.#buttons);
-        }
+        this.#tabindex.updateItems();
       }
-      this.#itemsInit = true;
       this.#updateOverflow();
     }
   }

elements/pf-dropdown/pf-dropdown-menu.ts

@@ -37,9 +37,9 @@ export class PfDropdownMenu extends LitElement {
     'click': this.#onMenuitemClick.bind(this),
   };
 
-  #itemsInit = false;
-
-  #tabindex = new RovingTabindexController(this);
+  #tabindex = new RovingTabindexController(this, {
+    getItems: () => this.#menuitems,
+  });
 
   #internals = InternalsController.of(this, { role: 'menu' });
 
@@ -108,15 +108,15 @@ export class PfDropdownMenu extends LitElement {
    */
   #handleItemChange(event: Event) {
     if (event instanceof DropdownItemChange) {
-      this.#updateItems();
+      this.#tabindex.updateItems();
     }
   }
 
   /**
    * handles slot change event
    */
   #handleSlotChange() {
-    this.#updateItems();
+    this.#tabindex.updateItems();
   }
 
   #nextMatchingItem(key: string) {
@@ -180,18 +180,6 @@ export class PfDropdownMenu extends LitElement {
       }
     }
   }
-
-  /**
-   * updates menu items list
-   */
-  #updateItems() {
-    if (this.#itemsInit) {
-      this.#tabindex.updateItems(this.#menuitems);
-    } else {
-      this.#tabindex.initItems(this.#menuitems);
-    }
-    this.#itemsInit = true;
-  }
 }
 
 declare global {

elements/pf-jump-links/pf-jump-links.ts

@@ -83,7 +83,9 @@ export class PfJumpLinks extends LitElement {
 
   #initialized = false;
 
-  #rovingTabindexController = new RovingTabindexController(this);
+  #rovingTabindexController = new RovingTabindexController<HTMLAnchorElement>(this, {
+    getItems: () => this.#getItems(),
+  });
 
   #spy = new ScrollSpyController(this, {
     rootMargin: `${this.offset}px 0px 0px 0px`,
@@ -121,18 +123,16 @@ export class PfJumpLinks extends LitElement {
     `;
   }
 
-  #updateItems() {
-    const items = Array.from(this.querySelectorAll(':is(pf-jump-links-item, pf-jump-links-list)'))
+  #getItems() {
+    return Array.from(this.querySelectorAll(':is(pf-jump-links-item, pf-jump-links-list)'))
       .flatMap(i => [
         ...i.shadowRoot?.querySelectorAll('a') ?? [],
         ...i.querySelectorAll('a') ?? [],
       ]);
-    if (this.#initialized) {
-      this.#rovingTabindexController.updateItems(items);
-    } else {
-      this.#rovingTabindexController.initItems(items);
-      this.#initialized = true;
-    }
+  }
+
+  #updateItems() {
+    this.#rovingTabindexController.updateItems();
   }
 
   async #onSelect(event: Event) {

elements/pf-select/pf-select.ts

@@ -147,6 +147,9 @@ export class PfSelect extends LitElement {
     kind: 'listbox',
     onChange: expanded => {
       this.dispatchEvent(new Event(expanded ? 'open' : 'close'));
+      if (expanded) {
+        this.querySelector<PfOption>('pf-option[tabindex="0"]')?.focus();
+      }
     }
   });