-
Notifications
You must be signed in to change notification settings - Fork 641
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
Fancy Crumbs a11y #13923
Fancy Crumbs a11y #13923
Conversation
Allows for a consistent label on the disclosure menu button.
CMS-1213 Fancy crumbs + action menus
|
@gcamacho079 could you give this a once over before I request a review from Brandon? |
@brianjhanson just re-reviewed and had a couple extra things I noticed:
Answers to your questions
Yeah this is a tricky one. If you install the browser extension HeadingsMap, or Accessibility Insights, and enable it when the disclosure is open, you'll be able to see the skip in heading levels. (If you use HeadingsMap try to ignore the fact that the contents are all at the end of the DOM, as that's a side effect of the disclosure positioning.)
Having trouble recreating this now, it might have been caused all the resizing I was doing and toggling the DevTools.
Yep, these font icons. I've done some tricky business in other places (I think the Ideally we would wrap and hide the pseudo element and use a Adding |
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.
Amazing updates so far! Just responded with a comment answering your questions.
@gcamacho079 this is ready for you to check out again. I read somewhere that if you make the icon unpronounceable screen readers will skip it. So I did this but let me know if that doesn't actually work. |
Great work, Brian! I tested on JAWS, NVDA, and Voiceover and it does indeed seem to prevent the readout (at least on the browser/SR combos I tested). 👏🏼 Just a couple still outstanding:
(Note: I added a comma while testing so that there's a bit of a pause in the readout) |
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.
@brianjhanson all the crumb issues have been resolved. 🥳 There are still 2 issues that were part of the initial feedback from auditing the action menus, but unsure if you want to tackle those in another PR:
Element action menu
- Focus outlines aren’t visible for element links (i.e.
.label-link
inside of.element
- On hover, the icon in the disclosure trigger (
.action-btn
) drops below 3:1 ratio against the background
I know Brandon wants to get this merged, so I'll open another PR for those. Is there a linear issue or something with them? |
@brianjhanson I'll create the issues in Linear! 👍🏼 |
Updated the markup so the
a
doesn't encompass thebutton
element in order to keep thingsAlso updated to use `var(--inner-focus-ring)
Probably the biggest change in here. Updated disclosures in order to remove the
label
element and to separate out the descriptive label and so we can add a different, consistent label on the disclosure button. For revisions, that means the label will be "Current" or "Revision XX" and the disclosure label will be "Context"False positive caused by the
.menubtn
being within thea
tag. Separating them cleaned this up.Restructured to no longer rely on the
label
element.Darkened the text color on hover
Darkened the text color on hover
@gcamacho079 I'm having trouble figuring this one out. What's the best way for me to see this?
@gcamacho079 same thing here, is there a specific menu where this happens that I can look at?
I don't think it's possible to add
role=img
to a psuedo element here especially because the image is actually text from a font. Is there another way to get this criteria? Can we add aselected
label to theli
itself?If this is minor, I don't think it's worth it. Styling is based on this being an
li
and I'm not sure that's worth the refactor.