Active toolbar tray has weak affordance and fails WCAG color criteria

How would the blue active state work with the green focus ring from the Toolbar Style Update 📌 Toolbar style update Needs work , since the green and blue don't have sufficient contrast with each other?

I can think of two solutions...

1. Have the focus ring for the active blue button be a lighter color. But it seems confusing to have the focus ring be a different color depending on the button.

2. Go back to the active state proposed in #44 🐛 Active toolbar tray has weak affordance and fails WCAG color criteria Needs work (White background, black text). Then, the same green focus state will work everywhere.

Cross-posted from slack:

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

Camille Davis: 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

Camille Davis: 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.

+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.

@ckrina Should the focus ring replace the current focus state (slight change in color) entirely? Or appear in addition to it?

@camilledavis it should be additional to underline & bg color

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 that would be nice 👍

@saschaeggi Got the focus ring to cover the left and right border on the 1st level tray items, but I have to remove the 1px amount from the left and right of both insets:

.toolbar-tray-horizontal .toolbar-menu-administration > .toolbar-menu > .menu-item > a:focus {
box-shadow: inset 2px 3px 0 0 #26a769, inset -2px -3px 0 0 #26a769, inset 4px 5px 0 0 #fff, inset -4px -5px 0 0 #fff, 1px 0 0 0px #26a769, -1px 0 0 0px #26a769;
}

Is there a way to make the css simpler? After I replace the colors and px values with focus variables (some of which are calculations of 2 more variables) it gets pretty unreadable.

And that's just picking the variables that are equal to the px size... it's actually worse if you consider that for example, the first "2px" value is actually the link's --focus-border-size (which is set to 3px), minus its parent's border size (1px). So to account for the --focus-border-size variable changing, that 2px value would have to be written as calc(var(--focus-border-size) - 1px). And so on...

Here's an MR for the blue active tab in Claro 9.5

It currently has no visible focus state (aside from the underline). At first I tried with this css:

.toolbar .toolbar-bar .toolbar-tab > .toolbar-item:hover,
.toolbar .toolbar-bar .toolbar-tab > .toolbar-item:focus {
  background-image: -webkit-linear-gradient(rgba(255, 255, 255, 0.125) 20%, transparent 200%);
  background-image: linear-gradient(rgba(255, 255, 255, 0.125) 20%, transparent 200%);
}
.toolbar .toolbar-bar .toolbar-tab > .toolbar-item.is-active {
  background-color: #1d7aff;
}

So that the active tab turned slightly lighter while under focus, as it did previously. However this causes it to fail WCAG AA contrast requirements for small text. So I ended up changing the style to just background: #1d7aff so the transparent overlay doesn't get applied on focus.

Looks like this:

With the focus styles from https://www.drupal.org/project/drupal/issues/3270230 🐛 Toolbar focus styles are not sufficiently obvious to know where your focus is Needs work it will look like this:

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Oh hold on I think I grabbed the wrong blue color from @saschaeggi's png... now I'm trying to grab the color from https://www.figma.com/file/OqWgzAluHtsOd5uwm1lubFeH/%F0%9F%92%A7-Drupal-... but getting a few different values

Does anyone have the exact hex/rgb code handy?

Updated the active background to --color-blue-500.

However I couldn't find --color-green-500 in Claro 9.5, to include it I think I'd have to add new variables to the MR.

Even if I did, the green background on focus isn't an accessible state change by itself, the button would still require the white-in-green focus ring from https://www.drupal.org/project/drupal/issues/3270230 🐛 Toolbar focus styles are not sufficiently obvious to know where your focus is Needs work to be accessible

WCAG contrast issue is fixed.

Left a comment in the MR. Hiding patches

THe issue summary doesn't say what the exact solution will be good. Has that been decided?

Unfortunately seems this issue has been spammed with screenshots, so requesting no more unless a new solution has been identified

Changes in last commit contained unnecessary extra line breaks, now removed.

@camilledavis you can review available scripts in package.json, we have to lint code every time before push

Restoring needs review

Forgot to tag in #114

@kostyashupenko The stylelint from my package.json doesn't flag line breaks... I have 15.10.1

Adding reference to Windows High Contrast Mode.

Added fallback for people not using Claro

My only concern with this change to:

background: #004eff;

background: var(--color-blue-500);

is that I'd like to know what the ratios are to the foreground color.

#004eff has lots of room with a foreground of #fff
https://www.whocanuse.com/?bg=004eff&fg=ffffff&fs=16&fw=

I need to know what the values would be for var(--color-blue-500).

Knowing this I think we can remove the Needs accessibility review tag.

That variable is set with --color-blue-500: #004eff; (line 46 on core/themes/claro/css/base/variables.pcss.css).

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 file — one in Claro, using the design system variable, and one in the toolbar module, using #004eff 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.

In the meantime I could try adding both to the toolbar.theme.css file in Claro:
background: #004eff;
background: var(--color-blue-500);

So that if it can't access the variable there's still a fallback.

Thanks for keeping this issue moving forward, @camilledavis!

I still wish @andrewmacphearson and @ckrina would directly discuss this issue with each other. In #93 I pasted Andrew's comments from the last time I managed to ping him in Slack about it. He was pretty clear at the time that the blue tab solution was "doubling down on colour" and not a sufficient fix for the original accessibility concern here:

https://www.w3.org/WAI/WCAG21/Understanding/use-of-color.html

Color is not used as the only visual means of conveying information, indicating an action, prompting a response, or distinguishing a visual element

#96 tried to pick up the pieces of this stalled effort and run with it. Thanks! But we've still basically only heard from the design side of this issue since then, and AFAICT, no one has confirmed if that's going to satisfy 1.4.1 which is the whole point of all the effort that we've spent trying to fix this bug. In #98 @ckrina writes "I hope this solves the accessibility concerns." but there's no explanation how the blue tab is anything other than using color alone. Yes, it's definitely higher contrast than what we have. Yes, it looks nice with the rest of the (Claro) design system. Yet it seems to be only using color to indicate what's the active tray.

I'm still pretty burned out from this whole experience, and re-reading the comment thread here isn't helping. 😅 I really do not want any of us to spend more time trying to get this fixed until the two most important voices here actually agree with a single plan. Until that happens, I fear we're wasting our lives on something that will never be solved. Once again, can the two of you please talk to each other instead of having the rest of us shuttle proposals, concerns and counter-proposals back and forth?

I'm going to take the bold step of calling this "postponed" and assigning to @andrewmacphearson to get an answer so we can proceed. @ckrina, please reach out to Andrew if you have a direct channel.

Thanks so much!
-Derek

Posting this here also to show that I've answered (as all the other times this came up in Slack):

As I mentioned in the past I don’t have a strong opinion on this, specially since we’ll replace the Toolbar with the new Navigation (hopefully it’ll land 10.3.x as experimental). @saschaeggi created a lot of good designs in #87 so you could use any of those for the Toolbar since they look fine. I haven’t heard anything from Andrew in a long time and I don’t have any other tool than Slack, so I would suggest to ask other accessibility maintainers like @rainbreaw @mgifford or @bnjmnm and unblock this.

Once again, can the two of you please talk to each other instead of having the rest of us shuttle proposals, concerns and counter-proposals back and forth?

Beyond that, @dww I’m really sorry you’re burned with this but I assumed Sascha’s contributions cleared the path, that’s why I didn’t comment again (in the issue, but we've discussed this several times on Slack in the last months). We’re several Claro and Accessibility maintainers with the same authority, and any of us can decide things, like what is happening on comment #122.

The only reason I'd suggest @camilledavis to not work on this would be because Toolbar is going to be replaced (hopefully soon), not because the people working in the issue can't reach an agreement. So I'll leave to the people working here or yourself @dww to un-postpone this if you want to keep working in this.

Yeah, I’m sorry, too. I’m sorry I’ve lost my cool (at least twice) over the frustrations of trying to get this fixed through all the various roadblocks, twists and turns.

This was RTBC to fix the use of color bug years ago, and you did feel strongly that the “inverted T” solution using shape and color wasn’t ok from your design role, and you blocked this from going in at #69. Since then, we’ve tried (a lot) to get designs that you’re happy with that also address the bug. I’ve played operator many times, attended many meetings with the other accessibility maintainers, etc.

@saschaeggi provided a bunch of proposals in #85, which I dutifully got @andrewmacphearson to review, and I pasted his concerns in #93. Doesn’t seem those have been considered since the. We’re back to the original “just blue” design that he pretty strongly rejected.

Anyway, I feel strongly that this bug has been very frustrating to fix. If you now say you don’t feel strongly about it, can we go back to the design that Andrew was certain would solve the accessibly concern, reroll #73 into an MR, and merge that? then hopefully that will at least fix this bug in 10.3 for apparently toolbar’s final release before being deprecated and replaced.

Per Slack thread (see attached transcript), @ckrina is now okay with the "Inverted T". I opened a new MR against 11.x with the code from patch #73.

• Removing 'Needs accessibility review', since #73 was already approved by @andrewmacphearson, and many others as solving the original accessibility bug.
• Removing "Needs design" since @ckrina approved this.
• Removing "Needs issue summary update" since the summary and screenshots are now accurate again.

Ready for re-review.

Thanks,
-Derek

Whoops, here's the Slack discussion.

dww → changed the visibility of the branch 11.x to hidden.

dww → changed the visibility of the branch 9.5.x to hidden.

dww → changed the visibility of the branch drupal-3097907-indicate-active-tray-11x to hidden.

Cleaning up the summary a bit.

Tested and reviewed the code, so moving this to RTBC. Also removing the NFMR tag so it can be committed. Thanks all!

Thanks for the efforts, I know those type of issues can be frustrating so thanks for sticking with it :)

Committed 4e26ae9 and pushed to 11.x and 10.3.x. Thanks!

Yay, thanks for the speedy review and commit now that we've reached consensus! 🎉 Extremely happy to have this bug fixed at last. 🙏

Automatically closed - issue fixed for 2 weeks with no activity.