fix: redirect focus to list's domNode when removing focused row to prevent focus loss#302618
Draft
TylerLeonhardt wants to merge 1 commit intomainfrom
Draft
fix: redirect focus to list's domNode when removing focused row to prevent focus loss#302618TylerLeonhardt wants to merge 1 commit intomainfrom
TylerLeonhardt wants to merge 1 commit intomainfrom
Conversation
…event focus loss Steps: 1. `workbench.quickOpen.closeOnFocusLost` MUST be true (it's the default) 1. click checkbox in a quick pick or quick tree 1. scroll that checkbox off the screen 1. 🐛 quick thing closes I also repro this when the action bar on the right is focused in a quick pick so it's not specific to checkboxes. This is because: 1. list widget kills the entry in the list (and the children, checkboxes, action bars, whatever) 2. checkboxes and action bars then `blur()` to the root element of vscode 3. quick pick reacts to being blurred (focused out) and closes itself This is probably not the right fix, but I want something like this... I'll make a similar fix catered to just the quick pick so that the issue can be resolved but @joaomoreno lmk if you have any ideas. #289980
Contributor
There was a problem hiding this comment.
Pull request overview
Prevents focus from escaping the base ListView widget when a focused row is recycled/removed (e.g., during scrolling), which can otherwise trigger closeOnFocusLost behavior in Quick Pick–style UIs.
Changes:
- Detects when the active element is inside a row that is about to be removed.
- Redirects focus back to the list container to avoid “focus loss” to the workbench/root.
You can also share your feedback on Copilot code review. Take the survey.
| // to be recycled, which would otherwise blur the whole quick pick). | ||
| const activeElement = getActiveElement(); | ||
| if (activeElement && isAncestor(activeElement, item.row.domNode)) { | ||
| this.domNode.focus(); |
| // If the row being removed contains the currently focused element, | ||
| // redirect focus to the list's domNode to prevent focus from escaping | ||
| // the widget entirely (e.g. when scrolling causes a focused checkbox | ||
| // to be recycled, which would otherwise blur the whole quick pick). |
| // to be recycled, which would otherwise blur the whole quick pick). | ||
| const activeElement = getActiveElement(); | ||
| if (activeElement && isAncestor(activeElement, item.row.domNode)) { | ||
| this.domNode.focus(); |
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.
Steps:
workbench.quickOpen.closeOnFocusLostMUST be true (it's the default)I also repro this when the action bar on the right is focused in a quick pick so it's not specific to checkboxes.
This is because:
blur()to the root element of vscodeThis is probably not the right fix, but I want something like this... I'll make a similar fix catered to just the quick pick so that the issue can be resolved but @joaomoreno lmk if you have any ideas.
#289980