diff --git a/frontend/src/__tests__/chip-select.test.ts b/frontend/src/__tests__/chip-select.test.ts index b96f8206b..23bf60410 100644 --- a/frontend/src/__tests__/chip-select.test.ts +++ b/frontend/src/__tests__/chip-select.test.ts @@ -337,4 +337,63 @@ describe('createChipSelect', () => { expect(empty?.getAttribute('aria-disabled')).toBe('true'); expect(empty?.getAttribute('aria-selected')).toBe('false'); }); + + // Issue #729 (UX-31): with the search input showing, focus is in the input, + // so aria-activedescendant must be there (screen readers ignore it on an + // unfocused element), and a pick must hand focus back to the trigger. + describe('search-input focus model (issue #729)', () => { + function openLong(onChange: (v: string) => void = () => {}): { + root: HTMLElement; trigger: HTMLButtonElement; search: HTMLInputElement; + } { + const { root } = createChipSelect({ label: 'Account', options: LONG_OPTIONS, value: 'acct-2', onChange }); + document.body.appendChild(root); + const trigger = root.querySelector('.chip-select')!; + trigger.focus(); + trigger.click(); + const search = root.querySelector('.chip-select-search')!; + return { root, trigger, search }; + } + + test('aria-activedescendant lives on the focused search input, not the trigger', () => { + const { root, trigger, search } = openLong(); + expect(document.activeElement).toBe(search); + expect(trigger.hasAttribute('aria-activedescendant')).toBe(false); + const listbox = root.querySelector('[role="listbox"]')!; + expect(search.getAttribute('aria-controls')).toBe(listbox.id); + expect(document.getElementById(search.getAttribute('aria-activedescendant')!)?.dataset['value']).toBe('acct-2'); + + search.dispatchEvent(new KeyboardEvent('keydown', { key: 'ArrowDown', bubbles: true })); + expect(document.getElementById(search.getAttribute('aria-activedescendant')!)?.dataset['value']).toBe('acct-3'); + expect(trigger.hasAttribute('aria-activedescendant')).toBe(false); + }); + + test('Enter on a highlighted option returns focus to the trigger', () => { + const onChange = jest.fn(); + const { trigger, search } = openLong(onChange); + search.dispatchEvent(new KeyboardEvent('keydown', { key: 'ArrowDown', bubbles: true })); + search.dispatchEvent(new KeyboardEvent('keydown', { key: 'Enter', bubbles: true })); + expect(onChange).toHaveBeenCalledWith('acct-3'); + expect(document.activeElement).toBe(trigger); + expect(search.hasAttribute('aria-activedescendant')).toBe(false); + }); + + test('a mouse pick from the open search menu also returns focus to the trigger', () => { + const { root, trigger } = openLong(); + const opt = root.querySelector('.chip-select-option[data-value="acct-5"]')!; + opt.dispatchEvent(new MouseEvent('mousedown', { bubbles: true, cancelable: true })); + expect(document.activeElement).toBe(trigger); + }); + + test('short lists keep aria-activedescendant on the trigger, which keeps focus', () => { + const { root } = createChipSelect({ label: 'Provider', options: SHORT_OPTIONS, value: 'aws', onChange: () => {} }); + document.body.appendChild(root); + const trigger = root.querySelector('.chip-select')!; + trigger.focus(); + trigger.dispatchEvent(new KeyboardEvent('keydown', { key: 'ArrowDown', bubbles: true })); + trigger.dispatchEvent(new KeyboardEvent('keydown', { key: 'ArrowDown', bubbles: true })); + expect(trigger.getAttribute('aria-activedescendant')).toBeTruthy(); + trigger.dispatchEvent(new KeyboardEvent('keydown', { key: 'Enter', bubbles: true })); + expect(document.activeElement).toBe(trigger); + }); + }); }); diff --git a/frontend/src/__tests__/html.test.ts b/frontend/src/__tests__/html.test.ts index d2a95f703..ef19507f1 100644 --- a/frontend/src/__tests__/html.test.ts +++ b/frontend/src/__tests__/html.test.ts @@ -367,6 +367,14 @@ describe('HTML Structure', () => { expect(href).toBeTruthy(); }); }); + + // Issue #729 (UX-36): aria-label on a role-less div is ignored by + // assistive tech (axe: aria-allowed-attr / aria-prohibited-attr). + test('the global filter container is a labelled group', () => { + const filters = document.getElementById('topbar-filters'); + expect(filters?.getAttribute('role')).toBe('group'); + expect(filters?.getAttribute('aria-label')).toBe('Global filters'); + }); }); describe('Select Options', () => { diff --git a/frontend/src/__tests__/mobile-nav.test.ts b/frontend/src/__tests__/mobile-nav.test.ts index efb795077..d21d5d25d 100644 --- a/frontend/src/__tests__/mobile-nav.test.ts +++ b/frontend/src/__tests__/mobile-nav.test.ts @@ -16,7 +16,7 @@ function buildDOM(): void { aria-expanded="false">

CUDly

-
+
diff --git a/frontend/src/__tests__/riexchange-active-ri-filters.test.ts b/frontend/src/__tests__/riexchange-active-ri-filters.test.ts index bdf83870f..2c95b5cac 100644 --- a/frontend/src/__tests__/riexchange-active-ri-filters.test.ts +++ b/frontend/src/__tests__/riexchange-active-ri-filters.test.ts @@ -265,4 +265,63 @@ describe('Active Convertible RIs column filters (issue #1414)', () => { expect(state.setRiExchangeColumnFilter).toHaveBeenCalledWith('source_count', { kind: 'expr', expr: '>1' }); expect(state.setActiveRiColumnFilter).not.toHaveBeenCalled(); }); + + // Issue #729 (UX-27): Escape used to drop focus to . It must close + // the popover and land back on the column's filter button, the way the + // History popover does. + test('Escape closes the Active RI popover and returns focus to its trigger', async () => { + await load(); + const btn = document.querySelector( + '#ri-exchange-instances-list th .column-filter-btn[data-column="instance_type"]', + )!; + btn.focus(); + btn.click(); + const popover = document.body.querySelector('.column-filter-popover')!; + expect(popover.contains(document.activeElement)).toBe(true); + + const esc = new KeyboardEvent('keydown', { key: 'Escape', bubbles: true, cancelable: true }); + document.activeElement!.dispatchEvent(esc); + + expect(document.body.querySelector('.column-filter-popover')).toBeNull(); + expect(esc.defaultPrevented).toBe(true); + expect(btn.getAttribute('aria-expanded')).toBe('false'); + expect(document.activeElement).toBe(btn); + }); + + test('Escape closes the reshape popover and returns focus to its trigger', async () => { + (api.getReshapeRecommendations as jest.Mock).mockResolvedValue({ + recommendations: [{ + source_ri_id: 'ri-1', source_instance_type: 'm5.xlarge', source_count: 4, + target_instance_type: 'm5.large', target_count: 8, utilization_percent: 50, + normalized_used: 4, normalized_purchased: 8, reason: 'underutilized', + }], + recs_staleness: '', + recs_collected_at: null, + }); + await load(); + const btn = document.querySelector( + '#ri-exchange-recommendations-list .column-filter-btn[data-column]', + )!; + expect(btn).not.toBeNull(); + btn.focus(); + btn.click(); + expect(document.body.querySelector('.column-filter-popover')).not.toBeNull(); + + document.activeElement!.dispatchEvent(new KeyboardEvent('keydown', { key: 'Escape', bubbles: true, cancelable: true })); + + expect(document.body.querySelector('.column-filter-popover')).toBeNull(); + expect(document.activeElement).toBe(btn); + }); + + test('closing by clicking outside does not move focus', async () => { + await load(); + const btn = document.querySelector( + '#ri-exchange-instances-list th .column-filter-btn[data-column="instance_type"]', + )!; + btn.click(); + const outside = document.getElementById('ri-exchange-history-list')!; + outside.dispatchEvent(new MouseEvent('mousedown', { bubbles: true })); + expect(document.body.querySelector('.column-filter-popover')).toBeNull(); + expect(document.activeElement).not.toBe(btn); + }); }); diff --git a/frontend/src/__tests__/skeleton.test.ts b/frontend/src/__tests__/skeleton.test.ts index feee3784c..767c8cadb 100644 --- a/frontend/src/__tests__/skeleton.test.ts +++ b/frontend/src/__tests__/skeleton.test.ts @@ -173,3 +173,50 @@ describe('skeleton show / teardown lifecycle', () => { expect(container.querySelectorAll('.skeleton')).toHaveLength(1); }); }); + +// Issue #729 (UX-34): the placeholders are aria-hidden, so the container +// carries aria-busy while one is showing, and drops it once real content +// (or nothing) replaces it, whichever path did the replacing. +describe('skeleton aria-busy', () => { + const flush = (): Promise => new Promise((r) => setTimeout(r, 0)); + + test.each([ + ['tiles', (c: HTMLElement): void => showSkeletonTiles(c, 2)], + ['rows', (c: HTMLElement): void => showSkeletonRows(c, 2, 3)], + ['block', (c: HTMLElement): void => showSkeletonBlock(c, '100%', '4rem')], + ])('show (%s) marks the container busy and keeps it busy after observers run', async (_name, show) => { + const c = document.createElement('div'); + c.innerHTML = '

old content

'; + document.body.appendChild(c); + show(c); + expect(c.getAttribute('aria-busy')).toBe('true'); + await flush(); + expect(c.getAttribute('aria-busy')).toBe('true'); + c.remove(); + }); + + test('teardownSkeleton clears aria-busy synchronously', () => { + const c = document.createElement('div'); + showSkeletonRows(c, 2, 3); + teardownSkeleton(c); + expect(c.hasAttribute('aria-busy')).toBe(false); + }); + + test('a success render that replaces the children clears aria-busy', async () => { + const c = document.createElement('div'); + document.body.appendChild(c); + showSkeletonRows(c, 2, 3); + c.innerHTML = '
real
'; + await flush(); + expect(c.hasAttribute('aria-busy')).toBe(false); + + // A second load re-arms the flag. + showSkeletonTiles(c, 1); + await flush(); + expect(c.getAttribute('aria-busy')).toBe('true'); + c.replaceChildren(); + await flush(); + expect(c.hasAttribute('aria-busy')).toBe(false); + c.remove(); + }); +}); diff --git a/frontend/src/index.html b/frontend/src/index.html index 1a2c654b0..1201a96f3 100644 --- a/frontend/src/index.html +++ b/frontend/src/index.html @@ -39,7 +39,7 @@

CUDly

-
+
diff --git a/frontend/src/lib/chip-select.ts b/frontend/src/lib/chip-select.ts index 05d968a5a..4784df610 100644 --- a/frontend/src/lib/chip-select.ts +++ b/frontend/src/lib/chip-select.ts @@ -24,14 +24,16 @@ * Enter / Space / Down on the trigger → open * Esc on the menu → close + focus trigger * Arrow Up / Down → move highlight - * Enter on highlighted option → select + close + * Enter on highlighted option → select + close + focus trigger * Tab on menu → close (and let focus continue) * * Search-filter input appears at top of popover when options.length > 8. * * ARIA: the trigger uses `aria-haspopup="listbox"` + `aria-expanded`. The * menu is `role="listbox"` with `role="option"` children; the active - * option carries `aria-selected="true"`. + * option carries `aria-selected="true"`. `aria-activedescendant` (the + * highlighted option) lives on whichever element holds focus while the menu + * is open: the search input when it renders, otherwise the trigger. */ export interface ChipSelectOption { @@ -148,6 +150,18 @@ export function createChipSelect(cfg: ChipSelectConfig): ChipSelectHandle { // Stable id prefix for this instance, used for aria-activedescendant. const idPrefix = `chip-select-opt-${cfg.label.toLowerCase().replace(/\s+/g, '-')}`; + optionsList.id = `${idPrefix}-listbox`; + search.setAttribute('aria-controls', optionsList.id); + + /** + * The element that owns focus while the menu is open, and so the one that + * must carry aria-activedescendant: screen readers only follow it on the + * focused element (issue #729). With the search input showing, focus is + * in the input, so a trigger-side reference was never announced. + */ + function activeDescendantHost(): HTMLElement { + return menu.contains(search) ? search : trigger; + } function renderOptions(): void { while (optionsList.firstChild) optionsList.removeChild(optionsList.firstChild); @@ -176,11 +190,15 @@ export function createChipSelect(cfg: ChipSelectConfig): ChipSelectHandle { }); optionsList.appendChild(li); }); - // Keep aria-activedescendant in sync with the highlighted option. + // Keep aria-activedescendant in sync with the highlighted option, on the + // focused element only. + const host = activeDescendantHost(); + const other = host === search ? trigger : search; + other.removeAttribute('aria-activedescendant'); if (activeOptId !== null) { - trigger.setAttribute('aria-activedescendant', activeOptId); + host.setAttribute('aria-activedescendant', activeOptId); } else { - trigger.removeAttribute('aria-activedescendant'); + host.removeAttribute('aria-activedescendant'); } // Empty-result hint when filter doesn't match anything. if (visible.length === 0) { @@ -197,9 +215,15 @@ export function createChipSelect(cfg: ChipSelectConfig): ChipSelectHandle { } function selectByValue(value: string): void { + // A pick from the search input hides the element holding focus, which + // would drop it to ; return it to the trigger (issue #729). Mouse + // picks keep focus on the trigger already (option mousedown is + // preventDefault'ed), so this is a no-op for them. + const focusWasInside = root.contains(document.activeElement); currentValue = value; renderTrigger(); closeMenu(); + if (focusWasInside) trigger.focus(); cfg.onChange(value); } @@ -259,6 +283,7 @@ export function createChipSelect(cfg: ChipSelectConfig): ChipSelectHandle { menu.classList.add('hidden'); trigger.setAttribute('aria-expanded', 'false'); trigger.removeAttribute('aria-activedescendant'); + search.removeAttribute('aria-activedescendant'); if (outsideListener) { document.removeEventListener('mousedown', outsideListener); outsideListener = null; diff --git a/frontend/src/lib/skeleton.ts b/frontend/src/lib/skeleton.ts index 4229e9e8b..582de643b 100644 --- a/frontend/src/lib/skeleton.ts +++ b/frontend/src/lib/skeleton.ts @@ -16,8 +16,36 @@ * * All helpers use `createElement` only — no `innerHTML` — to match the * codebase's XSS posture from issue #340. + * + * Accessibility (issue #729): the placeholders themselves are aria-hidden, + * so the container carries `aria-busy="true"` while a skeleton is showing. + * Success renders replace the children without calling back in here, so a + * MutationObserver clears the flag once no placeholder is left. */ +const SKELETON_SELECTOR = '.skeleton, .skeleton-group, .skeleton-tile, .skeleton-row, .skeleton-table'; + +const busyObservers = new WeakMap(); + +function clearBusy(container: HTMLElement): void { + container.removeAttribute('aria-busy'); + busyObservers.get(container)?.disconnect(); + busyObservers.delete(container); +} + +function markBusy(container: HTMLElement): void { + container.setAttribute('aria-busy', 'true'); + if (busyObservers.has(container) || typeof MutationObserver === 'undefined') return; + // Records are delivered after the current task, by which point the show* + // helper has appended its placeholders, so the wipe that starts a show + // does not clear the flag. + const observer = new MutationObserver(() => { + if (!container.querySelector(SKELETON_SELECTOR)) clearBusy(container); + }); + observer.observe(container, { childList: true, subtree: true }); + busyObservers.set(container, observer); +} + /** * A single shimmer block. Width/height are CSS sizes (e.g. "60%", "1rem", * "12px"). Useful as a primitive inside higher-level helpers. @@ -95,6 +123,7 @@ export function skeletonRow(cols: number): HTMLTableRowElement { function activateSkeleton(container: HTMLElement): void { while (container.firstChild) container.removeChild(container.firstChild); container.dataset['skeletonActive'] = '1'; + markBusy(container); } /** @@ -151,6 +180,7 @@ export function teardownSkeleton(container: HTMLElement): void { while (container.firstChild) container.removeChild(container.firstChild); delete container.dataset['skeletonActive']; } + clearBusy(container); } /** diff --git a/frontend/tests-e2e/a11y-focus-return.spec.ts b/frontend/tests-e2e/a11y-focus-return.spec.ts new file mode 100644 index 000000000..e26eef7a3 --- /dev/null +++ b/frontend/tests-e2e/a11y-focus-return.spec.ts @@ -0,0 +1,94 @@ +/** + * Keyboard focus return and ARIA wiring for issue #729. + * + * - UX-27: Escape in an RI Exchange column-filter popover returns focus to + * the column's filter button instead of dropping it to . + * - UX-31: the chip-select search input carries aria-activedescendant while + * it holds focus, and picking an option returns focus to the chip. + * - UX-36: #topbar-filters is a labelled group. + * + * Driven with real key presses in Chromium, since "where does focus land" + * is the whole point and jsdom does not move focus on its own. + */ + +import { test, expect, type Page } from '@playwright/test'; +import { ACCOUNTS, mockApi, seedAuth } from './fixtures/recs'; + +/* Enough accounts to push the Account chip past its 8-option search threshold. */ +const MANY_ACCOUNTS = [ + ...ACCOUNTS, + ...Array.from({ length: 6 }, (_, i) => ({ + id: `acct-30${i}`, name: `AWS Extra ${i} (acct-30${i})`, provider: 'aws', external_id: `30000000000${i}`, + })), +]; + +const activeId = (page: Page): Promise => + page.evaluate(() => { + const el = document.activeElement as HTMLElement | null; + return el ? `${el.tagName}#${el.id}.${el.className}` : ''; + }); + +test('Escape in the Active RI filter popover returns focus to its button', async ({ page }) => { + await seedAuth(page); + await mockApi(page); + await page.route('**/api/ri-exchange/**', async (route) => { + const end = new URL(route.request().url()).pathname.split('/').pop()!; + const responses: Record = { + instances: { instances: [{ + reserved_instance_id: 'ri-1', instance_type: 'm5.large', instance_count: 2, region: 'us-east-1', + state: 'active', offering_class: 'convertible', offering_type: 'No Upfront', + product_description: 'Linux/UNIX', start: '2026-01-01T00:00:00Z', end: '2027-01-01T00:00:00Z', + }] }, + utilization: { utilization: [] }, 'reshape-recommendations': { recommendations: [] }, history: { records: [] }, + }; + await route.fulfill({ json: responses[end] ?? {} }); + }); + await page.goto('/inventory/ri-exchange'); + + const btn = page.locator('#ri-exchange-instances-list .column-filter-btn[data-column="instance_type"]'); + await expect(btn).toBeVisible(); + await btn.focus(); + await page.keyboard.press('Enter'); + const popover = page.locator('.column-filter-popover'); + await expect(popover).toBeVisible(); + expect(await popover.evaluate((el) => el.contains(document.activeElement))).toBe(true); + + await page.keyboard.press('Escape'); + await expect(popover).toHaveCount(0); + await expect(btn).toBeFocused(); + await expect(btn).toHaveAttribute('aria-expanded', 'false'); +}); + +test('Account chip search: activedescendant on the input, focus back on the chip after a pick', async ({ page }) => { + await seedAuth(page); + await mockApi(page); + await page.route('**/api/accounts**', (route) => route.fulfill({ json: MANY_ACCOUNTS })); + await page.goto('/home'); + + const chip = page.locator('#topbar-filters button[aria-label="Account"]'); + await expect(chip).toBeVisible(); + await chip.focus(); + await page.keyboard.press('Enter'); + const search = page.locator('#topbar-filters .chip-select-search'); + await expect(search).toBeFocused(); + + await page.keyboard.type('Extra 3'); + await page.keyboard.press('ArrowDown'); + const descId = await search.getAttribute('aria-activedescendant'); + expect(descId).toBeTruthy(); + await expect(page.locator(`[id="${descId}"]`)).toContainText('AWS Extra 3 (acct-303)'); + await expect(chip).not.toHaveAttribute('aria-activedescendant', /.+/); + + await page.keyboard.press('Enter'); + await expect(search).toBeHidden(); + await expect(chip).toBeFocused(); + await expect(chip).toContainText('AWS Extra 3'); + expect(await activeId(page)).toContain('chip-select'); +}); + +test('the global filters container is a labelled group', async ({ page }) => { + await seedAuth(page); + await mockApi(page); + await page.goto('/home'); + await expect(page.getByRole('group', { name: 'Global filters' })).toHaveAttribute('id', 'topbar-filters'); +});