CKEditor 5 has its own focus styles, should use the admin theme's (e.g. Claro's)

I bet these are CKEditor 5's styles.

I think you're effectively asking that Claro and Seven provide overrides of CKEditor 5's default styling to use the same focus styling as those admin themes?

I bet these are CKEditor 5's styles.

I just confirmed that to be the case.

I very much see the appeal in making the focus rings consistent, but overriding CKEditor 5's default styling presents some risks, especially since CKEditor 5 releases new major versions at a Javascripty pace. If they change their focus styling approach, there could be conflicts such as box shadows and outlines clashing with each other, or focus ring inconsistency within the editor. These are kind of difficult to test, so the issue could be introduced without us realizing it. At least now the focus ring appearance change is predictable (inside CK it looks one way, outside it looks another).

The contrast/visibility concerns are worth addressing, but I wonder if this is best done upstream in CKEditor 5 itself. This still means inconsistent focus rings within a theme, but it would be predictably inconsistent, and not jarring. Uniform styling is ideal, but it's also understood that there may be differences with 3rd party widgets.

Per #6, re-classifying from bug to task.

I asked the CKEditor 5 team for advice. @Reinmar escalated this question to @oleq.

I asked:

https://www.drupal.org/project/drupal/issues/3272592#comment-14472622 → is it likely that CKE5 will at some point change its focus styles? We’re contemplating overriding them to make them consistent with the theme, but we don’t want to risk breakage with every CKE5 release.

@oleq's response:

No, we don’t plan anything like that.
But if you want to override our focus styles, all you have to do is override --ck-focus-ring and --ck-focus-outer-shadow CSS properties.
(Unless you override them, find out that something is inconsistent/broken/just wrong, then yes, we’ll have to address this sooner or later 😛

So … I think that means we could consider overriding this on the Drupal side. I defer to @bnjmnm and @lauriii on this.

Well, actually, no, this was found by @rkoller at home, not at DDD Ghent. 😅 I just responded while being at DDD. So untagging. Sorry for the noise.

I agree that it could be nice to render the focus style consistently in Claro. However, I don't think this is critical because CKEditor 5 has its own design language, and we are not planning to customize all of that to match the Drupal design system.

There are some challenges with the Claro focus styles because in CKEditor 5, the focus is around the preview container, not the whole editor. We probably don't want the outline to render over the toolbar since the toolbar will need to remain usable while the preview container is focused. We also need to decide what to do about selected widgets, should those styles follow Claro too?

Either way, it would be good to play around with this a bit, and see how it would look like to make a final decision on what makes sense.

Thanks for sharing your rationale in such detail! 🙏

Per #6 and #12, this is clearly not a stable blocker then.

https://ckeditor.com/docs/ckeditor5/latest/framework/guides/deep-dive/ui... actually seems to make this trivial to implement:

    --ck-color-focus-border: #26a769;

Also see packages/ckeditor5-theme-lark/theme/ckeditor5-ui/globals/_focus.css, which contains:

:root {
	/**
	 * The geometry of the of focused element's outer shadow.
	 */
	--ck-focus-outer-shadow-geometry: 0 0 0 3px;

	/**
	 * A visual style of focused element's outer shadow.
	 */
	--ck-focus-outer-shadow: var(--ck-focus-outer-shadow-geometry) var(--ck-color-focus-outer-shadow);

	/**
	 * A visual style of focused element's outer shadow (when disabled).
	 */
	--ck-focus-disabled-outer-shadow: var(--ck-focus-outer-shadow-geometry) var(--ck-color-focus-disabled-shadow);

	/**
	 * A visual style of focused element's outer shadow (when has errors).
	 */
	--ck-focus-error-outer-shadow: var(--ck-focus-outer-shadow-geometry) var(--ck-color-focus-error-shadow);

	/**
	 * A visual style of focused element's border or outline.
	 */
	--ck-focus-ring: 1px solid var(--ck-color-focus-border);
}

So this is totally doable. At least for a specific theme. For Claro, we'd want

    --ck-color-focus-border: var(--color-focus);

AFAICT.

And … if Drupal core settles on consistently named CSS variables, that single line should be able to work for any admin theme? 🤓

@rkoller reported this problem again at #3308911: The CKEditor 5 form-element is missing an outline color change on hover. Marked it as a duplicate.

See the video he posted there: https://www.drupal.org/files/issues/2022-09-09/outline.mp4.

Yes, same root cause AFAICT, because #3308911: The CKEditor 5 form-element is missing an outline color change on hover is also referring to Claro-only styles. 😅

So CKEditor's styles. Do they meet the color contrast requirements? Is it just that they look different, or can't be easily updated? Consistency is definitely a plus for usability. What WCAG SC would it lie under?

#20: @bnjmnm is a (provisional) accessibility maintainer of Drupal core (just like you!) and he reviewed CKEditor 5's accessibility in detail, and specifically reviewed it for regressions compared to CKEditor 4. Hence also his detailed comment at #6.

On the roadmap issue (#3238333: Roadmap to CKEditor 5 stable in Drupal 9) there is a long list (23 issues!) of "accessibility" issues to be addressed prior to marking it stable, @bnjmnm identified most of those. See #3239423: Toolbar UI accessibility review: round 2 in particular.

Clarifying that the original report is tightly coupled to Claro, which explains why the parent issue is #3163810: [META] Assess conformance with WCAG SC 2.4.7 "Focus Visible".

Tagging for WCAG SC

https://www.sarasoueidan.com/blog/focus-indicators/

I hear the CKEditor 5 team is investigating this too as they encountered this problem while working on https://www.drupal.org/project/ckeditor5_premium_features 😊

Really curious to get their feedback!

Closed #3327960: CKEditor 5 focus indicator is insufficient and is completely invisible in forced colors as a duplicate. Transferred tags & credit.

How is what we implemented different than what you can see in their demo https://ckeditor.com/ckeditor-5/demo/feature-rich/

The contrast I see for the focus of the boarder is #3879EB / #FFF which is 4.1.2. That's good enough for SC 1.4.11 Non-Text Contrast (Level AA) if the visual presentation only needs a contrast ratio of 3:1 against adjacent color(s).

I didn't see a corresponding problem in their issue queue https://github.com/ckeditor/ckeditor5/issues

but the bigger issue is that i couldn't replicate this in their demo.

Related: #3306209: Add outline to buttons with active state. just landed.

This issue is about focus styles, but shouldn't be about styles, in general?

What I first noticed is that the border is brighter in WYSIWYG fields:

(also, it is not resizable, but that would likely be a separate issue)

I agree this is not critical, nor very important. It would be great if consistency could be achieved, though. What I'm not sure is what would be the right solution, considering the toolbar also has a border.

Idea A:

Idea B:

Idea C:

Happy to create a separate issue, but there may be many other CSS inconsistencies, especially if this hasn't been a goal.

Regarding WCAG 2.2, shouldn't we aim to fix this upstream? Not only to avoid adding (and maintaining) more integration code that can break, but also to benefit the CKEditor project, and all the projects that use them.

Related to the focus, I also noticed that when clicking on any blank space inside the toolbar, the green outline appears around the toolbar, partially behind the preview/editor pane:

(not sure if this effect has been reported/identified before)

Issue created: https://www.drupal.org/project/drupal/issues/3478281

Hopefully addressed in https://git.drupalcode.org/issue/drupal-3478281/-/compare/11.x...3478281....

I took a look and I don't think this is easy, it requires multiple style changes to CKEditor.

So this is totally doable. At least for a specific theme. For Claro, we'd want

--ck-color-focus-border: var(--color-focus);

This changes the color of the border from blue to green, but it is not much better, because as far as I can see, Claro implements the green outline with box-shadow.

Overriding the following selector gave me a better result:

.ck.ck-editor__editable.ck-focused:not(.ck-editor__nested-editable) {
    box-shadow: var(--focus-box-shadow);
}

But still far from perfect: the CKEditor blue focus border is still there (and did not look good if removed, it should probably be converted to be var(--input-border-color);, but even then, the green outline still would not match Claro's, mostly because the focused element only has 2 rounded corners instead of 4 (which is another non-focus style difference):

Also, we would need to replicate the hover effect (border-color: var(--input--hover-border-color); box-shadow: inset 0 0 0 var(--input-border-size)...).

And the issue I described in #32 would still happen.

I found something simpler, looking at what Gin theme is doing:

--- a/core/themes/claro/css/components/ckeditor5.pcss.css
+++ b/core/themes/claro/css/components/ckeditor5.pcss.css
@@ -1,3 +1,6 @@
 .ck {
   --ck-color-base-border: var(--input-border-color);
+  --ck-focus-ring: 1px solid var(--color-focus);
+  --ck-focus-outer-shadow: 0 0 0 2px var(--color-white), 0 0 0 5px var(--color-focus);
+  --ck-inner-shadow: var(--ck-focus-outer-shadow);
 }

This has the problems mentioned above, and a few pixels of the toolbar buttons are now below (behind) the green shadow, but I can't see how we could improve this if we apply current Claro's shadow:

Setting it to Ready for review, as I've created an MR that could be acceptable.

Replicating the hover effect could be a separate issue.

Gin Admin theme also does this to mitigate #32:

.ck-toolbar_grouping:focus,
  .ck-toolbar__items:focus {
    box-shadow: none;
  }
}

This completely hides the green line around the toolbar. Not doing it in case it can have issues for accessibility. I think it's probably OK though: I'm not sure the toolbar should be focusable at all (the toolbar can still be used by the keyboard when the editor is focused), but in that case, we may consider focusing the editor automatically when the empty space in the toolbar is clicked.

Scanning issue summary and appears to be missing a few sections.

Proposed solution
User interface change = before/after screenshots probably can be moved.

My recommendation is even if a section doesn't apply to the issue just put N/A vs removing it.

Updating issue description

Still appears to be missing proposed solution section.

Per #41