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

Move aria-labelledby into computed menu props #729

Merged
merged 1 commit into from
Aug 23, 2024

Conversation

owenatgov
Copy link
Contributor

What/Why

The examples include aria-labelledby's passed to the autocomplete via menuAttributes. This change moves aria-labelledby into the computed menu attributes so that it's provided by default.

This was prompted by a recent DAC audit of the GOV.UK Design System website which noted that the aria-labelledby is absent from our site search. We could solve this fairly quickly by just adding menuAttributes to the site search autocomplete, but solving it upstream is the more holistic option. This also means all users of the autocomplete don't need to add their own aria-labelledby.

Resolves alphagov/govuk-design-system#4005

Notes

We could also use aria-label instead of aria-labelledby, however I'm not sure what aria-label could be. We could potentially explore a dynamic aria-label but this could be challenging when trying to take internationalisation into account. In my opinion, aria-labelledby is the more managable option.

@owenatgov owenatgov requested a review from a team August 16, 2024 14:07
Copy link

netlify bot commented Aug 16, 2024

Deploy Preview for accessible-autocomplete ready!

Name Link
🔨 Latest commit 389e8e8
🔍 Latest deploy log https://app.netlify.com/sites/accessible-autocomplete/deploys/66c885687ffba50008471687
😎 Deploy Preview https://deploy-preview-729--accessible-autocomplete.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Member

@romaricpascal romaricpascal left a comment

Choose a reason for hiding this comment

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

Neat, it does clean up all these examples with the menuAttributes 🙌🏻 It's bringing a tiny breaking change in its current state, though. Will other PRs bring breaking changes for the accessible-autocomplete? If not, I've proposed a fix to set the default aria-labelledby non breaking.

src/autocomplete.js Outdated Show resolved Hide resolved
src/autocomplete.js Outdated Show resolved Hide resolved
@@ -508,6 +508,8 @@ export default class Autocomplete extends Component {
}

const computedMenuAttributes = {
// set aria-labelledby first so that users can override it with menuAttributes
Copy link
Member

Choose a reason for hiding this comment

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

Good shout adding a comment 😍

@owenatgov owenatgov merged commit 5cd4854 into main Aug 23, 2024
7 checks passed
@owenatgov owenatgov deleted the aria-labelledby-default branch August 23, 2024 13:29
@selfthinker
Copy link

From the description it wasn't clear to me what this PR did, specifically how it fixed the issue raised.
Now that I've tested it, I found the fix is to label the listbox with "[search term] list".

It might have been better to give it the same name independent of the search term, like "results list".
Although there might also be benefits in labelling it differently every time to make clear it's a different list.
Altogether I think it works well enough as it is. 👍

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.

Autocomplete: Listbox missing accessible name
3 participants