Remove disabled toggle from the tab order#319693
Merged
connor4312 merged 1 commit intoJun 4, 2026
Merged
Conversation
A disabled `Toggle` only set `aria-disabled` and the `disabled` class but kept `tabIndex=0`, so it stayed reachable via Tab even though clicking or pressing it is a no-op while disabled. This is most visible in the editor find widget: after using Find once, the Match Case / Match Whole Word / Use Regular Expression / Preserve Case toggles remain in the tab order even while the widget is hidden (their owning input is disabled when hidden). Make `enable()`/`disable()` manage `tabIndex` so a disabled toggle is taken out of the tab order, matching the behaviour of `SimpleButton` in the find widget. Toggles created with `notFocusable` (e.g. toolbar action items that use a roving tabindex) keep their externally managed `tabIndex`. Fixes microsoft#147299
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds tab-order behavior for Toggle when enabling/disabling, and introduces browser UI tests to validate focusability behavior (including notFocusable toggles).
Changes:
- Update
Toggle.enable()/Toggle.disable()to adjusttabIndexwhen the toggle is focusable. - Add tests covering
tabIndexchanges for focusable vsnotFocusabletoggles.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/vs/base/test/browser/ui/toggle/toggle.test.ts | Adds regression tests asserting correct tabIndex behavior across enable/disable for focusable and notFocusable toggles. |
| src/vs/base/browser/ui/toggle/toggle.ts | Implements tabIndex updates on enable/disable for focusable toggles while leaving notFocusable toggles untouched. |
connor4312
approved these changes
Jun 4, 2026
dmitrivMS
approved these changes
Jun 4, 2026
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.
Fixes #147299
Problem
After invoking Find once (or toggling any of the option buttons via their keybinding, e.g. Alt+C), the find widget's Match Case, Match Whole Word, Use Regular Expression and Preserve Case toggles stay in the editor's tab order even while the find widget is hidden. With
Tab Moves Focus(Ctrl+M) enabled this is easy to observe: tabbing from the editor lands on these hidden toggles instead of going straight to the status bar. The buttons even flash into view when activated with Space/Enter.Root cause
When the find widget is hidden it disables its find/replace input (
FindInput.setEnabled(false)→Toggle.disable()). HoweverToggle.disable()only setaria-disabledand thedisabledclass — it never removed the element from the tab order, sotabIndexstayed0. A disabled toggle is also a no-op on click/keydown (both handlers early-return when not enabled), so being reachable via Tab serves no purpose.This differs from
SimpleButton(the close/prev/next/replace buttons in the same widget), whosesetEnabledalready togglestabIndexbetween0and-1.Fix
Toggle.enable()/disable()now managetabIndexso a disabled toggle is taken out of the tab order and restored when re-enabled. Toggles created withnotFocusable(e.g. toolbar action items that use a roving tabindex viaToggleActionViewItem) are left untouched so their externally managedtabIndexis preserved.Added unit tests covering both the focusable and
notFocusablecases.