Skip to content

fix(core): improve ATFocus, Combobox, Internals, Listbox, and RovingTabIndex controllers #2963

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
adamjohnson wants to merge 8 commits into
base: main
Choose a base branch
from
+138 −42
Open

fix(core): improve ATFocus, Combobox, Internals, Listbox, and RovingTabIndex controllers #2963

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
aae6b06
fix(core): if `relatedTarget` is toggle, let `#onClickButton` manage …
adamjohnson Jan 20, 2026
66793e6
fix(core): ensure RTI syncs AT focus when using a combo of mouse and …
adamjohnson Jan 20, 2026
97e9288
fix(core): force to always search forward for Home and backward for …
adamjohnson Jan 20, 2026
4132c4c
fix(core): hide listbox on Shift+Tab when moving to the toggle button
adamjohnson Jan 21, 2026
ba6a0dc
feat(core): add optional `setItems` callback to listbox and combobox …
adamjohnson Feb 2, 2026
17e43f1
Revert "feat(core): add optional `setItems` callback to listbox and c…
adamjohnson Feb 3, 2026
cdf6ea1
feat(core): let the internals controller handle `aria-posinset` and `…
adamjohnson Feb 3, 2026
e594b00
fix(core): narrow host type for combobox, internals controllers
bennypowers Feb 4, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion core/pfe-core/controllers/at-focus-controller.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,8 +49,13 @@ 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)!);
let itemToGainFocus = items.at(index);
let itemToGainFocusIsFocusable = atFocusableItems.includes(itemToGainFocus!);
Expand Down
45 changes: 36 additions & 9 deletions core/pfe-core/controllers/combobox-controller.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -242,6 +242,7 @@ export class ComboboxController<
#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: {
Expand Down Expand Up @@ -326,7 +327,7 @@ export class ComboboxController<
}

private constructor(
public host: ReactiveControllerHost,
public host: ReactiveControllerHost & HTMLElement,
options: ComboboxControllerOptions<Item>,
) {
host.addController(this);
Expand Down Expand Up @@ -425,6 +426,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');
Expand All @@ -434,6 +437,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() {
Expand Down Expand Up @@ -531,26 +536,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;
Expand Down Expand Up @@ -580,6 +591,17 @@ export class ComboboxController<
}
};

/**
* Distinguish click-to-toggle vs Tab/Shift+Tab
Copy link
Member

@bennypowers bennypowers Feb 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice addition

*/
#onMousedownButton = () => {
this.#buttonHasMouseDown = true;
};

#onMouseupButton = () => {
this.#buttonHasMouseDown = false;
};

#onClickListbox = (event: MouseEvent) => {
if (!this.multi && event.composedPath().some(this.options.isItem)) {
this.#hide();
Expand Down Expand Up @@ -735,9 +757,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();
}
}
Expand Down
94 changes: 69 additions & 25 deletions core/pfe-core/controllers/internals-controller.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ import {
isServer,
type ReactiveController,
type ReactiveControllerHost,
type LitElement,
} from 'lit';

function isARIAMixinProp(key: string): key is keyof ARIAMixin {
Expand Down Expand Up @@ -59,17 +58,19 @@ 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');
return ariaLabel ?? label.textContent;
}
}

type InternalsHost = ReactiveControllerHost & HTMLElement;

export class InternalsController implements ReactiveController, ARIAMixin {
private static instances = new WeakMap<ReactiveControllerHost, InternalsController>();
private static instances = new WeakMap<HTMLElement, InternalsController>();

declare readonly form: ElementInternals['form'];
declare readonly shadowRoot: ElementInternals['shadowRoot'];
Expand All @@ -79,17 +80,70 @@ 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[];
}

/**
* Sets 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');
}
}

/**
* Sets 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');
}
}

/**
* Gets aria-posinset from a listbox item (internals or attribute).
* @param host - The listbox item element.
*/
public static getAriaPosInSet(host: HTMLElement): string | null {
const instance = this.instances.get(host);
return instance != null ?
instance.ariaPosInSet
: host.getAttribute('aria-posinset');
}

/**
* Gets aria-setsize from a listbox item (internals or attribute).
* @param host - The listbox item element.
*/
public static getAriaSetSize(host: HTMLElement): string | null {
const instance = this.instances.get(host);
return instance != null ?
instance.ariaSetSize
: host.getAttribute('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,
Expand Down Expand Up @@ -149,21 +203,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 */
Expand All @@ -186,16 +235,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<HTMLElement>)
|| 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?.();
}
Expand All @@ -205,10 +252,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()`');
}
Expand Down
19 changes: 12 additions & 7 deletions core/pfe-core/controllers/listbox-controller.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -192,16 +193,11 @@ export class ListboxController<Item extends HTMLElement> 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();
});
}

/**
Expand Down Expand Up @@ -268,6 +264,10 @@ export class ListboxController<Item extends HTMLElement> 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?.() ?? [];
Expand All @@ -278,6 +278,11 @@ export class ListboxController<Item extends HTMLElement> 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 {
Expand Down
15 changes: 15 additions & 0 deletions core/pfe-core/controllers/roving-tabindex-controller.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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');
}
Expand Down
Loading