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 screenreader info about links in tabs for enabling navigation with keyboard shortcuts #4996

Merged
merged 2 commits into from
Apr 25, 2024

Conversation

dawidpieper
Copy link
Contributor

What does this PR do and why is it necessary?

In PR #4928 many screenreader related improvements were implemented.
One of them was, as proposed by WCAG, setting correct aria roles and labels.
Unfortunately, after final release of 1.10.0 I found out that this behavior, while with accordance with guidelines, makes it a lot harder to navigate between tabs as they are no longer seen as links by screenreaders. I used to navigate them using keyboard shortcuts or rotor on Mac, but now it is out of question.
This PR tries to make a workaround that allows for such navigation, while being compatible with WCAG and giving proper screenreader feedback.

How was it tested? How can it be tested by the reviewer?

Tested on Windows, Mac, iOS.

Any background context you want to provide?

N/A

What are the relevant tickets if any?

N/A

Screenshots (if appropriate)

N/A

Further notes

I'd like to thank you for your interest in making OctoPrint accessible software, recently many projects (also from the 3d printing world) ignore or assign low priority to a11y-related PRs.

I would be very grateful if that fix could be implemented in maintenance branch (version 1.10.1), as the issue (sorry for not noticing it previously) is quite problematic.

@github-actions github-actions bot added targets maintenance The PR targets the maintenance branch approved Issue has been approved by the bot or manually for further processing labels Apr 25, 2024
@foosel foosel added the bugfix release relevant Issue should be looked at for backporting into the next bugfix release label Apr 25, 2024
@foosel foosel added this to the 1.10.x milestone Apr 25, 2024
@foosel
Copy link
Member

foosel commented Apr 25, 2024

Hey @dawidpieper, I'll schedule this for 1.10.1 :) Have some other things going into that (as mentioned in the release notes). I'll try to get that out in early May, or at least that's the current goal. If something more urgent comes in earlier, I'll see I include this with it!

A question, giving the link itself the role="tab" parameter won't solve it? I'm trying to avoid divs like this, but if that's the only way to fix this and it doesn't cause issues during a quick test, it's also fine.

@dawidpieper
Copy link
Contributor Author

@foosel
Thanks!

It's also how I hoped it works, but now it turned out that screenreaders cannot get that it's a link.

When we make link with role="tab", screenreaders do not announce it as a link anymore, just as a tab.

As a result, when I try to navigate with keyboard or rotor shortcuts, the focus just ignores tabs. I can obviously focus them with arrow keys, but to be honest it's wasting a lot of time as I must get there over all tab's content. That's why the keyboard shortcuts were implemented in screenreaders.

On the other hand, when we do not assign role="tab", users are not informed that this is a tab. If someone hears screenreader announcing link, they expect some page to load. We don't get information that anything has changed (a tab opened).

So the only option I can see to preserve both information is to hide link inside a div. :(

@foosel
Copy link
Member

foosel commented Apr 25, 2024

Sorry, I was to much multitasking and didn't notice that your change exactly fixed what I suggested to do. My fault for doing too much at the same time, sorry for the confusion 😅

I'll look into merging this then ASAP and also backporting it to staging/bugfix. What about the tabs in the settings and the about dialog (which are more like a nav list, but internally handled like a tab)? Might they also need a similar treatment?

Oh, as I just cross checked that in the current code... would putting the role="tab" on the li element surrounding the link perhaps work with the screen reader behaviour?

@dawidpieper
Copy link
Contributor Author

Sorry, now it's me who didn't notice the easiest solution... :)
Yes, it works, just tested.

@foosel
Copy link
Member

foosel commented Apr 25, 2024

🙌 Teamwork 😊

@foosel foosel merged commit ded6199 into OctoPrint:maintenance Apr 25, 2024
27 checks passed
foosel pushed a commit that referenced this pull request Apr 25, 2024
* Add screenreader info about links in tabs for enabling navigation with keyboard shortcuts

* Move role attribute to the list item
@foosel
Copy link
Member

foosel commented Apr 25, 2024

Now also backported for inclusion in 1.10.1.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Issue has been approved by the bot or manually for further processing bugfix release relevant Issue should be looked at for backporting into the next bugfix release targets maintenance The PR targets the maintenance branch
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants