feat: improve/refactor focus handling (#17796)

* feat: improve focus

* test

* lint

* use modulus in loop

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 <Thumbnail/>
-    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 <Thumbnail/>
+    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();
   });
 });

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"
   >