Skip to content

🎨 Added keyboard navigation and shortcuts cheat sheet to theme editor#27922

Closed
betschki wants to merge 3 commits into
TryGhost:mainfrom
magicpages:feat/theme-editor-a11y
Closed

🎨 Added keyboard navigation and shortcuts cheat sheet to theme editor#27922
betschki wants to merge 3 commits into
TryGhost:mainfrom
magicpages:feat/theme-editor-a11y

Conversation

@betschki
Copy link
Copy Markdown
Contributor

@betschki betschki commented May 15, 2026

  • the file tree was mouse-only; users could not move between files, open folders, rename, or delete without a pointing device
  • adds a roving-tabindex tree with role=tree/treeitem, aria-expanded on folders, aria-selected on the current node, and arrow / Enter / Space / F2 / Delete bindings
  • adds a "?" toolbar button and global "?" shortcut that opens a cheat-sheet listing every keyboard shortcut the editor supports
  • gates CodeMirror's autoFocus to a one-shot flag so arrow-key navigation onto a file no longer drags focus into the editor mid-traversal
  • editor no longer swallows Esc, letting sub-dialogs (rename, delete confirm, review, shortcuts) dismiss themselves
27922-a11y-shortcuts

Got some code for us? Awesome 🎊!

Please take a minute to explain the change you're making:

  • Why are you making it?
  • What does it do?
  • Why is this something Ghost users or developers need?

Please check your PR against these items:

  • I've read and followed the Contributor Guide
  • I've explained my change
  • I've written an automated test to prove my change works

We appreciate your contribution! 🙏

- the file tree was mouse-only; users could not move between files, open folders, rename, or delete without a pointing device
- adds a roving-tabindex tree with role=tree/treeitem, aria-expanded on folders, aria-selected on the current node, and arrow / Enter / Space / F2 / Delete bindings
- adds a "?" toolbar button and global "?" shortcut that opens a cheat-sheet listing every keyboard shortcut the editor supports
- gates CodeMirror's autoFocus to a one-shot flag so arrow-key navigation onto a file no longer drags focus into the editor mid-traversal
- editor no longer swallows Esc, letting sub-dialogs (rename, delete confirm, review, shortcuts) dismiss themselves
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 15, 2026

Review Change Stack

Walkthrough

This PR adds keyboard shortcuts support and comprehensive keyboard navigation to the Ghost admin theme editor. The changes introduce a new shortcuts cheat-sheet modal accessible via a toolbar button or the ? key, while completely refactoring the file tree to support full keyboard navigation with Arrow keys for selection and folder expansion, F2 for rename, and Delete for removal. A one-shot autofocus flag prevents CodeMirror from stealing focus during tree navigation. Acceptance tests validate all new keyboard interactions and the shortcuts modal display.

Possibly related PRs

  • TryGhost/Ghost#27656: Related earlier work extending the in-browser theme code editor UI, involving many of the same files (theme-code-editor-modal, toolbar, file tree, shortcuts).
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The PR title clearly summarizes the main changes: keyboard navigation and a shortcuts cheat sheet added to the theme editor, matching the core improvements documented in the changeset.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description check ✅ Passed The pull request description clearly explains the motivation (mouse-only file tree), the implementation (roving-tabindex tree with keyboard bindings, shortcuts cheat sheet), and the benefits for users (keyboard accessibility improvements).

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

- the new CodeMirror autoFocus and onCreateEditor props landed out of order; react/jsx-sort-props wants reservedFirst then alphabetical then callbacks-last, so autoFocus moves above basicSetup and onCreateEditor below onChange
- the existing "Allows selecting a non-editable file" acceptance test queried tree items via getByRole('button'); switching the tree to ARIA role=treeitem made that query miss. Updated the locator to getByRole('treeitem')
@betschki betschki marked this pull request as ready for review May 16, 2026 10:32
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 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
`@apps/admin-x-settings/src/components/settings/site/theme/theme-file-tree.tsx`:
- Around line 227-229: The current keydown handler returns early on any modifier
(event.metaKey/event.ctrlKey/event.altKey) before checking for the Backspace
branch, so the modified Backspace path never reaches onDeleteSelected(); move or
change the modifier bailout so Backspace is handled first: in the handler
containing the existing modifier check (and the similar block at the later
handler around lines 285-293), check if event.key === 'Backspace' (or event.code
as appropriate) and call onDeleteSelected() before performing the modifier-key
early return, or restructure to only bail out for modifiers when the key is not
Backspace; update both occurrences to ensure Backspace can invoke
onDeleteSelected().
- Around line 321-341: The flattened tree items in theme-file-tree.tsx (the
button rendered for each node in the map using registerButtonRef(key),
onOpenFile(node.path), depth, node.name, isSelected, isTabbable, changeStatus)
need aria-posinset and aria-setsize to restore APG tree semantics; compute the
position of the node among its siblings at the same depth (posinset) and the
total sibling count at that depth (setsize) when rendering the button and add
aria-posinset={pos} and aria-setsize={size} to the button, or alternatively
restructure to use nested group elements (role="group") for directories so you
can omit these attributes.
🪄 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: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: e6dd0fae-1a06-4c5b-b041-03b0bb9f69bd

📥 Commits

Reviewing files that changed from the base of the PR and between 8b512d8 and 77305ae.

📒 Files selected for processing (5)
  • apps/admin-x-settings/src/components/settings/site/theme/theme-code-editor-modal.tsx
  • apps/admin-x-settings/src/components/settings/site/theme/theme-editor-shortcuts-modal.tsx
  • apps/admin-x-settings/src/components/settings/site/theme/theme-editor-toolbar.tsx
  • apps/admin-x-settings/src/components/settings/site/theme/theme-file-tree.tsx
  • apps/admin-x-settings/test/acceptance/site/theme.test.ts

Comment thread apps/admin-x-settings/src/components/settings/site/theme/theme-file-tree.tsx Outdated
- the Cmd/Ctrl+Backspace delete shortcut was unreachable: the handler's modifier bailout returned before the Backspace branch could run, so the inline comment described behaviour the code never delivered. Whitelist Cmd/Ctrl+Backspace through the bailout so it lands in the existing Delete/Backspace case
- the flattened treeitem structure had aria-level but was missing aria-posinset / aria-setsize, so screen readers couldn't announce "item N of M in folder". Track sibling position and count during the visible-node traversal and surface both attributes on every treeitem
- adds a Cmd/Ctrl+Backspace assertion to the existing F2/Delete acceptance test so the shortcut can't silently die again
@ErisDS
Copy link
Copy Markdown
Member

ErisDS commented May 29, 2026

Closing, ref #28250, the theme editor is not intended to be for developer workflows at the moment.

@ErisDS ErisDS closed this May 29, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants