-
Notifications
You must be signed in to change notification settings - Fork 396
Fix(Accessibility): Add aria-modal and aria-labelledby to UI Modals. #10489
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
base: release_10
Are you sure you want to change the base?
Conversation
…Ticket 43962
Fixes a WCAG violation by adding aria-modal="true" and aria-labelledby="{ID}_title" to the <dialog> element in all core modal templates. The corresponding id="{ID}_title" is also added to the modal's <h1> title.
This ensures screen readers properly announce the modal's purpose and correctly treat it as a priority overlay.
Annett7811
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From an accessibility perspective, this looks good. Thank you very much for your work.
Best regards,
Annett
|
Hello @ZallaxDev. |
oliversamoila
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello together,
from a UX and UI perspective, the result looks good. Thank you very much for your contribution.
I'm forwarding this to @thibsy for code review. He may contact you with change requests or incorporate the changes directly (That includes cherry picking too.)
Thibeau, can you resolve the failed checks or does this need to be passed back to the contributor?
Kind regards,
@oliversamoila (as UI coordinator)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @ZallaxDev,
Thx a lot for your contribution to the UI framework!
The changes look very straightforward and good to me, please ship the rest =).
Also note, you do not need to provide separate PR's for the different versions. I will cherry-pick them accordingly.
Kind regards,
@thibsy (as UI coordinator)
Updated expected HTML output for Interruptive and Lightbox modals to reflect accessibility improvements. - Added `aria-modal="true"` to the `<dialog>` element. - Added `aria-labelledby` pointing to the modal title ID. - Added `id="id_1_title"` to the modal title `<h1>` element.
…ents to include ARIA attributes on modal dialogs. The expected HTML now includes `aria-modal="true"` and `aria-labelledby="id_1_title"` on the `<dialog>` element, with a corresponding `id="id_1_title"` on the modal title `<h1>`.
Fixes a WCAG violation by adding
aria-modal="true"andaria-labelledby="{ID}_title"to the<dialog>element in all core modal templates. The correspondingid="{ID}_title"is also added to the modal's<h1>title.This ensures screen readers properly announce the modal's purpose and correctly treat it as a priority overlay.
To optimise time, we want to be sure that the implementation is correct, since if any additional changes are required, the tests will have to be updated again, with the corresponding time expenditure on this task.
Therefore, we would appreciate feedback on these changes, and when we can confirm that they are adequate, we will update the tests in this same PR before performing the merge.
Ticket 43962