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

Accessibility support for upcoming terminal environment changes feature #95573

Closed
Tyriar opened this issue Apr 17, 2020 · 15 comments · Fixed by #95759
Closed

Accessibility support for upcoming terminal environment changes feature #95573

Tyriar opened this issue Apr 17, 2020 · 15 comments · Fixed by #95759
Assignees
Labels
accessibility Keyboard, mouse, ARIA, vision, screen readers (non-specific) issues feature-request Request for new features or functionality on-testplan terminal General terminal issues that don't fall under another label
Milestone

Comments

@Tyriar
Copy link
Member

Tyriar commented Apr 17, 2020

Feature: #46696

@webczat @jvesouza @MarcoZehe @isidorn you've been active in terminal accessibility so I'd love your opinions here on how this should work in this feature.

How it works

Basically what it enables is extensions will be able to change the terminal process environment when it's launched by added to the start, end or replacing environment variables. The way the UI side of this works is there is a little indicator in the top-right corner of the terminal, that could have 2 different icons:

  • An info icon for when the terminal has changes active due to extensions
  • A warning icon for when an extension has changed or added how it changes the environment

When you hover these icons a hover will appear, an example of the text:

Extensions want to make the follow changes to the terminal's environment:

  • Replace FOO's value with BAR
  • Add /test/path to the end of PATH

The hover also has an action in the hover's status bar "Relaunch terminal" that allows a single click to "fix the problem".

Making it accessible

My ideas so far are:

  • Change the textarea aria label from "Terminal 1" to "Terminal 1, an extension wants to change the environment, restart the terminal to get the updated environment". This seems like it could be annoying and it could also be confusing unless the message is kind of long.
  • Should there be an alert when an extension makes changes? In the UI the indicator pops up which may come to the user's attention.
  • Expose a command to show the hover much like the "Show Hover" command in the terminal? This could suffer from bad discoverability.

I've also been thinking about whether it's important to make the info indicator accessible or not, it's "good to know" information but I'm guessing most users won't care and will end up trying to hide it (which they can via a setting).

@Tyriar Tyriar added feature-request Request for new features or functionality accessibility Keyboard, mouse, ARIA, vision, screen readers (non-specific) issues terminal General terminal issues that don't fall under another label labels Apr 17, 2020
@Tyriar Tyriar added this to the April 2020 milestone Apr 17, 2020
@Tyriar Tyriar self-assigned this Apr 17, 2020
@MarcoZehe
Copy link
Contributor

I‘d say that this is an example where an alert is warranted when the terminal input is focused. This is, after all, something the user must be made aware of.
And for the details and the interaction, something similar to the Notification Toast comes to mind. The one that asks if an Accessibility Service is being used and Accessibility Mode should be kept on or turned off. A keystroke to open that could be added (if it doesn‘t exist yet), and a hint appended to the alert text that tells the user to press this keystroke to review the changes and relaunch the terminal.

@jvesouza
Copy link

jvesouza commented Apr 18, 2020 via email

@isidorn
Copy link
Contributor

isidorn commented Apr 20, 2020

Thanks @Tyriar for the ping.
Agree with @MarcoZehe so

  1. Change the textarea aria label as you suggeted
  2. No I do not think there should be an additonal alert, a user will get it when he re-focuses the terminal
  3. Having a command with an assigned complex keybinding makes sense so you can mention the keybinding in the aria label of the terminal text area
  4. Just make the message you propose more concise.
  5. Once the user has acted on this make the textarea aria label short again if possible

Hope this makes sense :)

@Tyriar
Copy link
Member Author

Tyriar commented Apr 20, 2020

How about this for the label?

Terminal {0} environment is stale, run the 'Show Environment Information' command for more information

Then that command will trigger and focus the indicator's hover, allowing actioning with the "Relaunch action" at the bottom which will act the same as the editor's hover. Alternatively the user can just run the "Relaunch terminal" command but I don't call that out. Here's an example of what the hover looks like:

Extensions want to make the following changes to the terminal's environment:

  • Replace FOO's value with BAR
  • Add /test/path to the end of PATH

With the action

Relaunch terminal


Once the user has acted on this make the textarea aria label short again if possible

It'll be a new terminal so it will go back to normal.

@Tyriar
Copy link
Member Author

Tyriar commented Apr 20, 2020

Note that the above does not make the "changes active" part of this accessible, however this is just good to know information that extensions have changed the environment. It's very likely that most people will have changes active in all their terminals so I imagine going with an explicit approach here would get annoying and maybe confusing if the label said something like:

Terminal 1 extensions have modified the environment

The user can always run the Show Environment Information in this state if they know about it though.

@isidorn
Copy link
Contributor

isidorn commented Apr 20, 2020

I think your suggestion makes sense. Though I would like to hear feedback from our users as well.
Also once we have a version which can be tried out I believe you will get more precise feedback.

@Tyriar
Copy link
Member Author

Tyriar commented Apr 20, 2020

Also once we have a version which can be tried out I believe you will get more precise feedback.

This is a little tricky since it's an extension only feature currently in proposed. Setting up https://github.com/microsoft/vscode-extension-samples/tree/master/terminal-sample and running the Terminal API: Update environment command is the main way to test it now.

@Tyriar
Copy link
Member Author

Tyriar commented Apr 20, 2020

@isidorn are the actions in Show Hover accessible? I can't seem to tab to focus them, they're announced along with their keybindings, but I was expecting to be able to tab to them?

image

@Tyriar
Copy link
Member Author

Tyriar commented Apr 20, 2020

Oh, it's because the actions are a tags without href or tabIndex set.

@Tyriar
Copy link
Member Author

Tyriar commented Apr 20, 2020

Show environment info + tab to focus the action, announces "Relaunch terminal button":

image

I'll make an issue for the editor because they are only accessible via click currently.

@isidorn
Copy link
Contributor

isidorn commented Apr 20, 2020

@Tyriar great catch for editor hover actions.

@Tyriar
Copy link
Member Author

Tyriar commented Apr 21, 2020

Ok this will be in the next Insiders, it's a little hard to test but the behavior as you need to setup this extension. Here's what I went with:

  • The textarea aria label changes to this when an extension wants to change the environment: "Terminal (number) environment is stale, run the 'Show Environment Information' command for more information"
  • The command "Terminal: Show Environment Information" is available in the command palette which will show a hover of the same form as the editor hover (ctrl+k, ctrl+i). You can also use this command to find out what extensions have contributed to the terminal's environment.
  • You can tab to the actions in the environment information dialog which use role=button (this will also works in the editor hover soon Actions in hover are not keyboard accessible #95702)

Some design decisions I made intentionally:

  • There is no keybinding for the new show environment information command, this is because it would very rarely get used and typically you will just want to relaunch the terminal anyway.
  • The "changes are active" side of this feature is not surfaces at all, this is just maybe good to know information that users mainly only care about when there is a problem with the terminal's environment (in which case hopefully they'll discover the show environment information command). The intent is that extensions use this feature and everything will magically just work, the biggest example is a language extension setting the correct language/sdk version.

Thanks for all the input 🙂

@chrisnorman7
Copy link

Hi,
As an aside, is it possible that the label for the input field could be changed from "Terminal 1" for exaqmple, to the names we set for the terminals via the "rename terminal" action?

Only a small thing, but I feel like it would make more sense.

Thanks for reading.

@Tyriar
Copy link
Member Author

Tyriar commented Jun 2, 2020

@chrisnorman7 #99072

@chrisnorman7
Copy link

chrisnorman7 commented Jun 2, 2020 via email

@github-actions github-actions bot locked and limited conversation to collaborators Jun 5, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accessibility Keyboard, mouse, ARIA, vision, screen readers (non-specific) issues feature-request Request for new features or functionality on-testplan terminal General terminal issues that don't fall under another label
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants