Skip to content

Made Editor Hint reachable by Tab (Fixes #132085) #219274

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

manav014
Copy link

Fixes #132085

Adds the following features to empty text editor hint

  1. Allow user to navigate across hint options using tab.
  2. Allow user to press escape to directly go to editor.

Steps to Run the code

  1. Build and run the VSCode
  2. Create an Empty File
  3. press tab to move across hint items
  4. press escape to get back to editor.

@manav014
Copy link
Author

@microsoft-github-policy-service agree

@joyceerhl joyceerhl self-requested a review July 10, 2024 18:39
@manav014
Copy link
Author

Hey @joyceerhl . Would be great if you can review this PR.

@manav014
Copy link
Author

@joyceerhl could you please take a look at this when you have a moment? Thank you! 🙂

this.toDispose.push(this.editor.onKeyDown((e) => {
const shouldRenderHint = this._shouldRenderHint();
if (shouldRenderHint && e.keyCode === KeyCode.Tab) {
e.stopPropagation();
Copy link
Contributor

Choose a reason for hiding this comment

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

IMHO we shouldn't override tab in an empty editor, since this would break inserting tabs into an editor. We can probably safely use shift+tab instead.

Suggested change
e.stopPropagation();
if (shouldRenderHint && e.keyCode === KeyCode.Tab && e.shiftKey) {

Copy link
Member

Choose a reason for hiding this comment

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

When you're in "tab moves focus" mode (the "toggle tab key moves focus" command), then I think this would still trap focus in the editor?

Copy link
Author

Choose a reason for hiding this comment

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

I tried the "toggle tab key moves focus" feature in 2 scenerios

  1. With the changes of this PR
  2. Without the changes of this PR

Found that it was working fine and same in both the cases.

if (event.keyCode === KeyCode.Enter && event.target.innerText in hintLanguageMappings) {
event.stopPropagation();
event.preventDefault();
hintHandler.callback(hintLanguageMappings[event.target.innerText], event);
Copy link
Contributor

Choose a reason for hiding this comment

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

This won't work as expected for users with a configured display language because the display message above is localized and the keys in hintLanguageMappings are not. I'd also prefer not to duplicate text since we may change the wording in future. One alternative is to use the index when iterating through hintElement.querySelectorAll('a'):

		hintElement.querySelectorAll('a').forEach((anchor, index) => {
			// ...
			hintHandler.callback(String(index), event);

@manav014
Copy link
Author

@joyceerhl Thanks a ton for the review.
I have implemented the suggestions.

joyceerhl
joyceerhl previously approved these changes Jul 31, 2024
@vscodenpa vscodenpa added this to the August 2024 milestone Jul 31, 2024
TylerLeonhardt
TylerLeonhardt previously approved these changes Aug 1, 2024
@manav014
Copy link
Author

manav014 commented Aug 5, 2024

Hey @joyceerhl could this be merged, Or we have anything pending here?

@rzhao271
Copy link
Contributor

Merge conflict
Moving the milestone

@rzhao271 rzhao271 modified the milestones: October 2024, November 2024 Oct 25, 2024
@manav014 manav014 dismissed stale reviews from TylerLeonhardt and joyceerhl via acff5c2 November 3, 2024 13:09
@manav014
Copy link
Author

manav014 commented Nov 3, 2024

Resolved the conflicts

@rzhao271
Copy link
Contributor

rzhao271 commented Nov 4, 2024

Build error: emptyTextEditorHint.ts: line 10, col 25, Warning - Imports have to be relative to support ESM (local/code-import-patterns)

@joyceerhl joyceerhl modified the milestones: November 2024, November 2024 Recovery 1 Dec 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
postponed-during-endgame Endgame champ removed the planed milestone
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Editor hint in new file can't be reached by tabbing
9 participants