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

Integrated terminal does not support screen readers #8339

Closed
isidorn opened this issue Jun 28, 2016 · 36 comments
Closed

Integrated terminal does not support screen readers #8339

isidorn opened this issue Jun 28, 2016 · 36 comments
Assignees
Labels
accessibility Keyboard, mouse, ARIA, vision, screen readers (non-specific) issues feature-request Request for new features or functionality on-testplan terminal Integrated terminal issues upstream Issue identified as 'upstream' component related (exists outside of VS Code)
Milestone

Comments

@isidorn
Copy link
Contributor

isidorn commented Jun 28, 2016

#8222

  1. Turn on some screen reader (voice over on mac)
  2. open our integrated terminal -> any action you do the screen reader is silent
  3. I would expect that the screen reader reads out what is typed and the output of commands (similar to how it is done for terminal or iTerm on mac)
@isidorn isidorn added accessibility Keyboard, mouse, ARIA, vision, screen readers (non-specific) issues terminal Integrated terminal issues labels Jun 28, 2016
@Tyriar Tyriar added this to the Backlog milestone Jul 5, 2016
@Tyriar Tyriar added the feature-request Request for new features or functionality label Apr 20, 2017
@Tyriar Tyriar modified the milestones: July 2017, Backlog Jun 27, 2017
@Tyriar Tyriar added the upstream Issue identified as 'upstream' component related (exists outside of VS Code) label Jul 6, 2017
@Tyriar
Copy link
Member

Tyriar commented Jul 6, 2017

Upstream issue: xtermjs/xterm.js#731

@Tyriar
Copy link
Member

Tyriar commented Jul 6, 2017

Targets:

  • Windows: NVDA
  • macOS: Built-in screen reader

@Tyriar
Copy link
Member

Tyriar commented Jan 2, 2018

@Neurrone
Copy link

@Tyriar I would be happy to help test this.

Another thing I've noticed is that the terminal's edit box is read before the actual output. Could this be changed so that the edit box is placed after the output in HTML? That makes it easier to quickly go up to most recent output, instead of scrolling all the way down to find it.

#28448 also happens here.

@Tyriar
Copy link
Member

Tyriar commented Jan 12, 2018

@Neurrone do you mean it's read when you first focus the terminal?

On find, it's probably best to track the solution over in that issue as they are basically the same root cause (the editor and terminal don't use native selection).

@Neurrone
Copy link

Neurrone commented Jan 12, 2018

I land on the text entry box when launching the terminal as expected. But the output appears below the input box. This is the reading order when moving about the terminal window manually:

  1. panel buttons (new terminal, close, etc)
  2. the text entry box
  3. the last few lines of output (is there a way to scroll up?)
  4. terminal find widget

Visually, the order of presentation of the second and third elements are reversed - what you type appears below any recent output. I think this might have to do with the order it was defined in the HTML.

@Tyriar
Copy link
Member

Tyriar commented Jan 12, 2018

@Neurrone great feedback 😃

I'm in the process of adding a "navigation mode" which will allow easier navigation of the output. I think this is necessary since only the current viewport is exposed to the accessibility tree, this also answers the your question "is there a way to scroll up".

In this new navigation mode you will press some keybinding (still to be determined, any input would be helpful) and the line the current cursor is on will be highlighted. Using up/down will allow you to navigate up and down through the rows. When the boundary is hit it will scroll and tell the screen reader the correct item to read. This works very similar to how the explorer works with the main change of it needing to be toggled on since normally the terminal eats up/down keys.

I should be able to put the elements above the textarea without much trouble.

@Tyriar
Copy link
Member

Tyriar commented Jan 22, 2018

Support for the screen reader is basically done, here's a summary of the support:

  • Terminal is announced when focused
  • Characters entered into the terminal via keyboard are announced
    • Character and word echo settings are respected
  • Read command output upon execution (up to 20 rows to prevent DOM from growing)
  • Support screen reader navigation within current viewport
    • NVDA can read out row content on mouse over
  • Navigation mode (currently ctrl+n*) allows easy navigation through output

* I'd love to hear suggestions on good keybindings for navigation mode. We will stick this in the docs.

We should revisit once we have access to stable AOM in Electron.

@Neurrone
Copy link

@Tyriar how would the navigation mode work?

@Tyriar
Copy link
Member

Tyriar commented Jan 22, 2018

@Neurrone you press the keybinding or enter via the command palette, and it highlights the line with the cursor on it.

screen shot 2018-01-22 at 6 57 39 am

From here up, down, page up, page down, end and home allow navigating and reading out the rows one by one. Most other keys will exit navigation mode (eg. escape, tab, enter). This works similar to how the explorer works when in this mode.

@Neurrone
Copy link

@Tyriar is there a convenient way for me to try it?

@Tyriar
Copy link
Member

Tyriar commented Jan 22, 2018

@Neurrone it should work on Insiders. Are you using Windows?

@Neurrone
Copy link

@Tyriar just updated and see your changes.

I see what you mean now by a navigation mode. Some thoughts after testing:

  • Can the edit field for command entry be moved so that it is after the terminal's viewport in the DOM?
  • Characters are not echoed when pressing delete/backspace.
  • Each line in the output when outside of forms mode is now read as a menu item since that's how the navigation mode is implemented.

How would a sighted user review or select and copy terminal output if the command causes > 20 lines of output to be displayed? I understand the reasoning behind the current navigation mode implementation, but some things I'd expect to work such as copying and pasting output won't work properly since each line is a menu item.

Thanks a lot for all your work on this, I really appreciate it :)

@Tyriar
Copy link
Member

Tyriar commented Jan 23, 2018

@Neurrone

Can the edit field for command entry be moved so that it is after the terminal's viewport in the DOM?

Yep, added this to the todo list on the PR.

Characters are not echoed when pressing delete/backspace.

Unfortunately I'm not sure there's a reliable way to do this. At least until we know on the frontend what the prompt is (which is a difficult problem to solve cross platform) #20676

Each line in the output when outside of forms mode is now read as a menu item since that's how the navigation mode is implemented.

They're menu items as list items didn't seem to work initially, I can try again though. Added it to the PR.

I'd expect to work such as copying and pasting output won't work properly since each line is a menu item.

Will copy and paste just work if they're list items? What role would you expect for these?

@Neurrone
Copy link

@Tyriar usually expect the output to just be regular text. Maybe on-demand rendering of the full contents of the buffer when entering navigation mode? I believe the default size is 1000 lines. Another suggestion is to increase the number of lines from 20 to e.g, 40 or 50. I can't imagine needing to read past 50 lines of previous output often.

How does it handle a mouse user needing to go back? Is there something that can be clicked to scroll up?

@Neurrone
Copy link

@Tyriar it not echoing the deleted characters is also something relatively minor, mentioned for completeness.

@Tyriar
Copy link
Member

Tyriar commented Jan 23, 2018

@Neurrone

usually expect the output to just be regular text.

I just tried list again and the keybindings to navigate don't work using that (under NVDA), tree/treeitem works though. No role doesn't seem to work just like list. Also menu/tree announce position in the output which I think is important information.

Maybe on-demand rendering of the full contents of the buffer when entering navigation mode?

This would be pretty bad for perf, also the screen reader and screen would then go out of sync. I'll think about it some more.

How does it handle a mouse user needing to go back? Is there something that can be clicked to scroll up?

Mouse wheel typically, there are also keybindings for scrolling the terminal. Search "terminal scroll" in the command palette to see them. These won't work that well for screen readers though as the rows get reconstructed and the screen reader wouldn't know what's going on (as opposed to navigation mode).

@Tyriar
Copy link
Member

Tyriar commented Jan 23, 2018

@Neurrone there was also the idea of a "copy terminal buffer to an editor" command to allow navigation/copy/paste using the editor as a fallback. Would you find such a command useful?

@Tyriar
Copy link
Member

Tyriar commented Jan 23, 2018

An alternative to the explicit copy terminal buffer to an editor command is to run the select all terminal command, copy and paste it into an editor. Select all doesn't have a default keybinding on Windows yet though which I could change to ctrl+a.

@jcsteh
Copy link

jcsteh commented Jan 23, 2018

Rather than supporting navigation with your own keyboard shortcuts, an idea to consider is just relying on NVDA browse mode.

  • You would expose the lines within the viewport as you do now, either just as divs or perhaps with role="listitem".
  • To support scrolling, each line would be focusable (tabindex="-1").
  • When the top line gets focus, you would scroll up, removing content from the bottom and pushing more content above.
  • Vice versa when the bottom line gets focus.
  • To give things time to settle, you might need to scroll when focus hits the second line from top/second line from bottom.
  • This way, the user can just use their arrow keys to read and scrolling will just happen transparently.
  • Also consider wrapping this in role="dialog" so the document is isolated from other content and users can move aroun dit without falling into other content. Users will need a way to escape from it if you do this, though. Pressing tab or perhaps escape should do.

There are two notable problems with this approach:

  1. It won't work for JAWS, since JAWS doesn't focus things as the virtual cursor hits them. You might be able to do this with scroll events instead of focus events, but I don't know how exactly you'd do that.
  2. It still doesn't solve the selection problem when you want to select outside of the viewport. In browse mode, NVDA overrides select all to select the content of browse mode, which will obviously only be the viewport. In addition, scrolling will mess up the previous selection, since content within the selection will disappear.

@Neurrone
Copy link

@jcsteh do you mean tabindex=0 to make things focusable?

@Tyriar since selection wouldn't work using this approach, I was wondering if the use case of selecting previous output was selected for sighted users. If the integrated terminal only allows selecting the current viewport regardless, then this shouldn't be a problem.

@Neurrone
Copy link

@Tyriar oh, missed your last few comments. Suggest leaving the keystroke unbound by default, since a screen reader user will choose something other than ctrl+a. Think an explicit select all, copy and paste should suffice, unless there are others that will find a copy to empty editor useful as well.

@jcsteh
Copy link

jcsteh commented Jan 24, 2018 via email

@Neurrone
Copy link

@jcsteh wow, didn't know that NVDA still focuses stuff with tabindex = -1.

@Tyriar
Copy link
Member

Tyriar commented Jan 24, 2018

@jcsteh great ideas!

One problem I was seeing when doing this was "focus" was maintained separately in screen readers and standard focus when navigating about a page and therefore not getting events. I guess that might have been because I wasn't using tabindex=-1?

Rather than supporting navigation with your own keyboard shortcuts

Would home/end/page up/page down still need custom shortcuts? I use page up to scroll a lot when using the terminal personally.

It won't work for JAWS

I believe JAWS has issues with Chromium so that's not a target currently.

@Tyriar since selection wouldn't work using this approach, I was wondering if the use case of selecting previous output was selected for sighted users. If the integrated terminal only allows selecting the current viewport regardless, then this shouldn't be a problem.

So the select all command in the command palette with reliably select everything in the terminal as we maintain our own selection model, this was added 6-12 months ago so sighed users can select more than what's in the viewport via keyboard/mouse. As for selecting a subset for screen reader users, we may be adding keyboard selection/selecting to the last command in the future.

@jcsteh
Copy link

jcsteh commented Jan 24, 2018

@Neurrone commented on Jan 24, 2018, 11:50 PM GMT+10:

@jcsteh wow, didn't know that NVDA still focuses stuff with tabindex = -1.

Actually, NVDA can't tell the difference between something with a tabindex >= 0 and -1. All it knows is that it is focusable.

One problem I was seeing when doing this was "focus" was maintained separately in screen readers and standard focus when navigating about a page and therefore not getting events. I guess that might have been because I wasn't using tabindex=-1?

Unless the element is focusable by default (e.g. button, a href, input, etc.), if you're not using a tabindex (-1 or otherwise), it is not focusable as far as the DOM is concerned. Accessibility doesn't get any special treatment here; if it isn't focusable, an accessibility client can't "focus" it either.

Would home/end/page up/page down still need custom shortcuts? I use page up to scroll a lot when using the terminal personally.

The problem is that NVDA browse mode will override these to navigate within browse mode. That is indeed another problem with this approach, perhaps a showstopper.

It won't work for JAWS

I believe JAWS has issues with Chromium so that's not a target currently.

JAWS does work with Chrome IIRC, but I'm not sure how well it works with embedded Chromium.

@Tyriar
Copy link
Member

Tyriar commented Jan 25, 2018

@jcsteh I have a prototype and it's working very well. The only issue I see is that holding down up will skip over the remaining lines. I tried to add a bigger buffer at the top but NVDA just gets confused and continues to focus items above. I think it should be fine to accept that. It also feels more "solid" in general and supports reading through each char in a line etc.

On page up/page down/etc. there are other commands that deal with that already, for example shift page up and shift page down scroll up and down a page. I'm thinking I'll just make them aware of the focus inside the terminal and handle it correctly (no need to maintain two different keybindings).

@Tyriar
Copy link
Member

Tyriar commented Jan 28, 2018

The next Insiders on Monday will have the new mode.

I'm thinking I'll just make them aware of the focus inside the terminal and handle it correctly (no need to maintain two different keybindings).

This didn't work unfortunately as the screen reader takes full control of the keyboard in this mode.

@Neurrone
Copy link

@Tyriar thanks. About the keyboard override, it shouldn't be too problematic since there's a select all feature.

@jcsteh
Copy link

jcsteh commented Jan 28, 2018 via email

@Tyriar
Copy link
Member

Tyriar commented Jan 28, 2018

@jcsteh I thought that would be a possibility, I tried with cmd+page up but it didn't work. Do you know how to do this using NVDA or VoiceOver so I can test it out?

@Tyriar
Copy link
Member

Tyriar commented Jan 28, 2018

I'm going to close this off as the feature is done. @jcsteh we can continue the discussion about keys in browse mode in xtermjs/xterm.js#1257

@Tyriar Tyriar closed this as completed Jan 28, 2018
Tyriar added a commit to microsoft/vscode-docs that referenced this issue Jan 28, 2018
It should just work as expected by screen reader users now.

Part of microsoft/vscode#8339
@vscodebot vscodebot bot locked and limited conversation to collaborators Mar 14, 2018
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 Integrated terminal issues upstream Issue identified as 'upstream' component related (exists outside of VS Code)
Projects
None yet
Development

No branches or pull requests

5 participants