[Studio UI] Rewrite tree keyboard navigation with DOM-based traversal - #4048
Open
idaiv wants to merge 8 commits into
Open
[Studio UI] Rewrite tree keyboard navigation with DOM-based traversal#4048idaiv wants to merge 8 commits into
idaiv wants to merge 8 commits into
Conversation
Tree nodes had tabIndex={-1}, making them unreachable via Tab key.
Apply roving tabindex: the selected node (or the first root node if
nothing is selected) gets tabIndex={0}, all others stay at -1. Once
a node is focused, arrow keys already handle navigation.
Fixes WCAG 2.1 2.1.1 (Keyboard) — tree is now Tab-reachable.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
idaiv
added
the
Skip Milestone Check
Contributor
There was a problem hiding this comment.
Pull request overview
Verdict: Needs changes. The PR attempts to make element-tree nodes keyboard-reachable using roving tabindex.
Changes:
- Assigns
tabIndex={0}to selected nodes or a hard-coded fallback node.
Review:
- The fallback does not match rendered root/top-level nodes, leaving unselected trees unreachable.
- Selection could create two tabbable nodes instead of one.
- Shared behavior covers asset, document, and data-object trees, but lacks regression tests.
- No public API compatibility or documentation impact.
Suppressed comments (1)
assets/js/src/core/components/element-tree/node/tree-node.tsx:226
- This condition does not produce a valid initial tab stop. With
showRoot,ElementTreerenders the root at level-1; without it (the default), top-level children are level0but their generated keys are0-<id>, not0. Thus an unselected tree still has notabIndex={0}. Moreover, if this fallback ever matched, selecting another node would leave both nodes tabbable because of the||. Please choose one active key at the tree level from the selected node or the first rendered node, then pass that state to each node.
tabIndex={ isSelected || (level === 0 && internalKey === '0') ? 0 : -1 }
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| ref={ setRef } | ||
| role='button' | ||
| tabIndex={ -1 } | ||
| tabIndex={ isSelected || (level === 0 && internalKey === '0') ? 0 : -1 } |
Replace the broken nodeOrder()-based navigation with direct DOM querySelectorAll traversal. The previous approach sorted internalKey strings numerically which produced wrong ordering when parent keys were prefixes of child keys. New approach: - getAllTreeNodes() queries .tree-node__content-inner elements in the actual DOM, guaranteeing correct visual order - Uses event.currentTarget instead of nodesRefs lookup, eliminating stale ref mismatches - moveFocusToParent() walks up the DOM tree (.tree-node → .tree-list → parent .tree-node) instead of comparing level numbers - Roving tabindex: selected or first node gets tabIndex=0 - ArrowRight: expand if collapsed, move to first child if expanded - ArrowLeft: collapse if expanded, move to parent if collapsed - ArrowDown/Up: next/previous in visual order - Home/End: first/last visible node - aria-expanded on expandable nodes - focus-visible outline ring for keyboard users Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
idaiv
changed the title
This was referenced Sep 1, 2026
Refactor from chained if-statements to switch/case to reduce cognitive complexity flagged by SonarCloud. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Remove unused imports: KeyboardEvent, INodeRef - Move getVisibleNodes, moveFocus, moveFocusToParent to outer scope (pure DOM functions, no component state dependency) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
idaiv
removed
the
Skip Milestone Check
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
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.
Summary
Complete rewrite of tree keyboard navigation. The previous
nodeOrder()-based approach sortedinternalKeystrings numerically which produced wrong ordering — children appeared after siblings instead of after their parent.New approach:
querySelectorAll('.tree-node__content-inner')on the tree container, guaranteeing correct visual orderevent.currentTargetinstead ofnodesRefslookup, eliminating stale ref mismatchesmoveFocusToParent()walks the DOM hierarchy directlytabIndex=0, all others-1aria-expandedon expandable nodesfocus-visibleoutline ring for keyboard users (:focus:not(:focus-visible)suppresses it for mouse clicks)Keyboard map:
Test plan
Supersedes the scope of #4050 and #4051 which are now included here.
🤖 Generated with Claude Code
Relates to https://github.com/pimcore/product-management/issues/372