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

Fix: a11y combobox attributes (fixes #147) #198

Merged
merged 10 commits into from
Dec 4, 2024
Merged

Fix: a11y combobox attributes (fixes #147) #198

merged 10 commits into from
Dec 4, 2024

Conversation

oliverfoster
Copy link
Member

fixes #147

Fix

  • Added role, aria-controls and relevant id

@oliverfoster oliverfoster self-assigned this Nov 11, 2024
Copy link
Contributor

@kirsty-hames kirsty-hames left a comment

Choose a reason for hiding this comment

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

Testing this with VoiceOver in Safari functions as expected however the reading of the button isn't. "Misspelt" is announced before reading the button context (see screenshot below). I can't see why the "Select an option" text is being cut off. I'll test this with NVDA to compare.
Screenshot 2024-11-11 at 12 51 09

@oliverfoster
Copy link
Member Author

oliverfoster commented Nov 11, 2024

Testing this with VoiceOver in Safari functions as expected however the reading of the button isn't. "Misspelt" is announced before reading the button context (see screenshot below). I can't see why the "Select an option" text is being cut off. I'll test this with NVDA to compare. Screenshot 2024-11-11 at 12 51 09

looking at the example, we maybe have too many different aria-labelledbys, perhaps the one on the ul could be made the same as the one on the button?

https://www.w3.org/WAI/ARIA/apg/patterns/combobox/examples/combobox-select-only/#sc1_label

image

@kirsty-hames
Copy link
Contributor

kirsty-hames commented Nov 14, 2024

Hey @oliverfoster, I've made a couple of commits to align this with the WCAG example you shared. Overall this is reading much better.

We still have the issue mentioned above where VoiceOver in Safari reads "Misspelt" and cuts off the label. I can only replicate this in Safari. VoiceOver in Chrome reads beautifully. NVDA in Chrome and Edge read nicely too.

I'm running Safari v18.0.1 - not sure if this is a Safari specific issue or limited to a particular version. I'll give this a test on iPhone Safari and I'll see if I can replicate the issue. I'll also test the master branch (as it may be an issue prior to this PR).

Update: VoiceOver on iPhone works as expected so the issue seems to be specific to Safari on macOS. I couldn't replicate issue running the master branch so the issue has been introduced during this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

👀

Copy link
Contributor

@joe-allen-89 joe-allen-89 left a comment

Choose a reason for hiding this comment

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

Perfect, now working as expected across all browsers with screen reader/keyboard 👍

@oliverfoster
Copy link
Member Author

One second, just reading again.

@oliverfoster
Copy link
Member Author

What happens when there is no text? The questionTitleId element would go missing and the aria-labelledby would have no meaning.

@kirsty-hames
Copy link
Contributor

What happens when there is no text? The questionTitleId element would go missing and the aria-labelledby would have no meaning.

Good point! In this instance, should it read the following:

  • ariaQuestion
  • if ariaQuestion isn't set, then displayTitle body instruction ?

@kirsty-hames kirsty-hames self-requested a review November 20, 2024 15:53
@oliverfoster
Copy link
Member Author

Sounds great, yes.

@kirsty-hames
Copy link
Contributor

What happens when there is no text? The questionTitleId element would go missing and the aria-labelledby would have no meaning.

Good point! In this instance, should it read the following:

  • ariaQuestion
  • if ariaQuestion isn't set, then displayTitle body instruction ?

I've implemented the above in commit b77f232. Note, due to aria-labelledby requiring an id, ariaQuestion is provided as an aria-label instead.

In the instance, aria-labelledby and aria-label both exist, for example both item text and ariaQuestion are set, aria-labelledby takes precedence.

templates/matching.jsx Outdated Show resolved Hide resolved
templates/matchingDropDown.jsx Outdated Show resolved Hide resolved
Copy link
Member Author

@oliverfoster oliverfoster left a comment

Choose a reason for hiding this comment

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

do these need changing?

templates/matching.jsx Outdated Show resolved Hide resolved
templates/matchingDropDown.jsx Outdated Show resolved Hide resolved
- replace missed instances of ariaLabel with ariaLabelledBy (as per commit 0f5203a)
- remove questionTitleId from matchingDropDown template props (removed previously in commit c328cbc)
@oliverfoster
Copy link
Member Author

👍

@oliverfoster oliverfoster merged commit 9998391 into master Dec 4, 2024
@oliverfoster oliverfoster deleted the issue/147 branch December 4, 2024 15:21
github-actions bot pushed a commit that referenced this pull request Dec 4, 2024
## [7.6.1](v7.6.0...v7.6.1) (2024-12-04)

### Fix

* a11y combobox attributes (fixes #147) (#198) ([9998391](9998391)), closes [#147](#147) [#198](#198)
Copy link

github-actions bot commented Dec 4, 2024

🎉 This PR is included in version 7.6.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Align with wcag spec
4 participants