While reviewing #3097907: Active toolbar tray has weak affordance and fails WCAG color criteria during accessibility office hours (thanks!), @rainbreaw pointed out that while the new styles for the active tray are much better, the focus style is still very difficult to parse to see what item in the toolbar (either top-level tabs or even links within each subtray) currently has focus.
#3020422: Toolbar style update aims to improve this for Claro, but we should fix it in the core toolbar styles and Seven, too.
Add a focus ring to the toolbar styles so that the item with focus is truly obvious. Details TBD.
TBD
| Comment | File | Size | Author |
|---|---|---|---|
| #46 | 3270230-10-3.patch | 1.57 KB | chri5tia |
| #42 | Screenshot 2023-10-25 at 11.41.58 AM.png | 50.62 KB | smustgrave |
| #23 | focus state desaturated.png | 19.17 KB | camilledavis |
| #23 | focus state.png | 19.18 KB | camilledavis |
| #19 | Screenshot 2023-04-01 at 1.03.30 PM.png | 79.15 KB | smustgrave |
Show commandsStart 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:
changes, plain diff MR !5029changes, plain diff MR !5031changes, plain diff MR !5030changes, plain diff MR !3754compare compare compare compare 
Comments
Comment #3
dwwClarifying summary a bit, and crediting @rain for pointing out the bug...
Comment #5
dwwWhoops, sorry, wrong "@rain"! 😉
Comment #6
dwwMoving this to be a child of #3084529: [META] improve accessibility of toolbar, which is a more appropriate parent issue for this effort.
I believe this issue is deadlocked until we have a reasonable design to try to implement, so first tagging for 'Needs design'.
Also tagging to be smashed. 😉
Comment #10
camilledavis commentedCross-posted from Slack #frontend and 3097907 - Active toolbar tray has weak affordance and fails WCAG color criteria:
@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?
@saschaeggi: that would be nice 👍
Comment #12
dwwThanks for moving this forward! Looks like we now have a target design, so removing that tag.
seventheme 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!
Comment #13
camilledavis commented@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.
Comment #14
dwwPerhaps 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!
Comment #16
saschaeggi@camilledavis thanks for working on this, glad to see it happen 🙇
Comment #17
camilledavis commented@saschaeggi @dww no prob! Just updated the MR and copied the focus ring css to the toolbar module.
Comment #18
camilledavis commentedComment #19
smustgrave commentedMay be a non issue but when I tested the color contrast I got a AA failure.
Comment #20
camilledavis commentedI believe it's a pass since the focus ring is a UI component / graphical object
Comment #21
camilledavis commentedComment #22
smustgrave commentedBut won't users who may be color blind have trouble seeing the focus when tabbing?
Comment #23
camilledavis commentedI 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:
Comment #24
smustgrave commentedIf that's the case don't mind marking this as it does do what the screenshots are saying.
Comment #26
smustgrave commentedComment #27
bnjmnmIt 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.
Comment #28
mgiffordTagging this for https://www.w3.org/WAI/WCAG22/Understanding/focus-visible.html
Comment #30
camilledavis commentedJust created MR for 11.x
Comment #31
camilledavis commentedComment #34
smustgrave commentedReviewing MR 5029 as that's against 11.x and looks like 9.5 changes were rerolled corrected for 11.x
Comment #38
nod_Comment #41
dwwI believe MR 5029 is ready for review again, right?
Comment #42
smustgrave commentedSeems to introduce a bug
Comment #43
camilledavis commented@smustgrave I can't reproduce the bug, is it with a specific theme?
Comment #44
smustgrave commentedIssue I saw was in olivero but just applied 5029 and am no longer seeing the problem.
Comment #45
camilledavis commentedJust 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.
Comment #46
chri5tia commentedFor what it's worth, I've rerolled this patch for 10.3x
Comment #48
quietone commentedThe Toolbar Module was approved for removal in #3476882: [Policy] Move Toolbar module to contrib.
This is Postponed. The status is set according to two policies. The Remove a core extension and move it to a contributed project and the Extensions approved for removal policies.
The deprecation work is in #3484850: [meta] Tasks to deprecate Toolbar module and the removal work in #3488828: [Meta] Tasks to remove Toolbar module.
Toolbar will be moved to a contributed project before Drupal 12.0.0 is released.