gitbutlerapp · mtsgrd · Sep 24, 2025
diff --git a/packages/ui/src/lib/components/ContextMenu.svelte b/packages/ui/src/lib/components/ContextMenu.svelte
@@ -329,13 +329,6 @@
 		return isVisible;
 	}
 
-	function handleKeyNavigation(e: KeyboardEvent) {
-		if (e.key === 'Escape') {
-			e.preventDefault();
-			close();
-		}
-	}
-
 	$effect(() => {
 		if (!menuContainer) return;
 		const config = { attributes: false, childList: true, subtree: true };
@@ -358,17 +351,26 @@
 </script>
 
 {#if isVisible}
-	<div class="portal-wrap" use:portal={'body'}>
-		<!-- svelte-ignore a11y_autofocus -->
+	<!-- `use:focusable` must come before `use:portal`-->
+	<div
+		class="portal-wrap"
+		use:focusable={{
+			activate: true,
+			trap: true,
+			vertical: true,
+			onEsc: () => {
+				close();
+				return true;
+			}
+		}}
+		use:portal={'body'}
+	>
 		<div
 			data-testid={testId}
 			bind:this={menuContainer}
 			tabindex="-1"
-			use:focusable={{ activate: true, isolate: true, focusable: true, dim: true, trap: true }}
-			autofocus
 			{onclick}
 			{onkeypress}
-			onkeydown={handleKeyNavigation}
 			class="context-menu hide-native-scrollbar"
 			class:top-oriented={side === 'top'}
 			class:bottom-oriented={side === 'bottom'}

diff --git a/packages/ui/src/lib/components/ContextMenuItem.svelte b/packages/ui/src/lib/components/ContextMenuItem.svelte
@@ -1,6 +1,7 @@
 <script lang="ts">
 	import Icon from '$components/Icon.svelte';
 	import Tooltip from '$components/Tooltip.svelte';
+	import { focusable } from '$lib/focus/focusable';
 	import { keysStringToArr } from '$lib/utils/hotkeys';
 	import { getContext } from 'svelte';
 	import type iconsJson from '@gitbutler/ui/data/icons.json';
@@ -62,12 +63,13 @@
 	<button
 		data-testid={testId}
 		type="button"
-		class="menu-item focus-state no-select"
+		class="menu-item no-select"
 		style:--item-height={caption ? 'auto' : '1.625rem'}
 		class:disabled
 		{disabled}
 		onclick={handleClick}
 		onmouseenter={handleMouseEnter}
+		use:focusable={{ focusable: true, button: true }}
 	>
 		<div class="menu-item__content">
 			{#if icon}
@@ -119,6 +121,7 @@
 		padding: 6px 8px;
 		gap: 4px;
 		border-radius: var(--radius-s);
+		outline: none;
 		color: var(--clr-text-1);
 		text-align: left;
 		cursor: pointer;

diff --git a/packages/ui/src/lib/components/ContextMenuItemSubmenu.svelte b/packages/ui/src/lib/components/ContextMenuItemSubmenu.svelte
@@ -110,39 +110,6 @@
 		isSubmenuOpen = false;
 		contextMenu?.close();
 	}
-
-	// Handle arrow key navigation
-	function handleKeyDown(e: KeyboardEvent) {
-		if (disabled) return;
-
-		switch (e.key) {
-			case 'ArrowRight':
-				if (submenuSide === 'right') {
-					e.preventDefault();
-					e.stopPropagation();
-					isSubmenuOpen = true;
-					contextMenu?.open();
-				}
-				break;
-			case 'ArrowLeft':
-				if (submenuSide === 'left') {
-					e.preventDefault();
-					e.stopPropagation();
-					isSubmenuOpen = true;
-					contextMenu?.open();
-				}
-				break;
-			case 'Enter':
-			case ' ':
-				e.preventDefault();
-				e.stopPropagation();
-				if (disabled) return;
-
-				isSubmenuOpen = !isSubmenuOpen;
-				contextMenu?.toggle();
-				break;
-		}
-	}
 </script>
 
 <div
@@ -151,11 +118,7 @@
 	class:active={isSubmenuOpen}
 	onmouseenter={handleMouseEnter}
 	onmouseleave={handleMouseLeave}
-	onkeydown={handleKeyDown}
-	role="menuitem"
-	aria-haspopup="menu"
-	aria-expanded={isSubmenuOpen}
-	tabindex="-1"
+	role="group"
 >
 	<ContextMenuItem
 		{icon}
@@ -182,7 +145,6 @@
 	align={submenuVerticalAlign === 'top' ? 'start' : 'end'}
 	onclose={() => {
 		isSubmenuOpen = false;
-		menuItemElement?.focus();
 	}}
 >
 	{@render submenu({ close: closeSubmenu })}

diff --git a/packages/ui/src/lib/components/KebabButton.svelte b/packages/ui/src/lib/components/KebabButton.svelte
@@ -68,6 +68,17 @@
 		onclick?.(e.currentTarget as HTMLElement);
 	}
 
+	function onKeyDown(e: KeyboardEvent) {
+		if (e.key !== 'Enter') {
+			return;
+		}
+		e.stopPropagation();
+		e.preventDefault();
+		isContextMenuOpen = !isContextMenuOpen;
+		openedViaClick = isContextMenuOpen; // Track if opened via click
+		onclick?.(e.currentTarget as HTMLElement);
+	}
+
 	$effect(() => {
 		if (contextElement) {
 			contextElement.addEventListener('contextmenu', onContextMenu);
@@ -110,6 +121,7 @@
 		class:visible={visible || isContextMenuOpen}
 		class:activated={activated || (isContextMenuOpen && openedViaClick)}
 		onclick={onClick}
+		onkeypress={onKeyDown}
 		data-testid={testId}
 	>
 		<Icon name="kebab" />
@@ -123,6 +135,7 @@
 		kind="ghost"
 		activated={activated || (isContextMenuOpen && openedViaClick)}
 		onclick={onClick}
+		onkeydown={onKeyDown}
 	/>
 {/if}
 

diff --git a/packages/ui/src/lib/focus/focusManager.ts b/packages/ui/src/lib/focus/focusManager.ts
@@ -154,7 +154,7 @@ export class FocusManager {
 
 		this.unregisterElement(element);
 
-		const parentNode = options.isolate ? undefined : this.findParentNode(element);
+		const parentNode = this.findParentNode(element);
 
 		const newNode: FocusableNode = {
 			element,
@@ -166,14 +166,14 @@ export class FocusManager {
 		this.nodeMap.set(element, newNode);
 		this.establishParentChildRelationships(element, newNode, parentNode);
 		this.resolvePendingRelationships();
-		this.handlePendingRegistration(element, parentNode, options);
+		this.handlePendingRegistration(element, parentNode);
 
 		if (this.fModeManager.active) {
 			this.fModeManager.addElement(element, newNode);
 		}
 
 		if (options.activate) {
-			this.setActiveNode(newNode);
+			this.setActiveNode(this.findNavigableDescendant(newNode));
 		}
 	}
 
@@ -254,7 +254,7 @@ export class FocusManager {
 			(forward && currentIndex === navigableSiblings.length - 1) ||
 			(!forward && currentIndex === 0);
 
-		if (isAtBoundary) {
+		if (isAtBoundary && !parentNode.options.trap) {
 			// Traverse parents while vertical: true
 			const linkedTarget = this.findNextInColumnNode(currentNode, forward);
 			if (!linkedTarget) return false;
@@ -266,7 +266,7 @@ export class FocusManager {
 
 	focusNextVertical(forward: boolean): boolean {
 		if (!this.currentNode?.element) return false;
-		const nextChild = this.findInNextColumn(this.currentNode.element, forward);
+		const nextChild = this.findInNextColumn(this.currentNode, forward);
 		if (nextChild) {
 			return this.setActiveNode(nextChild);
 		}
@@ -326,6 +326,9 @@ export class FocusManager {
 				if (navigableChild) {
 					return navigableChild;
 				}
+				if (node.options.trap) {
+					return;
+				}
 			}
 			pointer = pointer.parentElement;
 		}
@@ -521,7 +524,7 @@ export class FocusManager {
 			case 'right':
 				if (trap) return true;
 				if (!this.focusNextVertical(true)) {
-					this.focusAnyNode();
+					// this.focusAnyNode();
 				}
 				return true;
 
@@ -620,8 +623,7 @@ export class FocusManager {
 	}
 
 	// Finds focusable in adjacent column for horizontal navigation
-	findInNextColumn(element: HTMLElement, forward: boolean): FocusableNode | undefined {
-		const node = this.getNode(element);
+	findInNextColumn(node: FocusableNode, forward: boolean): FocusableNode | undefined {
 		if (!node) return undefined;
 
 		let searchNode: FocusableNode | undefined = node.parent;
@@ -631,7 +633,7 @@ export class FocusManager {
 			if (!ancestorNode) return;
 
 			// Find the current branch from the non-list parent's perspective
-			const ancestorChild = this.findChildNodeByDescendent(ancestorNode, element);
+			const ancestorChild = this.findChildNodeByDescendent(ancestorNode, node);
 			if (!ancestorChild) return;
 
 			// Find non-button siblings of the current branch for navigation
@@ -663,20 +665,22 @@ export class FocusManager {
 			if (!current.options.vertical) {
 				return current;
 			}
+			if (current.options.trap) {
+				return;
+			}
 			current = current.parent;
 		}
-		return undefined;
 	}
 
 	// Finds which child branch contains the given element
 	private findChildNodeByDescendent(
 		ancestor: FocusableNode,
-		element: HTMLElement
+		node: FocusableNode
 	): FocusableNode | undefined {
 		// Check each child (including buttons) to see which one contains our current element
 		// We need to check all children here because we're looking for containment, not navigation
 		for (const child of ancestor.children) {
-			if (this.isNodeDescendantOf(child, element)) {
+			if (this.isNodeDescendantOf(child, node.element)) {
 				return child;
 			}
 		}
@@ -773,12 +777,8 @@ export class FocusManager {
 		});
 	}
 
-	private handlePendingRegistration(
-		element: HTMLElement,
-		parentNode: FocusableNode | undefined,
-		options: FocusableOptions
-	) {
-		if (!parentNode && !options.isolate) {
+	private handlePendingRegistration(element: HTMLElement, parentNode: FocusableNode | undefined) {
+		if (!parentNode) {
 			this.pendingRelationships.push(element);
 		}
 	}
@@ -1044,7 +1044,6 @@ export class FocusManager {
 		if (node.options.disabled) flags.push('disabled');
 		if (node.options.trap) flags.push('trap');
 		if (node.options.vertical) flags.push('vertical');
-		if (node.options.isolate) flags.push('isolate');
 		if (this.isContainerElement(node)) flags.push('container');
 		const flagsStr = flags.length > 0 ? ` [${flags.join(', ')}]` : '';
 

diff --git a/packages/ui/src/lib/focus/focusTypes.ts b/packages/ui/src/lib/focus/focusTypes.ts
@@ -23,12 +23,10 @@ export interface FocusableOptions {
 	disabled?: boolean;
 	// Treat children as vertically oriented (changes arrow key behavior, automatically skips during navigation)
 	vertical?: boolean;
-	// Prevent keyboard navigation from leaving this element
-	trap?: boolean;
 	// Automatically focus this element when registered
 	activate?: boolean;
-	// Don't establish parent-child relationships with other focusables
-	isolate?: boolean;
+	// Prevent keyboard navigation from leaving this element
+	trap?: boolean;
 	// Never highlight the element
 	dim?: boolean;
 	// Automatically trigger onAction when this element becomes active