feat(web): Add keyboard shortcut selection on grid (#16713)

* 15712: Added keyboard shortcuts for opening add to album modal and highlighting/selecting an album to add to.

* 15712: Re-factored logic from template code into script. Extracted new album button into separate cmponent.

* 15712: Document new keyboard shortucts now that they work everywhere.

* 15712: Extract some constants/helper functions.

* 15712: Missing comma.

* 15712: Pulled logic out into separate unit testable class.

* 15712: Added a unit test.

* 15712: Move the modal back up to keep the github PR happy.

* 15712: PR feedback - renamed typescript files and switch to class bind directive.

* 15712:Move selection modal into correct package.

* 15712: Better naming of module and files.

* 15712: Add asset highlight using arrow keys.

* 15172: Add escape behaviour everywhere.

* 15712: Don't allow highlighting past start or end.

* 15712: Clear the highlight on changes to the component state.

* 15712: Use focus to track highlighted element.

* 15712: Rename highlight -> focussed.

* 15712: Better naming.

* 15712: Cleanup.

* 15712: Cleanup & simplify.

* 15712: bugfix for clicking on button.

* 15712: Cleanup.

* 15712: Rollback unnecessary changes.

* 15712: Add unit test.

* 15712: Add thumbnail unit test.

* 15712: Prettier.

* 15712: Fix merge issue.

* 15712: Add shortcut info.

* 15712: Fix linter.
This commit is contained in:
Andreas 2025-03-12 02:18:14 +11:00 committed by GitHub
parent c80afea468
commit b8acae2f21
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
10 changed files with 198 additions and 6 deletions

View File

@ -1150,6 +1150,7 @@
"second": "Second", "second": "Second",
"see_all_people": "See all people", "see_all_people": "See all people",
"select_album_cover": "Select album cover", "select_album_cover": "Select album cover",
"select": "Select",
"select_all": "Select all", "select_all": "Select all",
"select_all_duplicates": "Select all duplicates", "select_all_duplicates": "Select all duplicates",
"select_avatar_color": "Select avatar color", "select_avatar_color": "Select avatar color",

View File

@ -0,0 +1,63 @@
import { getIntersectionObserverMock } from '$lib/__mocks__/intersection-observer.mock';
import Thumbnail from '$lib/components/assets/thumbnail/thumbnail.svelte';
import { assetFactory } from '@test-data/factories/asset-factory';
import { fireEvent, render, screen } from '@testing-library/svelte';
describe('Thumbnail component', () => {
beforeAll(() => {
vi.stubGlobal('IntersectionObserver', getIntersectionObserverMock());
});
it('should only contain a single tabbable element (the container)', () => {
const asset = assetFactory.build({ originalPath: 'image.jpg', originalMimeType: 'image/jpeg' });
render(Thumbnail, {
asset,
focussed: false,
overrideDisplayForTest: true,
selected: true,
});
const container = screen.getByTestId('container-with-tabindex');
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');
}
}
});
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, {
asset,
overrideDisplayForTest: true,
handleFocus: handleFocusSpy,
});
const container = screen.getByTestId('container-with-tabindex');
await fireEvent(container, new FocusEvent('focus'));
expect(handleFocusSpy).toBeCalled();
});
it('element will be focussed if not already', () => {
const asset = assetFactory.build({ originalPath: 'image.jpg', originalMimeType: 'image/jpeg' });
const handleFocusSpy = vi.fn();
render(Thumbnail, {
asset,
overrideDisplayForTest: true,
focussed: true,
handleFocus: handleFocusSpy,
});
expect(handleFocusSpy).toBeCalled();
});
});

View File

@ -40,6 +40,7 @@
thumbnailWidth?: number | undefined; thumbnailWidth?: number | undefined;
thumbnailHeight?: number | undefined; thumbnailHeight?: number | undefined;
selected?: boolean; selected?: boolean;
focussed?: boolean;
selectionCandidate?: boolean; selectionCandidate?: boolean;
disabled?: boolean; disabled?: boolean;
readonly?: boolean; readonly?: boolean;
@ -60,7 +61,9 @@
onRetrieveElement?: ((elment: HTMLElement) => void) | undefined; onRetrieveElement?: ((elment: HTMLElement) => void) | undefined;
onSelect?: ((asset: AssetResponseDto) => void) | undefined; onSelect?: ((asset: AssetResponseDto) => void) | undefined;
onMouseEvent?: ((event: { isMouseOver: boolean; selectedGroupIndex: number }) => void) | undefined; onMouseEvent?: ((event: { isMouseOver: boolean; selectedGroupIndex: number }) => void) | undefined;
handleFocus?: (() => void) | undefined;
class?: string; class?: string;
overrideDisplayForTest?: boolean;
} }
let { let {
@ -72,6 +75,7 @@
thumbnailWidth = undefined, thumbnailWidth = undefined,
thumbnailHeight = undefined, thumbnailHeight = undefined,
selected = false, selected = false,
focussed = false,
selectionCandidate = false, selectionCandidate = false,
disabled = false, disabled = false,
readonly = false, readonly = false,
@ -85,7 +89,9 @@
onRetrieveElement = undefined, onRetrieveElement = undefined,
onSelect = undefined, onSelect = undefined,
onMouseEvent = undefined, onMouseEvent = undefined,
handleFocus = undefined,
class: className = '', class: className = '',
overrideDisplayForTest = false,
}: Props = $props(); }: Props = $props();
let { let {
@ -94,6 +100,7 @@
const componentId = generateId(); const componentId = generateId();
let element: HTMLElement | undefined = $state(); let element: HTMLElement | undefined = $state();
let focussableElement: HTMLElement | undefined = $state();
let mouseOver = $state(false); let mouseOver = $state(false);
let intersecting = $state(false); let intersecting = $state(false);
let lastRetrievedElement: HTMLElement | undefined = $state(); let lastRetrievedElement: HTMLElement | undefined = $state();
@ -111,6 +118,12 @@
} }
}); });
$effect(() => {
if (focussed && document.activeElement !== focussableElement) {
focussableElement?.focus();
}
});
let width = $derived(thumbnailSize || thumbnailWidth || 235); let width = $derived(thumbnailSize || thumbnailWidth || 235);
let height = $derived(thumbnailSize || thumbnailHeight || 235); let height = $derived(thumbnailSize || thumbnailHeight || 235);
let display = $derived(intersecting); let display = $derived(intersecting);
@ -217,7 +230,7 @@
></canvas> ></canvas>
{/if} {/if}
{#if display} {#if display || overrideDisplayForTest}
<!-- svelte queries for all links on afterNavigate, leading to performance problems in asset-grid which updates <!-- svelte queries for all links on afterNavigate, leading to performance problems in asset-grid which updates
the navigation url on scroll. Replace this with button for now. --> the navigation url on scroll. Replace this with button for now. -->
<div <div
@ -226,14 +239,20 @@
class:cursor-pointer={!disabled} class:cursor-pointer={!disabled}
onmouseenter={onMouseEnter} onmouseenter={onMouseEnter}
onmouseleave={onMouseLeave} onmouseleave={onMouseLeave}
onkeypress={(evt) => { onkeydown={(evt) => {
if (evt.key === 'Enter') { if (evt.key === 'Enter') {
callClickHandlers(); callClickHandlers();
} }
if (evt.key === 'x') {
onSelect?.(asset);
}
}} }}
tabindex={0} tabindex={0}
onclick={handleClick} onclick={handleClick}
role="link" role="link"
bind:this={focussableElement}
onfocus={handleFocus}
data-testid="container-with-tabindex"
> >
{#if mouseOver && !disableMouseOver} {#if mouseOver && !disableMouseOver}
<!-- lazy show the url on mouse over--> <!-- lazy show the url on mouse over-->
@ -244,7 +263,7 @@
style:height="{height}px" style:height="{height}px"
href={currentUrlReplaceAssetId(asset.id)} href={currentUrlReplaceAssetId(asset.id)}
onclick={(evt) => evt.preventDefault()} onclick={(evt) => evt.preventDefault()}
tabindex={0} tabindex={-1}
aria-label="Thumbnail URL" aria-label="Thumbnail URL"
> >
</a> </a>
@ -258,6 +277,8 @@
class="absolute p-2 focus:outline-none" class="absolute p-2 focus:outline-none"
class:cursor-not-allowed={disabled} class:cursor-not-allowed={disabled}
role="checkbox" role="checkbox"
tabindex={-1}
onfocus={handleFocus}
aria-checked={selected} aria-checked={selected}
{disabled} {disabled}
> >

View File

@ -93,6 +93,10 @@
} }
}; };
const assetOnFocusHandler = (asset: AssetResponseDto) => {
assetInteraction.focussedAssetId = asset.id;
};
onDestroy(() => { onDestroy(() => {
assetStore.taskManager.removeAllTasksForComponent(componentId); assetStore.taskManager.removeAllTasksForComponent(componentId);
}); });
@ -223,6 +227,8 @@
onSelect={(asset) => assetSelectHandler(asset, dateGroup.assets, dateGroup.groupTitle)} onSelect={(asset) => assetSelectHandler(asset, dateGroup.assets, dateGroup.groupTitle)}
onMouseEvent={() => assetMouseEventHandler(dateGroup.groupTitle, asset)} onMouseEvent={() => assetMouseEventHandler(dateGroup.groupTitle, asset)}
selected={assetInteraction.selectedAssets.has(asset) || assetStore.albumAssets.has(asset.id)} selected={assetInteraction.selectedAssets.has(asset) || assetStore.albumAssets.has(asset.id)}
handleFocus={() => assetOnFocusHandler(asset)}
focussed={assetInteraction.isFocussedAsset(asset)}
selectionCandidate={assetInteraction.assetSelectionCandidates.has(asset)} selectionCandidate={assetInteraction.assetSelectionCandidates.has(asset)}
disabled={assetStore.albumAssets.has(asset.id)} disabled={assetStore.albumAssets.has(asset.id)}
thumbnailWidth={width} thumbnailWidth={width}

View File

@ -42,8 +42,8 @@
isSelectionMode?: boolean; isSelectionMode?: boolean;
singleSelect?: boolean; singleSelect?: boolean;
/** `true` if this asset grid is responds to navigation events; if `true`, then look at the /** `true` if this asset grid is responds to navigation events; if `true`, then look at the
`AssetViewingStore.gridScrollTarget` and load and scroll to the asset specified, and `AssetViewingStore.gridScrollTarget` and load and scroll to the asset specified, and
additionally, update the page location/url with the asset as the asset-grid is scrolled */ additionally, update the page location/url with the asset as the asset-grid is scrolled */
enableRouting: boolean; enableRouting: boolean;
assetStore: AssetStore; assetStore: AssetStore;
assetInteraction: AssetInteraction; assetInteraction: AssetInteraction;
@ -706,6 +706,36 @@
e.preventDefault(); e.preventDefault();
} }
}; };
const focusNextAsset = async () => {
if (assetInteraction.focussedAssetId === null) {
const firstAsset = assetStore.getFirstAsset();
if (firstAsset !== null) {
assetInteraction.focussedAssetId = firstAsset.id;
}
} else {
const focussedAsset = assetStore.assets.find((asset) => asset.id === assetInteraction.focussedAssetId);
if (focussedAsset) {
const nextAsset = await assetStore.getNextAsset(focussedAsset);
if (nextAsset !== null) {
assetInteraction.focussedAssetId = nextAsset.id;
}
}
}
};
const focusPreviousAsset = async () => {
if (assetInteraction.focussedAssetId !== null) {
const focussedAsset = assetStore.assets.find((asset) => asset.id === assetInteraction.focussedAssetId);
if (focussedAsset) {
const previousAsset = await assetStore.getPreviousAsset(focussedAsset);
if (previousAsset) {
assetInteraction.focussedAssetId = previousAsset.id;
}
}
}
};
onDestroy(() => { onDestroy(() => {
assetStore.taskManager.removeAllTasksForComponent(componentId); assetStore.taskManager.removeAllTasksForComponent(componentId);
}); });
@ -749,6 +779,8 @@
{ shortcut: { key: 'A', ctrl: true }, onShortcut: () => selectAllAssets(assetStore, assetInteraction) }, { shortcut: { key: 'A', ctrl: true }, onShortcut: () => selectAllAssets(assetStore, assetInteraction) },
{ shortcut: { key: 'PageDown' }, preventDefault: false, onShortcut: focusElement }, { shortcut: { key: 'PageDown' }, preventDefault: false, onShortcut: focusElement },
{ shortcut: { key: 'PageUp' }, preventDefault: false, onShortcut: focusElement }, { shortcut: { key: 'PageUp' }, preventDefault: false, onShortcut: focusElement },
{ shortcut: { key: 'ArrowRight' }, preventDefault: false, onShortcut: focusNextAsset },
{ shortcut: { key: 'ArrowLeft' }, preventDefault: false, onShortcut: focusPreviousAsset },
]; ];
if (assetInteraction.selectionActive) { if (assetInteraction.selectionActive) {

View File

@ -1,5 +1,5 @@
<script lang="ts"> <script lang="ts">
import { shortcuts, type ShortcutOptions } from '$lib/actions/shortcut'; import { type ShortcutOptions, shortcuts } from '$lib/actions/shortcut';
import { goto } from '$app/navigation'; import { goto } from '$app/navigation';
import type { Action } from '$lib/components/asset-viewer/actions/action'; import type { Action } from '$lib/components/asset-viewer/actions/action';
import Thumbnail from '$lib/components/assets/thumbnail/thumbnail.svelte'; import Thumbnail from '$lib/components/assets/thumbnail/thumbnail.svelte';
@ -178,6 +178,26 @@
} }
}; };
const focusNextAsset = () => {
if (assetInteraction.focussedAssetId === null && assets.length > 0) {
assetInteraction.focussedAssetId = assets[0].id;
} else if (assetInteraction.focussedAssetId !== null && assets.length > 0) {
const currentIndex = assets.findIndex((a) => a.id === assetInteraction.focussedAssetId);
if (currentIndex !== -1 && currentIndex + 1 < assets.length) {
assetInteraction.focussedAssetId = assets[currentIndex + 1].id;
}
}
};
const focusPreviousAsset = () => {
if (assetInteraction.focussedAssetId !== null && assets.length > 0) {
const currentIndex = assets.findIndex((a) => a.id === assetInteraction.focussedAssetId);
if (currentIndex >= 1) {
assetInteraction.focussedAssetId = assets[currentIndex - 1].id;
}
}
};
let shortcutList = $derived( let shortcutList = $derived(
(() => { (() => {
if ($isViewerOpen) { if ($isViewerOpen) {
@ -188,6 +208,8 @@
{ shortcut: { key: '?', shift: true }, onShortcut: () => (showShortcuts = !showShortcuts) }, { shortcut: { key: '?', shift: true }, onShortcut: () => (showShortcuts = !showShortcuts) },
{ shortcut: { key: '/' }, onShortcut: () => goto(AppRoute.EXPLORE) }, { shortcut: { key: '/' }, onShortcut: () => goto(AppRoute.EXPLORE) },
{ shortcut: { key: 'A', ctrl: true }, onShortcut: () => selectAllAssets() }, { shortcut: { key: 'A', ctrl: true }, onShortcut: () => selectAllAssets() },
{ shortcut: { key: 'ArrowRight' }, preventDefault: false, onShortcut: focusNextAsset },
{ shortcut: { key: 'ArrowLeft' }, preventDefault: false, onShortcut: focusPreviousAsset },
]; ];
if (assetInteraction.selectionActive) { if (assetInteraction.selectionActive) {
@ -306,6 +328,10 @@
} }
}; };
const assetOnFocusHandler = (asset: AssetResponseDto) => {
assetInteraction.focussedAssetId = asset.id;
};
let isTrashEnabled = $derived($featureFlags.loaded && $featureFlags.trash); let isTrashEnabled = $derived($featureFlags.loaded && $featureFlags.trash);
let idsSelectedAssets = $derived(assetInteraction.selectedAssetsArray.map(({ id }) => id)); let idsSelectedAssets = $derived(assetInteraction.selectedAssetsArray.map(({ id }) => id));
@ -382,10 +408,12 @@
}} }}
onSelect={(asset) => handleSelectAssets(asset)} onSelect={(asset) => handleSelectAssets(asset)}
onMouseEvent={() => assetMouseEventHandler(asset)} onMouseEvent={() => assetMouseEventHandler(asset)}
handleFocus={() => assetOnFocusHandler(asset)}
onIntersected={() => (i === Math.max(1, assets.length - 7) ? onIntersected?.() : void 0)} onIntersected={() => (i === Math.max(1, assets.length - 7) ? onIntersected?.() : void 0)}
{showArchiveIcon} {showArchiveIcon}
{asset} {asset}
selected={assetInteraction.selectedAssets.has(asset)} selected={assetInteraction.selectedAssets.has(asset)}
focussed={assetInteraction.isFocussedAsset(asset)}
selectionCandidate={assetInteraction.assetSelectionCandidates.has(asset)} selectionCandidate={assetInteraction.assetSelectionCandidates.has(asset)}
thumbnailWidth={geometry.boxes[i].width} thumbnailWidth={geometry.boxes[i].width}
thumbnailHeight={geometry.boxes[i].height} thumbnailHeight={geometry.boxes[i].height}

View File

@ -25,6 +25,7 @@
shortcuts = { shortcuts = {
general: [ general: [
{ key: ['←', '→'], action: $t('previous_or_next_photo') }, { key: ['←', '→'], action: $t('previous_or_next_photo') },
{ key: ['x'], action: $t('select') },
{ key: ['Esc'], action: $t('back_close_deselect') }, { key: ['Esc'], action: $t('back_close_deselect') },
{ key: ['Ctrl', 'k'], action: $t('search_your_photos') }, { key: ['Ctrl', 'k'], action: $t('search_your_photos') },
{ key: ['Ctrl', '⇧', 'k'], action: $t('open_the_search_filters') }, { key: ['Ctrl', '⇧', 'k'], action: $t('open_the_search_filters') },

View File

@ -8,6 +8,7 @@ export class AssetInteraction {
readonly selectedGroup = new SvelteSet<string>(); readonly selectedGroup = new SvelteSet<string>();
assetSelectionCandidates = $state(new SvelteSet<AssetResponseDto>()); assetSelectionCandidates = $state(new SvelteSet<AssetResponseDto>());
assetSelectionStart = $state<AssetResponseDto | null>(null); assetSelectionStart = $state<AssetResponseDto | null>(null);
focussedAssetId = $state<string | null>(null);
selectionActive = $derived(this.selectedAssets.size > 0); selectionActive = $derived(this.selectedAssets.size > 0);
selectedAssetsArray = $derived([...this.selectedAssets]); selectedAssetsArray = $derived([...this.selectedAssets]);
@ -63,4 +64,8 @@ export class AssetInteraction {
this.assetSelectionCandidates.clear(); this.assetSelectionCandidates.clear();
this.assetSelectionStart = null; this.assetSelectionStart = null;
} }
isFocussedAsset(asset: AssetResponseDto) {
return this.focussedAssetId === asset.id;
}
} }

View File

@ -308,6 +308,34 @@ describe('AssetStore', () => {
}); });
}); });
describe('firstAsset', () => {
let assetStore: AssetStore;
beforeEach(async () => {
assetStore = new AssetStore({});
sdkMock.getTimeBuckets.mockResolvedValue([]);
await assetStore.init();
await assetStore.updateViewport({ width: 0, height: 0 });
});
it('empty store returns null', () => {
expect(assetStore.getFirstAsset()).toBeNull();
});
it('populated store returns first asset', () => {
const assetOne = assetFactory.build({
fileCreatedAt: '2024-01-20T12:00:00.000Z',
localDateTime: '2024-01-20T12:00:00.000Z',
});
const assetTwo = assetFactory.build({
fileCreatedAt: '2024-01-15T12:00:00.000Z',
localDateTime: '2024-01-15T12:00:00.000Z',
});
assetStore.addAssets([assetOne, assetTwo]);
expect(assetStore.getFirstAsset()).toEqual(assetOne);
});
});
describe('getPreviousAsset', () => { describe('getPreviousAsset', () => {
let assetStore: AssetStore; let assetStore: AssetStore;
const bucketAssets: Record<string, AssetResponseDto[]> = { const bucketAssets: Record<string, AssetResponseDto[]> = {

View File

@ -848,6 +848,13 @@ export class AssetStore {
} }
} }
getFirstAsset(): AssetResponseDto | null {
if (this.buckets.length > 0 && this.buckets[0].assets.length > 0) {
return this.buckets[0].assets[0];
}
return null;
}
async getPreviousAsset(asset: AssetResponseDto): Promise<AssetResponseDto | null> { async getPreviousAsset(asset: AssetResponseDto): Promise<AssetResponseDto | null> {
const info = await this.getBucketInfoForAsset(asset); const info = await this.getBucketInfoForAsset(asset);
if (!info) { if (!info) {