-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Improve keyboard shortcuts & a11y. (Still a bunch remaining) #302
Comments
This is needed for accessibility to work properly |
I've forked matrix-react-sdk and vector-web in order to test this approach adding a few changes like this. It mostly works although after the latest changes introduced in Riot v 0.9 I am starting to believe we do need more help with accessibility as room sublists are very difficult to navigate and right pannel holding tab pannels such as member lists, files and notifications are impossible to reach with further tweaks. |
Would like to bump this. I was sad to see this regressed after returning to Matrix and attempting to use the non-vanilla React client. First I had to reject an invite, which involved routing my screen reader to the correct div and simulating a click. I then had to do the same to enter a room. Once inside rooms, the interface is painful to navigate since it no longer seems to include landmarks. I think I submitted some PRs to the vanilla React client to use more semantic HTML tags (I.e. Additionally, I suspect but cannot confirm that some controls are image-only, and are missing textual representations at all. If these controls aren't links/buttons then they can't be interacted with via the keyboard, but if they don't include an I have lots of respect for what you're doing, and as much as I'd like to roll up my sleeves and fix this, I feel like I already addressed some of these and my changes were rolled back. I understand it was accidental, but these tweaks really do make the difference between an easy-to-use client and one that I avoid. If we can restore the missing functionality and commit to keeping it, I'd be happy to dig in and submit additional accessibility-related PRs to make Riot the most accessible chat client out there. Slack will likely never fix their accessibility issues, so I actively recommend Matrix/Rocket.chat wherever I can precisely because you all try to create a more accessible experience. :) |
bumped priority. agreed that the current situation sucks :( |
@ndarilek I started to be afraid that you have moved to rocketchat exclusivelly. I haven't taken care of missing aria landmarks and proper list items in the chat output area however I am trying to keep interactive controls focusable and labelled in my forks. |
Hmm, any chance we can get your forks upstreamed as PRs?
|
@ndarilek I think it needs more work. I would preferably like to have someone sighted to at least look at it whether changing divs to buttons does not ruin the overal design before subbmitting it as a PR. |
I understand that, but if you submit the PR then someone sighted can
look over the changes as part of the review process, right? Also, if the
element-based controls look different than their div counterparts,
that's just a CSS tweak which designers could make on the Riot end.
Basically, I was about to give up and change all the divs, but I don't
have a sighted person to look that over so I'd be in the same boat. I'd
think that getting part of the job done would be appreciated, and might
inspire someone else to finish it.
|
Okay, I'll wait a bit to find out whether we have made enough noyse these days if someone will try to join us. If not I will update to the head revisions during one of the upcoming weekends and send it as a pull request. |
Thanks! Do you also address the noisy read receipt icons? If not, I'll
take a crack at those once your PR lands. Thanks for working on this.
|
I haven't yet found how to make them less anoying but have access to that information. I don't want aria-labels on the messages as some of the screen readers are not rendering aria-labels as a content e.g. orca with Firefox on linux. |
Two issues:
1. Please ensure that the button has `tabindex="0"` so it is
keyboard-focusable.
2. Please ensure that the button responds to the Space and Enter keys
for simulating clicks.
I think that, if you do those two things, it should work. Maybe package
it up in a React `<Button/>` component using a `<div/>` underneath, but
with the tabindex/keyboard behavior included so it doesn't get missed
for future buttons. This would also let us make any additional
modifications in one place and have them reflected across the codebase.
Thanks!
|
Hello,
By saying role"button" you are saying this control behaves similar to
button, let's present it accordingly. So if you cant change <div>this is a
button</div> into <button>This is a button</button> you will have to also
implement onKeyDown and in some cases some other things in order to
implement proper keyboard controls.
|
(thanks @JaniM!!) |
Indeed thanks @JaniM this is very functional. It's a pity I haven't discovered this earlier. It's almost a month since you have pushed this. Unfortunately there are some edge case and follow issues to be addressed:
Again some highlights in no particular order: |
@pvagner thanks for all the feedback - and sorry that this has been stuck out on /develop. we're doing a release today however. reopening to cover all the accessibility bulletpoints enumerated above. |
I have found another nifty example in the riot code to explain my thinking. Related to the TopLeftMenu it self. It's a HTML list with individual list items set as focusable by adding tabIndex=0. Other than that it has no other accessibility features. So items can be focused by navigating using tab and shift+tab keys however it would be more natural if using up and down arrow keys was implemented to move from item to item if it should resemble a desktop style popup menu. |
Thanks for the detailed information - I'm also hiding in #a11y if it's ever easier to send a message there. I'm personally on a different project for the next couple weeks, but will keep the keyboard accessibility in mind and try to come up with better solutions to our accessibility problem. Once I'm back in riot-web land, I'll get them on /develop and visit #a11y for review. |
so @t3chguy and @turt2live have just landed another wave of screenreader work following feedback by @jcsteh at mozilla. feedback would be very welcome as to whether the situation has improved by testing riot.im/develop |
Nice!
I assume you know about the various buttons that still remain unlabeled,
so I won't comment on those.
One minor nit: when announcing rooms, you don't necessarily need to say
"It has 5 unread messages." Specifically, the "it has" can probably go,
as a) it is fairly intuitive and b) it slows down interaction a bit.
Think of it like presenting any other button or visual control. You
don't necessarily have to explicitly spell everything out, as users
generally quickly figure out intent after a few interactions.
Put differently, "5 unread messages", "5 unreads" or even simply "5" is
pretty easy to figure out after an interaction or two, and the "it has"
just takes up more audio space.
Thanks again, this is looking good.
|
@ndarilek that is super valuable feedback, this is my first time doing any form of screen reader accessibility work and will take it all on board |
Shortcut discoverability is low. Please document https://github.com/vector-im/riot-web/blob/develop/docs/shortcuts.md . Or maybe we need a popup (from ^? or ^h) to show them, like gmail, grafana, ... |
Hello, I have checkmarked items in my old recap comment I think are now solved. I have noticed these improvements:
If there are some spare resources you can dedicate to accessibility related work, I'd appreciate the following:
This is really getting better and better all the time. |
I'm unable to trigger the new keyboard shortcut dialog on a Finnish layout. Maybe I'm doing something wrong. |
Its Ctrl + event.key= |
@t3chguy yes, so that's Ctrl+Shift+7 on my layout and it doesn't trigger. |
So keycode.info on shift+7 gives you a forward slash? |
Correct. |
Okay so this looks like it'll an issue elsewhere too where we check for "CmdOrCtrl" only so the "SHIFT" makes it fail. Will loosen it for this exact one. |
@pvagner would you be able to add an update on what outstanding items need to be resolved before this issue could be closed? |
@kittykat I have checkmarked things I think have been solved. |
Created by @ matthew:matrix.org.
See scrollback in #vector with Peter Vagner from Nov 1
The text was updated successfully, but these errors were encountered: