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

Fancy Crumbs a11y #13923

Conversation

brianjhanson
Copy link
Contributor

@brianjhanson brianjhanson commented Nov 9, 2023

For entry type and multi-site items, the menu button is nested inside the anchor; interactive elements should not be nested. This also causes some confusion on mobile, as the entire row (link and chevron) are inside the focus outline, so a user might think they are opening another disclosure and accidentally trigger the link to the new page.

Updated the markup so the a doesn't encompass the button element in order to keep things

Hamburger menu button focus outline is clipped and only visible on one side
Updated the hamburger to use var(--inner-focus-ring)

When tabbing through the breadcrumbs, the focus outline on the current entry is invisible

Also updated to use `var(--inner-focus-ring)

The site selector control should have a consistent name across all screens. I recommend using an icon button so that the accessible name of the button is consistent between entries and sites. For example: <li><a>{{ site name }}</a> <button aria-label=”Change site”>{{ world icon }}</button></li>

The name of the version switcher should be consistent across screens, for example “Change version”, instead of referring to the name of the version itself. (See recommendation for site switcher.)

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"

The .menubtn for switching between entry types or sites must be 24px large, or leave sufficient space. (Note: this may be resolved when the button is no longer nested in the anchor.)

False positive caused by the .menubtn being within the a tag. Separating them cleaned this up.

 #action-btn and #context-btn use the disclosure button pattern, but rely on semantic label for an accessible description. Semantic label elements should not be with the disclosure pattern. For example: for the Action menu specifically, the accessible text should come from an aria-label or visually-hidden text inside the button itself.

Restructured to no longer rely on the label element.

On hover, the mobile hamburger menu button drops below 3:1 contrast

Darkened the text color on hover

On hover, the context button text drops below 4.5:1 contrast ratio against the button background.

Darkened the text color on hover

 When context menu is open, heading levels skip from h2 to h6. Recommendation: Add a visually-hidden heading to the context menu (i.e. “Entry Context”), and make all the other headings inside the disclosure h3

@gcamacho079 I'm having trouble figuring this one out. What's the best way for me to see this?

There are some instances where disclosure menus overflow and cause a horizontal scroll. Unsure if this is related to this PR, or should be tracked as a separate issue.

@gcamacho079 same thing here, is there a specific menu where this happens that I can look at?

For instances where a checkbox is used to identify selected language/version, the “check” pseudo-element should be given the role="img" and accessible name of "Selected". Ideally, this should follow the name of the link.

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 a selected label to the li itself?

 The first list in the Context menu has list markup, but I think it only ever has one item it it (i.e. Current). If this is true, the item probably shouldn’t be wrapped in a list.

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.

Copy link

linear bot commented Nov 9, 2023

CMS-1213 Fancy crumbs + action menus

  • Support for disclosure menus in crumbs
  • Move site selection into a crumb menu
  • Include the current page
  • Move element context into a crumb menu
  • Page actions menu (...)
  • Update ElementEditor
    • Provisional draft creation
    • Site inclusion
  • Keep context links up-to-date with tab selection
  • Fix the overflow menu when there are nested disclosure menus
  • Stop showing the context menu for elements that don't have drafts or revisions (users, assets)

@brianjhanson brianjhanson marked this pull request as draft November 9, 2023 22:09
@brianjhanson brianjhanson marked this pull request as ready for review November 10, 2023 20:45
@brianjhanson
Copy link
Contributor Author

@gcamacho079 could you give this a once over before I request a review from Brandon?

@gcamacho079
Copy link
Contributor

gcamacho079 commented Nov 13, 2023

@brianjhanson just re-reviewed and had a couple extra things I noticed:

  • Remove title attribute from disclosure menus, or change to match the aria-label text
  • Tested the disclosure controls on the screen reader rotor and it's looking great. The only change I have (correcting my previous suggestion) is changing "Context" label to be "Select context", that way we have consistency between all the button labels in the breadcrumbs.
  • For multi-site breadcrumbs, the globe icon wrapper should be set to aria-hidden="true" (right now it's aria-hidden="", but not actually hidden from screen readers).
  • In the Site select context menu, the status icon doesn't have an accessible text alternative (see the status icon in .crumb.current for the expected markup)

Answers to your questions

What's the best way for me to see this?

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

Context disclosure open with Accessibility Insights enabled. Two h6's are inside the menu. Context disclosure showing in headingsMap; heading level jumps from h2 to h6

...is there a specific menu where this happens that I can look at?

Having trouble recreating this now, it might have been caused all the resizing I was doing and toggling the DevTools.

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 a selected label to the li itself?

Yep, these font icons. I've done some tricky business in other places (I think the t9n-indicator is an example):
<span class="t9n-indicator" title="This field is translated for each site." data-icon="language" aria-label="This field is translated for each site." role="img"></span>

Ideally we would wrap and hide the pseudo element and use a visually-hidden text alternative, or set an explicit label on it and add the img role, in order to keep the CSS content being read out. For example: a site called English, in the select site menu, reads "Link, checkEnglish" on a screen reader.

Adding aria-current="true" on the link would communicate the same information, but would unfortunately leave the "check" content as part of the accessible name.

Copy link
Contributor

@gcamacho079 gcamacho079 left a 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.

@brianjhanson
Copy link
Contributor Author

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

@gcamacho079
Copy link
Contributor

gcamacho079 commented Nov 14, 2023

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:

  • Add a visually-hidden h2(like "Context") to the context menu above the first ul
  • Add visually-hidden text inside the .sel link in Context/Site/Entry Type disclosures so there's a text alternative for the checkmark icon. For example:
<a id="menu-item-274200933" class="menu-item sel" href="https://demo.ddev.site/admin/entries/blog/5-my-second-entry?site=default" data-site-id="1">
  <span class="status enabled" role="img" aria-label="Status: Enabled"></span>
  <span class="menu-item-label">English</span>
  <span class="visually-hidden">, selected</span>
</a>

(Note: I added a comma while testing so that there's a bit of a pause in the readout)

Copy link
Contributor

@gcamacho079 gcamacho079 left a 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

@brianjhanson
Copy link
Contributor Author

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?

@gcamacho079
Copy link
Contributor

@brianjhanson I'll create the issues in Linear! 👍🏼

@brandonkelly brandonkelly merged commit 5979732 into feature/cms-1213-fancy-crumbs-action-menus Nov 17, 2023
@brandonkelly brandonkelly deleted the feature/cms-1213-fancy-crumbs-a11y branch November 17, 2023 22:50
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.

3 participants