Problem/Motivation

In forced-colors emulation on MacOS, the "hamburger" menu icon for the collapsed primary tabs (menu local tasks) does not adjust color. On a dark background, the blue icon has insufficient contrast.

Screenshot of Claro primary tabs menu hamburger icon in chrome with forced-colors emulation with dark theme

Screenshot of Claro primary tabs menu hamburger icon in Edge with Page Colors setting on dusk theme

I'd guess that this also applies to other platforms, but that needs confirmation.

Steps to reproduce

  1. With Claro as the admin theme, go to /admin/content.
  2. Do one of:
  3. Reduce the window size until the primary tabs collapse into a menu with the hamburger icon.
Expected

In forced-colors: active mode, the icon color should be ButtonText.

Proposed resolution

Add a media query to the SVG file for (forced-colors: active), specifying currentColor for the fill. This will allow the icon to inherit color from the parent button element in forced-colors mode. 

@Curtis Wilcox also contributed to this proposed resolution.

Remaining tasks

  • Implement
  • Test on Mac
  • (?) Test on other platforms with system high-contrast modes

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#13 3513439-after-firefox-forced-colors-dark.png9.85 KBkentr
#13 3513439-after-chrome-forced-colors-dark.png10.05 KBkentr
#13 3513439-after-edge-page-colors-dusk.png9.75 KBkentr
#10 3513439-before-edge-page-colors-dusk.png9.82 KBkentr
#10 3513439-before-chrome-forced-colors-dark.png10.14 KBkentr
#8 after-fix-3513439.png34.94 KBsandip
firefox-colors-preference.png112.35 KBkentr
claro-forced-colors-table-sort-icons.png15.53 KBkentr
claro-forced-colors-hamburger-menu-icon.png31.12 KBkentr

Issue fork drupal-3513439

Command icon Show commands

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

About issue forks

Comments

kentr created an issue. See original summary.

quietone’s picture

quietone commented
Version: 11.1.x-dev » 11.x-dev

Changes are made on on 11.x (our main development branch) first, and are then back ported as needed according to the Core change policies.

sandip’s picture

sandip commented

Hi @kentr, i think the proposed resolution may not work in this case as that hamberger menu is a svg image so we can add fill: linktext to its < path/> tag when forced color mode is active to fix this issue. Do you agree with it?

kentr’s picture

kentr commented
Issue summary: View changes

@sandip poddar:

It should work... That said, my proposal was biased because I had been working on an issue where a media query inside the svg didn't work.

Your proposal is simpler and less invasive, and I've also learned from @Curtis Wilcox in Slack that:

  • in this case the role is a button and the color should be ButtonText
  • because this is an inline SVG, using currentColor will allow the colors to inherit from the parent button which will have the correct color in forced-colors mode

I updated the IS proposed resolution based on the suggestions from you and @Curtis Wilcox.

kentr’s picture

kentr commented

I'm also only finding this one instance in Claro of an inline SVG. Maybe it would be better to convert it to a background image for consistency.

Ultimately, it's up to the maintainers.

sandip’s picture

sandip commented

Hi @kentr,
Thank you for the information. I'll proceed with raising a MR as outlined in the Proposed resolution.

I'm also only finding this one instance in Claro of an inline SVG. Maybe it would be better to convert it to a background image for consistency.

That sounds reasonable to me as well. If any maintainer agrees with this suggestion, I'll be happy to revert the changes. For now, I'll go ahead and raise the MR as mentioned in point #4.

sandip poddar opened merge request !11552

sandip’s picture

sandip commented
Status: Active » Needs review
StatusFileSize
new after-fix-3513439.png34.94 KB

Attaching Screenshot for review.

Img

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Seems there is some feedback on the MR.

kentr’s picture

kentr commented
Issue summary: View changes
StatusFileSize
new 3513439-before-chrome-forced-colors-dark.png10.14 KB
new 3513439-before-edge-page-colors-dusk.png9.82 KB

Updating the IS to include other browsers.

sandip’s picture

sandip commented

I am working on the feedback.

sandip’s picture

sandip commented
Issue summary: View changes
Status: Needs work » Needs review

Please review the changes.

kentr’s picture

kentr commented
Issue summary: View changes
StatusFileSize
new 3513439-after-edge-page-colors-dusk.png9.75 KB
new 3513439-after-chrome-forced-colors-dark.png10.05 KB
new 3513439-after-firefox-forced-colors-dark.png9.85 KB

Passes testing on Mac in Edge, Firefox, Chrome.

Does protocol dictate also testing on other platforms with system high-contrast modes (such as WHCM)?

Screenshot of result with changes applied, in chrome mac forced colors emulation with a dark theme
Screenshot of result with changes applied, in edge mac with page colors dusk theme
Screenshot of result with changes applied, in firefox mac forced colors emulation with a dark theme

Gitlab won't let me resolve this MR comment, but it is resolved.

sandip’s picture

sandip commented

Hi @kentr, I have marked the thread as resolved based on your confirmation.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Believe feedback to addressed for this one.

nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Reviewed & tested by the community » Needs work

small feedback, unnecessary nesting. You can RTBC once it's fixed

sandip’s picture

sandip commented

I am looking into it.

sandip’s picture

sandip commented
Status: Needs work » Reviewed & tested by the community

I have made the suggested changes and moving this issue to RTBC.

nod_ closed merge request !11552

  • nod_ committed 9031a4e9 on 11.x 
    Issue #3513439 by sandip, kentr, nod_: Claro primary tabs collapsed menu...
nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Reviewed & tested by the community » Fixed

Committed 9031a4e and pushed to 11.x. Thanks!

Status: Fixed » Closed (fixed)

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