Problem/Motivation

Discussed in #3283803: [upstream] CKE5 toggleable toolbar items not enough contrast, some icons include fills with low contrast colors. This makes it difficult to asses the meaning of these icons. For example, in low contrast sensitivity scenarios (biological, or just a sunny day) the image alignment buttons would be indistinguishable from each other, as it would just look like three tiny rectangles:

Steps to reproduce

👆

Proposed resolution

Fortunately, @reinmar offered a very simple solution for this 

.ck-button:not(.ck-disabled) .ck-icon * {
	opacity: 1 !important;
	fill-opacity: 1 !important;
}

Remaining tasks

Review.

User interface changes

Before

After

API changes

None.

Data model changes

None.

Release notes snippet

N/A

CommentFileSizeAuthor
#16 3306216-16-9.4.x.patch846 byteswim leers
#8 after-numberedlist.png3.63 KBwim leers
#8 before-numberedlist.png5.12 KBwim leers
#8 after-bulletedlist.png4.76 KBwim leers
#8 before-bulletedlist.png4.39 KBwim leers
#6 after-opacity-full.png252.32 KBbnjmnm
#6 before-opacity-full.png368.81 KBbnjmnm
#6 3306216-5.patch876 bytesbnjmnm
Screen Shot 2022-08-26 at 8.36.53 AM.png53.76 KBbnjmnm

Issue fork drupal-3306216

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

bnjmnm created an issue. See original summary.

bnjmnm credited Reinmar.

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
commented
Issue tags: -beta blocker, -Drupal 10 +Drupal 10 beta blocker

bnjmnm opened merge request !2684

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Issue tags: +stable blocker, +Needs screenshots
Parent issue: » #3238333: Roadmap to CKEditor 5 stable in Drupal 9
  1. Can we get before vs after screenshots?

    (It's my understanding based on #3283803-12: [upstream] CKE5 toggleable toolbar items not enough contrast that this will not be changed upstream in CKEditor 5 because it'd be a visual regression. So I'm curious how different this looks.)

  2. AFAICT this is not blocked on #3306153: Update CKEditor 5 to 35.1.0, is that correct?
bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
commented
Status: Active » Needs review
StatusFileSize
new 3306216-5.patch876 bytes
new before-opacity-full.png368.81 KB
new after-opacity-full.png252.32 KB

#5.1
Before

After

#5.2
Not blocked on anything 🙂. Just adding a stylesheet + style

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Issue summary: View changes
Issue tags: -Needs screenshots

Thanks!

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Title: Contrast perception issues with Icons including low-opacity fills » Contrast perception issues with icons including low-opacity fills
Assigned: bnjmnm » Unassigned
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new before-bulletedlist.png4.39 KB
new after-bulletedlist.png4.76 KB
new before-numberedlist.png5.12 KB
new after-numberedlist.png3.63 KB

Code review

+++ b/core/modules/ckeditor5/css/editor.css
@@ -0,0 +1,10 @@
+/* Convert low opacity icons to full opacity. */

This could benefit from a comment explaining the rationale documented in this issue and the previous issue 🤓

Visual impact review

What is the impact on other buttons besides the ones that triggered this change?

Checked using https://ckeditor.com/ckeditor-5/demo/#document:

What? Before After
bulleted list
numbered list

Conclusion: while this weakens the visual hierarchy and is arguably less pretty, it is not harmful while ensuring better accessibility.

  • lauriii committed 226fa76 on 10.1.x 
    Issue #3306216 by bnjmnm, Wim Leers, Reinmar: Contrast perception issues...

  • lauriii committed 5f3cb8c on 10.0.x 
    Issue #3306216 by bnjmnm, Wim Leers, Reinmar: Contrast perception issues...

  • lauriii committed 44b1b61 on 9.5.x 
    Issue #3306216 by bnjmnm, Wim Leers, Reinmar: Contrast perception issues...
lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Status: Reviewed & tested by the community » Fixed

I tested manually with all of the buttons Drupal core ships with and I personally can see how this fixes a major accessibility problem. Some of the icons does look slightly less nice with this but I didn't personally feel it was significant. I hope this gets fixed in CKEditor 5 so that broader group of CKEditor 5 users gets to benefit from this.

Committed 226fa76 and pushed to 10.1.x. Also cherry-picked to 10.0.x and 9.5.x.Thanks!

lauriii closed merge request !2684

Status: Fixed » Closed (fixed)

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

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Version: 9.5.x-dev » 9.4.x-dev
Status: Closed (fixed) » Patch (to be ported)

The fix should still be backported to 9.4.x.

Will provide a patch. Currently not cleanly cherry-pickable. So determining what the optimal cherry-pick order is …

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Patch (to be ported) » Reviewed & tested by the community
StatusFileSize
new 3306216-16-9.4.x.patch846 bytes

Does not cleanly apply to 9.4.x because #3291047: Move Quick Edit-specific styling of CKEditor 4 & 5 into Quick Edit module was committed only to Drupal >=9.5 (rightly so). Trivial conflict to resolve in ckeditor5.libraries.yml though :)

  • lauriii committed 26c3757 on 9.4.x 
    Issue #3306216 by bnjmnm, Wim Leers, Reinmar: Contrast perception issues...
lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Status: Reviewed & tested by the community » Fixed

Committed 26c3757 and pushed to 9.4.x. Thanks!

Status: Fixed » Closed (fixed)

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