From 13a4598ddef7a0fc09975a2a8b64cd19e6103d46 Mon Sep 17 00:00:00 2001
From: Ben Basten <45583362+ben-basten@users.noreply.github.com>
Date: Tue, 11 Jun 2024 06:57:05 -0400
Subject: [PATCH] fix: refining the little details

- adjust the aria attributes
- intuitive escape key propagation
- extract context into its own file
---
 web/src/lib/actions/list-navigation.ts        |  9 +++++++
 .../context-menu/button-context-menu.svelte   | 27 ++++++++++---------
 .../context-menu/context-menu.svelte          |  2 +-
 .../context-menu/menu-option.svelte           |  2 +-
 .../context-menu/menu.context.ts              |  5 ++++
 .../shared-components/context-menu/menu.ts    |  3 ---
 .../right-click-context-menu.svelte           | 26 +++++++++++++++---
 7 files changed, 54 insertions(+), 20 deletions(-)
 create mode 100644 web/src/lib/components/shared-components/context-menu/menu.context.ts
 delete mode 100644 web/src/lib/components/shared-components/context-menu/menu.ts

diff --git a/web/src/lib/actions/list-navigation.ts b/web/src/lib/actions/list-navigation.ts
index f27e18e64cb54c..0db3116960ab30 100644
--- a/web/src/lib/actions/list-navigation.ts
+++ b/web/src/lib/actions/list-navigation.ts
@@ -27,6 +27,10 @@ interface Options {
    * A function that is called when the dropdown should be closed.
    */
   closeDropdown: () => void;
+  /**
+   * Override the default behavior for the escape key.
+   */
+  onEscape?: (event: KeyboardEvent) => void;
 }
 
 export const listNavigation: Action<HTMLElement, Options> = (node, options: Options) => {
@@ -80,6 +84,11 @@ export const listNavigation: Action<HTMLElement, Options> = (node, options: Opti
   };
 
   const onEscape = (event: KeyboardEvent) => {
+    const { onEscape } = options;
+    if (onEscape) {
+      onEscape(event);
+      return;
+    }
     event.stopPropagation();
     close();
   };
diff --git a/web/src/lib/components/shared-components/context-menu/button-context-menu.svelte b/web/src/lib/components/shared-components/context-menu/button-context-menu.svelte
index 59ac58be9c115f..cdda8ca1c3d8ca 100644
--- a/web/src/lib/components/shared-components/context-menu/button-context-menu.svelte
+++ b/web/src/lib/components/shared-components/context-menu/button-context-menu.svelte
@@ -1,18 +1,12 @@
-<script lang="ts" context="module">
-  import { clickOutside } from '$lib/actions/click-outside';
-  import { createContext } from '$lib/utils/context';
-
-  const { get: getMenuContext, set: setContext } = createContext<() => void>('context-menu');
-  export { getMenuContext };
-</script>
-
 <script lang="ts">
   import CircleIconButton, { type Color } from '$lib/components/elements/buttons/circle-icon-button.svelte';
   import ContextMenu from '$lib/components/shared-components/context-menu/context-menu.svelte';
+  import { setMenuContext } from '$lib/components/shared-components/context-menu/menu.context';
   import { getContextMenuPosition, type Align } from '$lib/utils/context-menu';
   import { shortcuts } from '$lib/actions/shortcut';
   import { focusOutside } from '$lib/actions/focus-outside';
   import { listNavigation } from '$lib/actions/list-navigation';
+  import { clickOutside } from '$lib/actions/click-outside';
   import { generateId } from '$lib/utils/generate-id';
   import Portal from '$lib/components/shared-components/portal/portal.svelte';
 
@@ -43,10 +37,10 @@
   let menuContainer: HTMLUListElement;
   let buttonContainer: HTMLDivElement;
   let selectedId: string | undefined = undefined;
-  let id = generateId();
 
-  $: buttonId = `context-menu-button-${id}`;
-  $: menuId = `context-menu-${id}`;
+  const id = generateId();
+  const buttonId = `context-menu-button-${id}`;
+  const menuId = `context-menu-${id}`;
 
   const openDropdown = (event: KeyboardEvent) => {
     contextMenuPosition = getContextMenuPosition(event, align);
@@ -67,12 +61,20 @@
     }
   };
 
+  const onEscape = (event: KeyboardEvent) => {
+    if (showContextMenu) {
+      // if the dropdown is open, stop the event from propagating
+      event.stopPropagation();
+    }
+    closeDropdown();
+  };
+
   const closeDropdown = () => {
     selectedId = undefined;
     showContextMenu = false;
   };
 
-  setContext(() => {
+  setMenuContext(() => {
     showContextMenu = false;
     const button: HTMLButtonElement | null = buttonContainer.querySelector(`#${buttonId}`);
     button?.focus();
@@ -95,6 +97,7 @@
       openDropdown,
       closeDropdown,
       selectionChanged: (node) => (selectedId = node?.id),
+      onEscape,
     }}
     bind:this={buttonContainer}
   >
diff --git a/web/src/lib/components/shared-components/context-menu/context-menu.svelte b/web/src/lib/components/shared-components/context-menu/context-menu.svelte
index 259e74f9c8dec1..47a538748fdf79 100644
--- a/web/src/lib/components/shared-components/context-menu/context-menu.svelte
+++ b/web/src/lib/components/shared-components/context-menu/context-menu.svelte
@@ -34,7 +34,6 @@
 </script>
 
 <div
-  aria-labelledby={ariaLabelledBy}
   bind:clientHeight={height}
   class="absolute z-10 min-w-[200px] w-max max-w-[300px] overflow-hidden rounded-lg shadow-lg"
   style:left="{left}px"
@@ -45,6 +44,7 @@
   <ul
     {id}
     aria-activedescendant={ariaActiveDescendant}
+    aria-labelledby={ariaLabelledBy}
     bind:this={menuElement}
     class:max-h-[100vh]={isVisible}
     class:max-h-0={!isVisible}
diff --git a/web/src/lib/components/shared-components/context-menu/menu-option.svelte b/web/src/lib/components/shared-components/context-menu/menu-option.svelte
index c80839f0ee2b43..64e5e5b18a48f0 100644
--- a/web/src/lib/components/shared-components/context-menu/menu-option.svelte
+++ b/web/src/lib/components/shared-components/context-menu/menu-option.svelte
@@ -1,7 +1,7 @@
 <script lang="ts">
   import Icon from '$lib/components/elements/icon.svelte';
   import { generateId } from '$lib/utils/generate-id';
-  import { getMenuContext } from '$lib/components/shared-components/context-menu/button-context-menu.svelte';
+  import { getMenuContext } from '$lib/components/shared-components/context-menu/menu.context';
   import { createEventDispatcher } from 'svelte';
 
   export let text = '';
diff --git a/web/src/lib/components/shared-components/context-menu/menu.context.ts b/web/src/lib/components/shared-components/context-menu/menu.context.ts
new file mode 100644
index 00000000000000..e9c1436631a687
--- /dev/null
+++ b/web/src/lib/components/shared-components/context-menu/menu.context.ts
@@ -0,0 +1,5 @@
+import { createContext } from '$lib/utils/context';
+
+const { get: getMenuContext, set: setMenuContext } = createContext<() => void>();
+
+export { getMenuContext, setMenuContext };
diff --git a/web/src/lib/components/shared-components/context-menu/menu.ts b/web/src/lib/components/shared-components/context-menu/menu.ts
deleted file mode 100644
index 108b2fd1aadd61..00000000000000
--- a/web/src/lib/components/shared-components/context-menu/menu.ts
+++ /dev/null
@@ -1,3 +0,0 @@
-const key = {};
-
-export { key };
diff --git a/web/src/lib/components/shared-components/context-menu/right-click-context-menu.svelte b/web/src/lib/components/shared-components/context-menu/right-click-context-menu.svelte
index 5be0f77622c820..35818e8b6a9e4c 100644
--- a/web/src/lib/components/shared-components/context-menu/right-click-context-menu.svelte
+++ b/web/src/lib/components/shared-components/context-menu/right-click-context-menu.svelte
@@ -1,8 +1,10 @@
 <script lang="ts">
-  import { setContext, tick } from 'svelte';
+  import { tick } from 'svelte';
   import ContextMenu from '$lib/components/shared-components/context-menu/context-menu.svelte';
   import { shortcuts } from '$lib/actions/shortcut';
   import { listNavigation } from '$lib/actions/list-navigation';
+  import { generateId } from '$lib/utils/generate-id';
+  import { setMenuContext } from '$lib/components/shared-components/context-menu/menu.context';
 
   export let title: string;
   export let direction: 'left' | 'right' = 'right';
@@ -17,6 +19,10 @@
   let triggerElement: HTMLElement | undefined = undefined;
   let buttonElement: HTMLButtonElement;
 
+  const id = generateId();
+  const buttonId = `context-menu-button-${id}`;
+  const menuId = `context-menu-${id}`;
+
   $: {
     if (isOpen && buttonElement) {
       triggerElement = document.activeElement as HTMLElement;
@@ -63,12 +69,16 @@
     }
   };
 
-  setContext('context-menu', closeContextMenu);
+  setMenuContext(closeContextMenu);
 </script>
 
 {#key uniqueKey}
   {#if isOpen}
     <button
+      aria-controls={menuId}
+      aria-haspopup={true}
+      aria-expanded={isOpen}
+      id={buttonId}
       bind:this={buttonElement}
       type="button"
       class="sr-only"
@@ -102,7 +112,17 @@
       on:contextmenu|preventDefault={reopenContextMenu}
       role="presentation"
     >
-      <ContextMenu {x} {y} {direction} onClose={closeContextMenu} bind:menuElement={contextMenuElement} isVisible>
+      <ContextMenu
+        {direction}
+        {x}
+        {y}
+        ariaActiveDescendant={selectedId}
+        ariaLabelledBy={buttonId}
+        bind:menuElement={contextMenuElement}
+        id={menuId}
+        isVisible
+        onClose={closeContextMenu}
+      >
         <slot />
       </ContextMenu>
     </section>