mirror of
https://github.com/Comfy-Org/ComfyUI_frontend.git
synced 2026-03-13 00:50:01 +00:00
fix: improve canvas menu keyboard navigation and ARIA accessibility (#9526)
## Summary The canvas mode selector popover (Select/Hand mode) uses plain `div` elements for its menu items, making them completely inaccessible to keyboard-only users and screen readers. This PR replaces them with proper semantic HTML and ARIA attributes. ## Problem (AS-IS) As reported in #9519, the canvas mode selector popover has the following accessibility issues: 1. **Menu items are `div` elements** — they cannot receive keyboard focus, so users cannot Tab into the popover or activate items with Enter/Space. Keyboard-only users are completely locked out of switching between Select and Hand (pan) mode via the popover. 2. **No ARIA roles** — screen readers announce the popover content as generic text rather than an interactive menu. Users relying on assistive technology have no way to understand that these are selectable options. 3. **No keyboard navigation within the popover** — even if a user somehow focuses an item, there are no ArrowUp/ArrowDown handlers to move between options, which is the standard interaction pattern for `role="menu"` widgets (WAI-ARIA Menu Pattern). 4. **Decorative icons are not hidden from assistive technology** — icon elements (`<i>` tags) are exposed to screen readers, adding noise to the announcement. 5. **The bottom toolbar (`GraphCanvasMenu`) lacks semantic grouping** — the `ButtonGroup` container has no `role="toolbar"` or `aria-label`, so screen readers cannot convey that these buttons form a related control group. > Note: Pan mode itself already has keyboard shortcuts (`H` for Hand/Lock, `V` for Select/Unlock), but the popover UI that surfaces these options is not keyboard-accessible. ## Solution (TO-BE) 1. **Replace `div` → `button`** for menu items in `CanvasModeSelector` — buttons are natively focusable and activatable via Enter/Space without extra work. 2. **Add `role="menu"` on the container and `role="menuitem"` on each option** — screen readers now announce "Canvas Mode menu" with two menu items. 3. **Add ArrowUp/ArrowDown keyboard navigation** with wrap-around — pressing ArrowDown on "Select" moves focus to "Hand", and vice versa. This follows the WAI-ARIA Menu Pattern. 4. **Add `aria-label` to each menu item and `aria-hidden="true"` to decorative icons** — screen readers announce "Select" / "Hand" clearly without icon noise. 5. **Add `role="toolbar"` with `aria-label="Canvas Toolbar"` to the `ButtonGroup`** — screen readers identify the bottom control bar as a coherent toolbar. ## Changes - **What**: Accessibility improvements to `CanvasModeSelector` popover and `GraphCanvasMenu` toolbar - **Dependencies**: None — only HTML/ARIA attribute changes and two new i18n keys (`canvasMode`, `canvasToolbar`) ## Review Focus - Verify the `button` elements render visually identical to the previous `div` elements (same padding, hover styles, cursor) - Confirm ArrowUp/ArrowDown navigation works correctly in the popover - Check that screen readers announce the menu and toolbar correctly Fixes #9519 > Note: The issue also requests Space-bar hold-to-pan, Tab through node ports, and link creation mode keyboard shortcuts. These are significant new features beyond the scope of this accessibility fix and should be tracked separately. ## Test plan - [x] Unit tests for ARIA roles, button elements, aria-labels, aria-hidden, and arrow key navigation (7 tests) - [ ] Manual: open canvas mode selector popover → Tab focuses first item → ArrowDown/ArrowUp navigates → Enter/Space selects - [ ] Screen reader: verify "Canvas Mode menu" with "Select" and "Hand" menu items are announced ┆Issue is synchronized with this [Notion page](https://www.notion.so/PR-9526-fix-improve-canvas-menu-keyboard-navigation-and-ARIA-accessibility-31c6d73d3650814c9487ecf748cf6a99) by [Unito](https://www.unito.io) --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
153
src/components/graph/CanvasModeSelector.test.ts
Normal file
153
src/components/graph/CanvasModeSelector.test.ts
Normal file
@@ -0,0 +1,153 @@
|
||||
import { mount } from '@vue/test-utils'
|
||||
import { describe, expect, it, vi } from 'vitest'
|
||||
import { createI18n } from 'vue-i18n'
|
||||
|
||||
import CanvasModeSelector from '@/components/graph/CanvasModeSelector.vue'
|
||||
|
||||
const mockExecute = vi.fn()
|
||||
const mockGetCommand = vi.fn().mockReturnValue({
|
||||
keybinding: {
|
||||
combo: {
|
||||
getKeySequences: () => ['V']
|
||||
}
|
||||
}
|
||||
})
|
||||
const mockFormatKeySequence = vi.fn().mockReturnValue('V')
|
||||
|
||||
vi.mock('@/stores/commandStore', () => ({
|
||||
useCommandStore: () => ({
|
||||
execute: mockExecute,
|
||||
getCommand: mockGetCommand,
|
||||
formatKeySequence: mockFormatKeySequence
|
||||
})
|
||||
}))
|
||||
|
||||
vi.mock('@/renderer/core/canvas/canvasStore', () => ({
|
||||
useCanvasStore: () => ({
|
||||
canvas: { read_only: false }
|
||||
})
|
||||
}))
|
||||
|
||||
const i18n = createI18n({
|
||||
legacy: false,
|
||||
locale: 'en',
|
||||
messages: {
|
||||
en: {
|
||||
graphCanvasMenu: {
|
||||
select: 'Select',
|
||||
hand: 'Hand',
|
||||
canvasMode: 'Canvas Mode'
|
||||
}
|
||||
}
|
||||
}
|
||||
})
|
||||
|
||||
const mockPopoverHide = vi.fn()
|
||||
|
||||
function createWrapper() {
|
||||
return mount(CanvasModeSelector, {
|
||||
global: {
|
||||
plugins: [i18n],
|
||||
stubs: {
|
||||
Popover: {
|
||||
template: '<div><slot /></div>',
|
||||
methods: {
|
||||
toggle: vi.fn(),
|
||||
hide: mockPopoverHide
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
})
|
||||
}
|
||||
|
||||
describe('CanvasModeSelector', () => {
|
||||
it('should render menu with menuitemradio roles and aria-checked', () => {
|
||||
const wrapper = createWrapper()
|
||||
|
||||
const menu = wrapper.find('[role="menu"]')
|
||||
expect(menu.exists()).toBe(true)
|
||||
|
||||
const menuItems = wrapper.findAll('[role="menuitemradio"]')
|
||||
expect(menuItems).toHaveLength(2)
|
||||
|
||||
// Select mode is active (read_only: false), so select is checked
|
||||
expect(menuItems[0].attributes('aria-checked')).toBe('true')
|
||||
expect(menuItems[1].attributes('aria-checked')).toBe('false')
|
||||
})
|
||||
|
||||
it('should render menu items as buttons with aria-labels', () => {
|
||||
const wrapper = createWrapper()
|
||||
|
||||
const menuItems = wrapper.findAll('[role="menuitemradio"]')
|
||||
menuItems.forEach((btn) => {
|
||||
expect(btn.element.tagName).toBe('BUTTON')
|
||||
expect(btn.attributes('type')).toBe('button')
|
||||
})
|
||||
expect(menuItems[0].attributes('aria-label')).toBe('Select')
|
||||
expect(menuItems[1].attributes('aria-label')).toBe('Hand')
|
||||
})
|
||||
|
||||
it('should use roving tabindex based on active mode', () => {
|
||||
const wrapper = createWrapper()
|
||||
|
||||
const menuItems = wrapper.findAll('[role="menuitemradio"]')
|
||||
// Select is active (read_only: false) → tabindex 0
|
||||
expect(menuItems[0].attributes('tabindex')).toBe('0')
|
||||
// Hand is inactive → tabindex -1
|
||||
expect(menuItems[1].attributes('tabindex')).toBe('-1')
|
||||
})
|
||||
|
||||
it('should mark icons as aria-hidden', () => {
|
||||
const wrapper = createWrapper()
|
||||
|
||||
const icons = wrapper.findAll('[role="menuitemradio"] i')
|
||||
icons.forEach((icon) => {
|
||||
expect(icon.attributes('aria-hidden')).toBe('true')
|
||||
})
|
||||
})
|
||||
|
||||
it('should expose trigger button with aria-haspopup and aria-expanded', () => {
|
||||
const wrapper = createWrapper()
|
||||
|
||||
const trigger = wrapper.find('[aria-haspopup="menu"]')
|
||||
expect(trigger.exists()).toBe(true)
|
||||
expect(trigger.attributes('aria-label')).toBe('Canvas Mode')
|
||||
expect(trigger.attributes('aria-expanded')).toBe('false')
|
||||
})
|
||||
|
||||
it('should call focus on next item when ArrowDown is pressed', async () => {
|
||||
const wrapper = createWrapper()
|
||||
|
||||
const menuItems = wrapper.findAll('[role="menuitemradio"]')
|
||||
const secondItemEl = menuItems[1].element as HTMLElement
|
||||
const focusSpy = vi.spyOn(secondItemEl, 'focus')
|
||||
|
||||
await menuItems[0].trigger('keydown', { key: 'ArrowDown' })
|
||||
expect(focusSpy).toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it('should call focus on previous item when ArrowUp is pressed', async () => {
|
||||
const wrapper = createWrapper()
|
||||
|
||||
const menuItems = wrapper.findAll('[role="menuitemradio"]')
|
||||
const firstItemEl = menuItems[0].element as HTMLElement
|
||||
const focusSpy = vi.spyOn(firstItemEl, 'focus')
|
||||
|
||||
await menuItems[1].trigger('keydown', { key: 'ArrowUp' })
|
||||
expect(focusSpy).toHaveBeenCalled()
|
||||
})
|
||||
|
||||
it('should close popover on Escape and restore focus to trigger', async () => {
|
||||
const wrapper = createWrapper()
|
||||
|
||||
const menuItems = wrapper.findAll('[role="menuitemradio"]')
|
||||
const trigger = wrapper.find('[aria-haspopup="menu"]')
|
||||
const triggerEl = trigger.element as HTMLElement
|
||||
const focusSpy = vi.spyOn(triggerEl, 'focus')
|
||||
|
||||
await menuItems[0].trigger('keydown', { key: 'Escape' })
|
||||
expect(mockPopoverHide).toHaveBeenCalled()
|
||||
expect(focusSpy).toHaveBeenCalled()
|
||||
})
|
||||
})
|
||||
@@ -4,15 +4,21 @@
|
||||
variant="secondary"
|
||||
class="group h-8 rounded-none! bg-comfy-menu-bg p-0 transition-none! hover:rounded-lg! hover:bg-interface-button-hover-surface!"
|
||||
:style="buttonStyles"
|
||||
:aria-label="$t('graphCanvasMenu.canvasMode')"
|
||||
aria-haspopup="menu"
|
||||
:aria-expanded="isOpen"
|
||||
@click="toggle"
|
||||
>
|
||||
<div class="flex items-center gap-1 pr-0.5">
|
||||
<div
|
||||
class="rounded-lg bg-interface-panel-selected-surface p-2 group-hover:bg-interface-button-hover-surface"
|
||||
>
|
||||
<i :class="currentModeIcon" class="block size-4" />
|
||||
<i :class="currentModeIcon" class="block size-4" aria-hidden="true" />
|
||||
</div>
|
||||
<i class="icon-[lucide--chevron-down] block size-4 pr-1.5" />
|
||||
<i
|
||||
class="icon-[lucide--chevron-down] block size-4 pr-1.5"
|
||||
aria-hidden="true"
|
||||
/>
|
||||
</div>
|
||||
</Button>
|
||||
|
||||
@@ -24,31 +30,54 @@
|
||||
:close-on-escape="true"
|
||||
unstyled
|
||||
:pt="popoverPt"
|
||||
@show="onPopoverShow"
|
||||
@hide="onPopoverHide"
|
||||
>
|
||||
<div class="flex flex-col gap-1">
|
||||
<div
|
||||
class="flex cursor-pointer items-center justify-between px-3 py-2 text-sm hover:bg-node-component-surface-hovered"
|
||||
<div
|
||||
ref="menuRef"
|
||||
class="flex flex-col gap-1"
|
||||
role="menu"
|
||||
:aria-label="$t('graphCanvasMenu.canvasMode')"
|
||||
>
|
||||
<button
|
||||
type="button"
|
||||
role="menuitemradio"
|
||||
:aria-checked="!isCanvasReadOnly"
|
||||
:tabindex="!isCanvasReadOnly ? 0 : -1"
|
||||
class="flex w-full cursor-pointer items-center justify-between rounded-sm border-none bg-transparent px-3 py-2 text-sm text-text-primary outline-none hover:bg-node-component-surface-hovered focus-visible:bg-node-component-surface-hovered"
|
||||
:aria-label="$t('graphCanvasMenu.select')"
|
||||
@click="setMode('select')"
|
||||
@keydown.arrow-down.prevent="focusNextItem"
|
||||
@keydown.arrow-up.prevent="focusPrevItem"
|
||||
@keydown.escape.prevent="closeAndRestoreFocus"
|
||||
>
|
||||
<div class="flex items-center gap-2">
|
||||
<i class="icon-[lucide--mouse-pointer-2] size-4" />
|
||||
<i class="icon-[lucide--mouse-pointer-2] size-4" aria-hidden="true" />
|
||||
<span>{{ $t('graphCanvasMenu.select') }}</span>
|
||||
</div>
|
||||
<span class="text-[9px] text-text-primary">{{
|
||||
unlockCommandText
|
||||
}}</span>
|
||||
</div>
|
||||
</button>
|
||||
|
||||
<div
|
||||
class="flex cursor-pointer items-center justify-between rounded-sm px-3 py-2 text-sm hover:bg-node-component-surface-hovered"
|
||||
<button
|
||||
type="button"
|
||||
role="menuitemradio"
|
||||
:aria-checked="isCanvasReadOnly"
|
||||
:tabindex="isCanvasReadOnly ? 0 : -1"
|
||||
class="flex w-full cursor-pointer items-center justify-between rounded-sm border-none bg-transparent px-3 py-2 text-sm text-text-primary outline-none hover:bg-node-component-surface-hovered focus-visible:bg-node-component-surface-hovered"
|
||||
:aria-label="$t('graphCanvasMenu.hand')"
|
||||
@click="setMode('hand')"
|
||||
@keydown.arrow-down.prevent="focusNextItem"
|
||||
@keydown.arrow-up.prevent="focusPrevItem"
|
||||
@keydown.escape.prevent="closeAndRestoreFocus"
|
||||
>
|
||||
<div class="flex items-center gap-2">
|
||||
<i class="icon-[lucide--hand] size-4" />
|
||||
<i class="icon-[lucide--hand] size-4" aria-hidden="true" />
|
||||
<span>{{ $t('graphCanvasMenu.hand') }}</span>
|
||||
</div>
|
||||
<span class="text-[9px] text-text-primary">{{ lockCommandText }}</span>
|
||||
</div>
|
||||
</button>
|
||||
</div>
|
||||
</Popover>
|
||||
</template>
|
||||
@@ -56,7 +85,7 @@
|
||||
<script setup lang="ts">
|
||||
import Popover from 'primevue/popover'
|
||||
import type { ComponentPublicInstance } from 'vue'
|
||||
import { computed, ref } from 'vue'
|
||||
import { computed, nextTick, ref } from 'vue'
|
||||
|
||||
import Button from '@/components/ui/button/Button.vue'
|
||||
import { useCanvasStore } from '@/renderer/core/canvas/canvasStore'
|
||||
@@ -69,6 +98,8 @@ interface Props {
|
||||
defineProps<Props>()
|
||||
const buttonRef = ref<ComponentPublicInstance | null>(null)
|
||||
const popover = ref<InstanceType<typeof Popover>>()
|
||||
const menuRef = ref<HTMLElement | null>(null)
|
||||
const isOpen = ref(false)
|
||||
const commandStore = useCommandStore()
|
||||
const canvasStore = useCanvasStore()
|
||||
|
||||
@@ -106,6 +137,43 @@ const setMode = (mode: 'select' | 'hand') => {
|
||||
popover.value?.hide()
|
||||
}
|
||||
|
||||
async function onPopoverShow() {
|
||||
isOpen.value = true
|
||||
await nextTick()
|
||||
const checkedItem = menuRef.value?.querySelector<HTMLElement>(
|
||||
'[aria-checked="true"]'
|
||||
)
|
||||
checkedItem?.focus()
|
||||
}
|
||||
|
||||
function onPopoverHide() {
|
||||
isOpen.value = false
|
||||
}
|
||||
|
||||
function closeAndRestoreFocus() {
|
||||
popover.value?.hide()
|
||||
const el = buttonRef.value?.$el || buttonRef.value
|
||||
;(el as HTMLElement)?.focus()
|
||||
}
|
||||
|
||||
function focusNextItem(event: KeyboardEvent) {
|
||||
const items = getMenuItems(event)
|
||||
const index = items.indexOf(event.target as HTMLElement)
|
||||
items[(index + 1) % items.length]?.focus()
|
||||
}
|
||||
|
||||
function focusPrevItem(event: KeyboardEvent) {
|
||||
const items = getMenuItems(event)
|
||||
const index = items.indexOf(event.target as HTMLElement)
|
||||
items[(index - 1 + items.length) % items.length]?.focus()
|
||||
}
|
||||
|
||||
function getMenuItems(event: KeyboardEvent): HTMLElement[] {
|
||||
const menu = (event.target as HTMLElement).closest('[role="menu"]')
|
||||
if (!menu) return []
|
||||
return Array.from(menu.querySelectorAll('[role="menuitemradio"]'))
|
||||
}
|
||||
|
||||
const popoverPt = computed(() => ({
|
||||
root: {
|
||||
class: 'absolute z-50 -translate-y-2'
|
||||
|
||||
@@ -10,6 +10,8 @@
|
||||
></div>
|
||||
|
||||
<ButtonGroup
|
||||
role="toolbar"
|
||||
:aria-label="t('graphCanvasMenu.canvasToolbar')"
|
||||
class="absolute right-0 bottom-0 z-1200 flex-row gap-1 border border-interface-stroke bg-comfy-menu-bg p-2"
|
||||
:style="{
|
||||
...stringifiedMinimapStyles.buttonGroupStyles
|
||||
@@ -30,7 +32,7 @@
|
||||
class="size-8 bg-comfy-menu-bg p-0 hover:bg-interface-button-hover-surface!"
|
||||
@click="() => commandStore.execute('Comfy.Canvas.FitView')"
|
||||
>
|
||||
<i class="icon-[lucide--focus] size-4" />
|
||||
<i class="icon-[lucide--focus] size-4" aria-hidden="true" />
|
||||
</Button>
|
||||
|
||||
<Button
|
||||
@@ -44,7 +46,7 @@
|
||||
>
|
||||
<span class="inline-flex items-center gap-1 px-2 text-xs">
|
||||
<span>{{ canvasStore.appScalePercentage }}%</span>
|
||||
<i class="icon-[lucide--chevron-down] size-4" />
|
||||
<i class="icon-[lucide--chevron-down] size-4" aria-hidden="true" />
|
||||
</span>
|
||||
</Button>
|
||||
|
||||
@@ -59,7 +61,7 @@
|
||||
:class="minimapButtonClass"
|
||||
@click="onMinimapToggleClick"
|
||||
>
|
||||
<i class="icon-[lucide--map] size-4" />
|
||||
<i class="icon-[lucide--map] size-4" aria-hidden="true" />
|
||||
</Button>
|
||||
|
||||
<Button
|
||||
@@ -78,7 +80,7 @@
|
||||
:style="stringifiedMinimapStyles.buttonStyles"
|
||||
@click="onLinkVisibilityToggleClick"
|
||||
>
|
||||
<i class="icon-[lucide--route-off] size-4" />
|
||||
<i class="icon-[lucide--route-off] size-4" aria-hidden="true" />
|
||||
</Button>
|
||||
</ButtonGroup>
|
||||
</div>
|
||||
|
||||
@@ -1052,6 +1052,8 @@
|
||||
"logoProviderSeparator": " & "
|
||||
},
|
||||
"graphCanvasMenu": {
|
||||
"canvasMode": "Canvas Mode",
|
||||
"canvasToolbar": "Canvas Toolbar",
|
||||
"zoomIn": "Zoom In",
|
||||
"zoomOut": "Zoom Out",
|
||||
"resetView": "Reset View",
|
||||
|
||||
Reference in New Issue
Block a user