Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: selecting entry should focus back to editor #114493

Merged
merged 5 commits into from Jan 19, 2021
Merged

Fix: selecting entry should focus back to editor #114493

merged 5 commits into from Jan 19, 2021

Conversation

susiwen8
Copy link
Contributor

@susiwen8 susiwen8 commented Jan 17, 2021

This PR fixes #106824

If active editor wasn't focused after selection, then manually focus editor.

@chrmarti
Copy link
Contributor

Would it make sense to move this to ChangeModeAction? That way we know it is only triggered when the user went through the picker UI.

@susiwen8
Copy link
Contributor Author

Would it make sense to move this to ChangeModeAction? That way we know it is only triggered when the user went through the picker UI.

Yes, It makes more sense.

Copy link
Member

@bpasero bpasero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since there are more pickers for which this issue exists (e.g. changing encoding), I wonder if this should be added to all for consistency?

@bpasero bpasero added this to the On Deck milestone Jan 18, 2021
@susiwen8
Copy link
Contributor Author

Since there are more pickers for which this issue exists (e.g. changing encoding), I wonder if this should be added to all for consistency?

@bpasero Sure.

@susiwen8 susiwen8 requested a review from bpasero January 18, 2021 11:50
Copy link
Member

@bpasero bpasero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would have expected the code to focus the active editor after each individual picker, not in that place. Maybe I am missing something? Shouldn't the focus happen after the user picked something from quick pick?

@susiwen8
Copy link
Contributor Author

susiwen8 commented Jan 18, 2021

I would have expected the code to focus the active editor after each individual picker, not in that place. Maybe I am missing something? Shouldn't the focus happen after the user picked something from quick pick?

@bpasero Yes, that's right, after user pick something from quick pick, active editor should been focused. But somehow, for some quick pick, selecting by mouse click or enter is kind of different.

For mouse click, document.activeElement become editor after selection, but status bar for enter(some cases). That's why I decided to manually focus editor after each select if editor weren't focused.

doRenderNow is the 'converge' function where all post selection action would cross. So I have put logic here.

Kapture.2021-01-18.at.22.42.00.mp4

@bpasero
Copy link
Member

bpasero commented Jan 18, 2021

@susiwen8 that is the thing I question

doRenderNow is the 'converge' function where all post selection action would cross. So I have put logic here.

No, it is the function called whenever a label changes in the editor status bar, not when picking something.

@susiwen8 susiwen8 requested a review from bpasero January 19, 2021 02:11
@bpasero bpasero merged commit 2472798 into microsoft:master Jan 19, 2021
@bpasero
Copy link
Member

bpasero commented Jan 19, 2021

Thanks 👍

@bpasero bpasero modified the milestones: On Deck, January 2021 Jan 19, 2021
@susiwen8 susiwen8 deleted the 106824 branch January 19, 2021 06:39
@github-actions github-actions bot locked and limited conversation to collaborators Mar 5, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Quick pick: selecting entry with keyboard does not put focus back to editor
3 participants