fix(core): add requestUpdate to roving-tabindex-controller (#2446)

* fix(tools): add change callback to roving tab index controller

* fix(tabs): impleement RTI callback, remove keybindings

* chore: add changeset

* chore: update changeset

* refactor(tools): remove callback use requestUpdate instead

* fix(tabs): update rti active item on attribute change

* feat(core): make RovingTabIndex generic to type of item

* chore: update changeset

* refactor(tabs): remove type assertions

---------

Co-authored-by: Benny Powers <[email protected]>

Author: zeroedin

.changeset/fair-melons-cough.md

@@ -0,0 +1,5 @@
+---
+"@patternfly/elements": patch
+---
+
+`<pf-tabs>`: improved keyboard navigation so it correctly activates the focused tab

.changeset/rti-focus-update.md

@@ -0,0 +1,5 @@
+---
+"@patternfly/pfe-core": minor
+---
+
+`roving-tabindex-controller`: notify the host when the focused item changes.

.changeset/rti-generic.md

@@ -0,0 +1,4 @@
+---
+"@patternfly/pfe-core": minor
+---
+`roving-tabindex-controller`: allow component authors to specify the type of items.

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

@@ -11,20 +11,22 @@ const isFocusableElement = (el: Element): el is HTMLElement =>
  * Components Using a Roving
  * tabindex](https://www.w3.org/WAI/ARIA/apg/practices/keyboard-interface/#kbd_roving_tabindex)
  */
-export class RovingTabindexController implements ReactiveController {
+export class RovingTabindexController<
+  ItemType extends HTMLElement = HTMLElement,
+> implements ReactiveController {
   /** active focusable element */
-  #activeItem?: HTMLElement;
+  #activeItem?: ItemType;
 
   /** closest ancestor containing items */
   #itemsContainer?: HTMLElement;
 
   /** array of all focusable elements */
-  #items: HTMLElement[] = [];
+  #items: ItemType[] = [];
 
   /**
    * finds focusable items from a group of items
    */
-  get #focusableItems(): HTMLElement[] {
+  get #focusableItems(): ItemType[] {
     return this.#items.filter(isFocusableElement);
   }
 
@@ -45,38 +47,38 @@ export class RovingTabindexController implements ReactiveController {
   /**
    * active item of array of items
    */
-  get activeItem(): HTMLElement | undefined {
+  get activeItem(): ItemType | undefined {
     return this.#activeItem;
   }
 
   /**
    * first item in array of focusable items
    */
-  get firstItem(): HTMLElement | undefined {
+  get firstItem(): ItemType | undefined {
     return this.#focusableItems[0];
   }
 
   /**
    * last item in array of focusable items
    */
-  get lastItem(): HTMLElement | undefined {
+  get lastItem(): ItemType | undefined {
     return this.#focusableItems.at(-1);
   }
 
   /**
    * next item  after active item in array of focusable items
    */
-  get nextItem(): HTMLElement | undefined {
+  get nextItem(): ItemType | undefined {
     return (
-        this.#activeIndex < this.#focusableItems.length - 1 ? this.#focusableItems[this.#activeIndex + 1]
-      : this.firstItem
+        this.#activeIndex >= this.#focusableItems.length - 1 ? this.firstItem
+      : this.#focusableItems[this.#activeIndex + 1]
     );
   }
 
   /**
    * previous item  after active item in array of focusable items
    */
-  get prevItem(): HTMLElement | undefined {
+  get prevItem(): ItemType | undefined {
     return (
         this.#activeIndex > 0 ? this.#focusableItems[this.#activeIndex - 1]
       : this.lastItem
@@ -161,7 +163,7 @@ export class RovingTabindexController implements ReactiveController {
   /**
    * sets tabindex of item based on whether or not it is active
    */
-  updateActiveItem(item?: HTMLElement): void {
+  updateActiveItem(item?: ItemType): void {
     if (item) {
       if (!!this.#activeItem && item !== this.#activeItem) {
         this.#activeItem.tabIndex = -1;
@@ -174,15 +176,16 @@ export class RovingTabindexController implements ReactiveController {
   /**
    * focuses on an item and sets it as active
    */
-  focusOnItem(item?: HTMLElement): void {
+  focusOnItem(item?: ItemType): void {
     this.updateActiveItem(item || this.firstItem);
     this.#activeItem?.focus();
+    this.host.requestUpdate();
   }
 
   /**
    * Focuses next focusable item
    */
-  updateItems(items: HTMLElement[]) {
+  updateItems(items: ItemType[]) {
     const sequence = [...items.slice(this.#itemIndex), ...items.slice(0, this.#itemIndex)];
     const first = sequence.find(item => this.#focusableItems.includes(item));
     this.focusOnItem(first || this.firstItem);
@@ -191,7 +194,7 @@ export class RovingTabindexController implements ReactiveController {
   /**
    * from array of HTML items, and sets active items
    */
-  initItems(items: HTMLElement[], itemsContainer: HTMLElement = this.host) {
+  initItems(items: ItemType[], itemsContainer: HTMLElement = this.host) {
     this.#items = items ?? [];
     const focusableItems = this.#focusableItems;
     const [focusableItem] = focusableItems;
@@ -210,14 +213,14 @@ export class RovingTabindexController implements ReactiveController {
   }
 
   /**
-   * adds event listners to items container
+   * adds event listeners to items container
    */
   hostConnected() {
     this.#itemsContainer?.addEventListener('keydown', this.#onKeydown);
   }
 
   /**
-   * removes event listners from items container
+   * removes event listeners from items container
    */
   hostDisconnected() {
     this.#itemsContainer?.removeEventListener('keydown', this.#onKeydown);

elements/pf-tabs/BaseTabs.ts

@@ -61,7 +61,7 @@ export abstract class BaseTabs extends LitElement {
 
   @query('[part="tabs"]') private tabList!: HTMLElement;
 
-  #tabindex = new RovingTabindexController(this);
+  #tabindex = new RovingTabindexController<BaseTab>(this);
 
   #overflow = new OverflowController(this);
 
@@ -104,7 +104,7 @@ export abstract class BaseTabs extends LitElement {
 
     if (index === -1) {
       this.#logger.warn(`No active tab found, setting first focusable tab to active`);
-      const first = this.#tabindex.firstItem as BaseTab;
+      const first = this.#tabindex.firstItem;
       this.#tabindex.updateActiveItem(first);
       index = this.#activeItemIndex;
     }
@@ -140,7 +140,6 @@ export abstract class BaseTabs extends LitElement {
   override connectedCallback() {
     super.connectedCallback();
     this.addEventListener('expand', this.#onTabExpand);
-    this.addEventListener('keydown', this.#onKeydown);
     BaseTabs.#instances.add(this);
   }
 
@@ -149,6 +148,17 @@ export abstract class BaseTabs extends LitElement {
     BaseTabs.#instances.delete(this);
   }
 
+  override willUpdate(): void {
+    const { activeItem } = this.#tabindex;
+    // If RTI has an activeItem, update the roving tabindex controller
+    if (!this.manual &&
+        activeItem &&
+        activeItem !== this.#activeTab &&
+        activeItem.ariaDisabled !== 'true') {
+      activeItem.active = true;
+    }
+  }
+
   async firstUpdated() {
     this.tabList.addEventListener('scroll', this.#overflow.onScroll.bind(this));
   }
@@ -210,13 +220,14 @@ export abstract class BaseTabs extends LitElement {
 
   #onTabExpand = (event: Event): void => {
     if (!(event instanceof TabExpandEvent) ||
-        this.#allTabs.length === 0 || this.#allPanels.length === 0) {
+        !this.#allTabs.length ||
+        !this.#allPanels.length) {
       return;
     }
 
-    const target = event as TabExpandEvent;
-    if (target.active) {
-      this.activeIndex = this.#allTabs.findIndex(tab => tab === target.tab);
+    if (event.active) {
+      this.activeIndex = this.#allTabs.findIndex(tab => tab === event.tab);
+      this.#tabindex.updateActiveItem(this.#activeTab);
     }
   };
 
@@ -225,77 +236,27 @@ export abstract class BaseTabs extends LitElement {
     this.#allPanels.forEach((panel, i) => panel.hidden = i !== index);
   }
 
-  get #firstFocusable(): BaseTab {
-    return this.#tabindex.firstItem as BaseTab;
+  get #firstFocusable(): BaseTab | undefined {
+    return this.#tabindex.firstItem;
   }
 
-  get #lastFocusable(): BaseTab {
-    return this.#tabindex.lastItem as BaseTab;
-  }
-
-  get #firstTab(): BaseTab {
+  get #firstTab(): BaseTab | undefined {
     const [tab] = this.#allTabs;
     return tab;
   }
 
-  get #lastTab(): BaseTab {
-    return this.#allTabs.at(-1) as BaseTab;
+  get #lastTab(): BaseTab | undefined {
+    return this.#allTabs.at(-1);
   }
 
   get #activeItemIndex() {
     const { activeItem } = this.#tabindex;
     return this.#allTabs.findIndex(t => t === activeItem);
   }
 
-  #activate(selectedTab: BaseTab): void {
-    if (selectedTab.ariaDisabled !== 'true') {
-      selectedTab.active = true;
-    }
-  }
-
-  async #select(selectedTab: BaseTab): Promise<void> {
-    if (!this.manual) {
-      this.#activate(selectedTab);
-    }
-  }
-
-  // RTI: will handle key events
-  #onKeydown = (event: KeyboardEvent): void => {
-    const foundTab = this.#allTabs.find(tab => tab === event.target);
-    if (!foundTab) {
-      return;
-    }
-    switch (event.key) {
-      case 'ArrowUp':
-      case 'ArrowLeft':
-        event.preventDefault();
-        this.#select(this.#tabindex.activeItem as BaseTab);
-        break;
-
-      case 'ArrowDown':
-      case 'ArrowRight':
-        event.preventDefault();
-        this.#select(this.#tabindex.activeItem as BaseTab);
-        break;
-
-      case 'Home':
-        event.preventDefault();
-        this.#select(this.#firstFocusable);
-        break;
-
-      case 'End':
-        event.preventDefault();
-        this.#select(this.#lastFocusable);
-        break;
-
-      default:
-        return;
-    }
-  };
-
   #firstLastClasses() {
-    this.#firstTab.classList.add('first');
-    this.#lastTab.classList.add('last');
+    this.#firstTab?.classList.add('first');
+    this.#lastTab?.classList.add('last');
   }
 
   #scrollLeft() {