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.
- 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.
- The DIV wrapper is not contained within an aria landmark.
- The "title" attribute on the DIV wrapper (which provides hover text) is not considered accessible, as it is not readable text.
Proposed resolution
- The trigger to activate the flyout should be contained inside a
<button>tag with attributesaria-haspopup="true"to declare that it triggers a submenu, andaria-expanded="false"to declare that the submenu is hidden, andaria-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 thetitle="Show the environment switcher."attribute that shows up on the outer DIV. - The ARIA Landmark issue can be fixed by adding
role="region"to the outer wrapper, with an appropriate aria label. - 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.
| Comment | File | Size | Author |
|---|---|---|---|
| #21 | interdiff-20-21.patch | 687 bytes | jwilson3 |
| #21 | 3224593-accessibility-fixes-21-v4-0-21.patch | 3.1 KB | jwilson3 |
Issue fork environment_indicator-3224593
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
Comment #2
jwilson3Comment #3
jwilson3Comment #5
jwilson3Picking this up to take a stab at some fixes.
Comment #7
jwilson3MR 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.
Comment #8
jwilson3Probably 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 thearia-label="Environment indicator".Comment #9
jwilson3Comment #10
jwilson3Comment #11
jwilson3Comment #12
jwilson3Comment #15
jwilson3Sadly 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.
Comment #16
trackleft2@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...
Comment #18
trackleft2@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.
Comment #19
jwilson3My 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
buttonin 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.
Comment #20
jwilson3Neither MR31 nor MR92 apply to the current stable branch 4.0.21, so here is a reroll that does.
Comment #21
jwilson3I flubbed the manual re-roll. here is a fix.
Comment #24
trackleft2