fix: focus the tree, not its panel, for the Object Explorer shortcut - #10294
Open
dpage wants to merge 2 commits into
Open
fix: focus the tree, not its panel, for the Object Explorer shortcut#10294dpage wants to merge 2 commits into
dpage wants to merge 2 commits into
Conversation
The Object Explorer shortcut, Shift+Alt+B by default, is meant to put the keyboard into the tree so that the arrow keys move between nodes. It called focus() on the rc-dock tab pane wrapping the tree, and that pane is a plain div with no tabindex, so it cannot take focus at all: the shortcut selected a node and left focus wherever it already was, which for anybody navigating by keyboard means it appeared to do nothing. Nothing needs a tabindex adding. The tree that react-aspen renders inside the panel already carries tabindex="-1", so it is focusable programmatically; the fix is to aim at it, falling back to the panel if the tree is not there so the behaviour cannot get worse than it was. Found whilst reviewing pgadmin-org#10254, which made this visible: once the Object Explorer can be collapsed, a shortcut that silently fails to focus it is much easier to notice.
|
Caution Review failedAn error occurred during the review process. Please try again later. 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 |
Now that the shortcut actually lands keyboard focus on the tree, the focus indicator becomes something users see routinely, and left to the browser it is an outline-style: auto ring drawn in the host's accent colour. In practice that means it appears orange in one browser and blue in another, and Safari may not draw it at all, so a keyboard user there gets no indication of where focus has gone. Style it with theme.otherVars.activeBorder instead, matching how the dock tabs already indicate focus, so it is consistent across browsers and themes: #326690 on light, #d4d4d4 on dark, #fff on high contrast. The -1px outline offset keeps it inside the scrolling container rather than being clipped at the edges. Verified in the browser: with focus on the tree, the computed outline is "rgb(50, 102, 144) solid 1px" with a -1px offset, in place of the previous "auto" ring.
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.
The Object Explorer shortcut,
Shift+Alt+Bby default, is meant to put the keyboard into the tree so that the arrow keys then move between nodes. It callsfocus()on the rc-dock tab pane that wraps the tree, and that pane is a plaindivwith notabindex, so it cannot take focus at all. The result is that the shortcut selects a node and leaves focus wherever it already was, which for anybody navigating by keyboard looks like nothing happening.Nothing needs a
tabindexadding, as I first assumed. The tree react-aspen renders inside the panel already carriestabindex="-1", so it is focusable programmatically; the fix is simply to aim at it, falling back to the panel when the tree is not there so that the behaviour cannot end up worse than it is today.I found this whilst reviewing #10254, which is what made it visible: once the Object Explorer can be collapsed, a shortcut that silently fails to focus it is much easier to notice.
Testing
web/regression/javascript/browser/keyboard_left_tree_spec.jscovers focus landing on the tree, the fallback when no tree is present, and the case where the Object Explorer is not in the DOM at all. The first of those fails against the current code.I also drove it in a browser rather than trusting the unit test: starting from a collapsed Object Explorer with focus on
document.body, pressingShift+Alt+Bnow reveals the panel and leavesdocument.activeElementas thefile-treeelement, where before it stayed onbody.No release note entry, per the usual practice of batching those shortly before release.