- First commit to issue fork.
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?
- @camilledavis opened merge request.
- Status changed to Needs work
over 1 year ago 10:54pm 29 March 2023 - ๐บ๐ธUnited States dww
Thanks for moving this forward! Looks like we now have a target design, so removing that tag.
- 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.
- 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 tocore/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.
- ๐บ๐ธUnited States dww
- 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. ๐
- 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!
- ๐จ๐ญSwitzerland saschaeggi Zurich
@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.
- Status changed to Needs review
over 1 year ago 6:10pm 31 March 2023 - Status changed to Needs work
over 1 year ago 5:05pm 1 April 2023 - ๐บ๐ธUnited States smustgrave
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
- Status changed to Needs review
over 1 year ago 4:06pm 6 April 2023 - ๐บ๐ธUnited States smustgrave
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:
- Status changed to RTBC
over 1 year ago 6:19pm 7 April 2023 - ๐บ๐ธUnited States smustgrave
If that's the case don't mind marking this as it does do what the screenshots are saying.
- First commit to issue fork.
- last update
over 1 year ago 30,320 pass - last update
over 1 year ago 30,314 pass, 2 fail - last update
over 1 year ago 30,322 pass - last update
over 1 year ago 30,322 pass - last update
over 1 year ago 30,322 pass - last update
over 1 year ago 30,322 pass - last update
over 1 year ago 30,322 pass - last update
over 1 year ago 30,322 pass - Open on Drupal.org โEnvironment: PHP 8.1 & MySQL 5.7last update
over 1 year ago Waiting for branch to pass - last update
over 1 year ago 30,330 pass - last update
over 1 year ago 30,330 pass - last update
over 1 year ago 30,330 pass - last update
over 1 year ago 30,330 pass - last update
over 1 year ago 30,334 pass - last update
over 1 year ago 30,334 pass - last update
over 1 year ago 30,334 pass - last update
over 1 year ago 30,334 pass - last update
over 1 year ago 30,334 pass - Status changed to Needs work
over 1 year ago 12:42pm 22 May 2023 - ๐บ๐ธUnited States bnjmnm Ann Arbor, MI
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.
- ๐จ๐ฆCanada mgifford Ottawa, Ontario
Tagging this for https://www.w3.org/WAI/WCAG22/Understanding/focus-visible.html
- Merge request !5029Add green and white Claro focus ring to toolbar. โ (Open) created by camilledavis
- last update
11 months ago 30,415 pass - Status changed to Needs review
11 months ago 8:55pm 17 October 2023 - last update
11 months ago 30,415 pass - @camilledavis opened merge request.
- last update
11 months ago 30,411 pass, 2 fail - last update
11 months ago 29,672 pass - @camilledavis opened merge request.
- Status changed to RTBC
11 months ago 3:20pm 18 October 2023 - ๐บ๐ธUnited States smustgrave
Reviewing MR 5029 as that's against 11.x and looks like 9.5 changes were rerolled corrected for 11.x
- First commit to issue fork.
- last update
11 months ago 30,420 pass - last update
11 months ago 30,420 pass - @camilledavis opened merge request.
- last update
11 months ago Build Successful - last update
11 months ago 30,425 pass - Status changed to Needs work
11 months ago 2:56pm 20 October 2023 - last update
11 months ago Custom Commands Failed - last update
11 months ago 30,427 pass - last update
11 months ago Custom Commands Failed - last update
11 months ago 30,434 pass - last update
11 months ago 30,434 pass - @camilledavis opened merge request.
- last update
11 months ago 30,434 pass - Status changed to Needs review
11 months ago 8:14pm 24 October 2023 - ๐บ๐ธUnited States dww
I believe MR 5029 is ready for review again, right?
- Status changed to Needs work
11 months ago 3:42pm 25 October 2023 @smustgrave I can't reproduce the bug, is it with a specific theme?
- ๐บ๐ธUnited States smustgrave
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.