Settings editor - add focus next/previous setting commands#326375
Settings editor - add focus next/previous setting commands#326375soreavis wants to merge 6 commits into
Conversation
Add settings.action.focusNextSetting (F7) and settings.action.focusPreviousSetting (Shift+F7), which move focus directly to the next/previous setting's value control, skipping group headers. They work no matter whether focus is in the search input, on a tree row, or inside a setting's control, so pressing the shortcut N times from the top reaches the Nth setting's control. Navigation walks the tree model rather than the DOM since the tree is virtualized; the target row is revealed first so its control can be focused (same pattern as revealing a setting link target). Fixes microsoft#52815
📬 CODENOTIFYThe following users are being notified based on files changed in this PR: @rzhao271Matched files:
|
There was a problem hiding this comment.
Pull request overview
Adds keyboard commands for moving focus between setting controls in the Settings editor.
Changes:
- Adds next/previous setting-control navigation.
- Registers F7 and Shift+F7 commands.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
settingsEditor2.ts |
Implements adjacent-setting focus navigation. |
preferences.contribution.ts |
Registers commands and keybindings. |
Delayer.isTriggered is a method; referencing it without calling it made the expression always truthy, so isSearchUpToDate() always returned false and focusing the first setting from search never ran.
…ting Guard the navigation behind isSearchUpToDate() so a pending search cannot be navigated stale, query the focused row's control instead of looking it up by setting key (which can match a Commonly Used duplicate), and trim the anchor comment to one line.
|
Addressed all three: navigation now returns early while One thing surfaced while wiring the guard: |
The previously focused setting's container keeps its focused class until DOM focus moves, so a global .focused selector could match the old control first when navigating forward. Scope the lookup to the target row via its data-id, which is also unique for Commonly Used duplicates.
|
Committed fix — the previous container keeps |
The settings tree retains its focus when the user moves to the search input or the TOC, so navigation would continue from the previously visited setting. Start from the first/last setting instead unless focus is in the settings tree or a setting control.
|
Committed fix (bc0e9d1) — the anchor now only uses the retained tree focus when the focus context is the tree or a control; from the search input or the TOC, navigation starts at the first/last setting. Verified both ways: step, refocus search, F7 restarts at the top; stepping from a focused control still continues. |
Interpolating the setting id into the selector could throw for a key containing a quote. Use the list's focused-row trait class, which targets a single row (avoiding the stale focused container on the previously visited setting) without a dynamic attribute value.
|
Good point on the unescaped key. Took a slightly different route than the suggestion though — a plain |
|
I tested this and it seems fine to me, but yielding to @rzhao271 as he owns this area |
This implements the two commands @roblourens specced in #52815 (comment):
settings.action.focusNextSettingandsettings.action.focusPreviousSetting. They move focus directly to the next/previous setting's value control (input, checkbox, dropdown), skipping group headers, and they work no matter whether focus is currently in the search input, on a tree row, or inside a setting's control — so pressing the shortcut exactly N times from the top reaches the Nth setting's control, which was the acceptance test in that comment. With nothing focused yet, the backwards command starts from the last setting; at the first/last setting the commands are a no-op (no wrap), matching the tree's own navigation.Implementation notes. Navigation walks the tree model (
settingsTree.navigate) instead of the DOM the 2018 comment described, because the tree is virtualized and off-screen settings have no DOM; the target is revealed first and then its control is focused via the samedata-key+CONTROL_SELECTORpatternonDidClickSettinguses. Focusing the control directly announces the control's own label, the same as the existing Enter →focusSettingControlpath. The commands are in the palette (f1) likefocusLevelUpand the accessible diff viewer commands, since they're meant to be discoverable and rebindable.On the keybinding: the spec suggested cmd+down/up, but that can't work — the settings dropdowns swallow every up/down keydown regardless of modifiers before the keybinding service sees them (
SelectBoxListon desktop,selectBoxCustom.tskeydown handler stops arrow/Space/Enter withEventHelper.stop;SelectBoxNativeon iOS web does the same), so stepping would break at every enum setting. Cmd+arrows are also already taken inside the settings editor by widget navigation (search ↔ TOC) and list scrolling. F7 / Shift+F7 follows the accessible diff viewer /wordHighlight.next"go to next item" precedent and passes through the search box, text inputs, checkboxes, and dropdowns (verified on macOS). Can change the default or ship the commands unbound if you'd rather.Verified in a dev build on macOS: from a fresh editor the first press lands on the first setting (skipping the Commonly Used header), six presses reach the sixth setting stepping through two dropdowns with the list auto-scrolling, Shift+F7 steps back, Shift+F7 with nothing focused lands on the last setting, and the commands stop at the ends. Consistent with the sibling settings focus commands (#106897, #321571), no automated test is added — the navigation runs entirely through the existing tree model.
Fixes #52815