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 menuAttributes to fix Axe's 'aria-input-field-name' error #591

Merged
merged 3 commits into from Sep 20, 2023

Conversation

mchughbri
Copy link
Contributor

@mchughbri mchughbri commented Apr 25, 2023

Add the menuAttributes option to allow for attributes and their values to be added to the menu part of the component.

This fixes #361 as it allows for aria-labelledby to be set to the id of the existing label which in turn allows the code to pass the AXE rule "aria-input-field-name" (ARIA input fields have an accessible name).

No new tests have been added yet. While this PR adds the ability to add aria-label or aria-labelledby, by default these are not set, and so the default examples on the demo pages will still not pass the aXe tests.

What do you think we should do about this?

  • Set a default aria-label value (But what would it say by default?)
  • Forcibly set an id on the label and use aria-labelledby to associate the two
  • Update all the examples to include manually setting an aria-label

@romaricpascal
Copy link
Member

Thanks for this update @mchughbri (and your patience with us finding the time to review it 😬 ). The code update is looking good 🙌🏻

For the examples, it would be neat to update them now we have that new option. I think having an id and using aria-labelledby would nudge people towards the use intended for menuAttributes, so that would be my prefered way to go. If you have a moment to update them and add a little testing for the new option, that'd be ace! Thanks 😄

@domoscargin domoscargin added the submitted by user issues on behalf of users label Jun 6, 2023
@mchughbri
Copy link
Contributor Author

mchughbri commented Jun 15, 2023

Thanks @romaricpascal

Now updated examples and added test for the new menuAttributes option

@romaricpascal
Copy link
Member

@mchughbri Cheers for adding the tests and updating the examples, looks all neat to me! Seems our workflows have not started for some reason, though. Would you be able to run a quick force push on your branch to try and trigger them again, please? 😊

@mchughbri
Copy link
Contributor Author

@romaricpascal now done thanks.

@romaricpascal
Copy link
Member

romaricpascal commented Jul 27, 2023

Oh sorry for my unclear instructions, @mchughbri I meant a force push with a different commit hash to try and trigger whatever Github's do to relaunch the checks.

I usually git commit --amend -C HEAD which should generate a new hash for the last commit while keeping everything the same 😊

@mchughbri
Copy link
Contributor Author

All done @romaricpascal thanks.

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.

Cheers! Tests are all green. Just needs a second approval and it'll be good to merge 😊

@mchughbri
Copy link
Contributor Author

Hey @romaricpascal just wondered if we were any closer to getting another approval on this? Thanks.

@owenatgov owenatgov merged commit db6ccc7 into alphagov:main Sep 20, 2023
3 checks passed
@owenatgov
Copy link
Contributor

@mchughbri Sorry for the second delay on this! Looking good, merged :)

@romaricpascal romaricpascal changed the title Add menuAttributes to fix #361 Add menuAttributes to fix Axe's 'aria-input-field-name' error Feb 22, 2024
@romaricpascal romaricpascal changed the title Add menuAttributes to fix Axe's 'aria-input-field-name' error Add menuAttributes to fix Axe's 'aria-input-field-name' error Feb 22, 2024
@romaricpascal romaricpascal mentioned this pull request Apr 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
accessibility submitted by user issues on behalf of users
Projects
Development

Successfully merging this pull request may close these issues.

Failing AXE tests
4 participants