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

Make terminal tab hover keyboard accessible #177283

Merged
merged 10 commits into from Mar 20, 2023
Merged

Make terminal tab hover keyboard accessible #177283

merged 10 commits into from Mar 20, 2023

Conversation

Tyriar
Copy link
Member

@Tyriar Tyriar commented Mar 15, 2023

There's a new Terminal: Show or Focus Hover command which shows it and then focuses it if screen reader optimized mode is on.

image

image

focusTrap is used so if you hit escape when focused, focus will return to the previous element (probably the actual terminal).

Fixes #177232

@Tyriar Tyriar added this to the March 2023 milestone Mar 15, 2023
@Tyriar Tyriar requested a review from meganrogge March 15, 2023 19:46
@Tyriar Tyriar self-assigned this Mar 15, 2023
@Tyriar Tyriar marked this pull request as ready for review March 16, 2023 13:46
@Tyriar Tyriar enabled auto-merge March 16, 2023 14:05
@meganrogge
Copy link
Contributor

I didn't know this command existed. I wonder how we can make it more discoverable for both the editor and terminal - maybe it should be in the accessibility help menus?

Screenshot 2023-03-16 at 10 02 23 AM

Copy link
Contributor

@meganrogge meganrogge left a comment

Choose a reason for hiding this comment

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

has some conflicts

@Tyriar
Copy link
Member Author

Tyriar commented Mar 16, 2023

@meganrogge it's working well on latest/mac now.

@meganrogge
Copy link
Contributor

meganrogge commented Mar 16, 2023

@Tyriar now I see it in the command palette and running that works, but the keyboard shortcut still doesn't work for me when there's just a single tab. Terminal: clear takes precedence

@Tyriar
Copy link
Member Author

Tyriar commented Mar 16, 2023

@meganrogge oh ok, in that case it's kind of working as designed... That keybinding is important on macOS and it disables all chords. Does it work when the focus is in the tabs list? If so I think this situation is fine as it's accessible, the fact that you could show the hover while the terminal was focused was just a bonus.

@meganrogge
Copy link
Contributor

meganrogge commented Mar 16, 2023

I worry because this feature is super nice, but as I mentioned, most screen reader users will have only one terminal open here, which is the case where this won't work atm. Could we set a higher weight for this keybinding as compared with clear? Or use a different one that does not involve K? What about ctrlCmd+I so it's still discoverable?

@Tyriar
Copy link
Member Author

Tyriar commented Mar 17, 2023

ctrlCmd+I wouldn't be discoverable though as it's not used elsewhere. How about we disable cmd+k in a11y mode as I doubt it would be useful there anyway?

@Tyriar Tyriar requested a review from meganrogge March 17, 2023 14:01
@Tyriar
Copy link
Member Author

Tyriar commented Mar 17, 2023

@meganrogge could you test this again? I'm not on my mac atm

@meganrogge
Copy link
Contributor

Shows up in a funny spot, but now works, so 👍🏼

Screenshot 2023-03-17 at 7 56 28 AM

@Tyriar Tyriar disabled auto-merge March 20, 2023 16:57
@Tyriar Tyriar enabled auto-merge March 20, 2023 16:57
@Tyriar Tyriar merged commit f7a454f into main Mar 20, 2023
6 checks passed
@Tyriar Tyriar deleted the tyriar/177232 branch March 20, 2023 17:25
@github-actions github-actions bot locked and limited conversation to collaborators May 4, 2023
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.

Review keyboard accessibility of terminal tab hover
3 participants