Active toolbar tray has weak affordance and fails WCAG color criteria

Its the same in Claro theme too

The simplest solution is:

.toolbar .toolbar-bar .toolbar-tab > .toolbar-item.is-active {
  border-bottom: 4px solid white;
}

This is visible with low constrast simulation in NoCoffee. I don't have a Windows machine readily available to test with.

This makes the whole toolbar 4px higher, and makes the button content to not look vertically centered any more. I'll work a bit on that and see if I can improve it.

Patch

I'm working on this as part of DrupalCon Global 2020 Contribution Sprints. I am expecting to finish this within the next 2 hours.

Applied the patch on a fresh Drupal 9.1.x checkout.
The patch given in comment #8 applied successfully.
Tested to be working fine on both Seven and Claro themes.
Attaching screenshots of the same.

The failure is caused by the Composer Scaffold.

Could not delete /var/www/.composer/cache/files/zendframework/zend-diactoros/7726cf1db39dc403c97502d073a2c17370cefc71.zip

I am not sure it's related with this patch.

There are also 2 coding standards issues.

/var/lib/drupalci/workspace/jenkins-drupal_patches-54225/source/core/modules/toolbar/css/toolbar.theme.css
line 11 Unknown property 'tap-highlight-color'.
15 Unknown property 'touch-callout'.

Patch #14 looks good to me.

Follow images with the tests.

After adding patch #14, it looks good for me

RTBC + 1

The patch #14 works fine after applying on Drupal 9.1.x-dev.

Would be great to get a review on the design changes from a designer familiar with the Seven styleguide

Are there aria

1. +++ b/core/modules/toolbar/css/toolbar.theme.css
   @@ -1,6 +1,7 @@
   +
   
   @@ -14,40 +15,48 @@
   +
   ...
   -.toolbar .toolbar-item:hover,
   -.toolbar .toolbar-item:focus {
   +
   +.toolbar .toolbar-item:hover, .toolbar .toolbar-item:focus {
   ...
   +
    .toolbar .toolbar-bar {
      color: #ddd;
      background-color: #0f0f0f;
   -  box-shadow: -1px 0 3px 1px rgba(0, 0, 0, 0.3333); /* LTR */
   +  box-shadow: -1px 0 3px 1px rgba(0, 0, 0, 0.3333);
   +  /* LTR */
    }
   +
   ...
   +
    .toolbar .toolbar-bar .toolbar-item {
      color: #fff;
    }
   -.toolbar .toolbar-bar .toolbar-tab > .toolbar-item {
   +
   +.toolbar .toolbar-bar .toolbar-tab>.toolbar-item {
      font-weight: bold;
    }
   -.toolbar .toolbar-bar .toolbar-tab > .toolbar-item:hover,
   -.toolbar .toolbar-bar .toolbar-tab > .toolbar-item:focus {
   +
   +.toolbar .toolbar-bar .toolbar-tab>.toolbar-item:hover, .toolbar .toolbar-bar .toolbar-tab>.toolbar-item:focus {
   

   these changes are all out of scope here

2. +++ b/core/modules/toolbar/css/toolbar.theme.css
   @@ -55,88 +64,115 @@
   +
    .toolbar .toolbar-tray {
      background-color: #fff;
    }
   -.toolbar-horizontal .toolbar-tray > .toolbar-lining {
   -  padding-right: 5em; /* LTR */
   +
   +.toolbar-horizontal .toolbar-tray>.toolbar-lining {
   +  padding-right: 5em;
   +  /* LTR */
    }
   -[dir="rtl"] .toolbar-horizontal .toolbar-tray > .toolbar-lining {
   +
   +[dir="rtl"] .toolbar-horizontal .toolbar-tray>.toolbar-lining {
      padding-right: 0;
      padding-left: 5em;
    }
   +
    .toolbar .toolbar-tray-vertical {
   -  border-right: 1px solid #aaa; /* LTR */
   +  border-right: 1px solid #aaa;
   +  /* LTR */
      background-color: #f5f5f5;
   -  box-shadow: -1px 0 5px 2px rgba(0, 0, 0, 0.3333); /* LTR */
   +  box-shadow: -1px 0 5px 2px rgba(0, 0, 0, 0.3333);
   +  /* LTR */
    }
   +
    [dir="rtl"] .toolbar .toolbar-tray-vertical {
   ...
   +
    .toolbar-horizontal .toolbar-tray {
      border-bottom: 1px solid #aaa;
   -  box-shadow: -2px 1px 3px 1px rgba(0, 0, 0, 0.3333); /* LTR */
   +  box-shadow: -2px 1px 3px 1px rgba(0, 0, 0, 0.3333);
   +  /* LTR */
    }
   +
    [dir="rtl"] .toolbar-horizontal .toolbar-tray {
   ...
   -.toolbar-tray a:hover,
   -.toolbar-tray a:active,
   -.toolbar-tray a:focus,
   -.toolbar-tray a.is-active {
   +
   +.toolbar-tray a:hover, .toolbar-tray a:active, .toolbar-tray a:focus, .toolbar-tray a.is-active {
      text-decoration: underline;
      color: #000;
    }
   +
    .toolbar .toolbar-menu {
      background-color: #fff;
    }
   -.toolbar-horizontal .toolbar-tray .menu-item + .menu-item {
   -  border-left: 1px solid #ddd; /* LTR */
   +
   +.toolbar-horizontal .toolbar-tray .menu-item+.menu-item {
   +  border-left: 1px solid #ddd;
   +  /* LTR */
    }
   -[dir="rtl"] .toolbar-horizontal .toolbar-tray .menu-item + .menu-item {
   +
   +[dir="rtl"] .toolbar-horizontal .toolbar-tray .menu-item+.menu-item {
      border-right: 1px solid #ddd;
   ...
   +
    .toolbar-horizontal .toolbar-tray .menu-item:last-child {
   -  border-right: 1px solid #ddd; /* LTR */
   +  border-right: 1px solid #ddd;
   +  /* LTR */
    }
   +
    [dir="rtl"] .toolbar-horizontal .toolbar-tray .menu-item:last-child {
      border-left: 1px solid #ddd;
    }
   -.toolbar .toolbar-tray-vertical .menu-item + .menu-item {
   +
   +.toolbar .toolbar-tray-vertical .menu-item+.menu-item {
      border-top: 1px solid #ddd;
    }
   +
    .toolbar .toolbar-tray-vertical .menu-item:last-child {
      border-bottom: 1px solid #ddd;
    }
   +
    .toolbar .toolbar-tray-vertical .menu-item .menu-item {
      border: 0 none;
    }
   +
    .toolbar .toolbar-tray-vertical .toolbar-menu ul ul {
      border-top: 1px solid #ddd;
      border-bottom: 1px solid #ddd;
    }
   -.toolbar .toolbar-tray-vertical .menu-item:last-child > ul {
   +
   +.toolbar .toolbar-tray-vertical .menu-item:last-child>ul {
      border-bottom: 0;
    }
   +
    .toolbar .toolbar-tray-vertical .toolbar-menu .toolbar-menu .toolbar-menu .toolbar-menu {
   -  margin-left: 0.25em; /* LTR */
   +  margin-left: 0.25em;
   +  /* LTR */
    }
   +
    [dir="rtl"] .toolbar .toolbar-tray-vertical .toolbar-menu .toolbar-menu .toolbar-menu .toolbar-menu {
      margin-right: 0.25em;
      margin-left: 0;
    }
   +
    .toolbar .toolbar-menu .toolbar-menu a {
      color: #434343;
    }
   @@ -144,25 +180,33 @@
   
   @@ -144,25 +180,33 @@
    /**
     * Orientation toggle.
     */
   +
    .toolbar .toolbar-toggle-orientation {
      height: 100%;
      padding: 0;
      background-color: #f5f5f5;
    }
   +
    .toolbar-horizontal .toolbar-tray .toolbar-toggle-orientation {
   -  border-left: 1px solid #c9c9c9; /* LTR */
   +  border-left: 1px solid #c9c9c9;
   +  /* LTR */
    }
   +
    [dir="rtl"] .toolbar-horizontal .toolbar-tray .toolbar-toggle-orientation {
      border-right: 1px solid #c9c9c9;
   ...
   -.toolbar .toolbar-toggle-orientation > .toolbar-lining {
   -  float: right; /* LTR */
   +
   +.toolbar .toolbar-toggle-orientation>.toolbar-lining {
   +  float: right;
   +  /* LTR */
    }
   -[dir="rtl"] .toolbar .toolbar-toggle-orientation > .toolbar-lining {
   +
   +[dir="rtl"] .toolbar .toolbar-toggle-orientation>.toolbar-lining {
      float: left;
    }
   +
    .toolbar .toolbar-toggle-orientation button {
      display: inline-block;
      cursor: pointer;
   -}
   

   same here

3. Is indicating an item is active using a border any better than color? Neither will help a vision impaired user. Should we be using aria attributes instead/as well - e.g. aria-expanded or aria-pressed?

Yes, totally changing the background color is better than adding a white border. I find the screenshot in comment #22 clearer.

#22 looks good (icon notwithstanding, etc). But isn't that still "Active toolbar tray is indicated by color alone" ? It's a more dramatic color change, so it's certainly better, but it's still only color.

Reading Using CSS to change the presentation of a user interface component when it receives focus, I take that using colors to indicate an active element is not an issue. The issue is rather the chosen color. From their example, I get that changing the background color is preferable.

Should the IS be edited to make clearer what the proposed solution is? (The title doesn't seem to reflect what the proposed solution is, as the patch is still introducing a color change.)

No, that isn't really the point. The mockup in #22 isn't about using a better colour to indicate the active tray; it's about a more effective use of shape:

Fair enough. I'm always learning when it comes to Accessibility, so thanks for clarifying.

That said, how is this "shape" going to work when the trays are in the sidebar?

Needs updated screenshots, and perhaps new tests, but here's an initial implementation of #22.

Totally different approach from previous patches, so interdiff is pointless.

Tested with Seven and Claro. Embedding screenshots in the summary. Needs screenshots ;)

All working fine, except for the disruption for contrib modules that add their own toolbar trays. Those are going to need new #000000 versions of their icons, and updated CSS to point to them. Not sure what to do about that.

p.s. I'm not messing with the hover/focus styles here. I presume that's out of scope. Those are the same washed-out slightly-different gradients.

Re: "the disruption for contrib modules that add their own toolbar trays"

What if we add a new class to the toolbar as part of this change? Not sure what to call it. Then contrib (Devel, Toolbar Responsive Search, etc) that add a toolbar tray can target that class to know which version of the icon to use.

Something like so? Maybe won't fly. But then e.g. toolbar_responsive_search can do this:

.toolbar .toolbar-bar .toolbar-icon-search:active:before,
.toolbar .toolbar-bar .toolbar-icon-search.is-active:before {
  background-image: url(../icons/ffffff/loupe.svg);
}
.toolbar.toolbar-active-invert .toolbar-bar .toolbar-icon-search:active:before,
.toolbar.toolbar-active-invert .toolbar-bar .toolbar-icon-search.is-active:before {
  background-image: url(../icons/000000/loupe.svg);
}

Then it'd work for any version of core, before or after this change. Given how much effort we put into the 'continuous upgrade path' and letting contrib releases be compatible with multiple versions of core, it seems like it'd be nice to enable this to happen more painlessly for toolbar-providing contribs.

Thoughts?

Thanks!
-Derek

Applied patch #32 and it works fine.But for the toolbars added by contrib modules,the toolbar icon will be hidden.
Including screenshot after applying this patch:

Screenshot when the active toolbar is from a contrib module:

Re: #33 Thanks for testing. Indeed, contrib will be broken until they update. That's what I'm saying in #31. #32 would allow #3163252: Support both normal and inverted colors for active toolbar tray in which case you'd see the icon for contrib, too. You can try installing https://www.drupal.org/project/toolbar_responsive_search and https://www.drupal.org/files/issues/2020-08-04/3163252-2.patch if you want to see it in action.

Cheers,
-Derek

Now it works with toolbar icons of contrib modules.I have added some changes to #32 .
Including screenshots after applying this patch:

screenshot 2:

mobile view:

Thanks, @Abhijith S!

+++ b/core/modules/toolbar/css/toolbar.icons.theme.css
@@ -67,6 +67,9 @@
+  filter: invert(100%);

Interesting! Things I Learn. ;)

If that's widely supported by browsers, that would indeed be much simpler than messing with new versions of the icons, both for core and contrib.

https://caniuse.com/#feat=css-filters seems pretty decent, although (as usual) IE11 doesn't handle this. There are a lot of hits on there for "filter", I hope that's the correct one. ;)

Interdiff is nearly the whole patch, but attaching here for reference.

Curious to hear what other folks think.

Thanks,
-Derek

Confirming that #35 works great on a site with unpatched devel and toolbar_responsive_search, although only tested with modern Chrome and FF.

Patch #35 applies cleanly and as per the design expectations in #22. Toolbars of contributed modules are also covered in the patch (including inverting their icon colors).

No core themes are overriding these styles:

% find . -type f -name 'toolbar.icons.*'
./core/modules/toolbar/css/toolbar.icons.theme.css
./core/themes/stable/css/toolbar/toolbar.icons.theme.css
./core/themes/stable9/css/toolbar/toolbar.icons.theme.css
% find . -type f -name 'toolbar.theme.*'
./core/modules/toolbar/css/toolbar.theme.css
./core/themes/stable/css/toolbar/toolbar.theme.css
./core/themes/stable9/css/toolbar/toolbar.theme.css

We probably shouldn't touch stable* for this, although as a major accessibility bug, these are the sorts of issues where we've modified those themes.

Tagging for @lauriii's sign-off on:

A) Is filter: invert(100%); legit enough CSS for core?

B) Should we touch stable* or leave them unfixed?

Also, Remaining tasks has this:

• Tests - e.g. a functional test to confirm the presence (and absence) of a CSS class

Do we actually need that? We're not changing any class behavior, just making the style changes more obvious. Seems weird to test that the CSS colors the box white and the text black -- I don't think core tests need to verify CSS like this, but I could be wrong. Can any core committer confirm?

Thanks!
-Derek

Tagging for subsystem maintainer review to get sign off on the design change either from Toolbar subsystem maintainer or a usability topic maintainer.

Leaving the needs frontend framework manager tag to check whether this should be applied to Stable after we know the full scope of the changes.

+++ b/core/modules/toolbar/css/toolbar.icons.theme.css
@@ -67,6 +67,9 @@
+  filter: invert(100%);

IE 11 doesn't support the filter property https://caniuse.com/#feat=css-filters.

We've discussed this during today's UX meeting and we agreed that we're going to propose some designs for this based on the Drupal Design System as the work that needs to happen in #3070493: Introduce a mechanism to provide an alternate Claro design for the toolbar in the future. We'll take into account the options suggested here.
We'll do this next week during the Claro sprint on 25 and 26th of August, and since the Toolbar is one of the most prioritary design issues we have we expect to have a design solution for this issue by then.
Anyway, if anybody wants to keep working on this before that feel free to do so!

@lauriii re: #40: Thanks for reviewing. Indeed, at #36 I made the same observation about IE11 not supporting filter: invert(). The question still stands: "is this legit for core?" Does core actually care if this doesn't work on IE11? If so, would the approach in #32 be better? Otherwise, do you have a suggestion on how to implement this that is committable?

@ckrina re: #41: Thanks for that link! Nice to know Claro is thinking about this.

@all: No one should be confused that #41 means this issue shouldn't be fixed independently of Claro, or that this is somehow blocked on Claro's toolbar redesign. This is an existing, major accessibility bug that we can (should?) fix in core ASAP, and hopefully backport to 8.9.x. When we do, Claro's toolbar will immediately be improved in this regard, too (see screenshots in summary).

Thanks,
-Derek

@dww sorry, I forgot to mention on my comment specifically about the design proposed: it has some things that feels a bit off. The vertical line ends up in a middle of an odd white space and doesn't look "finished". A possible visual solutions discussed during the call was maybe adding a vertical top line, but we weren't sure how it would look like. I think this requires some explorations to find a visually working solution. As a designer, a don't feel comfortable with how the proposed solution looks like, although it's the path to follow and just needs some refinements (so thank to everybody that has worked on this!). If this issue needs to go in ASAP I won't oppose, but if we really can have a visual solution next week on the Claro design sprint (or someone else solves it sooner) I'd really appreciate if we can wait until then.

Ahhh, thanks for #43! That's very helpful. That's only a concern for horizontal tray layout, but it's still worth fixing. I agree this isn't yet ideal. Perhaps the thin grey line could work, although it kinda breaks up the inverted-T thing that @andrewmacpherson has been advocating for. But we don't have the inverted-T in the vertical tray layout, anyway, so having a light grey line in there on horizontal to keep things looking more slick seems okay.

As always, I defer to the actual accessibility experts in the room. ;)

To keep this moving, here's a trivial implementation + screenshot. It's not being smart about only adding the line for horizontal trays, but maybe this is okay.

Thoughts?

Thanks,
-Derek

Looks okay with vertical trays, too:

Random Fail. :/ Re-queueing #44.

Design change is OK with me

Applied patch #44. The horizontal border is working fine.
Screenshot 1:

Screenshot 2:

mobile view:

Tested #44 on 9.1
RTBC + 1

tagging

Patch applies cleanly in 9.2.x, and works as intended.

Attached images as evidence, as stated in comment #49.

Color contrast passed the a11y color test. Adding screenshots for reference.

RTBC+1

This fixes the stylelint problems that Drupal CI is now complaining about. Trivial re-ordering of the CSS properties I added in #44. No other changes, so leaving as RTBC.

Thanks,
-Derek

+++ b/core/modules/toolbar/css/toolbar.theme.css
@@ -48,6 +48,9 @@
   background-image: -webkit-linear-gradient(rgba(255, 255, 255, 0.25) 20%, transparent 200%);
   background-image: linear-gradient(rgba(255, 255, 255, 0.25) 20%, transparent 200%);

Can't the background-image be removed now - a white gradient on white is still just white - right?

This change is no fixing / changing Claro but the issue summary claims to be. Either the issue summary needs an update or Claro needs fixing.

Indeed, good point. ;) This fixes #57. Still looks fine:

Whoops, Claro. When I last worked on this for real, Claro wasn't overriding all those styles, so the module fix was sufficient. That's what the screenshots from #30 were showing. However, #3070493: Introduce a mechanism to provide an alternate Claro design for the toolbar in the future changed all that, and now we have to do more work to keep Claro looking right. Oh well. So here's a new patch that duplicates the work for the module styles into Claro's custom CSS world.

Claro theme are now also complying with the design changes of toolbar.

Random fail:

Testing Drupal\Tests\layout_builder\FunctionalJavascript\LayoutBuilderDisableInteractionsTest
F                                                                   1 / 1 (100%)

Back to RTBC.

The code has been RTBC for some time. Tagging "needs accessibility review" so the issue creator @andrewmacpherson can approve the solution.

If this is still waiting accessibility signoff on Jan 1, 2022, I can take care of it in my role as maintainer. I want @andrewmacpherson to have an opportunity to do this first since it's an issue he filed + I'd like to avoid the detail oriented wrath I'll likely incur were I to make the call right now.

@komalk Thanks for the patch
Applied successfully
After patch active tab in toolbar giving expected result
for ref sharing screenshots.....

@vikashsoni I'm removing credit for #64

• Please stop adding screenshots when prior comments already have done so. An additional set of screenshots can be helpful if there have been major changes to the path, but otherwise it's just noise. Please don't add screenshots when an earlier comment already provided them
• I notice you thanked @komalk for the patch in #14, and it looks like that is the patch you used for the screenshots. Multiple patches have been added since #14, and the proposed solution has changed. Providing screenshots of an old patch with an outdated approach is confusing to other contributors working on the patch. If you are providing screenshots, do so with the most recent patch

In

Signing off on "needs accessibility review" based on the solution of inverting the active tab to the opposite colors, which now makes it very clear. We reviewed this together in office hours, both Mike and Rain agreed.

Per the accessibility office hours discussion, I opened #3270230: Toolbar focus styles are not sufficiently obvious to know where your focus is as a child follow-up issue.

Also, restoring RTBC, since the accessibility review was the only thing needed per #63.

Thanks!
-Derek

I'm really sorry to remove the RTBC, but this is a design change that has an important visual impact in core's main navigation. The information received to solve the accessibility problem is really valuable, but there is a design process missing where other solutions need to be considered too. From dark to complete white is a huge visual change that might look right from a first look, but it breaks the black region the toolbar defines. Solutions like the one Gin Toolbar provides are more up-to-date with modern design navigation patterns.
So moving to Needs work to find a good design solution first.

I agree with @ckrina. To follow the correct process, this would first need design exploration to being in sync with the process. But anyway it seems this is based on the Seven toolbar and not the upcoming Claro implementation, which is due with #3020422: Toolbar style update.

I also agree this is an important topic we should look into it and improve it, but we might need to wait until #3020422 got merged. So we can either move this to Postponed or close it and create a new follow-up to the Claro implementation.

@ckrina and @saschaeggi: I hear your concerns that the Claro-specific portion of this patch isn't totally perfect, or necessarily in alignment with Claro's overall design. However, I strongly disagree with the change in status, the proposal on scope/dependencies/blockers, and the idea that there are any problems with the process we've followed in this issue.

This issue is a major accessibility bug in the core Toolbar. The component is "toolbar.module", not "Claro theme". This bug exists regardless of what admin theme a site is using. The primary fix here needs to be to the CSS in core/modules/toolbar/css/*. The secondary fix is making sure that if any stable core themes override those styles, to fix those, too. The tertiary concern is fixing experimental admin themes. The issue was opened only a few months after the very first commit of Claro to core.

@ckrina was involved in earlier design iterations (see #41, #43, etc). I happily incorporated the feedback into the patch (#44). But as I wrote at the end of #42 (which is still true today, except for the backport target):

@all: No one should be confused that #41 means this issue shouldn't be fixed independently of Claro, or that this is somehow blocked on Claro's toolbar redesign. This is an existing, major accessibility bug that we can (should?) fix in core ASAP, and hopefully backport to 8.9.x. When we do, Claro's toolbar will immediately be improved in this regard, too (see screenshots in summary).

The official Toolbar maintainer, @nod_, signed off on the design at #48 and removed the "Needs subsystem maintainer review" tag. If there were fundamental problems with the design here for Toolbar itself (or Seven), those can/should have been raised 2 years ago.

Claro went its own way with #3070493: Introduce a mechanism to provide an alternate Claro design for the toolbar in the future so we had to update this patch at #59 and later to keep the Claro fixes in line with the rest of the fixes to core.

Meanwhile, the Claro team is already planning to further override the "core" Toolbar styles at #3020422. At #3020422-114: Toolbar style update I commented that the current designs over there look like they would re-introduce this accessibility bug. Claro is always welcome to adopt whatever design it wants and override core styles. So long as the Claro-specific design for the toolbar is at least as accessible as what we're doing here, huzzah. If Claro decided to undo this work and introduce a toolbar design that was less accessible in this regard, I would open a follow-up issue to #3020422, with "Claro theme" as the component, and a title something like "New Claro toolbar designs from #3020422 are an accessibility regression for weak affordance and fails WCAG color criteria from #3020422". Whether that issue would be a Claro stable-blocker would be up to the release managers. But hopefully we never get to that point, and #3020422 will take this issue into account when designing the Claro-specific toolbar styles. To help ensure that's true, I'm moving the "Needs design" tag over there, and I just tagged that issue for "Needs accessibility review" to verify whatever design y'all come up with is at least as accessible as what we're doing here.

There's no reason to continue to block this issue from being fixed while Claro sorts out their Toolbar designs. Assuming this ticket lands first 🤞, it will be trivial to revert the Claro-specific CSS changes in here as part of introducing your whole new design. I'd be happy to rebase/reroll that effort once this lands. But at least in the interim, the Toolbar will be more accessible to folks using Claro on an experimental basis.

If Claro was already marked stable, I'd agree that we'd need more Claro-specific design sign-off in this issue. But that's not yet true, so that's an invalid concern/complaint.

We definitely do *not* want the scope of #3020422 to morph into "Make a whole new Toolbar design for Claro, plus fix accessibility bugs in the module CSS, Seven, et al". That's a mess. Let's fix the underlying bug here. Claro can and will override these styles once y'all agree on your new design. But please do not stand in the way of fixing Toolbar's own CSS (and Seven) while that happens.

I don't want to get in a pushing match over the RTBC status, so I'll return this to "Needs review" and tag for both product manager and release manager sign-off. The core toolbar maintainer thinks this is ready. The accessibility team thinks this is ready. The Bug Smash Initiative thinks it's ready. IMHO, all the "Needs design" (for Claro) work should be happening in #3020422, not here.

Thanks,
-Derek

Heh, although #3255809: Add nightwatch tests for toolbar added new tests that are now failing, causing the once green patch to now always trigger "Build Successful" failures:

✔ Running [0;32mtest tabbingmanager[0m[0;90m:[0m

[0;31m   Failed [equal]: (get("offsets") top has expected result) - expected [0;32m"true"[0m but got: [0;31m"false"[0m [0;90m(4ms)[0m[0m
[0;90m       at forEach (/var/www/html/core/modules/toolbar/tests/src/Nightwatch/Tests/toolbarApiTest.js:123:26)
       at Array.forEach (<anonymous>)
       at NightwatchAPI.<anonymous> (/var/www/html/core/modules/toolbar/tests/src/Nightwatch/Tests/toolbarApiTest.js:122:35)
       at processTicksAndRejections (node:internal/process/task_queues:96:5)[0m
error Command failed with exit code 5.

So back to NW for that. 😉

The additional 1px border suggested in #43 and added in #44 apparently changes the expected top offset in the new modules/toolbar/tests/src/Nightwatch/Tests/toolbarApiTest.js test added at #3255809. I hope fixing the expectation is okay, instead of trying to add the border without it taking up any space or something. I confess to not really understanding Nightwatch yet at all (this is the first time I've ever looked at it or run the tests locally). But this is at least passing again locally. Thoughts?

Thanks!
-Derek

While this is an accessibility bug, the change does have implications to usability and design. How I interpreted #69 was that there are concerns that the white background on the proposed resolution breaks the black toolbar region in half, making it not look like a one consistent area anymore – which is a usability concern.

Based on how I understand #69, I don't think the concern has anything to do with Claro or the Drupal design system. Claro was simply brought up because some exploration was done there, and @ckrina recommended that we would take a look what was done there because that could help provide some direction. If it's not helpful, I'd assume it would be fine to completely ignore what is happening there.

I want to stress that I 100% agree this issue should be focused on fixing the major bug – any other design changes are outside of the scope here, and need to happen in #3020422: Toolbar style update. While I agree with that, I also think that we should avoid causing usability regressions while we make this change, so it is good to have a review from an expert on that area.

@dww Please note that @ckrina is also a Usability topic maintainer, which is an overarching role that gives her the right to veto issues that impact her topic, even if the issue is signed-off by a subsystem maintainer. Therefore, the "Needs design" tag should be either removed by a Usability topic maintainer or a core committer. Please refer to https://www.drupal.org/contribute/core/maintainers#responsibility-assign... for more information about the governance.

@dww first of all, and as I said, I'm really sorry for changing the issue state. I don't want to dismiss all the work you've all done here.

But please understand the proposed changes constitute a huge visual change and I can't sign-off on something I don't agree. And while I said the direction made sense, I always stated there was still the need for some design work. A bottom line was added after my comments that solved one of the problems, but sadly it added other ones. Ideally, a design process would have caught that without giving you all so much work. But a design proposal that aims for this bigger visual changes needs more time and we don't have many designers helping.

So: would it be possible to find a solution that solves the accessibility concerns without changing the look&feel so much?

Re: #74: Indeed, I completely misunderstood #69 and #70 to be Claro-specific. I didn't realize @ckrina was speaking from their Usability maintainer role, I thought we were getting all confused on scope and process, and trying to impose Claro's design system on toolbar.module's CSS. Sorry to everyone for getting a little too worked up and not reading more closely.

Also, thanks to everyone who jumped in the next morning in the #admin-ui Slack meeting to discuss this topic (and loop me into that discussion). I was a little disheartened that the "Needs design" tag was going to doom this issue to another 2-3 years of not getting fixed, and was very happily surprised to see that projection also proved false before I even woke up. 😅 As my partner and I like to say to each other, "projection overruled". 😉

At #3020422-118: Toolbar style update, @saschaeggi posted this as a proposal that could address both the accessibility and UX concerns in here:

Since that's an attempt at a "quickfix" design for this bug fix, I'm posting my feedback here.

1. @andrewmacpherson’s original concern was:

   This doesn't satisfy two WCAG success criteria:
   SC 1.4.1 Use of Color (level A).
   SC 1.4.11 Non-text contrast (level AA). If we regard the current background gradient as indicating a state of a UI control (in this case "open" vs. "closed") then the colour difference doesn't have enough contrast.

   and the original proposed resolution says:

   Add a new signifier to the active toolbar tray, which doesn't rely on colour differences alone. Make more effective use of shape signifiers too.

   The bright blue is certainly a high enough contrast to handle 1.4.11, but it's still only a color difference. It seems like 1.4.1 would remain unsolved. @andrewmacpherson kept advocating for the "inverted T" as a shape, not just a different color than the non-active items. That's why breaking the line and messing with the design was intentional, but also why y'all don't seem to like it. 😉

   I don't consider myself nearly knowledgable enough to say one way or the other. I think this would need to go back for accessibility review to be sure, ideally with Andrew involved (if possible). So, re-tagging for "Needs accessibility review", although it might be hard to do that review unless we invest the energy to implement this new design.

2. We haven't really begun to look into how to solve #3270230: Toolbar focus styles are not sufficiently obvious to know where your focus is. It seems like some kind of focus ring concept would be useful (something Claro already does very well), but it seems like it could be tricky to keep the contrast up against that bright blue. Maybe there's a nice, clean way to do this. Again, I defer to folks more highly skilled in both design and accessibility.

Otherwise, I'm happy with the above screenshot, and it does look like it would at least satisfy WCAG 1.4.11, although I'm not sure about 1.4.1. Is it worth implementing this as a patch so we can play with it for real on test sites? I'd love to get a little more sign-off that it's worth exploring as a viable solution from the Accessibility folks, first. But I'm willing to do it if it'll help move the whole process along.

Thanks,
-Derek

p.s. Since core uses "American English" spellings, I guess our commit messages should, too. 😉 s/colour/color/ in the title.

Also, for now, removing the "Needs product manager review" and "Needs release manager review" tags that I erroneously added in #71 when I thought we needed the Core governance system to have those folks sort out disagreements between subsystem maintainers and "sub teams" (e.g. Claro vs. Accessibility). Now that we're all on the same page on the scope, we don't need any of those folks to be "tie-breakers" for this. Hopefully in the near future, UX team, Accessibility Team and Toolbar maintainer(s) will all be in agreement on what to do.

Thanks/sorry,
-Derek

We looked at this issue again at today's #accessibility office hours, with the following folks on the call:
- Mike Gifford (@mgifford)
- Ofer Shaal (@shaal)
- Rolf Koller (@rkoller)
- Mike Herchel (@mherchel)
- Me (@dww)
Crediting everyone who participated. Unfortunately, neither @rainbreaw nor @andrewmacpherson could make it this time.

I didn't have a mockup or patch for it, but I proposed a way to maintain both color and shape to tie the active tab and active tray together: use a border-top of ~3px of the same color blue as the active tab across the entire open subtray.

At least in horizontal view, that still has an element of the T shape from @andrewmacpherson, but mostly ties in with the design mockups from @saschaeggi.

It's not as ideal with the vertical toolbar. @rkoller suggested we extend the border from the active tab back to the edge of the screen (including the same border-top on the open vertical toolbar tray). @shaal is planning to mock-up both of these and post image(s). Once we have a screenshot, hopefully @mgifford will be able to remove the "Needs accessibility review" tag based on that approach, and the UX team can remove "Needs design". Then we just have to figure out the best way to implement it (making sure RTL keeps working), get frontend review, final reviews, RTBC, and commit. 😅

Thanks!
-Derek

Here are a few sample screenshots (draft)

If we can get this codified, I'm happy to take off Needs Accessibility Review.

Thanks @dww & @shaal

First of all, thanks for picking this up in the #accessibility office hours! 🙌

As it seems the examples were done with the original toolbar design in mind (for Seven), I've quickly adopted it to see how it will look like with the Claro toolbar redesign in mind.
I think the proposed 3px line might not be as visible as intented with the Claro's toolbar redesign (WDYT?) and we should take that into account for sure. While adapting the proposed solution I quickly came up with some new ideas and just wanted to quickly drop them here more or less uncommented 🙂

I like the inverted triangle indicator, but how can that be displayed when the menu is narrow, and the parent menu is far on the right? (like the `devel` menu example in the screenshots I posted yesterday)

@saschaeggi nice idea, I really like it! @shaal's point makes a lot of sense, but I still think it'd be useful to have this inverted triangle as an indicator because it would reinforce the active element. Also, I think using this instead of the 3px line plays better with the overall styles of the design system.

Ok, but the triangle only works reliably for horizontal toolbar.

If we do that for vertical toolbar, there’s nothing to associate the active tab with the open tray in some cases.

But I’m running out of steam to keep trying to make everyone happy here. 😉 I’m tired of trying to find perfect, and am more than happy to settle for good enough. If the better color contrast of blue + a shape change (that makes sense in most cases) is enough, great. It’s just very slow to iterate on this issue with monthly accessibility calls, and intermittent input for UX / design. It’d sure be great if @andrewmacpherson could weigh in, and/or we get both designers & accessibility folks on the same call or something.

I’m not at DrupalCon, so I can’t try to facilitate a face-to face among relevant parties. If anyone is able to try to get in-person agreement, that’d be amazing.

Thanks, -Derek

i also like the idea of the inverted triangle for the horizontal toolbar. and about @dww's point in regards of the missing association for the vertical toolbar i agree. would it make sense to perhaps combine the concept of the 3 or 4 pixel line with the triangle indicator for that case? connect the parent menu item, like in @shaals example devel, with the vertical toolbar by the pixel line and use the triangle indicator pointing downward in that case to the vertical toolbar. that way the parent menu item and the vertical sidebar would be associated and it would be played with the presentation of the triangle between toolbar states?

Re-reading #88, apologies that any frustration is leaking out into my communications here. I also forgot to write some things I was thinking that add much-needed context:

@saschaeggi: Thanks for the quick review and creative list of alternate proposals!
@ckrina: Thanks for reviewing and giving design direction.
@shaal: Thanks for raising the concern I share on the triangle shape.

Sorry for the previous omissions and tone, and I'm excited that this issue is really moving forward! Wish I was at DC to try to get everyone together on this myself, and I hope someone else is able to do so.

Thanks again!

p.s. Re: #89: Indeed, perhaps the blue line would be the same height as the triangle?

However, in #87 @ckrina is against the solid blue line in general design system aesthetics. So I'm not sure where we go from here.

Hopefully between DC, a possible multi-party call, Slack, or comments here, we can converge on something mutually agreeable soon. 🤞

Thanks!

I pinged @andrewmacpherson in Slack a while ago about this. He wrote the following comments and asked me to paste them into the issue for posterity.

I'm not clear what the alternate proposals are saying. It looks like they are doubling-down on colour.
I don't have a desktop computer available today, so hard for me to see detail or comment.

However I'd stress this isn't about Claro!

(I think we're all clear on that already -dww)

I don't understand the "proposed line".

NO to the underlined label. It's how hyperlinks should appear, and the toolbar tray buttons don't behave as hyperlinks when JS is used.

The little triangles din't rely solely on colour, but they are... somewhat little. if the pair, I prefer the upward-pointing where the white stabs into the blue.

It could work well in forced-colour modes though, so I like it in that sense.

Please copy these remarks to the issue. They are about comment 76. Keep in mind I haven't read all the arguments in other comments, and I'm reviewing the pictures on a tiny device.

My non-review opinion: the toolbar trays behave like tabs, so what's wrong with them actually looking like tabs?

So, still not sure how to proceed. I'd really appreciate it if @andrewmacpherson, @ckrina and any other interested parties would be willing to directly discuss this issue so I don't have to keep trying to play "operator" and pass messages between y'all. I'd truly love to finally fix this bug, but I can't do it on my own.

Thanks!
-Derek

How would the blue active state work with the green focus ring from the Toolbar Style Update, 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 (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 (underline, slight change in background 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 Just opened an MR for the focus ring on 9.5.x on this issue 3270230 - Toolbar focus styles are not sufficiently obvious to know where your focus is

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 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?

Edit: from @saschaeggi on slack:
Color would be --color-blue-500
Focus would be --color-green-500

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

Also the build:css script automatically adds linebreaks so shouldn't those be kept?

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.

Cleaning up the summary a bit.

Tested and reviewed the code, so moving this to RTBC. Also removing the NFFMR tag so it can be committed. Thanks all (edit: and specially to @camilledavis and @dww for pushing this)!

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