From a0f6d7444a3b48df8fc03484bed46ce012aa77b5 Mon Sep 17 00:00:00 2001
From: Michel Heusschen <59014050+michelheusschen@users.noreply.github.com>
Date: Tue, 9 Jul 2024 05:42:12 +0200
Subject: [PATCH] feat(web): improve show & hide people accessibility (#10954)

---
 .../manage-people-visibility.spec.ts          | 32 +++++++++----------
 .../manage-people-visibility.svelte           | 26 +++++++++------
 web/src/lib/i18n/en.json                      |  5 ++-
 web/src/routes/(user)/people/+page.svelte     |  8 +++--
 4 files changed, 41 insertions(+), 30 deletions(-)

diff --git a/web/src/lib/components/faces-page/manage-people-visibility.spec.ts b/web/src/lib/components/faces-page/manage-people-visibility.spec.ts
index e9120553ae..a6ad24ac2c 100644
--- a/web/src/lib/components/faces-page/manage-people-visibility.spec.ts
+++ b/web/src/lib/components/faces-page/manage-people-visibility.spec.ts
@@ -3,6 +3,7 @@ import ManagePeopleVisibility from '$lib/components/faces-page/manage-people-vis
 import type { PersonResponseDto } from '@immich/sdk';
 import { personFactory } from '@test-data/factories/person-factory';
 import { render } from '@testing-library/svelte';
+import { tick } from 'svelte';
 
 describe('ManagePeopleVisibility Component', () => {
   let personVisible: PersonResponseDto;
@@ -48,10 +49,8 @@ describe('ManagePeopleVisibility Component', () => {
       },
     });
 
-    const toggleButton = getByTitle('toggle_visibility');
-    toggleButton.click();
-    const saveButton = getByText('done');
-    saveButton.click();
+    getByTitle('hide_unnamed_people').click();
+    getByText('done').click();
 
     expect(sdkMock.updatePeople).toHaveBeenCalledWith({
       peopleUpdateDto: {
@@ -60,7 +59,7 @@ describe('ManagePeopleVisibility Component', () => {
     });
   });
 
-  it('hides all people on second button press', () => {
+  it('hides all people on second button press', async () => {
     const { getByText, getByTitle } = render(ManagePeopleVisibility, {
       props: {
         people: [personVisible, personHidden, personWithoutName],
@@ -68,11 +67,10 @@ describe('ManagePeopleVisibility Component', () => {
       },
     });
 
-    const toggleButton = getByTitle('toggle_visibility');
-    toggleButton.click();
-    toggleButton.click();
-    const saveButton = getByText('done');
-    saveButton.click();
+    getByTitle('hide_unnamed_people').click();
+    await tick();
+    getByTitle('hide_all_people').click();
+    getByText('done').click();
 
     expect(sdkMock.updatePeople).toHaveBeenCalledWith({
       peopleUpdateDto: {
@@ -84,7 +82,7 @@ describe('ManagePeopleVisibility Component', () => {
     });
   });
 
-  it('shows all people on third button press', () => {
+  it('shows all people on third button press', async () => {
     const { getByText, getByTitle } = render(ManagePeopleVisibility, {
       props: {
         people: [personVisible, personHidden, personWithoutName],
@@ -92,12 +90,12 @@ describe('ManagePeopleVisibility Component', () => {
       },
     });
 
-    const toggleButton = getByTitle('toggle_visibility');
-    toggleButton.click();
-    toggleButton.click();
-    toggleButton.click();
-    const saveButton = getByText('done');
-    saveButton.click();
+    getByTitle('hide_unnamed_people').click();
+    await tick();
+    getByTitle('hide_all_people').click();
+    await tick();
+    getByTitle('show_all_people').click();
+    getByText('done').click();
 
     expect(sdkMock.updatePeople).toHaveBeenCalledWith({
       peopleUpdateDto: {
diff --git a/web/src/lib/components/faces-page/manage-people-visibility.svelte b/web/src/lib/components/faces-page/manage-people-visibility.svelte
index 8744950111..1fa9ea5b9b 100644
--- a/web/src/lib/components/faces-page/manage-people-visibility.svelte
+++ b/web/src/lib/components/faces-page/manage-people-visibility.svelte
@@ -25,12 +25,13 @@
 
   export let people: PersonResponseDto[];
   export let onClose: () => void;
+  export let titleId: string | undefined = undefined;
 
   let toggleVisibility = ToggleVisibility.SHOW_ALL;
   let showLoadingSpinner = false;
 
   $: personIsHidden = getPersonIsHidden(people);
-  $: toggleIcon = toggleIconOptions[toggleVisibility];
+  $: toggleButton = toggleButtonOptions[getNextVisibility(toggleVisibility)];
 
   const getPersonIsHidden = (people: PersonResponseDto[]) => {
     const personIsHidden: Record<string, boolean> = {};
@@ -40,11 +41,13 @@
     return personIsHidden;
   };
 
-  const toggleIconOptions: Record<ToggleVisibility, string> = {
-    [ToggleVisibility.HIDE_ALL]: mdiEyeOff,
-    [ToggleVisibility.HIDE_UNNANEMD]: mdiEyeSettings,
-    [ToggleVisibility.SHOW_ALL]: mdiEye,
-  };
+  $: toggleButtonOptions = ((): Record<ToggleVisibility, { icon: string; label: string }> => {
+    return {
+      [ToggleVisibility.HIDE_ALL]: { icon: mdiEyeOff, label: $t('hide_all_people') },
+      [ToggleVisibility.HIDE_UNNANEMD]: { icon: mdiEyeSettings, label: $t('hide_unnamed_people') },
+      [ToggleVisibility.SHOW_ALL]: { icon: mdiEye, label: $t('show_all_people') },
+    };
+  })();
 
   const getNextVisibility = (toggleVisibility: ToggleVisibility) => {
     if (toggleVisibility === ToggleVisibility.SHOW_ALL) {
@@ -117,14 +120,14 @@
   <div class="flex items-center">
     <CircleIconButton title={$t('close')} icon={mdiClose} on:click={onClose} />
     <div class="flex gap-2 items-center">
-      <p class="ml-2">{$t('show_and_hide_people')}</p>
+      <p id={titleId} class="ml-2">{$t('show_and_hide_people')}</p>
       <p class="text-sm text-gray-400 dark:text-gray-600">({people.length.toLocaleString($locale)})</p>
     </div>
   </div>
   <div class="flex items-center justify-end">
     <div class="flex items-center md:mr-4">
       <CircleIconButton title={$t('reset_people_visibility')} icon={mdiRestart} on:click={handleResetVisibility} />
-      <CircleIconButton title={$t('toggle_visibility')} icon={toggleIcon} on:click={handleToggleVisibility} />
+      <CircleIconButton title={toggleButton.label} icon={toggleButton.icon} on:click={handleToggleVisibility} />
     </div>
     {#if !showLoadingSpinner}
       <Button on:click={handleSaveVisibility} size="sm" rounded="lg">{$t('done')}</Button>
@@ -137,14 +140,17 @@
 <div class="flex flex-wrap gap-1 bg-immich-bg p-2 pb-8 dark:bg-immich-dark-bg md:px-8 mt-16">
   <div class="w-full grid grid-cols-2 sm:grid-cols-3 lg:grid-cols-5 xl:grid-cols-7 2xl:grid-cols-9 gap-1">
     {#each people as person, index (person.id)}
+      {@const hidden = personIsHidden[person.id]}
       <button
         type="button"
         class="group relative"
-        on:click={() => (personIsHidden[person.id] = !personIsHidden[person.id])}
+        on:click={() => (personIsHidden[person.id] = !hidden)}
+        aria-pressed={hidden}
+        aria-label={person.name ? $t('hide_named_person', { values: { name: person.name } }) : $t('hide_person')}
       >
         <ImageThumbnail
           preload={index < 20}
-          hidden={personIsHidden[person.id]}
+          {hidden}
           shadow
           url={getPeopleThumbnailUrl(person)}
           altText={person.name}
diff --git a/web/src/lib/i18n/en.json b/web/src/lib/i18n/en.json
index 4b4763f0e6..549322ed98 100644
--- a/web/src/lib/i18n/en.json
+++ b/web/src/lib/i18n/en.json
@@ -692,9 +692,12 @@
   "group_year": "Group by year",
   "has_quota": "Has quota",
   "hi_user": "Hi {name} ({email})",
+  "hide_all_people": "Hide all people",
   "hide_gallery": "Hide gallery",
+  "hide_named_person": "Hide person {name}",
   "hide_password": "Hide password",
   "hide_person": "Hide person",
+  "hide_unnamed_people": "Hide unnamed people",
   "host": "Host",
   "hour": "Hour",
   "image": "Image",
@@ -1022,6 +1025,7 @@
   "sharing_sidebar_description": "Display a link to Sharing in the sidebar",
   "shift_to_permanent_delete": "press ⇧ to permanently delete asset",
   "show_album_options": "Show album options",
+  "show_all_people": "Show all people",
   "show_and_hide_people": "Show & hide people",
   "show_file_location": "Show file location",
   "show_gallery": "Show gallery",
@@ -1084,7 +1088,6 @@
   "to_trash": "Trash",
   "toggle_settings": "Toggle settings",
   "toggle_theme": "Toggle theme",
-  "toggle_visibility": "Toggle visibility",
   "total_usage": "Total usage",
   "trash": "Trash",
   "trash_all": "Trash All",
diff --git a/web/src/routes/(user)/people/+page.svelte b/web/src/routes/(user)/people/+page.svelte
index 1b646d7d08..d40ef625b6 100644
--- a/web/src/routes/(user)/people/+page.svelte
+++ b/web/src/routes/(user)/people/+page.svelte
@@ -28,6 +28,7 @@
   import { quintOut } from 'svelte/easing';
   import { fly } from 'svelte/transition';
   import type { PageData } from './$types';
+  import { focusTrap } from '$lib/actions/focus-trap';
 
   export let data: PageData;
 
@@ -376,11 +377,14 @@
 </UserPageLayout>
 
 {#if selectHidden}
-  <!-- TODO: Add focus trap -->
   <section
     transition:fly={{ y: innerHeight, duration: 150, easing: quintOut, opacity: 0 }}
     class="absolute left-0 top-0 z-[9999] h-full w-full bg-immich-bg dark:bg-immich-dark-bg"
+    role="dialog"
+    aria-modal="true"
+    aria-labelledby="manage-visibility-title"
+    use:focusTrap
   >
-    <ManagePeopleVisibility bind:people onClose={() => (selectHidden = false)} />
+    <ManagePeopleVisibility bind:people titleId="manage-visibility-title" onClose={() => (selectHidden = false)} />
   </section>
 {/if}