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.

Sign up for GitHub

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

Jump to bottom

Improve accessibility of Buttons in Webapp #6070

Merged
merged 1 commit into from
Jul 17, 2020
Merged

Improve accessibility of Buttons in Webapp #6070

merged 1 commit into from
Jul 17, 2020

Conversation

t3chguy
Copy link
Contributor

@t3chguy t3chguy commented Apr 16, 2020

Rebirth of #5432 without the conflicting Space onKeyUp handler which made PTT not work when focus was on an accessible button.

@jitsi-jenkins
Copy link

Hi, thanks for your contribution!
If you haven't already done so, could you please make sure you sign our CLA (https://jitsi.org/icla for individuals and https://jitsi.org/ccla for corporations)? We would unfortunately be unable to merge your patch unless we have that piece :(.

@davidacm
Copy link

davidacm commented May 6, 2020

Hi, what happened with this PR? Blind people need it!
I'm planning some online free workshops for blind people, and I'd prefer to use jitsi because is very intuitive, but the accessibility need to be improved first.
Thanks in advance!

@t3chguy
Copy link
Contributor Author

t3chguy commented May 14, 2020

What is holding this up? It improves a11y. It makes the app at least moderately usable with a screen reader.

@luixxiul luixxiul added the web Issue related to the web frontend label May 15, 2020
@saghul
Copy link
Member

saghul commented May 15, 2020

Some manual testing to make sure nothing breaks, that’s all.

@saghul
Copy link
Member

saghul commented May 15, 2020

Jenkins please test this please.

@Echolon Echolon added accessibility Issue related to accessibility topics/handicapped users ui/ux User Interface / User Experience related issues labels May 27, 2020
@t3chguy
Copy link
Contributor Author

t3chguy commented Jun 1, 2020

Any update on this?

@johntyree
Copy link

Jenkins please test this please.

@saghul

What would be considered an acceptable level of testing for this? If there aren't webdriver tests in place to exercise the UI, could volunteers manually test it to help this get merged? Maybe screen cap a video for review?

@pvagner
Copy link

pvagner commented Jun 17, 2020

I'm running this on my server and I have participated in a few meetings with great success. I'm blind using orca screen reader on linux with Firefox and Chrome.
It's working great over here. If only I wouldn't have to add this patch each time I'm updating jitsi meet.
Thanks @t3chguy

@t3chguy
Copy link
Contributor Author

t3chguy commented Jun 17, 2020

You are welcome Peter.
It would be great if this got some real attention, Jitsi is right now inaccessible to Accessible Technologies users and at a time when the world needs good, easily accessible voip, it really is important.

@t3chguy
Copy link
Contributor Author

t3chguy commented Jun 27, 2020

Any chance of accessibility being taken seriously any time soon? A patch more invasive than this was already merged and reverted, this one takes care to not affect the UX for non-AT users at the expense of imperfect AT contract fulfilment.

@MarcoZehe
Copy link
Contributor

Friendly ping on this issue. The situation really is very very unsatisfactory. And the fact that we even have to resort to this compromise is, too. :-( But it would at least be a start.

saghul
saghul reviewed Jul 17, 2020
View reviewed changes
Copy link
Member

@saghul saghul left a comment

Choose a reason for hiding this comment

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

Sorry for the delay. Left a couple of comments, otherwise LGTM.

react/features/base/toolbox/components/ToolboxItem.web.js
onClick: disabled ? undefined : onClick
onClick: disabled ? undefined : onClick,
onKeyDown: this._onKeyDown,
tabIndex: 0,
Copy link
Member

Choose a reason for hiding this comment

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

is tabIndex necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is what puts the li or div into the tab rotation which makes it accessible by keyboard in the first place. If you were using a button here then it wouldn't be needed.

react/features/toolbox/components/web/ToolbarButton.js
onClick = { this.props.onClick }
onKeyDown = { this._onKeyDown }
role = 'button'
tabIndex = { 0 }>
Copy link
Member

Choose a reason for hiding this comment

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

ditto

Copy link
Contributor Author

Choose a reason for hiding this comment

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

same as above for this div, without it would not be reachable via keyboard

@saghul saghul merged commit 335b430 into jitsi:master Jul 17, 2020
@saghul
Copy link
Member

saghul commented Jul 17, 2020

Thanks for this! I know we have a long way to go here, hopefully we can make steady progress.

@muscat1 muscat1 mentioned this pull request Oct 2, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@saghul saghul saghul approved these changes

Assignees
No one assigned
Labels
accessibility Issue related to accessibility topics/handicapped users ui/ux User Interface / User Experience related issues web Issue related to the web frontend
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@t3chguy @jitsi-jenkins @davidacm @saghul @johntyree @pvagner @MarcoZehe @luixxiul @Echolon