Problem/Motivation

Even with the improvements coming with #3283803: [upstream] CKE5 toggleable toolbar items not enough contrast, there is still an issue with CKEditor 5 conveying button's active state using color only. This can be addressed fairly easily by adding an outline to the .ck-on class.

This was also an issue in CKEditor 4 so it's not a regression and thus not stable blocking, but it's an easily implemented accessibility improvement that should at the very latest be in the 10.0.0 release.

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#26 wcag-test.png287.91 KBjannakha
#26 after-patch.png40.97 KBjannakha
#23 3306209_23.patch512 bytesathyamvidyasagar
#23 editor-after.png6.94 KBathyamvidyasagar
#18 3306209-nr-bot.txt85 bytesneeds-review-queue-bot
#15 Screen Shot 2023-02-16 at 1.44.36 PM.png157.2 KBsmustgrave
#14 Screen Shot 2023-02-01 at 10.42.29 pm.png14.37 KBpameeela
#14 Screen Shot 2023-02-01 at 10.41.40 pm.png10.72 KBpameeela
#10 Screenshot 2023-02-01 at 3.53.31 PM.png25.41 KBgauravvvv
#10 3306209-10.patch449 bytesgauravvvv
#9 3306209-9.patch443 bytesgauravvvv
#8 3306209-nr-bot.txt143 bytesneeds-review-queue-bot
#7 Screen Shot 2023-01-24 at 11.16.57 am.png15.83 KBpameeela
#7 Screen Shot 2023-01-24 at 11.16.46 am.png15.92 KBpameeela
#3 3306209-3.patch354 bytessmustgrave

Issue fork drupal-3306209

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’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
CreditAttribution: bnjmnm at Acquia commented
Related issues: +#3283803: [upstream] CKE5 toggleable toolbar items not enough contrast
smustgrave’s picture

smustgrave CreditAttribution: smustgrave at Mobomo commented
StatusFileSize
new 3306209-3.patch354 bytes

Like this?

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: wim leers at Acquia commented
Status: Active » Needs review

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

mgifford’s picture

mgifford
he/him
Primary language English
CreditAttribution: mgifford at CivicActions commented
Issue tags: +wcag141

Sounds like a WCAG 1.4.1 issue.

pameeela’s picture

pameeela CreditAttribution: pameeela commented
StatusFileSize
new Screen Shot 2023-01-24 at 11.16.46 am.png15.92 KB
new Screen Shot 2023-01-24 at 11.16.57 am.png15.83 KB

Seems simple enough but I know better than to RTBC a CSS patch :)

Passes manual testing.

Before patch:

After patch:

needs-review-queue-bot’s picture

needs-review-queue-bot CreditAttribution: needs-review-queue-bot as a volunteer commented
Status: Needs review » Needs work
StatusFileSize
new 3306209-nr-bot.txt143 bytes

The Needs Review Queue Bot tested this issue. It either no longer applies to Drupal core, or fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

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

gauravvvv’s picture

gauravvvv CreditAttribution: gauravvvv at Axelerant for Drupal India Association commented
Status: Needs work » Needs review
StatusFileSize
new 3306209-9.patch443 bytes

Re-rolled patch #3, for 10.1.x. Also I have removed !important and improved the nesting of element. please review.

gauravvvv’s picture

gauravvvv CreditAttribution: gauravvvv at Axelerant for Drupal India Association commented
StatusFileSize
new 3306209-10.patch449 bytes
new Screenshot 2023-02-01 at 3.53.31 PM.png25.41 KB

Active state with #2977ff color outline.

After patch #10:

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: wim leers at Acquia commented
Issue tags: +Needs accessibility review

I think this needs to be RTBC'd by one of these folks:

Accessibility
- Rain Breaw Michaels 'rainbreaw' https://www.drupal.org/u/rainbreaw
- Mike Gifford 'mgifford' https://www.drupal.org/u/mgifford
- Andrew Macpherson 'andrewmacpherson' https://www.drupal.org/u/andrewmacpherson
- (provisional) Ben Mullins 'bnjmnm' https://www.drupal.org/u/bnjmnm
pameeela’s picture

pameeela CreditAttribution: pameeela commented

Patch #10 looks much better!

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: wim leers at Acquia commented
Issue tags: +Needs manual testing

#12 Agreed! But that made me realize something … this is currently being tested in a single theme (Claro).

We also need to verify that this works correctly in other themes. At minimum also Olivero (e.g. on the comment form). Tagging for that.

pameeela’s picture

pameeela CreditAttribution: pameeela commented
Issue tags: -Needs manual testing
StatusFileSize
new Screen Shot 2023-02-01 at 10.41.40 pm.png10.72 KB
new Screen Shot 2023-02-01 at 10.42.29 pm.png14.37 KB

It works in Olivero and Stark. Seems very RTBC-able but will await accessibility review.

smustgrave’s picture

smustgrave CreditAttribution: smustgrave at Mobomo commented
Status: Needs review » Needs work
Issue tags: +Needs Review Queue Initiative
StatusFileSize
new Screen Shot 2023-02-16 at 1.44.36 PM.png157.2 KB

Not on the accessibility team but tested and this failures color contrast.

color

pameeela’s picture

pameeela CreditAttribution: pameeela commented

@smustgrave this issue adds an outline matching the existing color scheme, so I think changing the color scheme should be a separate issue?

pameeela’s picture

pameeela CreditAttribution: pameeela commented
Status: Needs work » Needs review

Yeah, #3283803-12: [upstream] CKE5 toggleable toolbar items not enough contrast explains the current active state behaviour which is handled upstream in CKEditor.

needs-review-queue-bot’s picture

needs-review-queue-bot CreditAttribution: needs-review-queue-bot as a volunteer commented
Status: Needs review » Needs work
StatusFileSize
new 3306209-nr-bot.txt85 bytes

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

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

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

smustgrave’s picture

smustgrave CreditAttribution: smustgrave at Mobomo commented

So what's the accessibility review needed for? If it fails and we accept it fails do they still need to check?

That failure is valid too meant to add in my comment. Didn't apply anymore.

pameeela’s picture

pameeela CreditAttribution: pameeela commented

I’m not sure, since it’s sticking to the existing color scheme, and changing that is definitely out of scope.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

mgifford’s picture

mgifford
he/him
Primary language English
CreditAttribution: mgifford at CivicActions commented
Issue tags: -Needs accessibility review

Just make sure it meets color contrast requirements. Removing "Needs Accessibilty Review" unless this is more complicated.

athyamvidyasagar’s picture

athyamvidyasagar CreditAttribution: athyamvidyasagar at Melity commented
StatusFileSize
new editor-after.png6.94 KB
new 3306209_23.patch512 bytes

Highlighted the active buttons on CKEditor 5 .

jannakha made their first commit to this issue’s fork.

jannakha opened merge request !6984

jannakha’s picture

jannakha CreditAttribution: jannakha as a volunteer and at Tomato Elephant Studio commented
Issue summary: View changes
Status: Needs work » Needs review
StatusFileSize
new after-patch.png40.97 KB
new wcag-test.png287.91 KB

Patch in merge request: MR !6984

After patch:

WCAG test shows pass for Graphical Objects and User Interface Components:

smustgrave’s picture

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

Change makes sense.

  • nod_ committed 229f626d on 11.x 
    Issue #3306209 by jannakha, Gauravvvv, smustgrave, athyamvidyasagar,...

  • nod_ committed f935d32e on 10.3.x 
    Issue #3306209 by jannakha, Gauravvvv, smustgrave, athyamvidyasagar,...

  • nod_ committed 0c6312a0 on 10.2.x 
    Issue #3306209 by jannakha, Gauravvvv, smustgrave, athyamvidyasagar,...

nod_ closed merge request !6984

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ at Très Bien Tech commented
Status: Reviewed & tested by the community » Fixed

Committed 229f626 and pushed to 11.x. Thanks!

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ at Très Bien Tech commented
Version: 11.x-dev » 10.2.x-dev
pameeela’s picture

pameeela CreditAttribution: pameeela at Technocrat commented

Status: Fixed » Closed (fixed)

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