From 4b1ced439be597e0da8b4f0fbfb702c19ccffae8 Mon Sep 17 00:00:00 2001 From: Min Idzelis Date: Wed, 30 Apr 2025 00:19:38 -0400 Subject: [PATCH] feat: improve/refactor focus handling (#17796) * feat: improve focus * test * lint * use modulus in loop --- .../lib/actions/__test__/focus-trap.spec.ts | 4 ++ web/src/lib/actions/focus-trap.ts | 19 +++++---- .../thumbnail/__test__/thumbnail.spec.ts | 41 +++++++++---------- .../assets/thumbnail/thumbnail.svelte | 35 +++------------- .../elements/buttons/skip-link.svelte | 8 +++- .../photos-page/asset-date-group.svelte | 5 --- .../components/photos-page/asset-grid.svelte | 31 ++------------ .../gallery-viewer/gallery-viewer.svelte | 28 ++----------- .../scrubber/scrubber.svelte | 4 +- .../lib/stores/asset-interaction.svelte.ts | 5 --- web/src/lib/utils/focus-util.ts | 41 +++++++++++++++++-- 11 files changed, 92 insertions(+), 129 deletions(-) diff --git a/web/src/lib/actions/__test__/focus-trap.spec.ts b/web/src/lib/actions/__test__/focus-trap.spec.ts index d92d8e037d4..b03064a91da 100644 --- a/web/src/lib/actions/__test__/focus-trap.spec.ts +++ b/web/src/lib/actions/__test__/focus-trap.spec.ts @@ -1,8 +1,11 @@ import FocusTrapTest from '$lib/actions/__test__/focus-trap-test.svelte'; +import { setDefaultTabbleOptions } from '$lib/utils/focus-util'; import { render, screen } from '@testing-library/svelte'; import userEvent from '@testing-library/user-event'; import { tick } from 'svelte'; +setDefaultTabbleOptions({ displayCheck: 'none' }); + describe('focusTrap action', () => { const user = userEvent.setup(); @@ -38,6 +41,7 @@ describe('focusTrap action', () => { const openButton = screen.getByText('Open'); await user.click(openButton); + await tick(); expect(document.activeElement).toEqual(screen.getByTestId('one')); screen.getByText('Close').click(); diff --git a/web/src/lib/actions/focus-trap.ts b/web/src/lib/actions/focus-trap.ts index 1564dd90d0a..2b03282c2db 100644 --- a/web/src/lib/actions/focus-trap.ts +++ b/web/src/lib/actions/focus-trap.ts @@ -1,5 +1,5 @@ import { shortcuts } from '$lib/actions/shortcut'; -import { getFocusable } from '$lib/utils/focus-util'; +import { getTabbable } from '$lib/utils/focus-util'; import { tick } from 'svelte'; interface Options { @@ -18,18 +18,21 @@ export function focusTrap(container: HTMLElement, options?: Options) { }; }; - const setInitialFocus = () => { - const focusableElement = getFocusable(container)[0]; - // Use tick() to ensure focus trap works correctly inside - void tick().then(() => focusableElement?.focus()); + const setInitialFocus = async () => { + const focusableElement = getTabbable(container, false)[0]; + if (focusableElement) { + // Use tick() to ensure focus trap works correctly inside + await tick(); + focusableElement?.focus(); + } }; if (withDefaults(options).active) { - setInitialFocus(); + void setInitialFocus(); } const getFocusableElements = () => { - const focusableElements = getFocusable(container); + const focusableElements = getTabbable(container); return [ focusableElements.at(0), // focusableElements.at(-1), @@ -67,7 +70,7 @@ export function focusTrap(container: HTMLElement, options?: Options) { update(newOptions?: Options) { options = newOptions; if (withDefaults(options).active) { - setInitialFocus(); + void setInitialFocus(); } }, destroy() { diff --git a/web/src/lib/components/assets/thumbnail/__test__/thumbnail.spec.ts b/web/src/lib/components/assets/thumbnail/__test__/thumbnail.spec.ts index 503ea8aefd3..7f6a9d588e3 100644 --- a/web/src/lib/components/assets/thumbnail/__test__/thumbnail.spec.ts +++ b/web/src/lib/components/assets/thumbnail/__test__/thumbnail.spec.ts @@ -1,7 +1,8 @@ import { getIntersectionObserverMock } from '$lib/__mocks__/intersection-observer.mock'; import Thumbnail from '$lib/components/assets/thumbnail/thumbnail.svelte'; +import { getTabbable } from '$lib/utils/focus-util'; import { assetFactory } from '@test-data/factories/asset-factory'; -import { fireEvent, render, screen } from '@testing-library/svelte'; +import { fireEvent, render } from '@testing-library/svelte'; vi.hoisted(() => { Object.defineProperty(globalThis, 'matchMedia', { @@ -31,51 +32,47 @@ describe('Thumbnail component', () => { it('should only contain a single tabbable element (the container)', () => { const asset = assetFactory.build({ originalPath: 'image.jpg', originalMimeType: 'image/jpeg' }); - render(Thumbnail, { + const { baseElement } = render(Thumbnail, { asset, - focussed: false, selected: true, }); - const container = screen.getByTestId('container-with-tabindex'); - expect(container.getAttribute('tabindex')).toBe('0'); + const container = baseElement.querySelector('[data-thumbnail-focus-container]'); + expect(container).not.toBeNull(); + expect(container!.getAttribute('tabindex')).toBe('0'); - // This isn't capturing all tabbable elements, but should be the most likely ones. Mainly guarding against - // inserting extra tabbable elments in future in - let allTabbableElements = screen.queryAllByRole('link'); - allTabbableElements = allTabbableElements.concat(screen.queryAllByRole('checkbox')); - expect(allTabbableElements.length).toBeGreaterThan(0); - for (const tabbableElement of allTabbableElements) { - const testIdValue = tabbableElement.dataset.testid; - if (testIdValue === null || testIdValue !== 'container-with-tabindex') { - expect(tabbableElement.getAttribute('tabindex')).toBe('-1'); - } - } + // Guarding against inserting extra tabbable elments in future in + const tabbables = getTabbable(container!); + expect(tabbables.length).toBe(0); }); it('handleFocus should be called on focus of container', async () => { const asset = assetFactory.build({ originalPath: 'image.jpg', originalMimeType: 'image/jpeg' }); const handleFocusSpy = vi.fn(); - render(Thumbnail, { + const { baseElement } = render(Thumbnail, { asset, handleFocus: handleFocusSpy, }); - const container = screen.getByTestId('container-with-tabindex'); - await fireEvent(container, new FocusEvent('focus')); + const container = baseElement.querySelector('[data-thumbnail-focus-container]'); + expect(container).not.toBeNull(); + await fireEvent(container as HTMLElement, new FocusEvent('focus')); expect(handleFocusSpy).toBeCalled(); }); - it('element will be focussed if not already', () => { + it('element will be focussed if not already', async () => { const asset = assetFactory.build({ originalPath: 'image.jpg', originalMimeType: 'image/jpeg' }); const handleFocusSpy = vi.fn(); - render(Thumbnail, { + const { baseElement } = render(Thumbnail, { asset, - focussed: true, handleFocus: handleFocusSpy, }); + const container = baseElement.querySelector('[data-thumbnail-focus-container]'); + expect(container).not.toBeNull(); + await fireEvent(container as HTMLElement, new FocusEvent('focus')); + expect(handleFocusSpy).toBeCalled(); }); }); diff --git a/web/src/lib/components/assets/thumbnail/thumbnail.svelte b/web/src/lib/components/assets/thumbnail/thumbnail.svelte index 076b0b17cdd..3a00af34ad8 100644 --- a/web/src/lib/components/assets/thumbnail/thumbnail.svelte +++ b/web/src/lib/components/assets/thumbnail/thumbnail.svelte @@ -19,7 +19,7 @@ import { thumbhash } from '$lib/actions/thumbhash'; import { authManager } from '$lib/managers/auth-manager.svelte'; import { mobileDevice } from '$lib/stores/mobile-device.svelte'; - import { getFocusable } from '$lib/utils/focus-util'; + import { focusNext } from '$lib/utils/focus-util'; import { currentUrlReplaceAssetId } from '$lib/utils/navigation'; import { TUNABLES } from '$lib/utils/tunables'; import { onMount } from 'svelte'; @@ -35,7 +35,6 @@ thumbnailWidth?: number | undefined; thumbnailHeight?: number | undefined; selected?: boolean; - focussed?: boolean; selectionCandidate?: boolean; disabled?: boolean; disableLinkMouseOver?: boolean; @@ -58,7 +57,6 @@ thumbnailWidth = undefined, thumbnailHeight = undefined, selected = false, - focussed = false, selectionCandidate = false, disabled = false, disableLinkMouseOver = false, @@ -79,17 +77,11 @@ } = TUNABLES; let usingMobileDevice = $derived(mobileDevice.pointerCoarse); - let focussableElement: HTMLElement | undefined = $state(); + let element: HTMLElement | undefined = $state(); let mouseOver = $state(false); let loaded = $state(false); let thumbError = $state(false); - $effect(() => { - if (focussed && document.activeElement !== focussableElement) { - focussableElement?.focus(); - } - }); - let width = $derived(thumbnailSize || thumbnailWidth || 235); let height = $derived(thumbnailSize || thumbnailHeight || 235); @@ -236,31 +228,14 @@ if (evt.key === 'x') { onSelect?.(asset); } - if (document.activeElement === focussableElement && evt.key === 'Escape') { - const focusable = getFocusable(document); - const index = focusable.indexOf(focussableElement); - - let i = index + 1; - while (i !== index) { - const next = focusable[i]; - if (next.dataset.thumbnailFocusContainer !== undefined) { - if (i === focusable.length - 1) { - i = 0; - } else { - i++; - } - continue; - } - next.focus(); - break; - } + if (document.activeElement === element && evt.key === 'Escape') { + focusNext((element) => element.dataset.thumbnailFocusContainer === undefined, true); } }} onclick={handleClick} - bind:this={focussableElement} + bind:this={element} onfocus={handleFocus} data-thumbnail-focus-container - data-testid="container-with-tabindex" tabindex={0} role="link" > diff --git a/web/src/lib/components/elements/buttons/skip-link.svelte b/web/src/lib/components/elements/buttons/skip-link.svelte index 40b1b9c5264..a21fc60cafe 100644 --- a/web/src/lib/components/elements/buttons/skip-link.svelte +++ b/web/src/lib/components/elements/buttons/skip-link.svelte @@ -1,6 +1,7 @@