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

Add headings across the app for easier screen reader navigation #12427

Merged
merged 1 commit into from
Feb 28, 2023

Conversation

manuhabitela
Copy link
Contributor

@manuhabitela manuhabitela commented Oct 20, 2022

This doesn't change anything for sighted users, but it can make a noticeable difference for users using a screen reader. Because a screen reader has keyboard shortcuts to jump from heading to heading, a user can quickly scan the content of a webpage or quickly navigate from one part to the other thanks to them.

I didn't add any heading to the "video space" central part of the UI (except when the whiteboard is shown), because I'm not sure there is actual content to announce to screen reader users in this part, but maybe I'm wrong?

Below is a screenshot with the headings list popup of NVDA, a free and open source screen reader on Windows, to show the benefits of adding this. Before this PR, the headings list only contained the headings from Excalidraw when the whiteboard was shown.

headings-2

Related to #12379

@saghul

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

@manuhabitela
Copy link
Contributor Author

Allowing myself to bump this in case you missed it 🙏 @saghul

@saghul
Copy link
Member

saghul commented Dec 8, 2022

Sorry for the delay! @Calinteodor will be looking at these shortly!

@@ -446,6 +447,12 @@ class Filmstrip extends PureComponent <IProps, State> {
_verticalViewGrid && 'no-vertical-padding',
_verticalViewBackground && classes.filmstripBackground) }
style = { filmstripStyle }>
<span
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this useful here? I am using VoiceOver and it does not read/see this section.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's weird, works great for me… Don't you see "Video thumbnails" in the headings list if you open the VoiceOver rotor (via the VO+U shortcut)?

Copy link
Contributor

Choose a reason for hiding this comment

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

Will check it.

Copy link
Contributor

Choose a reason for hiding this comment

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

All good!
I was more familiar with NVDA than with VoiceOver.
Thank you for the indications!

@Calinteodor
Copy link
Contributor

Calinteodor commented Dec 8, 2022

@Leimi thanks so much for your contribution!
It is a great thing that our app is getting help and it's starting to become accessible for everyone!
Regarding this PR, I left some comments/questions based on your updates.

@manuhabitela
Copy link
Contributor Author

manuhabitela commented Dec 8, 2022

Thanks for the feedback, will take the time to answer to this hopefully by monday 🙏

edit : unfortunately i won't have the time right now, will go back to this next week.

@Calinteodor
Copy link
Contributor

Please rebase and resolve conflicts. Thanks!

@Calinteodor Calinteodor marked this pull request as draft February 27, 2023 13:00
className = 'sr-only'
role = 'heading'>
{ t('participantsPane.title') }
</span>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed the logic here after the rebase. Since 50c4748 added a potential visitors count in addition to the participants count, we can't rely on the participants count sentence to be the heading anymore. If we did, the visitors count would certainly be missed by people navigating from heading to heading.

@manuhabitela manuhabitela marked this pull request as ready for review February 27, 2023 13:36
@Calinteodor
Copy link
Contributor

Calinteodor commented Feb 27, 2023

Same steps as #12657 to fix errors.

This doesn't change anything for sighted users, but it can make a
noticeable difference for users using a screen reader. Because a screen
reader has keyboard shortcuts to jump from heading to heading, a user
can quickly scan the content of a webpage or quickly navigate from one
part to the other thanks to them.
@manuhabitela
Copy link
Contributor Author

So I guess it should be good now, thanks for your patience, still figuring out this project, first PR is bound to be messy 🙃

@Calinteodor
Copy link
Contributor

So I guess it should be good now, thanks for your patience, still figuring out this project, first PR is bound to be messy 🙃

Don't worry, Leimi!
Your contribution is much appreciated and I am here to help work things through!

@Calinteodor
Copy link
Contributor

jenkins test this please

@damencho
Copy link
Member

jenkins test this please

@Calinteodor Calinteodor merged commit 72dd609 into jitsi:master Feb 28, 2023
hristoterezov pushed a commit that referenced this pull request Jul 19, 2023
…12427)

feat(a11y): added headings across the app for easier screen reader nav
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants