Toolbar focus styles are not sufficiently obvious to know where your focus is

Cross-posted from slack and issue 3097907 - Active toolbar tray has weak affordance and fails WCAG color criteria 🐛 Active toolbar tray has weak affordance and fails WCAG color criteria Needs work :

Re: adding focus ring to the toolbar

@saschaeggi: the focus ring we use in Claro has a 2px white inset/offset so it passes the contrast.

@camilledavis: Is there somewhere I can grab that css from?

@saschaeggi: https://git.drupalcode.org/project/drupal/-/blob/10.1.x/core/themes/clar...

Note:
/**
* Default focus styles for focused elements.
*
* This is applied globally to all interactive elements except Toolbar and
* Settings Tray since they have their own styles.
*/

so that’s maybe what we want to change cc @ckrina

@camilledavis: thanks! So if that was applied to the toolbar it could go two ways, not sure which is best. Inside, you could see the whole focus ring. Outside, it would match the other focus styles.

@ckrina: +1 to this change as long as this visual language pattern is limited to the existing Toolbar.

And to answer @camilledavis, I would go with the inside focus ring, but just for the Toolbar and its specific placement.

I hope this solves the accessibility concerns.

@camilledavis: Should the focus ring replace the current focus state (underline, slight change in background color) entirely? Or appear in addition to it?

@saschaeggi: it should be additional to underline & bg color

@camilledavis: What about the tray -- should the focus ring cover the gray border on the left and right?

And here's what it looks like with admin_toolbar module enabled... should the focus ring cover the borders on all sides here?

Thanks for moving this forward! Looks like we now have a target design, so removing that tag.

1. Maybe it was your IDE trying to be helpful, but the MR includes a huge number of added newlines that aren't relevant to this change. We should probably revert all those so that reviewers are only seeing what's necessary to fix this bug. Makes it less likely to conflict with other changes, too.
2. Although we're adopting a Claro-esque focus ring here, the original scope/point of this issue was to fix the bug in the core markup and styles from toolbar.module itself, so that you didn't need to install and use Claro to have an accessible/usable toolbar. Since you're targeting 9.5.x here, there's still a (deprecated) seven theme in that branch that could be fixed. And either way, it'd be nice to add the necessary styles to core/modules/toolbar/css/* as needed to get this working, even without Claro.

Thanks again!

@dww For sure!

1. Here's a commit without the newlines - they get added when I run yarn run build:css, I'm guessing that's not normal?

2. Should I remove the styles from Claro and add them to the toolbar module?

Maybe since the green is Claro-specific I could use some sort of "default" color in the module, and overwrite it as green in Claro? Not sure what the "default" color would be though.

1. Not sure, sorry. 😅 I'm not constantly up on the latest core frontend tooling changes. I just know that the MR diff included a bunch more lines than it needed. 😂
2. No, the Claro maintainers probably want to have 100% control over all their own styles and don't want to inherit anything. That seems to have been how they operate in the past, and I don't imagine that's going to change for this issue. I'm assuming we'll need Claro-specific styles in Claro (what you've got already), and then equivalently accessible styles in toolbar/css/* as appropriate.

   Perhaps for the 9.5.x backport (if that's going to fly), Seven can inherit the toolbar CSS for this, and we don't need to explicitly also fix Seven (which is already deprecated in 9.5.x and gone from 10.0+). If we are going to make the effort to fix this in 9.5.x, it'd at least be worth testing what happens in Seven if you define better focus styles in toolbar/css/*.

Hope that's clear. Thanks again!

saschaeggi → credited ckrina → .

@camilledavis thanks for working on this, glad to see it happen 🙇

@saschaeggi @dww no prob! Just updated the MR and copied the focus ring css to the toolbar module.

May be a non issue but when I tested the color contrast I got a AA failure.

I believe it's a pass since the focus ring is a UI component / graphical object

But won't users who may be color blind have trouble seeing the focus when tabbing?

I don't think so because the focus ring has 2 color rings in it with 3:1+ contrast between the colors

Here's a screenshot of the focus state with the white-in-green:

And here it is desaturated:

If that's the case don't mind marking this as it does do what the screenshots are saying.

It looks like all the work done here targets 9.5.x, but this is also an issue in later versions.

The typical approach is to do the work on the most recent branch where the problem is taking place (in this case 11.x), commit that, then backport to earlier versions. It's also quite understandable if someone is not aware of this procedure as there are many procedures to learn when contributing to core 🙂.

To get this issue moving again, it will need an 11.x patch/MR. The 9.5.x version will probably make any future backports easier, but that can't be the first thing to land, unfortunately.

Tagging this for https://www.w3.org/WAI/WCAG22/Understanding/focus-visible.html

Just created MR for 11.x

Reviewing MR 5029 as that's against 11.x and looks like 9.5 changes were rerolled corrected for 11.x

camilledavis → closed merge request !5055

camilledavis → closed merge request !5110

I believe MR 5029 is ready for review again, right?

Seems to introduce a bug

@smustgrave I can't reproduce the bug, is it with a specific theme?

Issue I saw was in olivero but just applied 5029 and am no longer seeing the problem.

Just ran into an issue when adding this patch to a site that has Claro as the admin theme, but has a different theme set for non-admin pages.

In the patch I’ve updated 2 toolbar.theme.css files — one in Claro, using design system variables, and one in the toolbar module, using hex codes as a fallback.

However, when Claro is only set as the admin theme, if you are logged in and visit a non-admin page, the Claro version of toolbar.theme.css still gets loaded but the variables don’t — so the module fallback doesn’t work. This is a core issue I think.

For what it's worth, I've rerolled this patch for 10.3x