-
Notifications
You must be signed in to change notification settings - Fork 33.7k
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
base: main
Are you sure you want to change the base?
Conversation
@microsoft-github-policy-service agree |
Hey @joyceerhl . Would be great if you can review this PR. |
@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(); |
There was a problem hiding this comment.
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.
e.stopPropagation(); | |
if (shouldRenderHint && e.keyCode === KeyCode.Tab && e.shiftKey) { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
- With the changes of this PR
- 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); |
There was a problem hiding this comment.
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);
@joyceerhl Thanks a ton for the review. |
Hey @joyceerhl could this be merged, Or we have anything pending here? |
Merge conflict |
acff5c2
Resolved the conflicts |
Build error: |
Fixes #132085
Adds the following features to empty text editor hint
Steps to Run the code