feat(a11y): keyboard & ARIA pass on the choice and macro GUIs (#1250)#1251
Merged
chhoumann merged 7 commits intoMay 30, 2026
Conversation
Make the choice and macro editors fully keyboard operable and replace the faux-button / ARIA patterns left after the Svelte 5 migration (#1250). - Replace role="button"+tabindex+onkeypress faux-buttons with native <button>s via two shared components, IconButton and DragHandle (Enter/Space activation, focus, and aria-label for free). - Keyboard reorder for both lists: ArrowUp/ArrowDown on a row's drag handle moves it and persists via the same path as a pointer drag. Set autoAriaDisabled on the dndzones so svelte-dnd-action stops injecting its own conflicting role/tabindex/keyboard handling onto rows. - Keyboard-reachable context menu (Rename / Move-to) via a new "More options" button -> Menu.showAtPosition; drop the dead role="button" row that announced a menu it couldn't open. - aria-pressed on the command-palette toggle; aria-expanded on the Multi collapse toggle; valid <li> command rows (was <ol> > <div> > <li>); keyboard-operable rename in the modal <h2> headers; migrate FolderList. - Enable svelte/valid-compile as an a11y / regression lint ratchet. Also fixes a pre-existing data-loss bug this work exposed: reordering a Multi nested inside another Multi (depth >= 2) bubbled the root array into an ancestor's onReorderChoices override, overwriting its children (a cycle that got persisted to data.json). Nested reorders now persist through a threaded top-level rootReorder handler; this affected both keyboard reorder and pointer drag. Tests added: IconButton, DragHandle, keyboard reorder, a depth>=2 nested reorder regression, collapse aria-expanded, and the keyboard-opened menu. svelte-check, eslint (incl. valid-compile), 1452 unit tests, and the production build all pass. Closes #1250
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds accessible IconButton and DragHandle components, replaces faux-button UI with native buttons in headers and list controls, implements arrow-key reordering for commands and choices (with persistence), centralizes context-menu construction for anchored/keyboard opening, expands tests and stubs, updates styles, and enables svelte/valid-compile in ESLint. ChangesKeyboard-accessible UI and reordering
🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 435b557bdc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Deploying quickadd with
|
| Latest commit: |
e9e09cf
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://29a5904c.quickadd.pages.dev |
| Branch Preview URL: | https://1250-accessibility-keyboard.quickadd.pages.dev |
svelte-dnd-action sets zoneItemTabIndex (default 0) on every dndzone row even with autoAriaDisabled, so each choice/command row was a focusable tab stop that does nothing. Pass zoneItemTabIndex:-1 so Tab visits only the action buttons and drag handle inside each row. Verified in the dev vault: rows are now tabindex=-1, the per-row buttons remain focusable, no "Context menu for ..." aria-label/tooltip remains (removed with the old role="button" row), and no runtime errors.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/gui/MacroGUIs/Components/ConditionalCommand.svelte`:
- Around line 43-60: The three IconButton instances for editing ("Edit
condition", "Edit then branch", "Edit else branch") produce identical aria
labels; update them to include the command identity so screen readers can
distinguish rows—e.g. build the label/aria-label using command.name (or another
unique command property) when rendering the IconButton used in
onConfigureCondition, onEditThenBranch, and onEditElseBranch; ensure the label
string includes both the action and command.name so the accessible name becomes
unique per row.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1eccd938-7721-4b6d-b982-cd0e0f31aed0
📒 Files selected for processing (28)
eslint.config.mjssrc/gui/ChoiceBuilder/FolderList.sveltesrc/gui/ChoiceBuilder/choiceBuilder.tssrc/gui/MacroGUIs/AIAssistantCommandSettingsModal.tssrc/gui/MacroGUIs/AIAssistantInfiniteCommandSettingsModal.tssrc/gui/MacroGUIs/CommandList.keyboardReorder.test.tssrc/gui/MacroGUIs/CommandList.sveltesrc/gui/MacroGUIs/Components/AIAssistantCommand.sveltesrc/gui/MacroGUIs/Components/ConditionalCommand.sveltesrc/gui/MacroGUIs/Components/NestedChoiceCommand.sveltesrc/gui/MacroGUIs/Components/OpenFileCommand.sveltesrc/gui/MacroGUIs/Components/StandardCommand.sveltesrc/gui/MacroGUIs/Components/UserScriptCommand.sveltesrc/gui/MacroGUIs/Components/WaitCommand.sveltesrc/gui/MacroGUIs/Components/macroCommands.test.tssrc/gui/MacroGUIs/MacroBuilder.tssrc/gui/choiceList/ChoiceItemRightButtons.sveltesrc/gui/choiceList/ChoiceList.a11y.test.tssrc/gui/choiceList/ChoiceList.sveltesrc/gui/choiceList/ChoiceListItem.sveltesrc/gui/choiceList/MultiChoiceListItem.sveltesrc/gui/choiceList/contextMenu.tssrc/gui/components/DragHandle.sveltesrc/gui/components/DragHandle.test.tssrc/gui/components/IconButton.sveltesrc/gui/components/IconButton.test.tssrc/styles.csstests/obsidian-stub.ts
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
The shared DragHandle armed the list's pointer drag by calling startDrag(e) on pointerdown, and startDrag called e.preventDefault(). preventDefault on pointerdown suppresses the compatibility mousedown event in Chromium, and svelte-dnd-action starts its drag from mousedown (pointerAction attaches mousedown/touchstart only) — so the drag silently stopped working. The pre-refactor choice-row handle passed no event (never preventDefaulted on pointerdown), which is why drag worked there. Fix: DragHandle fires onDragStart() with no event purely to flip dragDisabled=false, and the startDrag handlers no longer call preventDefault (the library handles preventDefault during the actual drag). Drop the now-unused event params. Regression test: DragHandle "arms the drag on pointerdown WITHOUT preventing default" asserts pointerdown is not canceled. Verified in the dev vault (no console errors); svelte-check, eslint, 1452 tests, and the build pass.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/gui/components/DragHandle.svelte (1)
1-66: ⚡ Quick winUse tab indentation for this new file.
This new
.sveltefile uses 4-space indentation throughout. As per coding guidelines,src/**/*.{ts,tsx,svelte,css,js}: "Use tab indentation and LF endings (see.editorconfig)". Please convert to tabs to match the project's editor settings.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/gui/components/DragHandle.svelte` around lines 1 - 66, The file DragHandle.svelte uses 4-space indentation but the project requires tabs and LF endings; update the file to replace the 4-space indents with tab characters throughout (including inside the <script> block, JSX-like markup, and any nested blocks) while preserving all identifiers and behavior (props: label, dragDisabled, onDragStart, onMoveUp, onMoveDown, size; derived const reorderable; function onKeyDown; and the <button> with its attributes and event handlers). Ensure line endings are LF and re-run a quick lint/format to confirm no functional changes were introduced.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/gui/components/DragHandle.svelte`:
- Around line 1-66: The file DragHandle.svelte uses 4-space indentation but the
project requires tabs and LF endings; update the file to replace the 4-space
indents with tab characters throughout (including inside the <script> block,
JSX-like markup, and any nested blocks) while preserving all identifiers and
behavior (props: label, dragDisabled, onDragStart, onMoveUp, onMoveDown, size;
derived const reorderable; function onKeyDown; and the <button> with its
attributes and event handlers). Ensure line endings are LF and re-run a quick
lint/format to confirm no functional changes were introduced.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 367ef321-c758-4a3d-aea5-d33982067dca
📒 Files selected for processing (10)
src/gui/MacroGUIs/CommandList.sveltesrc/gui/MacroGUIs/Components/AIAssistantCommand.sveltesrc/gui/MacroGUIs/Components/ConditionalCommand.sveltesrc/gui/MacroGUIs/Components/NestedChoiceCommand.sveltesrc/gui/MacroGUIs/Components/OpenFileCommand.sveltesrc/gui/MacroGUIs/Components/StandardCommand.sveltesrc/gui/MacroGUIs/Components/UserScriptCommand.sveltesrc/gui/MacroGUIs/Components/WaitCommand.sveltesrc/gui/components/DragHandle.sveltesrc/gui/components/DragHandle.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/gui/MacroGUIs/Components/WaitCommand.svelte
- src/gui/MacroGUIs/Components/NestedChoiceCommand.svelte
- src/gui/MacroGUIs/Components/ConditionalCommand.svelte
- src/gui/components/DragHandle.test.ts
Converting the row icons from <div>/<span> to native <button> meant Obsidian's mobile touch-target sizing (a high-specificity .is-mobile settings rule, padding:10px -> 40px boxes) inflated every icon, so six 40px buttons crushed the choice name into a one-word-per-line column. The old faux-buttons were never touch-sized. Force the compact reset (padding/min-*/width/height) with !important so it wins over that rule; the buttons render at icon size again and names wrap normally. Verified via mobile screenshot in the dev vault.
On mobile the six per-row icons were cramped and tiny. Hide the four inline action icons (toggle / configure / duplicate / delete) on .is-mobile and keep only the "More options" (⋮) menu — which already exposes every action incl. rename & move — plus the drag handle, both enlarged to 36px touch targets. Names now get the full row width. Desktop is unchanged (rules are .is-mobile-scoped); macro command rows are unaffected (no menu). Verified via mobile screenshot in the dev vault. Test: asserts the secondary action buttons carry the qa-row-secondary-action collapse hook while More / drag handle do not.
On mobile the drag handle next to the More (⋮) menu felt redundant. Replace it with the native mobile gesture: long-press the row to reorder. Both lists set delayTouchStart:200 so a hold initiates the drag while a quick swipe still scrolls, and dragging is enabled by default on .is-mobile (no handle to arm). The handle is hidden on mobile (choice and command rows), leaving choice rows as just name + ⋮. Desktop is unchanged: the handle stays and arms the drag on grab (the shared state is now a `dragArmed` flag with a derived `dragDisabled`, behaviourally identical to before). Platform.isMobile gates the difference; added to the obsidian test stub (defaults to desktop so existing tests are unaffected).
Addresses CodeRabbit review: the three Edit buttons on a conditional row had
static aria-labels ("Edit condition"/"Edit then branch"/"Edit else branch"),
identical across rows. The reviewer suggested appending command.name, but a
conditional's name defaults to the non-unique "If condition", so that wouldn't
disambiguate. Use the condition summary instead (the per-row-unique identifier
already shown in the row): "Edit condition for <summary>", etc.
Updated the unit assertions (macroCommands, CommandList.conditional) and the
e2e selector (conditional-branch-persistence) — the latter now matches the
aria-label by prefix so it survives the summary suffix and future wording.
Verified via adversarial review of the change: svelte-check 0/0, eslint clean,
1453 unit tests pass, build OK.
chhoumann
deleted the
1250-accessibility-keyboard-aria-pass-on-the-choice-macro-guis-post-svelte-5
branch
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #1250.
The Svelte 5 rewrite (#1248) was deliberately behavior-preserving and didn't touch accessibility. This PR makes the choice & macro editors fully keyboard operable and replaces the leftover faux-button / ARIA patterns.
What changed
Shared components
IconButton.svelte— a real<button>replacing every<div|span role="button" tabindex onclick onkeypress>faux-button (Enter/Space, focus, andaria-labelfor free; optionalaria-pressed/aria-haspopup).DragHandle.svelte— a real<button>that keeps pointer drag and adds keyboard reorder (ArrowUp/ArrowDown, withpreventDefault+stopPropagation).Keyboard operability (CRUD + reorder)
autoAriaDisabled: trueon both dndzones so svelte-dnd-action stops injecting its own conflictingrole/tabindex/keyboard handlers onto rows.Menu.showAtPosition. Removed the deadrole="button"row that advertised a menu it couldn't open.aria-pressedon the command-palette toggle;aria-expandedon the Multi collapse toggle; valid<li>command rows (was<ol> > <div> > <li>); the click-to-rename modal<h2>headers (MacroBuilder, ChoiceBuilder, both AI Assistant modals) now wrap a real<button>;FolderListfaux-button migrated.Lint
svelte/valid-compile(eslint-plugin-svelte v3 has no dedicateda11y-*rules — this surfaces the Svelte 5 compiler'sa11y_*warnings) as a regression ratchet. Shared button CSS is global with doubled-class specificity so themes can't re-skin the new native buttons.Bug fixed along the way (data loss)
Reordering a Multi nested inside another Multi (depth ≥ 2) bubbled the root array into an ancestor's
onReorderChoicesoverride, overwriting its children — silent data loss + a self-referential cycle persisted todata.json. This is a pre-existing rewrite bug that affected pointer drag too; the new keyboard reorder just exposed it. Fixed by threading a top-levelrootReorderhandler through every nesting level. Covered by a regression test that fails on the old code and passes on the fix.Verification
svelte-check: 0 errors / 0 warningseslint(incl.valid-compile): cleanvitest: 1452 passed / 31 skipped, incl. 6 new test files (IconButton, DragHandle, keyboard reorder, depth ≥ 2 nested-reorder regression, collapsearia-expanded, keyboard-opened menu)Notes
aria-controlson the collapse toggle, and the rare case of a Multi named with a markdown link nesting an<a>inside the toggle button.Summary by CodeRabbit
New Features
Improvements
Tests