From e973efb44a0fef8170d813c2db19329419111a85 Mon Sep 17 00:00:00 2001 From: Dante Date: Wed, 11 Mar 2026 11:15:17 +0900 Subject: [PATCH] fix: improve canvas menu keyboard navigation and ARIA accessibility (#9526) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## 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 (`` 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 --- .../graph/CanvasModeSelector.test.ts | 153 ++++++++++++++++++ src/components/graph/CanvasModeSelector.vue | 92 +++++++++-- src/components/graph/GraphCanvasMenu.vue | 10 +- src/locales/en/main.json | 2 + 4 files changed, 241 insertions(+), 16 deletions(-) create mode 100644 src/components/graph/CanvasModeSelector.test.ts diff --git a/src/components/graph/CanvasModeSelector.test.ts b/src/components/graph/CanvasModeSelector.test.ts new file mode 100644 index 0000000000..1133873544 --- /dev/null +++ b/src/components/graph/CanvasModeSelector.test.ts @@ -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: '
', + 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() + }) +}) diff --git a/src/components/graph/CanvasModeSelector.vue b/src/components/graph/CanvasModeSelector.vue index c339d5f989..6c5a05e45e 100644 --- a/src/components/graph/CanvasModeSelector.vue +++ b/src/components/graph/CanvasModeSelector.vue @@ -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" >
- +