Instead of adding back the hidden menu titles per #105, what about reworking the block titles as so:
If (label_display) Display the block title/label normally. Else Hide the block title/label visually.
Otherwise:
I imagine problems when both the hidden menu title is added back per #105 and the blocks have been customized to display the block title per #107.
- There will be redundant adjacent text (with the menu titles hidden visually but perceivable to screen reader users).
- If the hidden menu titles (per #105) are
h*elements, both these title and the block titles will display as headings to screen reader users.
AFAIK headings would cause navigation problems. But even if the menu titles were standard text instead of headings, it seems this would be confusing verbosity. A screen reader expert should double-check me on this.
Here's a screenshot of the VoiceOver rotor with the block titles displayed and the hidden menu titles added back as headings on line 2 of navigation-menu.html.twig:
Here's a screenshot with the block titles visually-hidden in block--navigation.html.twig and without the hidden menu titles added back in navigation-menu.html.twig. The only headings in the menu are the block titles.:
There's overlap with 🐛 on the mobile viewport with the navigation sidebar collapsed the menu items are still available in the aural interface Active .
The proposed fix here does remove them from the VoiceOver rotor, so I think it hides them from the aural interface.
I made some changes based on @mstrelan's suggestions and raised some questions in comments on the MR.
The menu titles appear to be the same as the block labels.
There's already the optional block label display in block--navigation.html.twig.
{% if label %} {% if label %} <h2{{ title_attributes.addClass('toolbar-block__title').setAttribute('data-drupal-tooltip', label).setAttribute('data-drupal-tooltip-class', 'toolbar-block__title-tooltip') }}>{{ label }}</h2> {% endif %} {% endif %}
However, AFAICT this code not used by default because the module install configuration sets label_display: '0'.
Does it make sense to leverage that instead of adding a heading to navigation-menu.html.twig?
Followup:
In my last screenshot, "Submenu 4" is not wrapped in a nav and so doesn't appear in the rotor landmarks menu.
I think the problem is that a nav with an aria label becomes a landmark, which is what we've been specifically trying to avoid since #3452724-56: Navigation side bar and top bar should have appropriate aria labels. I'm basing this on https://www.w3.org/WAI/ARIA/apg/patterns/landmarks/examples/navigation.html
So if we add the aria labels back, we go back to 7 or more landmarks instead of two.
The options then would be:
1. Continue to use nav without a label, because the nav elements are wrapped in another landmark, which is what the MR was previously doing when RTBCed.
2. Stop using the nav element for the individual menu items.
My understanding is that nav elements without names are still landmarks (they're just unnamed). Here's screenshot of VoiceOver from a pared-down demo of various submenus inside a labeled parent nav.
If it's any guidance, the ARIA Authoring Practices Guide Example Disclosure Navigation Menu doesn't wrap the dropdown submenus in nav elements. It looks like this would correspond to option #2 above.
@benjifisher: First of all, I really do hope that your commenting means that your health issues have resolved, or are resolving.
The pipeline is passing now.
Or maybe this is the time for you to take on the Reviewer role, which is very important. I think you are familiar with the code and test changes on this issue. Why not review it?
RTBC means Reviewed and Tested By the Community, not Ready To Be Committed. (It used to stand for that.) You are a member of the community. Your review represents your best effort to improve, then approve the issue.
I guess I'd say that I have reviewed it and that I've made my best effort to improve it by suggesting changes. I'm not ready to approve it, though, because I feel strongly about using .getAccessibleName(). I would write the test that way and then leave it up to maintainers or a vote in the review process. I can leave a comment on the MR, though.
If we translate the text, either using
$this->t()in PHP code orDrupal.t()in JS, won't that be the translated text?
Wouldn't the inner text content that is found and checked by .textEquals() by also be affected by this? The Nightwatch page says "Check if an element's inner text equals the expected text.". I don't understand how the text that it finds and checks could be different than what is output from $this->t() or Drupal.t().
Another way to put my argument about .getAccessibleName() is that I see the goal as making sure that the control is perceivable to the user in the way that we specify.
Checking the text content alone doesn't do that reliably because there are multiple methods for naming an element, and the final result is computed by the UA.
To really accomplish the goal, I think we'd need to also check that there's nothing overriding the text content (check for absence of aria-label, aria-labelledby, etc.). It's complex and depends on the role (reference).
.getAccessibleName() should do this for us so that we don't have to reinvent the wheel. It's also in the WebDriver spec, which Nightwatch may use (I couldn't tell from looking at the Nightwatch code).
@benjifisher:
I'm sorry to hear about your health issues.
I reverted the commit. Now the pipeline is failing with what appears to be 🐛 [10.5.x] test DB transaction isolation set to read-committed breaks tests Active 😔. I'll update the fork in the Gitlab UI to see if that fixes the pipeline.
I see myself more as a collaborator than a sole reviewer. I apologize if I botched the issue queue workflow or etiquette. I believe that this is a better way to test, though, so I won't mark it as RTBC. Folks with more authority can make the call.
Second, I searched the codebase and this seems like the first use of that test method. Maybe there is a reason it is not being used? Or maybe it should be used widely, in which case it might be a better idea to have a big issue for updating lots of tests instead of starting small with the test for this issue. In other words, I see a lot of value in keeping a single test self-consistent. Even if
.getAccessibleName()is a better way to test, it might not be worth the inconsistency.
My thought is that there's already a lot of inconsistency in tests. I don't love that either, but I haven't found detailed coding standards for test implementation, and broad changes are harder to accomplish. I wish there were coding standards on this issue. It's difficult, though, because PHPUnit doesn't have this functionality yet AFAIK.
I see your point about consistency within a test, but I see this as a (slightly) different problem than locating the element because it's an explicit assertion that the accessible name is what we want it to be. If the markup changes to use aria-label or aria-labelledby, then checking the text content could lead to a falsely-passing test:
- Markup changes to use
aria-labelledbythat resolves to "overriding, incorrect label", but text content doesn't change. - Test passes because it checks the text content.
- Actual users perceive "overriding, incorrect label" because it overrides the text.
Third, I think
.getAccessibleName()relies on the translated message. The usual practice in automated tests is not to use translated text unless you are testing translations. But see my first point above: I may be wrong about this.
I don't understand what you mean regarding the translated value. My understanding is that .getAccessibleName() computes the accessible name based on the page HTML, as the browser would:
Returns the computed WAI-ARIA label of an element.
Anyway, my point is that there is a lot of room for improvement in our current test suite. Maybe
.getAccessibleName()is part of it, maybe not. But this is what I am thinking in my second point, where I suggest making broad changes to the tests instead of an isolated change to the test for this issue.
I agree that there is a lot of room for improvement and with @larowlan's comments about using findByRole, findByLabelText, and findByText.
If this is a better testing method, then we'll at least have one test that's better. Maybe it will bring awareness.
As an aside, Playwright agrees with Testing Library and @larowlan:
We recommend prioritizing role locators to locate elements, as it is the closest way to how users and assistive technology perceive the page.
and recommends including the accessible name (rather than the text content) as part of finding by role:
When locating by role, you should usually pass the accessible name as well, so that the locator pinpoints the exact element.
Ok, now I think it's ready.
I removed the existing Nightwatch a11y tests, but I left the yarn nightwatch-axe-verbose dependency to make the transition smoother in case other tests are added in the near future.
If we're following WCAG 2.2, the duplicate-id-aria error appears to be obsolete.
Dequeue lists it as SC 4.1.1 Parsing, which was removed in WCAG 2.2.
If we're keeping that item active, the priority should perhaps be Critical (matching Dequeue's User Impact level).
The other one (heading-order) is a Dequeue Best Practice.
To be robust, it really needs to check for the computed accessible names.
It would be possible to check the underlying HTML (like looking for label elements), but to me that's indirect because the end goal is the computed name that users perceive, not the specific HTML.
It's also more brittle because tests could fail or have false positives if the page output has another naming method for some reason. Tests would have to keep up with the HTML changes. They could, but that would bring its own maintenance problems...
AFAIK, our current functional javascript tests can't compute the accessible name, and it's not easy to do it with vanilla JS.
Nightwatch can do it for individual elements with the getAccessibleName() function, but a full Axe scan will check the whole page.
I did it this way because it looks in line with ✨ Automate Accessibility Checks for Core Active (esp Phase 2), and Views / Views UI don't have good coverage. It's a starting point for adding more complex cases.
Even if "standard" Nightwatch tests get replaced there's no good alternative for Axe tests until 📌 Automated A11y tests in PHPUnit Active lands.
I have to admit that I don't love the specifics of the test, though. It would be better if it used findByLabelText() in the setup for the page. It would probably be more robust if it created a simple test view instead of depending on an existing view that comes with the installation profile. But these won't matter if the test is going to be converted to a functional javascript test with
📌
Automated A11y tests in PHPUnit
Active
.
I realized that the Nightwatch Axe tests need to be removed. And when I reviewed them, I realized the breadcrumb case that was added for 🐛 Mobile friendly breadcrumb (Lighthouse) Needs work isn't represented here.
Also, the current Nightwatch a11y tests can switch themes based on CLI arguments. These replacement's don't.
It doesn't seem necessary right now, but when Gin comes to core it would be handy. I think it would be better in a follow-up issue.
@smustgrave, this will add axe-core as a main dependency so that nightwatch-axe-verbose isn't required anymore.
It probably won't remove nightwatch-axe-verbose though, unless all of the existing Axe tests are also to be converted in this issue (rather than a followup).
@catch,
It's already a JavaScript dev dependency as a dependency of nightwatch-axe-verbose, but it wasn't available to the tests.
Here are snippets from pipeline.yml.
'🖱️️️ PHPUnit Functional Javascript':
<<: [ *with-composer, *run-tests, *default-job-settings ]
... etc ...
'🦉️️️ Nightwatch':
<<: [ *with-composer-and-yarn, *default-job-settings ]
... etc ...
Based on your comment, I'll assume that it's OK to change pipeline.yml and play around with it from there.
Checking just for the label element is brittle because there are multiple ways to name elements. It's more robust and more closely resembles user interaction to use a tool that gets the computed name. AFAICT, PHPUnit can't do this out of the box even with the selenium webdriver, but Axe and Nightwatch can.
This Nightwatch test also uses a full Axe check that covers the whole page and can be expanded to cover more exposed filter cases, in line with ✨ Automate Accessibility Checks for Core Active . Views doesn't have good coverage.
📌 Automated A11y tests in PHPUnit Active was chosen to replace the Nightwatch + Axe tests, but it's stalled.
It may start moving soon, but if it doesn't:
Even if standard Nightwatch tests move to Playwright, the accessibility tests will have no replacement until until 📌 Automated A11y tests in PHPUnit Active lands. I vote for having better accessibility test coverage in the interim and converting tests to PHPUnit then.
I tried to use one of the existing test views but didn't find a means outside of PHPUnit. Using one of the existing standard views (like Frontpage or Content) seems brittle because they may be removed from the testing profile. This is how I arrived at creating a simple test view in the test.
as long as we've got a way to run a test but have some kind of ground where we can assert the things that should pass, while adding todos for things that don't
That's very doable. We would be able to to disable individual rules for a given test like we've done for the Nightwatch Axe tests.
@catch, can you take a look at #20 📌 Automated A11y tests in PHPUnit Active on 📌 Automated A11y tests in PHPUnit Active to see if you have thoughts to help get it moving?
Bumping to major due to @catch's comment #8 on #3542476-08: [Policy] Officially move to WCAG 2.2 AA Support → .
Adding to #16:
AFAICT, to move this forward we just need to make a built version of the axe-core javascript available to these functional javascript tests.
It doesn't matter whether the script is stored in the repo, passed in as an artifact from another part of the gitlab build process, installed with yarn install for these tests, installed via php-forge/foxy, or something else.
What is the desired mechanism for that?
Your screenshot is how they SHOULD look without JS
Yes, definitely. That's a screenshot with the problematic CSS for the height disabled.
I think the appearance still ins't optimal, though. The highlighted first item ("Manage fields") and the border / box shadow are funky for a list of equal links.
After thinking about it more, "usable" is a stretch. I was thinking in terms of keyboard / screen reader, not standard users.
The site is still usable, but it's definitely not obvious that "Manage fields" takes you to a page which as the other dropdown options as tabs.
@joachim, are you sure they're supposed to be ol, not ul?
The incorrect height appears to come from dropbutton.pcss.css. Disabling the 1px allows them to expand:
It's certainly less convenient, non-intuitive, and ugly—but still usable. So maybe not "Major"?
- They are a
ulelement, but the display is messed up. - The links are accessible via keyboard. Some of them are hidden until you tab to them.
- VoiceOver announces them correctly.
- They are present in the VoiceOver rotor.
- The "Manage fields" link takes me to
/admin/structure/types/manage/article/fields, where the different tabs are visible. Though, that page also has dropdowns affected by this.
The remaining failing case should be fixed by 🐛 Numeric value on exposed filter form is missing accessible name Active .
First draft in issue branch. Still needs tests.
It looks like the original plan was to check for non-empty #title properties, but the MR only checked with isset().
Empty text doesn't provide an accessible name, so I changed the check to fully disallow empty #title properties. This caused a lot of new errors. Some of them are from
🐛
Content: Publishing status (grouped) is missing labels for inputs
Active
. I think the others are from the Views exposed filter forms themselves.
I also added aria-label to the conditions.
The Remove checkboxes are also visible in Gin. Based on
#3375806-14: Views 'Rearrange' dialog show the 'Remove' checkbox, which should be visually hidden →
, it looks like they should be hidden with the js-hide class in another issue.
Yeah, the Remove checkboxes have display: none in the views_ui module and in Claro.
Claro then overrides that with .form-boolean. It's probably an accident because it's a generic selector. I haven't checked Gin.
I think there's a bigger issue of whether they should be visible at all, but I'm planning to give them a hidden label due to
📌
Test that all form elements have a title for accessibility
Needs work
(which will require a #title property).
I was wrong about the Remove checkboxes. They are still missing labels.
The strange thing is that it looks like they're not supposed to be visible. There's this comment in the code, and they don't appear when the admin theme is Stark.
// No title is given here, since this input is never displayed. It is
// only triggered by JavaScript.
It looks like the Remove checkboxes have been fixed.
I found other cases: The textfields for numerical filters. I think it's called the "in operator".
I think I have a fix, and will update the IS and create an MR. I think this is testable in Nightwatch.
It looks like the MR uses aria-label in places where there's already text.
I believe the problem is that all of the existing text is hidden with display: none (which completely hides an element, even from AT).
My suggestion is to avoid aria-label and hide the text names visually, so that they are still available to assistive tech. This avoids redundancy, and recent Slack discussions in the #accessibility channel indicate that visually-hidden text is preferred over aria-label. There's more information on that in the issue summary.
My quick experiment indicated that using core's visually-hidden CSS solves the problem.
Off the top of my head, I think it would be possible to replicate the CSS rule into a selector that applies to the text of the elements (they appear to all be wrapped in span) when the toolbar is collapsed. Maybe there's already a selector for this state. I don't remember. I also don't know what the Gin maintainers prefer regarding implementation.
I think the the proposed fix for 🐛 Elements in closed sidebar are focusable Active will also fix this.
I didn't point this issue out explicitly, but experimenting with visibility: hidden did remove the items from the VoiceOver rotor.
Question about the new validation code:
The code checks for the aria-labelledby attribute but not the aria-label attribute.
Was aria-label intentionally omitted? It's a valid method (though not preferred) of providing an accessible name, and it looks like core uses it for fields.
I'm also working on other test failures.
There are some uses of #label in the Content Translation and Language modules. Removing it (comments #197 & #202) caused several Exception: Warning: Undefined array key "#label" errors in tests.
Another error is caused by the missing #title for the test element in WeightTest.php.
I'll push an update that resolves these.
I found that some errors are due to:
- A mismatch between the expected message and the actual message used in
FormBuilder::doBuildForm. - The deprecation of
@expectedDeprecationannotations.
I'm updating the MR for these.
Added 🐛 Links and buttons in toolbar missing accessible name Active to IS.
@catch:
I would also want the option to set it to a lower level if we can get tests to pass.
The current MR on 📌 Automated A11y tests in PHPUnit Active supports passing Axe options with individual tests. The options can include the WCAG versions / levels to run (as tags). The test for that can be improved to explicitly test for disabling WCAG 2.2. Would that do it?
Suggestion for remaining tasks: Configure our Nightwatch implementation to use 2.2 for all Axe tests.
Looks like STR is still needed.
Adding Novice tag so novices can find this for those todo items in the IS.
Are tests still needed, or can that todo item be marked as done?
This point about WHCM (aka, forced-colors) by @andrewmacpherson in #56 hasn't been addressed. Is that intentional?
I was chatting with @rainbreaw about another potential use for this is in Windows high-contrast mode. Instead of trying to get an asterisk to display in :after, Windows high-contrast mode could use the actual required text (i.e. don't hide it).
Added 🐛 Elements in closed sidebar are focusable Active .
I see where the update behavior is being cancelled: https://git.drupalcode.org/project/drupal/-/blob/11.x/core/misc/autocomp....
I can't determine why that code is necessary, though. Maybe for cases where the field can contain multiple items?
I tested without any focus handler at all, and the input value does change as I arrow down. Attaching a video to illustrate.
It might be worth testing that configuration in JAWS.
I'm not experienced with JAWS, so just throwing out some thoughts.
This comment on an old JQuery UI Autocomplete issue indicates that JAWS exits Forms Mode if the input is not updated.
If you do not update the input field then you will encounter a problem with JAWS where multiple down arrow keypresses simultaneously cause JAWS to pop out of forms mode and stop accepting further user arrow keys as inputs to the input element.
I noticed that in our case the input does not update when arrowing through the options, so I'm wondering if this is causing JAWS to exit Forms Mode.
This correlates with this JAWS page on Forms Mode, which describes JAWS automatically exiting with multiple Down Arrow presses. In case it's helpful to diagnose, that page describes a sound that indicates when JAWS exits Forms Mode.
For a comparison, does the problem happen in the JQuery UI Autocomplete demo?
Added ✨ Escape key closes multiple submenu levels Active .
Adding proposed resolution.
Minor edit to IS.
kentr → created an issue.
Added 🐛 Focus does not move to submenu when opened by enter or space key Active .
Added WCAG SC.
Fixed parent issue.
I think checking the name with .getAccessibleName() instead of checking the button text would be more robust.
I understand the goal to be presenting a usable name for AT. The name is currently in the button text, but it could get overridden in the future by aria-label or aria-labelledby. Checking with .getAccessibleName() should catch that.
Since @benjifisher said he'll be away, I'll make that change and let it go through review.
I'd also like to have a policy / coding standard for tests regarding the locating of elements by name, or role & name, when possible.
It's closer to how users interact with the page and would build in a check for the correctness of the name (if the name is incorrect, the element won't be found). Sweeps with Axe will find missing names but won't confirm that they are correct.
It looks like Nightwatch doesn't currently offer a clean solution, but Playwright does. So, maybe after 📌 Consider dropping Nightwatch in favor of Functional Javascript tests Active . Or, we could implement the workaround described in the Nightwatch GitHub issue as a custom Nightwatch command.
Another option is to replicate (fully or partially) the standard profile into the nightwatch_a11y_testing and continue using that profile with any changes this issue brings to the tests themselves.
If that's the chosen path, feel free to close 📌 Update the profile used in Nightwatch A11y tests Active and repurpose this one.
Actually two assertions, to show that the name stays constant with the toggling (i.e., to show that we fixed that problem also).
I'd love to have an assertion for the (accessible) name of the button because the choice was very deliberate.
After a little discussion on Slack, a few days ago, with @nod_, we agreed that the
stable9theme should not be changed. It already has its own version oftoolbar.icons.theme.css.
Wanted to double-check because removing "Extend" and "Collapse" was a big part of the original issue: Was the intent to keep "Extend" and "Collapse" in the names for stable9?
Make some custom assertions about the component we're interested in. A dialog appears, has expected markup, focus moves to dialog. And finally run all the general ruleset AGAIN.
I can see also this for:
- Disclosure pattern in general.
- Menus 🌱 [PP1][PLAN] Accessibility review Postponed .
Uncertain if this is in scope or warrants a separate issue.
In 📌 Use ARIA disclosure pattern for submenu buttons in vertical toolbar orientation Needs work , the recommendation was to remove "Extend" and "Collapse" from the accessible names of menu toggle buttons.
This is also relevant to the Navigation toggle buttons. For example, the accessible name of the "Shortcuts" menu item toggles between "Extend Shortcuts" and "Collapse Shortcuts".
From 📌 Use ARIA disclosure pattern for submenu buttons in vertical toolbar orientation Needs work :
@andrewmacpherson (issue summary):
Currently these buttons conflate the name and state; the name of the button is the opposite of the current state. There's some unnecessary cognitive effort there, to figure out what the current state is. (This is the standard challenge with labeling a toggle control.)
@cwilcox808 ( #35 📌 Use ARIA disclosure pattern for submenu buttons in vertical toolbar orientation Needs work ):
Using the aria-expanded state instead of words like Extend/Collapse in the button name is definitely the right thing to do.
Clarified in STR that it's with the standard installation profile.
in "regular" mode you have a green background for the workspace that is live while all the other none live workspaces have an orange background (SC1.4.1 might apply here?).
Looks like that is mentioned in #2910673: Workspace UI usability review → .
in addition to that the yellow font color used on the live workspace implies that it would lead to a page but strictly speaking just an area on top of the current page is displayed.
I'd vote for making that element a progressively-enhanced button like in the MR for 📌 Use ARIA disclosure pattern for Toolbar buttons with trays Active .
Then the style can be something like this:
@media (forced-colors: active) {
.toolbar-button[role="button"] {
color: buttontext;
}
}
I'm uncertain if this is a Navigation issue or a Workspaces issue. It would be nice if Navigation handled it for all menu components.
I'd also really like to see this pattern be global, with a policy of progressively-enhancing all cases like this. Then it could be:
@media (forced-colors: active) {
a[role="button"] {
color: buttontext;
}
}
It looks like #8 demonstrates the Olivero menu, not the Navigation module menu. I can reproduce this on 11.x.
Needs tests for #5.
It should also be possible to test that the Close button is tab-able. It would be good to do that to mitigate against a regression.
I believe this would be SC 2.4.11 Focus Not Obscured (Minimum) (Level AA). It directly addresses user openable, persistent disclosures and uses slide-out navigation as an example.
When the additional content is opened, it takes focus and the tab ring is constrained to the new content until it is dismissed. This modality is somewhat like a dialog, in that a user cannot navigate beyond the opened content by keyboard without dismissing it first (typically by pressing Esc). However, unlike in a modal dialog, in some implementations a pointer user may be able to interact with content outside the opened section without dismissing it. Since this pattern potentially creates an inequitable experience between keyboard and pointer users, it should be used cautiously. That said, it does prevent the opened content from obscuring the keyboard focus in the main content, and thus should pass this SC. This is described and demonstrated in a short video in the Knowbility article in the reference section, under the section heading Keep keyboard focus in the slide-out navigation until it's closed.
Did a little research, and I believe I misunderstood ActiveText. Apparently it's for links that are being activated (pressed), not for the "active" link that corresponds to the current page.
@rkoller
in regard to updating the issue summary, instead of removing point 4 i wonder would it be the better choice to strike it through and leave a note
👍 Good idea.
on the other hand the active menu item for the current site is close to invisible, but that has already an issue for the none forced color mode (#3426468: Make the active menu item visually stand out more). so i wonder about two things, should the forced color mode with the active menu item be fixed in the aforementioned issue or within the scope of this issue?
I don't know. It may need more than a system-color change.
I tested ActiveText in all Edge Page Colors themes and using the emulation in Chromium DevTools. The color is indistinguishable from LinkText. Shouldn't ActiveText be a distinct color?
There was a Chromium bug related to this.
Here's a screenshot in Page Colors Dusk theme. The "Appearance" item is current and has the following CSS.
@media (forced-colors: active) {
.toolbar-button.current {
color: ActiveText;
}
}
Firefox uses a distinct color for ActiveText.
maybe point 6 should be rescoped to the not visible bottom border in desktop view port and point 8 should be solely about the hamburger. that way the two images used are more appropriate? if you agree i can update both points in a follow up comment.
👍 Sounds good.
Adding a new issue: More actions button should have a border (related Slack discussion).
This may be fixed by 📌 Improve visibility of More actions menu button Active , but does the menu "Close" icon also need a border?
Add Usability tag to emphasize that I believe this also affects regular users. AFAIK, I have "normal" (though imperfect) vision.
Added 📌 Improve visibility of More actions menu button Active as a child issue. It's different from 🐛 The icon for the more actions button is not visible Active .
I was uncertain whether I should change the IS here.
Regarding tests, I suggest looking into calling the Nightwatch admin a11y tests with the --adminTheme option.
Based on the README, this might be the command:
yarn test:nightwatch --tag a11y:admin --adminTheme ginJust a heads-up that in
#3093378-35: Use ARIA disclosure pattern for submenu buttons in vertical toolbar orientation →
@cwilcox808 cautioned against using aria-label instead of hidden text for buttons due to lack of support in translation software.
I inquired about similar concerns with other html elements for this issue in Slack.
I retract my previous comment about the interrupted menu right border. I now see it in Edge with Page Colors.
Suggestion for the remaining hard-to-see inline SVG icons:
- For the color issue, use
currentColorin some form. - For the chevrons, give them a stroke with a larger width.
Here's POC CSS:
@media (forced-colors: active) {
svg[class*="toolbar"] path[stroke] {
stroke: currentColor
}
svg[class*="toolbar"] path[fill] {
fill: currentColor
}
:is(
.toolbar-button__chevron,
.admin-toolbar__expand-button-chevron
) path {
stroke-width: 5%;
stroke: currentColor;
}
}Screenshot:
For IS #9 (gap in menu right border) I'm only seeing the issue in Firefox (Mac).
Also noticed another issue: The icon next to the username at top is hard to see. It may not be a UX / accessibility problem, though.
Screenshot attached from Chromium.
Tweaking the STR a little more.
I believe #4 in the IS (regarding the outline of the hovered menu entry) is caused by automatic dark mode. I can't reproduce it in basic forced colors emulation or MacOS Edge with Page Colors.
I suggest removing that item. @rkoller, does this sound right, and do you agree that item can be removed?
Is #6 in the IS referring to the hamburger icon that #8 refers to?
I'm also adding Page Colors to the IS for seeing forced-colors mode. It's a little nicer DX than forced-colors emulation in DevTools.
Adding a suggestion:
📌
Use ARIA disclosure pattern for Toolbar buttons with trays
Active
will hopefully result in the role="button" added by JavaScript instead of it being present in the initial markup.
If that issue lands before this is completed, it would be great to include here some CSS rule(s) based on role="button" that will set the system-color of toolbar buttons to ButtonText after they are processed by JavaScript.
Something along these lines:
@media (forced-colors: active) {
.toolbar .toolbar-item[role="button"] {
color: buttontext;
}
.toolbar .toolbar-item[role="button"]::before {
/* Assuming background images are converted to mask images. */
background-color: buttontext;
}
}
There's related discussion here.
I do not recommend letting 📌 Use ARIA disclosure pattern for Toolbar buttons with trays Active block this issue, though.
Hero text has insufficient contrast
Both the Axe extension and Accessibility Insights are reporting insufficient contrast for the normal text in the hero. Confirmed with the WebAIM contrast checker.
Attached screenshot of Accessibility Insights report and report html export.
To reproduce
Run a test on the home page with Accessibility Insights or the Axe extension.