Tabbing order does not satisfy 508 accessibility requirements

camille.davis@civicactions.com → made their first commit to this issue’s fork.

The usually behavior for keyboard users (pressing Enter on the parent link to open and close submenus) doesn't work here since the parent link also links to a separate page.

However I noticed in the mobile view, parent links have an Extend/Collapse button next to them which makes the toolbar a lot more usable.

In this merge request I've added those buttons to the desktop view, so it should behave in the following ways:

For cursor users:
- Hovering over a parent link opens its submenu, but doesn't change the parent button. Hovering away then closes the submenu.
- Clicking the parent button flips the button and keeps the submenu open.
- If the submenu has been clicked open, you can close it by clicking the button again.
- Clicking outside of the toolbar closes all open submenus.

For keyboard users:

- Tabbing to a parent link does not automatically open its submenu.
- Pressing enter when focused on the parent's button flips the button and opens its submenu.
- You can close a specific submenu by pressing enter on its button again.
- Pressing "Esc" closes all open submenus.

It was also hard to see the keyboard focus state in the mobile view, so I added the blue-green focus background already found in the desktop view to the mobile view.

camille.davis@civicactions.com → closed merge request !44

camille.davis@civicactions.com → closed merge request !45

Thanks so much for the walkthrough @camilledavis this looks great. Seems like it provides a lot of extended functionality for keyboard only users.

This patch makes a much needed correction to the admin toolbar's accessible behavior. Currently, keyboard users can access each menu and option, but have to do so consecutively through every menu and option. The correction allows keyboard users to move among top-level menu options and then select which sub-menu to open.

Thanks

This is a great change in terms of accessibility and I really like to see it, however it would have been nice to coordinate such a change a bit better. As this feels like a major change to how the module works it feels a bit weird to include this in a patch release instead of a minor release.

I've added some of the issues it has caused.

You've messed up the module on many devices! Accessibility mode should be enabled if someone needs it and in no other way. Bring it back!

@kvantstudio Strongly disagree that there should be an extra step to enable accessibility features. Accessibility should be the default. Disabling the feature is what should require an extra step (and even then I'd be careful about disabling it for all users).

Agree with @rkoller in https://www.drupal.org/project/admin_toolbar/issues/3357166 🐛 Upset theme since update to 3.3.1 Closed: duplicate and @itmaybejj in this thread https://drupal.slack.com/archives/C2ANFUGGG/p1680208825935249 that the arrow buttons could be made less visually intrusive.

One way could be to make the arrows not be in a circle (the way the arrows on the sub-menus are), and/or make them smaller (but keep enough padding for an adequate touch target.) Would be good to have more input from designers

@kvantstudio I think you need to moderate the way you're talking here.

I reverted this feature and create a new release, and reopened the issue to continue working on it.

@camilledavis we have changed the appearance of the chevron to be smaller, less visually intrusive in Gin, see 📌 [admin_toolbar] 3.3.1 compatibility issues Fixed

cc @adriancid

This adds a bunch of extra icons that overlap each other, unexpectedly. I've attached a screenshot. This is a fairly disruptive change. I will lock my admin_toolbar to 3.3.0 until this can be readily resolved. Otherwise, I have to add custom CSS to undo whatever change this is. Ultimately the same issue as How to remove newly added blue arrows at top level of menu ✨ How to remove newly added blue arrows at top level of menu Closed: duplicate , and reported a few times in the issue queue of this project.

I and my users really liked the change adding the chevrons and improving the menu's accessibility, and I'm disappointed to see them gone in 3.4.

They did add a lot of real estate for us admin users with lots of menu items, so I did add some style overrides to get rid of the cutesy icons which didn't add much in terms of usability.

Thanks for working on this. I look forward to their reintroduction!

@Kasey_MK I don't know if you followed the issue queue in the last few days with many users reporting problems with the feature, this is why I reverted all the code and invited all the users to come here to try to improve the feature, we can't have something on this module widely used that impact the sites. We can continue working here until this feature will be ready to be added again to the module.

@adriancid thank you so much for reverting this until further testing can ensure existing installs don't break the UI of active sites. I always support the addition of improved accessibility, so thanks for the continued effort.

At this point I can install the latest stable release without issue on my site :)

@bdanin, it looks like you also are using the adminimal admin toolbar module → ? Seeing how Adminimal Admin tool bar is a Skin on-top of Admin toolbar it would be up to them to adjust their styling once Admin Toolbar has decided the best layout for their icons.

I really appreciate all the work going on here, and look forward to seeing where this lands after taking people's feedback into consideration.

In the meantime, we made a patch for ourselves so our users can keep the accessibility improvement they were so excited about last week, and I'm sharing it here in case others feel similarly.

I left out the z-index update which did break things for us as well as for others (we had just overridden that in our CSS with a value of 9990). We didn't encounter any other issues that we couldn't also resolve with some CSS overrides.

The following CSS overrides in our themes make the toolbar a little less huge overall. We decided to set the visual-only icons to width:0 in order to make room for what we perceive as a vastly improved UI. We left the chevrons the size they are, recognizing the value of them being finger-sized, but tightened up some of the padding around the words, trusting that the words themselves are big enough to target.

.toolbar-icon-system-admin-content,
.toolbar-icon-system-admin-structure,
.toolbar-icon-system-themes-page,
.toolbar-icon-system-admin-config,
.toolbar-icon-system-modules-list,
.toolbar-icon-entity-user-collection,
.toolbar-icon-system-admin-reports {
  padding-left: 0.75em !important;
  padding-right: 0.5em !important;
}
.toolbar-icon-system-admin-content:before,
.toolbar-icon-system-admin-structure:before,
.toolbar-icon-system-themes-page:before,
.toolbar-icon-system-admin-config:before,
.toolbar-icon-system-modules-list:before,
.toolbar-icon-entity-user-collection:before,
.toolbar-icon-system-admin-reports:before,
.toolbar-icon-help-main:before {
  width: 0 !important;
}

Thanks again for continuing to work on this!

As the new design breaks styles with other modules/themes made it configurable. Please review Continue from https://www.drupal.org/project/admin_toolbar/issues/3357738 🐛 Latest versions break styles Closed: duplicate .

Here are a few options to make the arrows less visually intrusive, any thoughts?

Using the chevron from core level-2 dropdowns:

Using the chevron from Gin - vertical placement could vary too:

New MR looks like this:

I tested it with Claro and Oliver 9 and 10 - any suggestions for other themes/modules to test against?

Also tested with Gin which will need to be updated

@camilledavis can you share a screenshot how it would look like in Gin?

Also can we make sure that once this is RTBC that we have time to update Gin to support this (maybe we can remove the hotfix for the previously commited change as well again) so we can avoid breaking sites?

That would be lovely, cheers! :)

@saschaeggi Here's how it would impact Gin:

I'm also wrapping up a commit that would allow the keyboard navigation arrows to be disabled. They'd be enabled by default, but users could disable them temporarily (I hope!) while the dependent theme/modules are updated.

Added a config option to remove keyboard accessibility, if this option is checked the toolbar will fall back on the previous behavior (force keyboard users to tab through all child elements.)

I've re-included this previous keyboard navigation code in lines 103-118 of admin_toolbar.js but one thing I find annoying is that it gets run 5 times (every time drupal.behaviors is triggered)

In the rest of the code I've prevented things from running multiple times by putting it in a once() function. However the previous keyboard navigation code passes a 'context' variable to jQuery, which the once() function would break. I can't figure out what this 'context' variable is for. Usually it's for running jQuery on updated DOM but I don't know of any module/theme that adds toolbar items dynamically. If such a module/theme is in use, the code I've added may not work - to fix it I'd have to take it out of the once() and pass the 'context' variable as well. I'm reluctant to do this though unless someone can point me to an actually theme/module that would cause this issue

thanks for all your work on this issue @camilledavis! i finally found some time to manually test the patch and was also finally able to successfully apply MR58. by accident i've tried to apply MR46, the closed MR from the same line which is empty, the days before. :/ i've applied MR58 to admin_toolbar 3.x-dev on drupal 10.1.0-beta1. the config option mentioned in #32 ✨ Tabbing order does not satisfy 508 accessibility requirements Needs review is in place. but the problem is somehow the arrows for the top level menu items aren't displayed and available in the horizontal toolbar no matter if the checkbox Remove keyboard accessibility on desktop is checked or not. i've tried in claro as well as the latest gin dev version.

Ok i've figured out why the changes from the patch haven't been shown. A restart of the project in my local environment (DDEV) has fixed the problem (realized when the arrows were showing up after i spun up my computer again today - several drush cr haven't done the trick yesterday night). A few observations:

In general:

• The Remove keyboard accessibility on desktop checkbox is functioning correctly. When checked the arrows are hidden and the regular sequential tabbing is back, unchecked the new behavior is available.
• In regards of the new tabbing behavior one question. If i am expanding the submenu for one top level menu item by pressing the space bar and then tab through the submenu that i am getting out of the submenu and the next top level menu item gets into focus the sub menu remains expanded instead of collapsing. the expanded sub menu gets collapsed as soon as another sub menu gets expanded. In comparison in Olivero the sub menu closes as soon as the focus gets out of the submenu and onto the next top level menu item. Would that be an option here as well? That way the behavior would be consistent?

Claro:

• Without the admin toolbar installed the focus style for the top level menu item in focus is in a light color. with admin toolbar i have to admit i never really noticed the styling of the focus style since i've mainly relied on navigating by the mouse cursor. therefore at first i thought the styling was newly introduced but it is the standard styling already in place before the patch. a mint background color (#ABE9E4) without any outline which has with the used dark text color (#212B2B) a color contrast of 10.7:1. but i wonder if it would make sense to use a focus outline in line with the one used in the admin theme instead. gin for example uses an outline (with gin toolbar installed alongside). perhaps an idea for a follow up issue?
• the vertical toolbar in claro still uses the more prominent arrows, those solid blue circles with white arrows inside, for top level menu items. would it make sense to use the less intrusive arrows used for the horizontal toolbar instead there as well?

Gin

• As shown in #31 ✨ Tabbing order does not satisfy 508 accessibility requirements Needs review the vertical toolbar is too narrow. therefore the arrows are cut off visually. In addition the tabbing behavior is sequential and not adopting the tabbing pattern the patch introduces. In contrast the tabbing behavior for the horizontal gin toolbar in the new design is behaving correctly.

@rkoller Thanks for testing this!

For Claro I agree that a focus outline would be better, Claro has a white-in-green outline on most stuff and I created a patch to add this to the toolbar here: https://www.drupal.org/project/drupal/issues/3270230 🐛 Toolbar focus styles are not sufficiently obvious to know where your focus is Needs work

@camilledavis ahhh excellent. thank you! and i've quickly tested the patch in microsoft edge's high contrast mode. i completely forgot about that on my initial run through. i've attached a small video. looks like the arrow in focus visually disappears.

Weird, looks like it treats the text differently from the icons. Think the easy fix here would be to not have the arrows turn black on focus

yes i agree. i've quickly tested your change and that way the arrow in focus stays visible in expanded (dark grey) as well as collapsed state (blue). i think it is the correct behavior anyway not to change the component's color on focus. not aware of any case in core aside the toolbar. elements in focus get an outline which is handled in the linked issue of yours ( 🐛 Toolbar focus styles are not sufficiently obvious to know where your focus is Needs work ). but the color itself remains the same in focus while on hover you usually have the change of color to create some sort of affordance (that is a detail that is missing here but i would consider that out of the scope of this issue?).

Tagging for https://www.w3.org/WAI/WCAG21/Understanding/keyboard

This works way better for tablet users too.

Here's a screenshot without interaction. Makes it easy to see the impact before/after (after patch is at the bottom of the window).

Here's a screenshot with the down arrow being pressed, also set up to show before/after (after patch is at the bottom of the window).

If you click on the text, it works the same as without the patch. If you click on the down arrow, it becomes sticky. It is arguable that the button should have a 44x44 target size, but the text right next to it provides affordances to the same content. We may have to review this in the future, but I think it is an improvement in the behavior now for both keyboard only users and mobile/tablet users.

I like this patch.

I was asked to leave a review in #accessibility Drupal Slack channel. The first thing I'd like to say is wow, this is a huge improvement, and much needed!

Note that in this review, I haven't extensively looked through the code, and have done some cursory testing. The issues within the functionality I've found should be straightforward to resolve, and if not should be relegated to followup issues and should not be blockers.

• Buttons should toggle aria-expanded to indicate their state to screen readers. Ideally this would be handled in core's toolbar.menu.js, but there's already a mutation observer set up, so this should be super straightforward to add.
• No need to flip chevron on click. This is weird because the hover state doesn't flip the chevron, but the click action does.
• If you're on a tertiary level menu, and you hit ESC on your keyboard, it closes the entire menu. Ideally, this should only close the last menu level that was opened.

On a side note, it might be useful to add some comprehensive testing steps to the issue summary (including testing Gin, etc).

@mherchel thanks for your feedback!

Re: the following -

Buttons should toggle aria-expanded to indicate their state to screen readers. Ideally this would be handled in core's toolbar.menu.js, but there's already a mutation observer set up, so this should be super straightforward to add.

If I add aria-expanded attributes I'd also have to add aria-controls attributes to the buttons and a unique id for all the submenus. This feels a bit messy to do here since I am just pulling keyboard functionality from Core's mobile/vertical toolbar mode. Since the expanded/collapsed state is already indicated by the visually hidden label changing, I think it would make more sense to make a core issue for this.

No need to flip chevron on click. This is weird because the hover state doesn't flip the chevron, but the click action does.

The "opened on hover" and "opened by clicking" states are slightly different though - if a dropdown is opened on hover, it'll close on focusout, but if it's opened by clicking it will stay open until clicked again. So the chevron rotating could indicate that the menu will now stay open until clicked closed.
If we want it consistent though I think it would make more sense to flip the chevron on hover as well.

@camilledavis testing your changes on a Mac using Chrome and VoiceOver the changes work as described in the testing steps.

I also add the related issue that was mentioned in comment #35 and #38.

It would be great if somebody with knowledge about accessibility can review this issue.

I've got reminded to this issue by the notifications and finally found time to revisit and test again. A few minor details I've noticed:

• There seems to be a regression to #36 ✨ Tabbing order does not satisfy 508 accessibility requirements Needs review , when an arrow is in focus when the high contrast mode is active in for example microsoft edge it disappears again.
• In regards of #46 ✨ Tabbing order does not satisfy 508 accessibility requirements Needs review . I agree that tackling that problem in the context of 📌 Use ARIA disclosure pattern for submenu buttons in vertical toolbar orientation Needs work is the better choice. But i wonder if aria-expanded is the right choice for a toggle button. I always have problem as a sighted user understanding the current state for toggle buttons that change their label inbetween states. A point that Leonie Watson also illustrated in their talk for smashing magazine: https://youtu.be/OUDV1gqs9GA?t=3222 . She advocated to use aria-pressed instead. That way the button state is either pressed/selected or not and the button label remains the same between states. That way things are completely clear and unambiguous for screen reader users?
• Talking of the different labels for the toggle button states in this issue. i wonder if it would make sense to use expand instead of extend? extend, me as a none native speaker associate with to "add" something, and i've also searched and the more commonly used combination of labels for such toggles is expand/collapse compared to extend/collapse?
• One other detail i've noticed. without the patch applied or with the patch applied and the high contrast active, menu items in focus have an underlined menu item label. but with the patch applied and no high contrast active the menu item in focus gets a blueish background in addition. but if you are currently for example on /node/add/article then if you reach in the admin menu the corresponding menu item, that menu item only gets underlined but gets no blueish background. the underline i hardly notice and the blueish background was a welcome addition but for the menu item for the page the user is currently on it feels like it is missing that focus because the blueish background is missing.
• the last detail i have noticed is more of a question. lets say you have the submenu for structure expanded. now you go to the top level menu item shortcuts and expand that. the structure submenu collapses. now you go to the manage top level menu item and expand that, now the expanded structure submenu is expanded again automatically? is that an intended behavior? at least for me it was unexpected. cuz if you have structure expanded and now expand appearance instead, structure collapses

except probably the first point mentioned about the regression all other points are definitely no blockers for this issue. and yep as mentioned before this patch would be a huge improvement for the admin_toolbar. thanks again to @camilledavis for all the work on this issue!

I would be concerned about a setting which disables keyboard accessibitily for all users. I would expect disabling keyboard accessibility to be a user setting, not a sitewide setting.

@Charles Belov I agree with you, but based on the previous discussion such a compromise may be necessary to get the feature merged...

If I read this issue correctly, there are 4 related merge requests:

1. MR 44: closed
2. MR 45: closed
3. MR 46: merged
4. MR 58: open

The open MR applies to version 3.4.2, but there are conflicts with 3.5.0 or the 3.x branch.

It is also confusing that the feature branch for MR 58 is named 3.x.

I am setting the issue status to NW: MR 58 needs to be updated, with the merge conflicts resolved.

Rerolling for 3.5