Claro is missing focus in "Available buttons" within CKEditor toolbar configuration

Added styling for hover and focus on toolbar configuration for ckeditor admin UI.
Here is a current patch that will work.

The issue is scoped as an issue with Claro, but the changes are made to the CSS in the CKEditor module. This would result in Claro's focus styling being applied to all themes (that don't override this style). If it's a Claro-specific issue, the change should be made in Claro.

I performed Novice Triage on this issue. I added the Novice tag as well as NorthAmerica2021 and Easy out of the Box to increase visibility. I unassigned the issue because there hasn't been any work in a few weeks. We should leave it unassigned and use comments to mention that we are working on the issue and when we're done.

It looks like the next steps per bnjmnm's review in comment #11 is to assess the scope of the issue and apply jenniferaube's patch in Claro directly if it does not apply to all themes.

It looks like there is a merge request and a patch. We should choose one of those options and mention which is the work to review in the issue summary.

Coming back to this in DrupalCon NA 2021 contribution April 14th

Created new MR with patch attached. Added css to Claro core theme rather than ckeditor module.

Hi @jenniferaube, it looks like your patch/MR is failing because you are editing the css file directly instead of the .pcss.css file that is used for compiling the CSS. This is not particularly intuitive 🙂. Info on working with Claro CSS can be found here https://www.drupal.org/docs/8/modules/claro/development

This is a pretty simple change.

Here's a before shot:

And the after shot:

I think this is good.

Restoring status after HEAD was broken.

After patch #20, I am getting double focus on Active toolbar items.

Attaching a screenshot for reference.

The focus outline isn't missing in the entire toolbar configuration, just the "Available buttons" area. This is due to the following rule in CKEditor's ckeditor.admin.css

.ckeditor-toolbar-disabled .ckeditor-buttons li a {
border: 1px solid #a6a6a6;
    border-bottom-color: #979797;
    border-radius: 3px;
    /* THIS BOX SHADOW CANCELS CLARO'S FOCUS OUTLINE */
    box-shadow: 0 1px 0 rgb(255 255 255 / 50%), 0 0 2px rgb(255 255 255 / 15%) inset, 0 1px 0 rgb(255 255 255 / 15%) inset;
}

The issue summary should be updated to specify where the problem is happening. Ideally, the solution would apply Claro's green focus outline instead of the browser default, so it is consistent with the other outlines.

Providing updated version of patch #29 as per comment #28

+++ b/core/themes/claro/claro.libraries.yml
@@ -59,6 +59,7 @@ global-styling:
       css/theme/colors.css: {}
+      css/theme/ckeditor-editor.css: {}
     layout:

This is added to the global styling library, which loads it on every page. This only needs to be loaded on pages with CKEditor. This can be accomplished by extending the ckeditor/drupal.ckeditor library instead.

@bnjmnm I have worked on #30, I found that library(ckeditor/drupal.ckeditor) already extended, After adding css file withing the library, Its not working. However Its working fine with library(core/ckeditor). I have created the patch, Please have a look and advise.

Manual test for new patch extending library(core/ckeditor) work as intended.

Before patch:

After patch:

RTBC +1

Verified and tested patch MR !556 i.e. https://git.drupalcode.org/project/drupal/-/merge_requests/556.patch on the drupal 9.3.x-dev version and Claro 9.3.0-dev version. Patch applied successfully and looks good to me.

Testing Steps:

1. Install drupal 9.3.x-dev version with Umami site.
2. Login as admin
3. Go to Appearance -> Set Claro theme as admin and default theme
4. Go to /admin/config/content/formats/manage/restricted_html
5. Select a "Text editor", ie. CKEditor
6. Try to focus one of the toolbar items in the toolbar configuration, observe items receive focus but it's not visible
7. Now apply the patch and clear the cache.
8. After the patch verify by pressing the tab focus should visible for the toolbar items.

Testing Results:

After applying the patch by pressing the tab focus is visible for the toolbar items.
Please refer attached Before patch and After patch images for reference.

Moving this ticket to RTBC.

Switching back to needs work

• This is still tagged with "needs issue summary update". The summary still needs to specify what buttons aren't working, since some are working fine.
• #35 reviewed the merge request, which is behind the patch so the issue #30 is still present. This merge request also fails custom commands so tests don't even run. A branch that doesn't pass tests shouldn't be RTBC'd.
• #32 is close, but it's not extending the correct library. The problem stems from the contents of ckeditor.admin.css, so the solution should be an extension of the library that adds that file. ckeditor.admin.css, is loaded by the library ckeditor/drupal.ckeditor.admin (see ckeditor.libraries.yml), so that is the correct library to extend.

Providing updated version of patch #38 as per comment #37

1. +++ b/core/themes/claro/claro.info.yml
   @@ -113,7 +113,7 @@ libraries-override:
   -  ckeditor/drupal.ckeditor:
   +  ckeditor/drupal.ckeditor.admin:
   

   The existing extension of ckeditor/drupal.ckeditor shouldn't be changed.
   A new libraries-extend item should be added for ckeditor/drupal.ckeditor.admin

2. +++ b/core/themes/claro/css/theme/ckeditor-editor.pcss.css
   @@ -104,3 +104,6 @@
   +.ckeditor-toolbar-disabled .ckeditor-buttons li a:focus {
   +  box-shadow: 0 0 0 2px #fff, 0 0 0 5px #26a769;
   +}
   

   The CSS shouldn't be added to this existing file. This stylesheet is for styling the editor itself. The fix is updating the styles of the ckeditor admin form.

   This should be in a new stylesheet that is part of a new library, specifically created to extend ckeditor/drupal.ckeditor.admin. To get a better sense of what needs to go in what library/extension, check out ckeditor.libraries.yml to see which files belong to which library. Claro's extensions should maintain that structure.

It doesn't look like I have permissions to change the version the merge request is compared against, so here it is in patch form.

This loads the fix CSS only when the problem-causing CSS is loaded. It fixes the lack of focus in the available buttons row without impacting the already-working focus elsewhere.

(I wanted to mention there's a bit of unexpected time sensitivity to this issue due to CKEditor 5. Typically I wouldn't want to jump in on something I've already been reviewing when contributors haven't had much time to respond to feedback. Find me on Drupal slack if anyone wants help finding/finishing other issues 🙂)

The patch in #42 extends the ckeditor/drupal.ckeditor.admin library which is where the conflicting box-shadow is added.
That means this claro override only loads with that library.

The claro/ckeditor-admin just loads one file, css/theme/ckeditor.admin.css

which overrides core/modules/ckeditor/css/ckeditor.admin.css

.ckeditor-toolbar-disabled .ckeditor-buttons li a box-shadow by targeting the :focus
This is the solution pointed out in #28.

It's non-destructive, as opposed to just deleting that box shadow from the core ckeditor css, so any themes that expected the old box shadow won't be affected. It adds it just for the ckeditor disabled toolbar items.

Followed the steps in #35 and confirm that the library loads correctly and the focus style from Claro is no longer overridden by the ckeditor box-shadow. edit: more specifically, the override matches Claro's normal focus style for box-shadow.