fix(jump-links): keyboard nav for jump-links list (#2413)

* fix(jump-links): keyboard nav for jump-links list

* refactor: use slotchange instead of custom event

* perf: avoid using decorators

* docs: add changeset

* perf: remove query decorator

* fix: prevent automatic focus

* test(jump-links): refactor test

---------

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

Author: nikkimk

.changeset/jump-links-arrow-nav.md

@@ -0,0 +1,4 @@
+---
+"@patternfly/elements": patch
+---
+`<pf-jump-links>`: improved accessibility for keyboard users

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

@@ -1,7 +1,6 @@
 import { html, LitElement } from 'lit';
 import { customElement } from 'lit/decorators/custom-element.js';
 import { property } from 'lit/decorators/property.js';
-import { query } from 'lit/decorators/query.js';
 
 import { ifDefined } from 'lit/directives/if-defined.js';
 
@@ -23,10 +22,13 @@ import { observed } from '@patternfly/pfe-core/decorators/observed.js';
 export class PfJumpLinksItem extends LitElement {
   static readonly styles = [style];
 
-  @query('a') link!:HTMLAnchorElement;
+  static readonly shadowRootOptions: ShadowRootInit = { ...LitElement.shadowRootOptions, delegatesFocus: true };
+
+  /** Whether this item is active. */
   @observed('activeChanged')
   @property({ type: Boolean, reflect: true }) active = false;
 
+  /** hypertext reference for this link */
   @property({ reflect: true }) href?: string;
 
   #internals = new InternalsController(this, {
@@ -47,10 +49,6 @@ export class PfJumpLinksItem extends LitElement {
     `;
   }
 
-  focus(): void {
-    this.link.focus();
-  }
-
   private activeChanged() {
     this.#internals.ariaCurrent = this.active ? 'location' : null;
   }

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

@@ -1,16 +1,15 @@
 import { html, LitElement } from 'lit';
 import { customElement } from 'lit/decorators/custom-element.js';
 import { property } from 'lit/decorators/property.js';
-import { queryAssignedElements } from 'lit/decorators/query-assigned-elements.js';
 
 import { ScrollSpyController } from '@patternfly/pfe-core/controllers/scroll-spy-controller.js';
 import { RovingTabindexController } from '@patternfly/pfe-core/controllers/roving-tabindex-controller.js';
 
-import './pf-jump-links-item.js';
 import '@patternfly/elements/pf-icon/pf-icon.js';
 
+import './pf-jump-links-item.js';
+
 import style from './pf-jump-links.css';
-import { PfJumpLinksItem } from './pf-jump-links-item.js';
 
 /**
  * Jump links allow users to navigate to sections within a page.
@@ -64,22 +63,26 @@ import { PfJumpLinksItem } from './pf-jump-links-item.js';
 export class PfJumpLinks extends LitElement {
   static readonly styles = [style];
 
-  @queryAssignedElements() _items!:PfJumpLinksItem[];
-
+  /** Whether the element features a disclosure widget around the nav items */
   @property({ reflect: true, type: Boolean }) expandable = false;
 
+  /** Whether the expandable element's disclosure widget is expanded */
   @property({ reflect: true, type: Boolean }) expanded = false;
 
+  /** Whether the layout of children is vertical or horizontal. */
   @property({ reflect: true, type: Boolean }) vertical = false;
 
+  /** Whether to center children. */
   @property({ reflect: true, type: Boolean }) centered = false;
 
+  /** Offset to add to the scroll position, potentially for a masthead which content scrolls under. */
   @property({ type: Number }) offset = 0;
 
+  /** Label to add to nav element. */
   @property() label?: string;
 
-  #items!:PfJumpLinksItem[];
-  #init = false;
+  #initialized = false;
+
   #rovingTabindexController = new RovingTabindexController(this);
 
   #spy = new ScrollSpyController(this, {
@@ -106,22 +109,25 @@ export class PfJumpLinks extends LitElement {
             <pf-icon icon="chevron-right"></pf-icon>
             <span id="label">${this.label}</span>
           </summary>
-          <slot role="listbox" @slotchange="${this.#onSlotchange}"></slot>
+          <slot role="listbox" @slotchange="${this.#updateItems}"></slot>
         </details>` : html`
         <span id="label">${this.label}</span>
-        <slot role="listbox" @slotchange="${this.#onSlotchange}"></slot>`}
+        <slot role="listbox" @slotchange="${this.#updateItems}"></slot>`}
       </nav>
     `;
   }
 
-  #onSlotchange() {
-    this.#items = this._items;
-    const items = this.#items?.map(item=>item.link);
-    if (this.#init) {
+  #updateItems() {
+    const items = 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.#init = true;
+      this.#initialized = true;
     }
   }
 

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

@@ -1,5 +1,5 @@
 import type { ReactiveElement } from 'lit';
-import { expect, html, nextFrame } from '@open-wc/testing';
+import { expect, html, nextFrame, aTimeout } from '@open-wc/testing';
 import { createFixture } from '@patternfly/pfe-tools/test/create-fixture.js';
 import { sendKeys } from '@web/test-runner-commands';
 
@@ -22,29 +22,31 @@ async function allUpdates(element: ReactiveElement) {
 
 describe('<pf-jump-links>', function() {
   let element: PfJumpLinks;
+  let firstItem: PfJumpLinksItem;
+  let secondItem: PfJumpLinksItem;
+  let thirdItem: PfJumpLinksItem;
 
   beforeEach(async function() {
     element = await createFixture<PfJumpLinks>(html`
       <pf-jump-links>
         <pf-jump-links-item id="first">Inactive section</pf-jump-links-item>
-        <pf-jump-links-item id="second">Active section</pf-jump-links-item>
+        <pf-jump-links-item id="second" active>Active section</pf-jump-links-item>
         <pf-jump-links-item id="third">Inactive section</pf-jump-links-item>
       </pf-jump-links>
     `);
     await allUpdates(element);
+    [firstItem, secondItem, thirdItem] = element.querySelectorAll<PfJumpLinksItem>('pf-jump-links-item');
   });
 
   describe('tabbing to first item', function() {
-    let firstItem: PfJumpLinksItem;
-    let secondItem: PfJumpLinksItem;
     let initialActiveElement: Element|null;
     beforeEach(async function() {
-      [firstItem, secondItem] = element.querySelectorAll<PfJumpLinksItem>('pf-jump-links-item');
-      await sendKeys({ press: 'Tab' });
       initialActiveElement = document.activeElement;
+      await sendKeys({ press: 'Tab' });
+      await nextFrame();
     });
 
-    it('should focus a this first jump-links-item', function() {
+    it('should focus the first jump-links-item', function() {
       expect(document.activeElement).to.equal(firstItem);
     });