Skip to content

Commit 8f5a26e

Browse files
adamjohnsonadamjohnson
authored
Merge 75aecb2 into f43c1bc 
2 parents f43c1bc + 75aecb2 commit 8f5a26e

File tree

7 files changed

+197
-59
lines changed

7 files changed

+197
-59
lines changed

‎core/pfe-core/controllers/activedescendant-controller.ts‎

Lines changed: 32 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -134,7 +134,10 @@ export class ActivedescendantController<
134134
super.atFocusedItemIndex = index;
135135
const item = this._items.at(this.atFocusedItemIndex);
136136
for (const _item of this.items) {
137-
this.options.setItemActive?.(_item, _item === item);
137+
const isActive = _item === item;
138+
// Map clone back to original item for setItemActive callback
139+
const originalItem = this.#shadowToLightMap.get(_item) ?? _item;
140+
this.options.setItemActive?.(originalItem, isActive);
138141
}
139142
const container = this.options.getActiveDescendantContainer();
140143
if (!ActivedescendantController.supportsCrossRootActiveDescendant) {
@@ -150,6 +153,12 @@ export class ActivedescendantController<
150153
}
151154

152155
protected set controlsElements(elements: HTMLElement[]) {
156+
// Avoid removing/re-adding listeners if elements haven't changed
157+
// This prevents breaking event listeners during active event dispatch
158+
if (elements.length === this.#controlsElements.length
159+
&& elements.every((el, i) => el === this.#controlsElements[i])) {
160+
return;
161+
}
153162
for (const old of this.#controlsElements) {
154163
old?.removeEventListener('keydown', this.onKeydown);
155164
}
@@ -159,6 +168,22 @@ export class ActivedescendantController<
159168
}
160169
}
161170

171+
/**
172+
* Check the source item's focusable state, not the clone's.
173+
* This is needed because filtering sets `hidden` on the light DOM item,
174+
* and the MutationObserver sync to clones is asynchronous.
175+
*/
176+
override get atFocusableItems(): Item[] {
177+
return this._items.filter(item => {
178+
// Map clone to source item to check actual hidden state
179+
const sourceItem = this.#shadowToLightMap.get(item) ?? item;
180+
return !!sourceItem
181+
&& sourceItem.ariaHidden !== 'true'
182+
&& !sourceItem.hasAttribute('inert')
183+
&& !sourceItem.hasAttribute('hidden');
184+
});
185+
}
186+
162187
/** All items */
163188
get items() {
164189
return this._items;
@@ -195,6 +220,11 @@ export class ActivedescendantController<
195220
this.#shadowToLightMap.set(item, item);
196221
return item;
197222
} else {
223+
// Reuse existing clone if available to maintain stable IDs
224+
const existingClone = this.#lightToShadowMap.get(item);
225+
if (existingClone) {
226+
return existingClone;
227+
}
198228
const clone = item.cloneNode(true) as Item;
199229
clone.id = getRandomId();
200230
this.#lightToShadowMap.set(item, clone);
@@ -214,6 +244,7 @@ export class ActivedescendantController<
214244
protected options: ActivedescendantControllerOptions<Item>,
215245
) {
216246
super(host, options);
247+
this.initItems();
217248
this.options.getItemValue ??= function(this: Item) {
218249
return (this as unknown as HTMLOptionElement).value;
219250
};

‎core/pfe-core/controllers/at-focus-controller.ts‎

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -49,8 +49,13 @@ export abstract class ATFocusController<Item extends HTMLElement> {
4949

5050
set atFocusedItemIndex(index: number) {
5151
const previousIndex = this.#atFocusedItemIndex;
52-
const direction = index > previousIndex ? 1 : -1;
5352
const { items, atFocusableItems } = this;
53+
// - Home (index=0): always search forward to find first focusable item
54+
// - End (index=last): always search backward to find last focusable item
55+
// - Other cases: use comparison to determine direction
56+
const direction = index === 0 ? 1
57+
: index >= items.length - 1 ? -1
58+
: index > previousIndex ? 1 : -1;
5459
const itemsIndexOfLastATFocusableItem = items.indexOf(this.atFocusableItems.at(-1)!);
5560
let itemToGainFocus = items.at(index);
5661
let itemToGainFocusIsFocusable = atFocusableItems.includes(itemToGainFocus!);
@@ -113,6 +118,14 @@ export abstract class ATFocusController<Item extends HTMLElement> {
113118
this.itemsContainerElement ??= this.#initContainer();
114119
}
115120

121+
/**
122+
* Refresh items from the getItems option. Call this when the list of items
123+
* has changed (e.g. when a parent controller sets items).
124+
*/
125+
refreshItems(): void {
126+
this.initItems();
127+
}
128+
116129
hostConnected(): void {
117130
this.hostUpdate();
118131
}

‎core/pfe-core/controllers/combobox-controller.ts‎

Lines changed: 42 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -159,7 +159,7 @@ export class ComboboxController<
159159
Item extends HTMLElement
160160
> implements ReactiveController {
161161
public static of<T extends HTMLElement>(
162-
host: ReactiveControllerHost,
162+
host: ReactiveControllerHost & HTMLElement,
163163
options: ComboboxControllerOptions<T>,
164164
): ComboboxController<T> {
165165
return new ComboboxController(host, options);
@@ -237,11 +237,13 @@ export class ComboboxController<
237237

238238
#lb: ListboxController<Item>;
239239
#fc?: ATFocusController<Item>;
240+
#initializing = false;
240241
#preventListboxGainingFocus = false;
241242
#input: HTMLElement | null = null;
242243
#button: HTMLElement | null = null;
243244
#listbox: HTMLElement | null = null;
244245
#buttonInitialRole: string | null = null;
246+
#buttonHasMouseDown = false;
245247
#mo = new MutationObserver(() => this.#initItems());
246248
#microcopy = new Map<string, Record<Lang, string>>(Object.entries({
247249
dimmed: {
@@ -280,6 +282,7 @@ export class ComboboxController<
280282

281283
set items(value: Item[]) {
282284
this.#lb.items = value;
285+
this.#fc?.refreshItems?.();
283286
}
284287

285288
/** Whether the combobox is disabled */
@@ -326,7 +329,7 @@ export class ComboboxController<
326329
}
327330

328331
private constructor(
329-
public host: ReactiveControllerHost,
332+
public host: ReactiveControllerHost & HTMLElement,
330333
options: ComboboxControllerOptions<Item>,
331334
) {
332335
host.addController(this);
@@ -362,7 +365,7 @@ export class ComboboxController<
362365
}
363366

364367
hostUpdated(): void {
365-
if (!this.#fc) {
368+
if (!this.#fc && !this.#initializing) {
366369
this.#init();
367370
}
368371
const expanded = this.options.isExpanded();
@@ -380,7 +383,7 @@ export class ComboboxController<
380383
ComboboxController.hosts.delete(this.host);
381384
}
382385

383-
async _onFocusoutElement(): Promise<void> {
386+
private async _onFocusoutElement(): Promise<void> {
384387
if (this.#hasTextInput && this.options.isExpanded()) {
385388
const root = this.#element?.getRootNode();
386389
await new Promise(requestAnimationFrame);
@@ -397,13 +400,15 @@ export class ComboboxController<
397400
* Order of operations is important
398401
*/
399402
async #init() {
403+
this.#initializing = true;
400404
await this.host.updateComplete;
401405
this.#initListbox();
402406
this.#initItems();
403407
this.#initButton();
404408
this.#initInput();
405409
this.#initLabels();
406410
this.#initController();
411+
this.#initializing = false;
407412
}
408413

409414
#initListbox() {
@@ -425,6 +430,8 @@ export class ComboboxController<
425430
#initButton() {
426431
this.#button?.removeEventListener('click', this.#onClickButton);
427432
this.#button?.removeEventListener('keydown', this.#onKeydownButton);
433+
this.#button?.removeEventListener('mousedown', this.#onMousedownButton);
434+
this.#button?.removeEventListener('mouseup', this.#onMouseupButton);
428435
this.#button = this.options.getToggleButton();
429436
if (!this.#button) {
430437
throw new Error('ComboboxController getToggleButton() option must return an element');
@@ -434,6 +441,8 @@ export class ComboboxController<
434441
this.#button.setAttribute('aria-controls', this.#listbox?.id ?? '');
435442
this.#button.addEventListener('click', this.#onClickButton);
436443
this.#button.addEventListener('keydown', this.#onKeydownButton);
444+
this.#button.addEventListener('mousedown', this.#onMousedownButton);
445+
this.#button.addEventListener('mouseup', this.#onMouseupButton);
437446
}
438447

439448
#initInput() {
@@ -531,26 +540,32 @@ export class ComboboxController<
531540
return strings?.[lang] ?? key;
532541
}
533542

534-
// TODO(bennypowers): perhaps move this to ActivedescendantController
535-
#announce(item: Item) {
543+
/**
544+
* Announces the focused item to a live region (e.g. for Safari VoiceOver).
545+
* @param item - The listbox option item to announce.
546+
* TODO(bennypowers): perhaps move this to ActivedescendantController
547+
*/
548+
#announce(item: Item): void {
536549
const value = this.options.getItemValue(item);
537550
ComboboxController.#alert?.remove();
538551
const fragment = ComboboxController.#alertTemplate.content.cloneNode(true) as DocumentFragment;
539552
ComboboxController.#alert = fragment.firstElementChild as HTMLElement;
540553
let text = value;
541554
const lang = deepClosest(this.#listbox, '[lang]')?.getAttribute('lang') ?? 'en';
542-
const langKey = lang?.match(ComboboxController.langsRE)?.at(0) as Lang ?? 'en';
555+
const langKey = (lang?.match(ComboboxController.langsRE)?.at(0) as Lang) ?? 'en';
543556
if (this.options.isItemDisabled(item)) {
544557
text += ` (${this.#translate('dimmed', langKey)})`;
545558
}
546559
if (this.#lb.isSelected(item)) {
547560
text += `, (${this.#translate('selected', langKey)})`;
548561
}
549-
if (item.hasAttribute('aria-setsize') && item.hasAttribute('aria-posinset')) {
562+
const posInSet = InternalsController.getAriaPosInSet(item);
563+
const setSize = InternalsController.getAriaSetSize(item);
564+
if (posInSet != null && setSize != null) {
550565
if (langKey === 'ja') {
551-
text += `, (${item.getAttribute('aria-setsize')} 件中 ${item.getAttribute('aria-posinset')} 件目)`;
566+
text += `, (${setSize} 件中 ${posInSet} 件目)`;
552567
} else {
553-
text += `, (${item.getAttribute('aria-posinset')} ${this.#translate('of', langKey)} ${item.getAttribute('aria-setsize')})`;
568+
text += `, (${posInSet} ${this.#translate('of', langKey)} ${setSize})`;
554569
}
555570
}
556571
ComboboxController.#alert.lang = lang;
@@ -580,6 +595,17 @@ export class ComboboxController<
580595
}
581596
};
582597

598+
/**
599+
* Distinguish click-to-toggle vs Tab/Shift+Tab
600+
*/
601+
#onMousedownButton = () => {
602+
this.#buttonHasMouseDown = true;
603+
};
604+
605+
#onMouseupButton = () => {
606+
this.#buttonHasMouseDown = false;
607+
};
608+
583609
#onClickListbox = (event: MouseEvent) => {
584610
if (!this.multi && event.composedPath().some(this.options.isItem)) {
585611
this.#hide();
@@ -735,9 +761,14 @@ export class ComboboxController<
735761
#onFocusoutListbox = (event: FocusEvent) => {
736762
if (!this.#hasTextInput && this.options.isExpanded()) {
737763
const root = this.#element?.getRootNode();
764+
// Check if focus moved to the toggle button via mouse click
765+
// If so, let the click handler manage toggle (prevents double-toggle)
766+
// But if focus moved via Shift+Tab (no mousedown), we should still hide
767+
const isClickOnToggleButton =
768+
event.relatedTarget === this.#button && this.#buttonHasMouseDown;
738769
if ((root instanceof ShadowRoot || root instanceof Document)
739770
&& !this.items.includes(event.relatedTarget as Item)
740-
) {
771+
&& !isClickOnToggleButton) {
741772
this.#hide();
742773
}
743774
}

0 commit comments

Comments
 (0)