-
-
Notifications
You must be signed in to change notification settings - Fork 33k
Fixed #34041 -- Improve accessibility of admin page breadcrumbs area. #19492
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: main
Are you sure you want to change the base?
Conversation
edfbf8f
to
4d1f73b
Compare
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.
Since the last breadcrumb item was changed to an tag, it now has an underline.
Should we remove the underline?
(The ticket mentioned that the existing design should be preserved as is.)
Additionally, removing the underline would likely require a selector with higher specificity. 🥲
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.
As far as I understand the ticket, yes, the underline should be removed. There doesn't seem to be a utility class for that in the CSS so far, so you have to add something like:
.nolink {
color: inherit !Important;
text-decoration: none !Important;
}
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.
In admin-doc, there is no page like template_index for templates.
Therefore, when accessing the detail page, I set the href of the breadcrumb item labeled "template" to an empty string ("").
Is there a better way to handle this?
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.
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.
The relevant example in ATAG also recommends to use CSS for this. However, I think CSS content will still show up in the accessibility tree. So maybe you have to use borders instead? I am not sure about this though, better test with a screen reader.
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.
I saw this implemented with CSS on another site and tested it with a screen reader.
I'll try applying it myself and test it with a screen reader as well.
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.
I found that when the ">" is implemented using content, it is accessible to screen readers. 😥
However, when content is used to render an image and a size is specified inside the content, screen readers do not recognize it.
If we actually want to apply this, using the mask property would allow for color customization as well.
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.
You can add an alternative text to CSS-generated content. Since here it's decorative, the alternative should be empty. Like this:
.myclass::after {
content: ">" / "";
}
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.
I've left comments on a few parts where I had difficulty making decisions...
Could you please review this work? @tut-tuuut 🥰 |
e5f107b
to
8dc9b31
Compare
ooops sorry I did not see the mention, I will review soon! |
I notice you used an unordered list |
Thank you for the review @xi . I agree that using
|
34c389d
to
9deaa0b
Compare
9deaa0b
to
2e3fb7c
Compare
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.
Adding alternative to hide the decoration from screen readers:
content: ' \203A '; | |
content: ' \203A ' / ''; |
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.
Nice, I didn’t know this approach, now screen readers no longer access unnecessary elements like ">" !
2e3fb7c
to
70aa938
Compare
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 think django/contrib/admin/templates/registration/logged_out.html
still needs updating 👍
nitpick, make sure commit messages are in past tense (see https://docs.djangoproject.com/en/5.2/internals/contributing/committing-code/#committing-guidelines)
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.
I think you can also use the css for aria-current=page rather than last child logic here
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.
<li><a href="" aria-current="page">{% translate 'Server error' %}</a></li> | |
<li><a href="#" aria-current="page">{% translate 'Server error' %}</a></li> |
I think hashes are better to prevent a page reload on click
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.
I referred to @thibaudcolas comment from the previous PR regarding this part.
I also agree that using # has its advantages.
Instead of changing the href, how about keeping it as is and applying pointer-events: none
?
That way, regular users won’t experience any change on click, but screen reader users can still access the link via keyboard (tab).
What do you think about this approach?
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.
++ This change would also include pagination ...
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.
What you have currently is aligned with https://www.w3.org/WAI/ARIA/apg/patterns/breadcrumb/examples/breadcrumb/ , so I think you're correct to use href=""
and I don't think an update of css is needed 👍
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.
Yes, I’ve reverted the CSS back to its original state.
70aa938
to
4d82fa4
Compare
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 for the updates @Antoliny0919 ⭐
This looks good to me
Unfortunately the screen readers I test with do not support the aria-current attribute (https://a11ysupport.io/tech/aria/aria-current_attribute Narrator/Orca). This will need testing and approval from an accessibility team member 👍
…a-current attribute.
4d82fa4
to
611e8e9
Compare
Trac ticket number
ticket-34041
Branch description
Continue farita1699's PR
I have improved the accessibility of the breadcrumb area in the admin page as follows.
<li>
items within an<ol>
list.This change allows screen reader users to know how many items are present in the breadcrumbs area.
This change allows screen reader users to identify the current page by announcing it as the "current page" when navigating to the corresponding breadcrumb item.
This change makes the last breadcrumb item accessible via the "Tab key" when using a screen reader.
Checklist
main
branch.