Make Jitsi WCAG 2.1 compliant #8921

skolmer · 2021-03-31T13:28:55Z

WCAG 2.1 AA Compliance / Accessibility

This PR is the followup to #8423 @saghul @damencho

We managed to make jitsi-meet (web) fully WCAG 2.1 AA compliant. This has been tested and approved by two independent entities.
One feedback I want to share with the community came from a person who relies on a screen-reader for his daily work. He was very excited that he was finally able to participate in video conferences at his workplace.

We would really appreciate if you could give this review some priority because it looks like there is currently a lot of work being done on UI elements and we already had a hard time keeping up with the master branch while doing our final code review 🙂

This is a rather large PR (sorry 😉) because we had to modify almost every element of jitsi-meet web. But don't be afraid there are a lot of similar changes like aria attributes, color contrasts or keypress handler.
I will go into more detail regarding PTT and global keyboard shortcuts below to give you more insights what we did and why we did it. This should make your life easier reviewing all these changes. If there are still things we missed to explain please let us know in the comments.

Features

Full keyboard access to all UI elements
Better contrast for improved readability/visibility on some elements
Descriptive aria attributes and labels for screen readers
Highlighting of focused elements
Unified wording / More descriptive labels
A setting to disable keyboard shortcuts

Fixes

No interference between input elements like buttons and PTT or other global keyboard shortcuts
Hide Mute Audio/Video moderation features from toolbox overflow menu if user is not a moderator
Re-integrated video mute button to toolbox overflow menu
Auto-Hide of toolbox will now recognize focused elements and stay open until unfocused
Auto-Hide of toolbox will now recognize when a user is hovering the remote-video-control menu
Fixed all places where eslint-disable-next-line react/jsx-no-bind was used to avoid unnecessary rerender
The SPACE key will now only unmute while pressed and you were muted before. The old behavior was to also activate mute on SPACE similar to the M key. If you take push-to-talk literally this was a wrong behavior and M should be the only shortcut to mute while SPACE will allow you to talk while holding SPACE when muted.
Minor UI fixes like z-index, padding issues, missing headlines in dialogs, responsiveness or overflowing elements

Push-To-Talk (PTT) / Keyboard Shortcuts

We decided on an approach that allowed us to keep global shortcuts (like PTT) the jitsi community is used to while adding space bar support to UI elements like buttons and menus.
This took some extra work because PTT and other shortcuts like numbers can interfere with the expected experience of screen-reader users and keyboard users in general.

We allow people to disable keyboard shortcuts entirely if features like PTT or number shortcuts are causing too much confusion when using a screen-reader for example. This is a new checkbox in the settings dialog. Keyboard shortcuts stay active by default though to make this a non-breaking change.
We prioritized the keyboard handling of keyboard-focused elements (like input, button, checkbox, ...) over global keyboard shortcuts. Which means when you intentionally focus a button using the tab key the function of the space bar will be to trigger the button. You are also now able to remove focus from an element by pressing ESC. This allows keyboard users to have the native web experience they are used to while also having great features like PTT.

Credits

Big thanks to my team who did most of the hard work @agnesboldt @ahmadkadri @MaikHannemann @nurjinjafar @phk-nord

Dataport would like to thank the Jitsi Open Source Community for a great product with this active contribution from its Phoenix Open Source Initiative – for further information please visit https://www.phoenix-werkstatt.de or https://www.dataport.de

jitsi-jenkins · 2021-03-31T13:30:26Z

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 :(.

damencho · 2021-03-31T13:59:11Z

Jenkins, test this please.

skolmer · 2021-03-31T14:04:33Z

Jenkins, test this please.

We expected some failing tests regarding changed toolbox button labels. If I remember correctly they are used to select the DOM elements in some tests. Can we move to test-id attributes instead to avoid issues in the future or will this cause to many issues because of failing tests on other branches?

damencho · 2021-03-31T14:09:06Z

If I remember correctly there were problems with test-id, it was not accessible on some mobiles through the apis. So we moved to not using those...

saghul · 2021-03-31T14:47:30Z

Amazing work! Please bear with us as we review such a large amount of code. I suppose the answer is "no", but I'll try anyway: any chance this can be split into smaller units of work for easier review?

skolmer · 2021-03-31T14:51:17Z

Amazing work! Please bear with us as we review such a large amount of code. I suppose the answer is "no", but I'll try anyway: any chance this can be split into smaller units of work for easier review?

Unfortunately no, these changes all kind of depended on each other so we squashed them all together.

damencho · 2021-03-31T15:19:07Z

I will give it a try today, to fix the tests.

damencho · 2021-03-31T15:33:55Z

I gave it a quick test and I see that PTT does not work, other shortcuts seems to work (at least video mute).

skolmer · 2021-03-31T15:47:03Z

I gave it a quick test and I see that PTT does not work, other shortcuts seems to work (at least video mute).

It will not mute you on first press of the spacebar (previous behavior) instead it will only unmute you when you hold space while you are muted. This was kind of confusing behavior before because the expectation for push to talk was that holding SPACE will unmute you while you are muted. <- was this what happened in your case?

damencho · 2021-03-31T15:52:20Z

Yes, exactly. That's it ...

damencho · 2021-03-31T17:53:14Z

@skolmer Hey, the author needs to be the same for the PRs to work together, and the branch names of course. Can you open a PR in jitsi-meet-torture, please?
Like this one, the branch name, but needs to be from nordeck jitsi/jitsi-meet-torture#372. Thanks.

skolmer · 2021-03-31T21:03:00Z

Yes, exactly. That's it ...

I have added a better description to "Fixes" in the PR description to make this more obvious. I'm aware this is a change that can cause confusion If you are used to the old behavior and tried to better explain our interpretation how PTT should work. Let us know if you can follow our argumentation or if this needs some more discussion.

damencho · 2021-03-31T21:11:55Z

Oh and first conflicts came in, sorry for that.

So one other thing I see, so the tests are failing around the room password and setting password on pre join screen, I will take a look at the tests ... but one change in behaviour, room key/password field, when setting - Enter was specially handled to set the password/key where now it closes the Dialog and actually does not set a password to the room. And since we moved the security button away from the toolbar this makes it a real concern as people may think that password is set after hitting Enter, where they just dismissed the Dialog. Can you look at that? ... I will try checking the tests tomorrow.

damencho · 2021-03-31T21:13:26Z

Hide Mute Audio/Video moderation features from toolbox overflow menu if user is not a moderator

This was already the case, or I'm missing something?

skolmer · 2021-03-31T21:14:46Z

but one change in behaviour, room key/password field, when setting - Enter was specially handled to set the password/key where now it closes the Dialog and actually does not set a password to the room.

This definitely sounds like a regression. I will take a look tomorrow.

skolmer · 2021-03-31T21:19:19Z

Hide Mute Audio/Video moderation features from toolbox overflow menu if user is not a moderator

This was already the case, or I'm missing something?

This was an interesting bug I noticed while testing. You can still see this on alpha.jitsi.net. There was a check like visible && isModerator so when the visible prop of the button was not set (which was the case) it never evaluated the isModerator flag and defaulted to the default visbility of the base component which was true. So the simple fix here was to give visible a default of true in the inheriting component too. https://github.com/jitsi/jitsi-meet/pull/8921/files#diff-4596482d83bfbae7eb9a2b8400c2558b0e9a7f9bc27674cb8e67d55244dcc63fR65

damencho · 2021-03-31T21:28:36Z

but one change in behaviour, room key/password field, when setting - Enter was specially handled to set the password/key where now it closes the Dialog and actually does not set a password to the room.

This definitely sounds like a regression. I will take a look tomorrow.

Thanks, yeah the passwords tests are failing because of that. https://github.com/jitsi/jitsi-meet-torture/blob/7537ee65a7d43248e9c3a42a2e353be3e8de129d/src/test/java/org/jitsi/meet/test/pageobjects/web/SecurityDialog.java#L88

damencho · 2021-03-31T21:51:36Z

Yep, all other tests are failng because of https://github.com/jitsi/jitsi-meet-torture/blob/7537ee65a7d43248e9c3a42a2e353be3e8de129d/src/test/java/org/jitsi/meet/test/pageobjects/web/SecurityDialog.java#L88, just checked and Lobby.

skolmer · 2021-03-31T21:53:40Z

Yep, all other tests are failng because of https://github.com/jitsi/jitsi-meet-torture/blob/7537ee65a7d43248e9c3a42a2e353be3e8de129d/src/test/java/org/jitsi/meet/test/pageobjects/web/SecurityDialog.java#L88, just checked and Lobby.

Thanks for testing and taking the time to explain, sounds like we will be able to provide a fix tomorrow.

damencho · 2021-06-09T19:24:53Z

There will be one for the other PR and I will schedule and rerun for this one
Correction: the other PR is not whitelisted ... so we are testing only this, and is currently running.

skolmer · 2021-06-09T20:26:34Z

There will be one for the other PR and I will schedule and rerun for this one
Correction: the other PR is not whitelisted ... so we are testing only this, and is currently running.

I'm a little bit confused by the test that is now failing.
It looks like it is testing the toolbox buttons and trying to click the invite button in the toolbox menu... but this button is only visible when 'invite' is in the TOOLBAR_BUTTONS array. Which is not the case. At least I can't find this config in the torture project.
So maybe it was not testing the toolbar button it should have but the invite button in the sidebar instead, which is now renamed? We didn't change the toolbar invite button and it always had a different label (Invite someone) so looks like this test never tested the right button.

damencho · 2021-06-09T20:45:20Z

There will be one for the other PR and I will schedule and rerun for this one
Correction: the other PR is not whitelisted ... so we are testing only this, and is currently running.

I'm a little bit confused by the test that is now failing.
It looks like it is testing the toolbox buttons and trying to click the invite button in the toolbox menu... but this button is only visible when 'invite' is in the TOOLBAR_BUTTONS array. Which is not the case. At least I can't find this config in the torture project.
So maybe it was not testing the toolbar button it should have but the invite button in the sidebar instead, which is now renamed? We didn't change the toolbar invite button and it always had a different label (Invite someone) so looks like this test never tested the right button.

Hum I see it fails on org.jitsi.meet.test.pageobjects.web.InviteDialog.close(InviteDialog.java:102)` humm seems like it tries to click on a button away from the modal dialog expecting it to close .. and it doesn't

Not sure why ...

damencho · 2021-06-09T20:47:03Z

Is there a change around that dialog .... ?

skolmer · 2021-06-09T20:59:21Z

Is there a change around that dialog .... ?

Not recently. But there was a change in the ModalHeader we had to merge in. c4677be#diff-5dc31303b04eb9e5630274591a2dcbd1a4a6df33db756d64530cf7c0ec7bf893R86

I've looked in the test log and it says

[11] TestFailure:
org.openqa.selenium.NoSuchElementException: no such element: Unable to locate element: {"method":"css selector","selector":"[aria-label="Invite people"]"}

This would indicate an issue locating the button that opens the dialog. In the stacktrace it also shows the InviteDialog.close though

at org.openqa.selenium.remote.RemoteWebDriver.findElement(RemoteWebDriver.java:315)
at org.jitsi.meet.test.pageobjects.web.Toolbar.getInviteButton(Toolbar.java:151)
at org.jitsi.meet.test.pageobjects.web.InviteDialog.close(InviteDialog.java:102)
at org.jitsi.meet.test.InviteTest.testInviteURLDisplays(InviteTest.java:69)

damencho · 2021-06-09T21:01:04Z

@skolmer can you update torture once again, please. For some reason I cannot push directly.
https://github.com/jitsi/jitsi-meet-torture/blob/00973955a6e44b7af6ae7d86f790a7989d9449bb/src/test/java/org/jitsi/meet/test/pageobjects/web/Toolbar.java#L41

damencho · 2021-06-09T21:01:34Z

I tested it locally and it seems to work after changing that.

damencho · 2021-06-09T21:05:19Z

Thanks, I started the tests again 🤞

damencho · 2021-06-09T21:23:01Z

Tadaaa green! @muscat1 and @vp8x8 your turn now. If you decide to squash and merge it, make sure you merge and the torture one. Thank you.

vp8x8 · 2021-06-10T07:35:06Z

Jenkins please test this please.

muscat1 · 2021-06-10T08:05:31Z

Took it for another spin and the conference behaves properly! 🚀

On a separate note, as Saul has stated waaay above, in the future, we should probably try to avoid such PRs as they are a nightmare both to review and to maintain.

Waiting on the ok from @vp8x8 as well, but in the meantime, the CI seems to be failing for some reason 🤔

skolmer · 2021-06-10T08:10:22Z

On a separate note, as Saul has stated waaay above, in the future, we should probably try to avoid such PRs as they are a nightmare both to review and to maintain.

I totally agree. I think it was the nature of this change that required to extend almost every component with keypress handler and aria tags. Next time we will try to do it component after component even if it means to have a 2-digit number of PRs... still better than maintaining THIS. 😊 Thanks for taking the time though

muscat1 · 2021-06-10T08:14:33Z

Yeah, I think multiple small PRs would be much easier to treat in isolation.

Regarding the failing pipeline, it might have something to do with some discrepancy between package and package-lock. Could you possibly try to run a clean npm i with a lower version of node, 12, as well as npm 6 (We need to upgrade 🙄) as that might fix it.

skolmer · 2021-06-10T08:55:49Z

Yeah, I think multiple small PRs would be much easier to treat in isolation.

Regarding the failing pipeline, it might have something to do with some discrepancy between package and package-lock. Could you possibly try to run a clean npm i with a lower version of node, 12, as well as npm 6 (We need to upgrade 🙄) as that might fix it.

Not today I'm afraid. But if you send me a fixed lockfile I can push it to the branch.

vp8x8 · 2021-06-10T10:45:38Z

Test it and looks great. So when the pipeline issue is fixed we can merge. 🎊

skolmer · 2021-06-10T12:05:36Z

Test it and looks great. So when the pipeline issue is fixed we can merge. 🎊

OK cool, CI seems to be running now 🎊

damencho · 2021-06-10T12:07:58Z

The tests were green yesterday when I left the message ... The only failing is the github action, but it is something on github side so, I'm not worried after the PR test passed.

damencho · 2021-06-10T12:09:15Z

@skolmer after merging this, some tool for detecting accessibility problems will be good to add so we keep the UI up-to-date, maybe you can help with that :)

skolmer · 2021-06-10T12:14:12Z

@skolmer after merging this, some tool for detecting accessibility problems will be good to add so we keep the UI up-to-date, maybe you can help with that :)

My time in the next two weeks will be very limited but after that I will be happy to integrate some tooling and also make some suggestions how the rules for future feature PRs can look like. Tooling is limited so we will need a combination of tooling and a checklist of best practices a contributor/reviewer should follow.

damencho · 2021-06-10T12:32:58Z

👍

muscat1

Astounding work! Thanks for taking the time and having patience with the entire review process! 🚀🎊

damencho · 2021-06-10T13:14:58Z

Congrats @skolmer and Thank you!

Looking forward to implementing something automated + manual so we can fix and keep the code compliant. Thank you once again for the dedication and help.

* Make Jitsi WCAG 2.1 compliant * Fixed password form keypress handling * Added keypress handler to name form * Removed unneccessary dom query * Fixed mouse hove style * Removed obsolete css rules * accessibilty background feature * Merge remote-tracking branch 'upstream/master' into nic/fix/merge-conflicts * fix error * add german translation * Fixed merge issue * Add id prop back to device selection * Fixed lockfile Co-authored-by: AHMAD KADRI <[email protected]>

Make Jitsi WCAG 2.1 compliant

7110f47

skolmer mentioned this pull request Mar 31, 2021

Meta - accessibility issues #6090

Closed

damencho requested review from vp8x8, saghul, hristoterezov and muscat1 and removed request for vp8x8 and saghul March 31, 2021 13:58

skolmer mentioned this pull request Mar 31, 2021

fix: Fix toolbar strings for WCAG. jitsi/jitsi-meet-torture#373

Merged

Fixed lockfile

1a6c1ac

vp8x8 approved these changes Jun 10, 2021

View reviewed changes

muscat1 approved these changes Jun 10, 2021

View reviewed changes

damencho merged commit e967545 into jitsi:master Jun 10, 2021

muscat1 mentioned this pull request Jun 11, 2021

Make clickable elements reachable by keyboard users #7822

Closed

ghost mentioned this pull request Mar 2, 2022

Keyboard accessibility - web interface #5992

Open

Make Jitsi WCAG 2.1 compliant #8921

Make Jitsi WCAG 2.1 compliant #8921

Conversation

skolmer commented Mar 31, 2021 • edited Loading

WCAG 2.1 AA Compliance / Accessibility

Features

Fixes

Push-To-Talk (PTT) / Keyboard Shortcuts

Credits

jitsi-jenkins commented Mar 31, 2021

damencho commented Mar 31, 2021

skolmer commented Mar 31, 2021

damencho commented Mar 31, 2021

saghul commented Mar 31, 2021

skolmer commented Mar 31, 2021

damencho commented Mar 31, 2021

damencho commented Mar 31, 2021

skolmer commented Mar 31, 2021

damencho commented Mar 31, 2021

damencho commented Mar 31, 2021

skolmer commented Mar 31, 2021

damencho commented Mar 31, 2021

damencho commented Mar 31, 2021 • edited Loading

skolmer commented Mar 31, 2021

skolmer commented Mar 31, 2021 • edited Loading

damencho commented Mar 31, 2021

damencho commented Mar 31, 2021

skolmer commented Mar 31, 2021

damencho commented Jun 9, 2021 • edited Loading

skolmer commented Jun 9, 2021

damencho commented Jun 9, 2021

damencho commented Jun 9, 2021

skolmer commented Jun 9, 2021

damencho commented Jun 9, 2021

damencho commented Jun 9, 2021

damencho commented Jun 9, 2021

damencho commented Jun 9, 2021

vp8x8 commented Jun 10, 2021

muscat1 commented Jun 10, 2021

skolmer commented Jun 10, 2021

muscat1 commented Jun 10, 2021

skolmer commented Jun 10, 2021

vp8x8 commented Jun 10, 2021

skolmer commented Jun 10, 2021

damencho commented Jun 10, 2021

damencho commented Jun 10, 2021

skolmer commented Jun 10, 2021 • edited Loading

damencho commented Jun 10, 2021

muscat1 left a comment

Choose a reason for hiding this comment

damencho commented Jun 10, 2021

skolmer commented Mar 31, 2021 •

edited

Loading

damencho commented Mar 31, 2021 •

edited

Loading

skolmer commented Mar 31, 2021 •

edited

Loading

damencho commented Jun 9, 2021 •

edited

Loading

skolmer commented Jun 10, 2021 •

edited

Loading