Problem/Motivation

There are several accessibility issues with the rendered HTML from the banner added to the page from environment indicator module, when visible for users that do not have permissions to use the Drupal 8/9 toolbar.

  1. There is no way to activate the fly-out functionality with keyboard navigation only. The fact that there is a hidden LI with additional links to other environments is not detectable from screen readers.
  2. The DIV wrapper is not contained within an aria landmark.
  3. The "title" attribute on the DIV wrapper (which provides hover text) is not considered accessible, as it is not readable text.

Proposed resolution

  1. The trigger to activate the flyout should be contained inside a <button> tag with attributes aria-haspopup="true" to declare that it triggers a submenu, and aria-expanded="false" to declare that the submenu is hidden, and aria-controls="environment-switcher-container" to indicate the element that will popup when clicked. The BUTTON should have an aria-label with the same text as the title="Show the environment switcher." attribute that shows up on the outer DIV.
  2. The ARIA Landmark issue can be fixed by adding role="region" to the outer wrapper, with an appropriate aria label.
  3. Add aria-label="Environment Indicator" to the wrapper DIV. Adding the aria-label to the DIV instead of the title attribute, also has the added benefit of satisfying the requirement that when there are multiple aria-landmark roles of the same type on a page, then aria-label is required to distinguish them, so this becomes a more bulletproof solution to avoid conflicts with other modules and themes.

Before:

<div style="..." title="Show the environment switcher." id="environment-indicator">
  [current environment name]
  <span class="description">[current release]</span>
  <ul class="environment-switcher-container" style="border-top: 1px solid white; display: none;">
    <li>
      <a href="[another environment link]">Open in: [another environment]</a>
   </li>
  </ul>
</div>

After:

<div role="region" aria-label="Environment indicator" title="Show the environment switcher." id="environment-indicator">
  [current environment name]
  <span class="description">[current release]</span>
  <button class="environment-switcher visually-hidden focusable" aria-haspopup="menu" aria-expanded="false" aria-controls="environment-switcher-container">
      Show the environment switcher.
  </button>
  <ul id="environment-switcher-container" class="environment-switcher-container" aria-label="available environments" aria-hidden="false">
    <li>
      <a href="[another environment link]" >Open in: [another environment]</a>
   </li>
  </ul>
</div>

With this refactoring, it then becomes nicer to refactor and simplify the JS (in a follow-up issue) so that the only thing it does is toggle the value of aria-expanded between false and true, and leave visibility rules up to CSS with something like:

.environment-switcher-container {
   display: none;
}
.environment-switcher-container[aria-hidden="false"] {
   display: block;
}

Remaining tasks

  • Needs patch
  • Needs review
  • Needs follow-up to simplify javascript, remove jquery, etc.

User interface changes

Environment indicator bar passes accessibility and becomes keyboard navigable.

API changes

The default markup in environment-indicator.html.twig file's will necessarily change to add the <button> tag. I'm not sure this is considered to be an API, per-se, but worth noting.

Similarly, if you consider the module's default CSS an API (I'm not entirely sure I would) then this part of the refactoring might be considered an API breaking change.

Data model changes

None.

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

jwilson3 created an issue. See original summary.

jwilson3’s picture

Issue summary: View changes
jwilson3’s picture

Issue summary: View changes

andy-blum made their first commit to this issue’s fork.

jwilson3’s picture

Version: 4.0.3 » 4.x-dev

Picking this up to take a stab at some fixes.

jwilson3’s picture

Issue summary: View changes
Status: Active » Needs review

MR is ready for review.

I've tested it with the Siteimprove chrome extension and the WebAIM WAVE chrome extension, and there are no issues found.

jwilson3’s picture

Issue summary: View changes

Probably refactoring the JS and the toggle animation, as described in the issue summary, should be dealt with in a follow-up issue.

The only doubt I had, for any reviewer to consider, is whether or not to introduce a heading <h2 id="environment-indicator-heading" class="visually-hidden">Environment indicator</h2> and use that with an aria-labelledby attribute on the outer wrapper container, instead of the aria-label="Environment indicator".

jwilson3’s picture

Issue summary: View changes
jwilson3’s picture

Issue summary: View changes
jwilson3’s picture

Issue summary: View changes
jwilson3’s picture

Issue summary: View changes

trackleft2 made their first commit to this issue’s fork.

trackleft2 changed the visibility of the branch 4.x to hidden.

jwilson3’s picture

Sadly the longer this goes without getting in the more of these kinds of unrelated formatting changes to pass CI tests are going to happen.

I'd almost say we need to create a separate issue to pass CI (stylelint and eslint), then rebase this issue once that one gets in.

There are all kinds of wrong CSS in this module. E.g. using #html-id selectors instead of .class selectors, among other things.

trackleft2’s picture

Status: Needs review » Needs work

@jwilson3, I don't think I understand what you are saying. The code for this merge request is what is triggering the eslint and stylelint errors at this point.

BTW I've created a different merge request that also touches the JS file updated in your MR. Not sure if it could be useful here or not.
https://git.drupalcode.org/project/environment_indicator/-/merge_request...

trackleft2’s picture

Status: Needs work » Needs review

@jwilson3, I've refactored your merge request slightly in order to roll in some fixes to the slideToggle functionality, and fix some syntax issues.

I've opened an additional merge request on this issue in order to trigger a new tugboat build. I feel like this is in good place at this point.

jwilson3’s picture

Status: Needs review » Reviewed & tested by the community

@jwilson3, I don't think I understand what you are saying. The code for this merge request is what is triggering the eslint and stylelint errors at this point.

My bad. You're totally right. (Verified with a code review of the latest changes in the MR).

I still stand by my point in a comment above the CSS should avoid using an HTML id to target styling elements, and ideally (as mentioned in the issue summary) the button itself would be placed inline into the HTML and use classname instead of increasing specificity and using the button in CSS. This would be a follow-up.

Code change looks good, IMO, but I haven't tested this using Wave WebAIM locally, so will not RTBC yet.

There is also one unresolved comment thread on the MR.

jwilson3’s picture

Neither MR31 nor MR92 apply to the current stable branch 4.0.21, so here is a reroll that does.

jwilson3’s picture

StatusFileSize
new3.1 KB
new687 bytes

I flubbed the manual re-roll. here is a fix.

trackleft2 changed the visibility of the branch 3224593-accessibility-fixes-for to hidden.

  • trackleft2 committed 5412ea39 on 4.x
    Issue #3224593 by jwilson3, trackleft2: Accessibility fixes for...
trackleft2’s picture

Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.