Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(web): navigate with keyboard on person page #5486

Merged
merged 7 commits into from
Feb 13, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions web/src/lib/components/elements/buttons/button.svelte
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
export let fullwidth = false;
export let border = false;
export let title: string | undefined = '';
export let ref: HTMLButtonElement | null = null;
martabal marked this conversation as resolved.
Show resolved Hide resolved

const colorClasses: Record<Color, string> = {
primary:
Expand Down Expand Up @@ -55,6 +56,7 @@
</script>

<button
bind:this={ref}
{type}
{disabled}
{title}
Expand Down
49 changes: 45 additions & 4 deletions web/src/lib/components/faces-page/merge-suggestion-modal.svelte
Original file line number Diff line number Diff line change
@@ -1,12 +1,14 @@
<script lang="ts">
import FullScreenModal from '$lib/components/shared-components/full-screen-modal.svelte';
import { api, type PersonResponseDto } from '@api';
import { createEventDispatcher } from 'svelte';
import { createEventDispatcher, onDestroy, onMount } from 'svelte';
import ImageThumbnail from '../assets/thumbnail/image-thumbnail.svelte';
import Button from '../elements/buttons/button.svelte';
import CircleIconButton from '../elements/buttons/circle-icon-button.svelte';
import { mdiArrowLeft, mdiClose, mdiMerge } from '@mdi/js';
import Icon from '$lib/components/elements/icon.svelte';
import { browser } from '$app/environment';
import { assetViewingStore } from '$lib/stores/asset-viewing.store';

const dispatch = createEventDispatcher<{
reject: void;
Expand All @@ -18,10 +20,47 @@
export let personMerge2: PersonResponseDto;
export let potentialMergePeople: PersonResponseDto[];

let { isViewing: showAssetViewer } = assetViewingStore;
let choosePersonToMerge = false;

let changeFocus = false;
let buttonNo: HTMLButtonElement;
let buttonYes: HTMLButtonElement;
const title = personMerge2.name;

const onKeyboardPress = (event: KeyboardEvent) => handleKeyboardPress(event);

onMount(() => {
document.addEventListener('keydown', onKeyboardPress);
});

onDestroy(() => {
if (browser) {
document.removeEventListener('keydown', onKeyboardPress);
}
});

const handleKeyboardPress = (event: KeyboardEvent) => {
if (!$showAssetViewer) {
event.stopPropagation();
martabal marked this conversation as resolved.
Show resolved Hide resolved
switch (event.key) {
case 'Tab':
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tabbing through focusable elements is something that is built into the browser. We don't need to re-invent the wheel here.

https://developer.mozilla.org/en-US/docs/Web/HTML/Global_attributes/tabindex

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I know but I couldn't find a way to limit the focus to the suggested people only.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tabindex=-1 tells the browser to skip it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I tried it but the issue is that you want to navigate only through the suggested people, not all the buttons on the page. Do you have any ideas 🤔 ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does using an actual browser model still let you focus on background elements?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's an actual browser model ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mmmh, just tried it, does it work only for dialogs ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we want dialogs to display suggested people 😅

event.preventDefault();

if (changeFocus) {
buttonYes.focus();
} else {
buttonNo.focus();
}

changeFocus = !changeFocus;
return;
case 'Escape':
dispatch('close');
return;
}
}
};

const changePersonToMerge = (newperson: PersonResponseDto) => {
const index = potentialMergePeople.indexOf(newperson);
[potentialMergePeople[index], personMerge2] = [personMerge2, potentialMergePeople[index]];
Expand Down Expand Up @@ -114,8 +153,10 @@
<p class="text-sm text-gray-500 dark:text-gray-300">They will be merged together</p>
</div>
<div class="mt-8 flex w-full gap-4 px-4 pb-4">
<Button color="gray" fullwidth on:click={() => dispatch('reject')}>No</Button>
<Button fullwidth on:click={() => dispatch('confirm', [personMerge1, personMerge2])}>Yes</Button>
<Button bind:ref={buttonNo} color="gray" fullwidth on:click={() => dispatch('reject')}>No</Button>
<Button bind:ref={buttonYes} fullwidth on:click={() => dispatch('confirm', [personMerge1, personMerge2])}
>Yes</Button
>
</div>
</div>
</div>
Expand Down
127 changes: 92 additions & 35 deletions web/src/routes/(user)/people/[personId]/+page.svelte
Original file line number Diff line number Diff line change
Expand Up @@ -30,14 +30,15 @@
import { websocketStore } from '$lib/stores/websocket';
import { handleError } from '$lib/utils/handle-error';
import { AssetResponseDto, PersonResponseDto, api } from '@api';
import { onMount } from 'svelte';
import { onDestroy, onMount } from 'svelte';
import type { PageData } from './$types';
import { clickOutside } from '$lib/utils/click-outside';
import { assetViewingStore } from '$lib/stores/asset-viewing.store';
import LoadingSpinner from '$lib/components/shared-components/loading-spinner.svelte';
import { mdiPlus, mdiDotsVertical, mdiArrowLeft } from '@mdi/js';
import { isExternalUrl } from '$lib/utils/navigation';
import { searchNameLocal } from '$lib/utils/person';
import { browser } from '$app/environment';

export let data: PageData;

Expand Down Expand Up @@ -86,25 +87,10 @@
**/
let searchWord: string;
let isSearchingPeople = false;

const searchPeople = async () => {
if ((people.length < 20 && name.startsWith(searchWord)) || name === '') {
return;
}
const timeout = setTimeout(() => (isSearchingPeople = true), 100);
try {
const { data } = await api.searchApi.searchPerson({ name });
people = data;
searchWord = name;
} catch (error) {
people = [];
handleError(error, "Can't search people");
} finally {
clearTimeout(timeout);
}

isSearchingPeople = false;
};
let focusedElements: (HTMLButtonElement | null)[] = Array(20)
.fill(null)
.map(() => null);
let indexFocus: number | null = null;

$: isAllArchive = Array.from($selectedAssets).every((asset) => asset.isArchived);
$: isAllFavorite = Array.from($selectedAssets).every((asset) => asset.isFavorite);
Expand All @@ -114,10 +100,14 @@
$: {
if (people) {
suggestedPeople = !name ? [] : searchNameLocal(name, people, 5, data.person.id);
indexFocus = null;
}
}

const onKeyboardPress = (event: KeyboardEvent) => handleKeyboardPress(event);

onMount(() => {
document.addEventListener('keydown', onKeyboardPress);
const action = $page.url.searchParams.get('action');
const getPreviousRoute = $page.url.searchParams.get('previousRoute');
if (getPreviousRoute && !isExternalUrl(getPreviousRoute)) {
Expand All @@ -127,8 +117,75 @@
viewMode = ViewMode.MERGE_FACES;
}
});

onDestroy(() => {
if (browser) {
document.removeEventListener('keydown', onKeyboardPress);
}
});

const handleKeyboardPress = (event: KeyboardEvent) => {
if (suggestedPeople.length === 0) {
return;
}
if (!$showAssetViewer) {
event.stopPropagation();
switch (event.key) {
case 'Tab':
case 'ArrowDown':
event.preventDefault();
if (indexFocus === null) {
indexFocus = 0;
} else if (indexFocus === suggestedPeople.length - 1) {
indexFocus = 0;
} else {
indexFocus++;
}
focusedElements[indexFocus]?.focus();
return;
case 'ArrowUp':
if (indexFocus === null) {
indexFocus = 0;
return;
}
if (indexFocus === 0) {
indexFocus = suggestedPeople.length - 1;
} else {
indexFocus--;
}
focusedElements[indexFocus]?.focus();

return;
case 'Enter':
if (indexFocus !== null) {
handleSuggestPeople(suggestedPeople[indexFocus]);
}
}
}
};

const searchPeople = async () => {
if ((people.length < 20 && name.startsWith(searchWord)) || name === '') {
return;
}
const timeout = setTimeout(() => (isSearchingPeople = true), 100);
try {
const { data } = await api.searchApi.searchPerson({ name });
indexFocus = null;
people = data;
searchWord = name;
} catch (error) {
people = [];
handleError(error, "Can't search people");
} finally {
clearTimeout(timeout);
}

isSearchingPeople = false;
};

const handleEscape = () => {
if ($showAssetViewer) {
if ($showAssetViewer || viewMode === ViewMode.SUGGEST_MERGE) {
return;
}
if ($isMultiSelectState) {
Expand Down Expand Up @@ -468,24 +525,24 @@
</div>
{:else}
{#each suggestedPeople as person, index (person.id)}
<div
class="flex border-t border-x border-gray-400 dark:border-immich-dark-gray h-14 place-items-center bg-gray-200 p-2 dark:bg-gray-700 hover:bg-gray-300 hover:dark:bg-[#232932] {index ===
<button
bind:this={focusedElements[index]}
class="flex w-full border-t border-gray-400 dark:border-immich-dark-gray h-14 place-items-center bg-gray-200 p-2 dark:bg-gray-700 hover:bg-gray-300 hover:dark:bg-[#232932] focus:bg-gray-300 focus:dark:bg-[#232932] {index ===
suggestedPeople.length - 1
? 'rounded-b-lg border-b'
: ''}"
on:click={() => handleSuggestPeople(person)}
>
<button class="flex w-full place-items-center" on:click={() => handleSuggestPeople(person)}>
<ImageThumbnail
circle
shadow
url={api.getPeopleThumbnailUrl(person.id)}
altText={person.name}
widthStyle="2rem"
heightStyle="2rem"
/>
<p class="ml-4 text-gray-700 dark:text-gray-100">{person.name}</p>
</button>
</div>
<ImageThumbnail
circle
shadow
url={api.getPeopleThumbnailUrl(person.id)}
altText={person.name}
widthStyle="2rem"
heightStyle="2rem"
/>
<p class="ml-4 text-gray-700 dark:text-gray-100">{person.name}</p>
</button>
{/each}
{/if}
</div>
Expand Down