diff --git a/core/pfe-core/controllers/activedescendant-controller.ts b/core/pfe-core/controllers/activedescendant-controller.ts index a5d1e9a47b..e6cb595289 100644 --- a/core/pfe-core/controllers/activedescendant-controller.ts +++ b/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, ) { super(host, options); + this.initItems(); this.options.getItemValue ??= function(this: Item) { return (this as unknown as HTMLOptionElement).value; }; diff --git a/core/pfe-core/controllers/at-focus-controller.ts b/core/pfe-core/controllers/at-focus-controller.ts index c8d099df06..519dd2a4ba 100644 --- a/core/pfe-core/controllers/at-focus-controller.ts +++ b/core/pfe-core/controllers/at-focus-controller.ts @@ -49,8 +49,13 @@ export abstract class ATFocusController { 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)!); let itemToGainFocus = items.at(index); let itemToGainFocusIsFocusable = atFocusableItems.includes(itemToGainFocus!); @@ -113,6 +118,14 @@ export abstract class ATFocusController { 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(); } diff --git a/core/pfe-core/controllers/combobox-controller.ts b/core/pfe-core/controllers/combobox-controller.ts index c69e129ea5..00523a0cc9 100644 --- a/core/pfe-core/controllers/combobox-controller.ts +++ b/core/pfe-core/controllers/combobox-controller.ts @@ -159,7 +159,7 @@ export class ComboboxController< Item extends HTMLElement > implements ReactiveController { public static of( - host: ReactiveControllerHost, + host: ReactiveControllerHost & HTMLElement, options: ComboboxControllerOptions, ): ComboboxController { return new ComboboxController(host, options); @@ -237,11 +237,13 @@ export class ComboboxController< #lb: ListboxController; #fc?: ATFocusController; + #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>(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, ) { 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 { + private async _onFocusoutElement(): Promise { if (this.#hasTextInput && this.options.isExpanded()) { const root = this.#element?.getRootNode(); await new Promise(requestAnimationFrame); @@ -397,6 +400,7 @@ export class ComboboxController< * Order of operations is important */ async #init() { + this.#initializing = true; await this.host.updateComplete; this.#initListbox(); this.#initItems(); @@ -404,6 +408,7 @@ export class ComboboxController< 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() { @@ -531,26 +540,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 +595,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 +761,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(); } } diff --git a/core/pfe-core/controllers/internals-controller.ts b/core/pfe-core/controllers/internals-controller.ts index de9684c366..84b8c7a6c3 100644 --- a/core/pfe-core/controllers/internals-controller.ts +++ b/core/pfe-core/controllers/internals-controller.ts @@ -2,7 +2,6 @@ import { isServer, type ReactiveController, type ReactiveControllerHost, - type LitElement, } from 'lit'; function isARIAMixinProp(key: string): key is keyof ARIAMixin { @@ -59,8 +58,8 @@ function aria( protos.get(target).add(key); } -function getLabelText(label: HTMLElement) { - if (label.hidden) { +function getLabelText(label: Node) { + if (!(label instanceof HTMLElement) || label.hidden) { return ''; } else { const ariaLabel = label.getAttribute?.('aria-label'); @@ -68,8 +67,10 @@ function getLabelText(label: HTMLElement) { } } +type InternalsHost = ReactiveControllerHost & HTMLElement; + export class InternalsController implements ReactiveController, ARIAMixin { - private static instances = new WeakMap(); + private static instances = new WeakMap(); declare readonly form: ElementInternals['form']; declare readonly shadowRoot: ElementInternals['shadowRoot']; @@ -79,17 +80,68 @@ export class InternalsController implements ReactiveController, ARIAMixin { declare readonly willValidate: ElementInternals['willValidate']; declare readonly validationMessage: ElementInternals['validationMessage']; - public static getLabels(host: ReactiveControllerHost): Element[] { + public static getLabels(host: InternalsHost): Element[] { return Array.from(this.instances.get(host)?.internals.labels ?? []) as Element[]; } + /** + * Gets the ARIA posinset value from a listbox item (attribute takes precedence over internals). + * @param host - The listbox item element. + */ + public static getAriaPosInSet(host: HTMLElement): string | null { + return host.getAttribute('aria-posinset') + ?? this.instances.get(host)?.ariaPosInSet + ?? null; + } + + /** + * Sets the ARIA posinset on a listbox item. Uses ElementInternals when the host has + * an InternalsController instance; otherwise sets/removes the host attribute. + * @param host - The listbox item element (option or option-like). + * @param value - Position in set (1-based), or null to clear. + */ + public static setAriaPosInSet(host: HTMLElement, value: number | string | null): void { + const instance = this.instances.get(host); + if (instance) { + instance.ariaPosInSet = value != null ? String(value) : null; + } else if (value != null) { + host.setAttribute('aria-posinset', String(value)); + } else { + host.removeAttribute('aria-posinset'); + } + } + + /** + * Gets the ARIA setsize from a listbox item (aria attribute if set or defaulting to internals). + * @param host - The listbox item element. + */ + public static getAriaSetSize(host: HTMLElement): string | null { + return host.getAttribute('aria-setsize') + ?? this.instances.get(host)?.ariaSetSize + ?? null; + } + + /** + * Sets the ARIA setsize on a listbox item. Uses ElementInternals when the host has + * an InternalsController instance; otherwise sets/removes the host attribute. + * @param host - The listbox item element (option or option-like). + * @param value - Total set size, or null to clear. + */ + public static setAriaSetSize(host: HTMLElement, value: number | string | null): void { + const instance = this.instances.get(host); + if (instance) { + instance.ariaSetSize = value != null ? String(value) : null; + } else if (value != null) { + host.setAttribute('aria-setsize', String(value)); + } else { + host.removeAttribute('aria-setsize'); + } + } + public static isSafari: boolean = !isServer && /^((?!chrome|android).)*safari/i.test(navigator.userAgent); - public static of( - host: ReactiveControllerHost, - options?: InternalsControllerOptions, - ): InternalsController { + public static of(host: InternalsHost, options?: InternalsControllerOptions): InternalsController { constructingAllowed = true; // implement the singleton pattern // using a public static constructor method is much easier to manage, @@ -149,21 +201,16 @@ export class InternalsController implements ReactiveController, ARIAMixin { @aria ariaValueNow: string | null = null; @aria ariaValueText: string | null = null; - /** WARNING: be careful of cross-root ARIA browser support */ + /** As of April 2025, the following are considered Baseline supported in evergreen browsers */ @aria ariaActiveDescendantElement: Element | null = null; - /** WARNING: be careful of cross-root ARIA browser support */ @aria ariaControlsElements: Element[] | null = null; - /** WARNING: be careful of cross-root ARIA browser support */ @aria ariaDescribedByElements: Element[] | null = null; - /** WARNING: be careful of cross-root ARIA browser support */ @aria ariaDetailsElements: Element[] | null = null; - /** WARNING: be careful of cross-root ARIA browser support */ @aria ariaErrorMessageElements: Element[] | null = null; - /** WARNING: be careful of cross-root ARIA browser support */ @aria ariaFlowToElements: Element[] | null = null; - /** WARNING: be careful of cross-root ARIA browser support */ @aria ariaLabelledByElements: Element[] | null = null; - /** WARNING: be careful of cross-root ARIA browser support */ + + /** As of February 2026, this is not supported in Chromium browsers */ @aria ariaOwnsElements: Element[] | null = null; /** True when the control is disabled via it's containing fieldset element */ @@ -186,16 +233,14 @@ export class InternalsController implements ReactiveController, ARIAMixin { /** A best-attempt based on observed behaviour in FireFox 115 on fedora 38 */ get computedLabelText(): string { return this.internals.ariaLabel - || Array.from(this.internals.labels as NodeListOf) + || Array.from(this.internals.labels) .reduce((acc, label) => `${acc}${getLabelText(label)}`, ''); } private get element() { if (isServer) { - // FIXME(bennyp): a little white lie, which may break - // when the controller is applied to non-lit frameworks. - return this.host as LitElement; + return this.host; } else { return this.host instanceof HTMLElement ? this.host : this.options?.getHTMLElement?.(); } @@ -205,10 +250,7 @@ export class InternalsController implements ReactiveController, ARIAMixin { private _formDisabled = false; - private constructor( - public host: ReactiveControllerHost, - private options?: InternalsControllerOptions, - ) { + private constructor(public host: InternalsHost, private options?: InternalsControllerOptions) { if (!constructingAllowed) { throw new Error('InternalsController must be constructed with `InternalsController.for()`'); } diff --git a/core/pfe-core/controllers/listbox-controller.ts b/core/pfe-core/controllers/listbox-controller.ts index 22c4123c05..491d77dab0 100644 --- a/core/pfe-core/controllers/listbox-controller.ts +++ b/core/pfe-core/controllers/listbox-controller.ts @@ -3,6 +3,7 @@ import type { RequireProps } from '../core.ts'; import { isServer } from 'lit'; import { arraysAreEquivalent } from '../functions/arraysAreEquivalent.js'; +import { InternalsController } from './internals-controller.js'; /** * Options for listbox controller @@ -192,16 +193,14 @@ export class ListboxController implements ReactiveCont } /** - * register's the host's Item elements as listbox controller items - * sets aria-setsize and aria-posinset on items - * @param items items + * Registers the host's item elements as listbox controller items. + * @param items - Array of listbox option elements. */ set items(items: Item[]) { - this.#items = items; - this.#items.forEach((item, index, _items) => { - item.ariaSetSize = _items.length.toString(); - item.ariaPosInSet = (index + 1).toString(); - }); + if (!arraysAreEquivalent(items, this.#items)) { + this.#items = items; + this.host.requestUpdate(); + } } /** @@ -268,6 +267,10 @@ export class ListboxController implements ReactiveCont } } + /** + * Called during host update; syncs control element listeners and + * applies aria-posinset/aria-setsize to each item via InternalsController. + */ hostUpdate(): void { const last = this.#controlsElements; this.#controlsElements = this.#options.getControlsElements?.() ?? []; @@ -278,6 +281,11 @@ export class ListboxController implements ReactiveCont el.addEventListener('keyup', this.#onKeyup); } } + const items = this.#items; + items.forEach((item, index) => { + InternalsController.setAriaPosInSet(item, index + 1); + InternalsController.setAriaSetSize(item, items.length); + }); } hostUpdated(): void { @@ -379,12 +387,15 @@ export class ListboxController implements ReactiveCont if (this.items.includes(shadowRootItem)) { return shadowRootItem; } else { - const index = - Array.from(shadowRootListboxElement?.children ?? []) - .filter(this.#options.isItem) - .filter(x => !x.hidden) - .indexOf(shadowRootItem); - return this.#items.filter(x => !x.hidden)[index]; + // Shadow clone needs to be mapped back to light DOM item. + // Match by value attribute or text content since index-based matching + // doesn't work when items are filtered (hidden state differs between clone and source) + const cloneValue = shadowRootItem.getAttribute('value') + ?? shadowRootItem.textContent?.trim(); + const sourceItem = this.#items.find(item => + (item.getAttribute('value') ?? item.textContent?.trim()) === cloneValue + ); + return sourceItem ?? null; } } diff --git a/core/pfe-core/controllers/roving-tabindex-controller.ts b/core/pfe-core/controllers/roving-tabindex-controller.ts index 9659a11095..6189ac689b 100644 --- a/core/pfe-core/controllers/roving-tabindex-controller.ts +++ b/core/pfe-core/controllers/roving-tabindex-controller.ts @@ -77,6 +77,21 @@ export class RovingTabindexController< if (container instanceof HTMLElement) { container.addEventListener('focusin', () => this.#gainedInitialFocus = true, { once: true }); + // Sync atFocusedItemIndex when an item receives DOM focus (e.g., via mouse click) + // This ensures keyboard navigation starts from the correct position + container.addEventListener('focusin', (event: FocusEvent) => { + const target = event.target as Item; + const index = this.items.indexOf(target); + // Only update if the target is a valid item and index differs + if (index >= 0 && index !== this.atFocusedItemIndex) { + // Update index via setter, but avoid the focus() call by temporarily + // clearing #gainedInitialFocus to prevent redundant focus + const hadInitialFocus = this.#gainedInitialFocus; + this.#gainedInitialFocus = false; + this.atFocusedItemIndex = index; + this.#gainedInitialFocus = hadInitialFocus; + } + }); } else { this.#logger.warn('RovingTabindexController requires a getItemsContainer function'); } diff --git a/core/pfe-core/functions/arraysAreEquivalent.ts b/core/pfe-core/functions/arraysAreEquivalent.ts index 475520cd1e..a7d4c8b57f 100644 --- a/core/pfe-core/functions/arraysAreEquivalent.ts +++ b/core/pfe-core/functions/arraysAreEquivalent.ts @@ -7,18 +7,13 @@ * @param b second array */ export function arraysAreEquivalent(a: unknown, b: unknown): boolean { - if (!Array.isArray(a) || !Array.isArray(b)) { + if (!Array.isArray(a) || !Array.isArray(b)) { // one or both are not an array return a === b; } else if (a.length !== b.length) { // lengths are different return false; } else if (!a.length && !b.length) { // both are empty return true; } else { // multi and length of both is equal - for (const [i, element] of a.entries()) { - if (element !== b[i]) { - return false; - } - } - return true; + return a.every((v, i) => b[i] === v); } }