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

Improving accessibility with ARIA attributes in templates #1865

Closed
wants to merge 23 commits into from

Conversation

ACK1D
Copy link

@ACK1D ACK1D commented Dec 23, 2023

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.

Copy link
Member

@matthiask matthiask left a 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 does role="columnheader" for a cell inside thead (this also concerns other changes)
  • tagging both dt and dt using listitem 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.

@ACK1D
Copy link
Author

ACK1D commented Dec 28, 2023

@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.
ss1

@tim-schilling
Copy link
Contributor

@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.

@ACK1D
Copy link
Author

ACK1D commented Dec 28, 2023

@tim-schilling done.

@matthiask
Copy link
Member

@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.

@sabderemane
Copy link
Member

@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 !

@thibaudcolas
Copy link

thibaudcolas commented Jan 15, 2024

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,

  • Adding role="link" on a a element with a href does nothing. It’s already a link.
  • Panel button list items can’t be role="checkbox" and have interactive elements inside, nor can a ul have descendants that are role="checkbox"
  • <h4 role="heading" aria-level="2"> should just be an h2.

Please have a look at the first rule of ARIA. In particular, use of the role attribute should be a last resort and I can’t imagine it would ever be the right approach except for ARIA roles with no corresponding HTML element (and if so, follow ARIA authoring practices). See ARIA in HTML for a list of HTML elements’ built-in roles.


@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.

@matthiask
Copy link
Member

@thibaudcolas Thank you, that's very helpful!

@tim-schilling
Copy link
Contributor

I'm closing this in favor of Thibaud's direction here: #1865 (comment)

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.

None yet

5 participants