-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Improving accessibility with ARIA attributes in templates #1865
Conversation
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.
Thank you!
I don't really know enough about a11y to judge this pull request, but
role="status"
for a stacktrace seems strange?role="row"
for a tr element seems very redundant, as doesrole="columnheader"
for a cell insidethead
(this also concerns other changes)- tagging both
dt
anddt
usinglistitem
doesn't seem correct to me, one is a title and one isn't
If someone can confirm that this change fixes real compatibility issues I'm for it; if not I'm definitely against introducing all those role
attributes. I fear it's more noise than signal at this point.
@matthiask I'm not sure if attributes would be useful for use in Django. But when debugging website accessibility in Wagtail, the lack of attributes gets in the way. |
@ACK1D it looks like the changes are also causing problems with the test suite. Can you look into getting that to pass? I suspect getting it to pass one environment will resolve it for all of them. |
@tim-schilling done. |
@sabderemane Feel free to ignore this, but I saw in #1842 that you're (probably/surely) much more knowledgeable in a11y than I am and I would appreciate a review or a few opinions on this. |
I will have a look ! |
I wouldn’t recommend proceeding with those changes as-is. There’s lots of attributes added that wouldn’t do anything, lots where using the more correct HTML elements to start with would be a better fix, and lots where ARIA is used incorrectly. I think it’d be simpler to assess those fixes if the issues were defined in isolation from one-another, and solutions devised separately, rather than a blanket addition of ARIA attributes. For example,
Please have a look at the first rule of ARIA. In particular, use of the @ACK1D if you want to spend more time on this I would recommend to restart by sharing the results of accessibility testing (#1842), possibly from automated tests. Then report those findings, and fix the issues one by one, making sure to test appropriately. If you want to add ARIA attributes in particular, I would recommend testing with at least one screen reader. See django/django#17338 for recommended accessibility testing steps for Django contributors. |
@thibaudcolas Thank you, that's very helpful! |
I'm closing this in favor of Thibaud's direction here: #1865 (comment) |
Description
This PR aims to enhance accessibility across templates tool. The changes involve the addition of appropriate attributes, such as role attributes and those related to content narration, to improve the user experience for individuals with limited capabilities.
The motivation behind these changes is to create a more inclusive and user-friendly environment within the tool.