Get CKEditor 5 to work in (modal) dialogs

Does CKEditor 4 work correctly in (modal) dialogs?

Thanks for confirming!

Then this is a regression and hence a new stable blocker.

Haven't looked at the code but from the description it's probably related to tabbingManager

So what is happening is that the CKEditor tooltip is added outside the modal pane (makes sense to prevent issues with overflow hidden and that kind of things).

But jQuery UI dialog checks for focusable elements within the dialog only, since the new tooltip is not part of the dialog tree the focus is sent back to the ckeditor area.
The relevant code that manage the focus: https://git.drupalcode.org/project/drupal/-/blob/10.0.x/core/misc/dialog... (which overrides the original method) and the focus trap is created here: https://git.drupalcode.org/project/drupal/-/blob/10.0.x/core/assets/vend...

Here is a patch that makes it work by breaking that whole focus trap feature, added the CSS wherever and the patch from #2741187-35: Allow usage of WYSIWYG in views text area fields to make it easier to test by opening a views UI dialog with a textarea. Tabbable nightwatch tests are passing so it's interesting.

As for the resolution, not quite sure how to handle it yet.

Wow, nice investigation! I can’t believe your hunch you had within seconds of me explaining it yesterday was spot-on! 😄👏

It wasn't tabbingmanager though 😂

1. +++ b/core/misc/dialog/dialog.jquery-ui.es6.js
   @@ -69,7 +69,6 @@
   -      $(hasFocus).eq(0).trigger('focus');
   
   +++ b/core/misc/dialog/dialog.jquery-ui.js
   @@ -71,8 +71,6 @@
   -      $(hasFocus).eq(0).trigger('focus');
   

   This seems to have consequences?

   Testing Drupal\Tests\system\FunctionalJavascript\ModalRendererTest
   F                                                                   1 / 1 (100%)
   
   Time: 00:16.598, Memory: 4.00 MB
   
   There was 1 failure:
   
   1) Drupal\Tests\system\FunctionalJavascript\ModalRendererTest::testModalRenderer
   JavaScript condition met:
   document.activeElement === document.querySelector(".ui-dialog .ui-dialog-titlebar-close")
   Failed asserting that false is true.
   

2. +++ b/core/misc/normalize-fixes.css
   @@ -38,3 +38,8 @@ input[type="search"]::-webkit-search-cancel-button {
   +.ck-body-wrapper {
   +  position: fixed;
   +  z-index: 1300;
   +}
   

   Needs comments to explain why 🤓

3. +++ b/core/modules/views/src/Plugin/views/area/Text.php
   @@ -39,7 +39,7 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
   -      '#editor' => FALSE,
   +      '#editor' => TRUE,
   

   Wow, this … means that this didn't have a text editor at all before?!

   That's the hunch/vague memory I had in #2.

   Looks like this goes back all the way to Views getting moved into core…

`position: fixed;`
caused the ck5 table tools to be miss aligned. I just changed the `--ck-z-modal` property which works nicely for me.

Looks like this could fix it. not too pretty but functional.

+++ b/core/modules/ckeditor5/ckeditor5.module
@@ -528,6 +531,14 @@ function ckeditor5_library_info_alter(&$libraries, $extension) {
+    //$javascript[$dialog_file_fix]['weight'] += $javascript[$dialog_file]['weight'];

I suspect this should not have been commented out? 🤓😅

indeed :)

trying again

all right, let's not loose faith :D

1. 🤔 I see that this is tagged Needs tests — that's something we should still do. If it's possible, that is. But I think it is?

2. +++ b/core/modules/ckeditor5/js/ckeditor5.dialog.fix.es6.js
   @@ -0,0 +1,21 @@
   +    // Override core override of jQuery UI's `_focusTabbable()` so that ckeditor
   

   🤓 Nit: s/ckeditor/CKEditor 5/

3. +++ b/core/modules/ckeditor5/js/ckeditor5.dialog.fix.es6.js
   @@ -0,0 +1,21 @@
   +      // When the focused element is a CKEditor 5 editor, do not change the
   ...
   +      // In case the element is a CKEditor 5 Instance, do not change focus
   

   🤓 Nit: Inconsistent language: "CKEditor 5 editor" vs "CKEditor 5 Instance"

4. +++ b/core/modules/views/src/Plugin/views/area/Text.php
   @@ -39,7 +39,7 @@ public function buildOptionsForm(&$form, FormStateInterface $form_state) {
   -      '#editor' => FALSE,
   +      '#editor' => TRUE,
   

   IMHO we should remove this change.

   Because this changes behavior not just for CKEditor 5, but also CKEditor 4.

will get to it tomorrow, at least i didn't break anything else :)

fixed a few suggestions, still needs tests.

#3065095: CKEditor native dialogs not clickable inside of jQuery UI dialogs sounds suspiciously similar but is for CKEditor 4 😅

failing test

Queued test run 😊

1. +++ b/core/modules/ckeditor5/tests/modules/ckeditor5_test/src/Controller/Ckeditor5DialogTestController.php
   @@ -0,0 +1,40 @@
   +  public function testDialog() {
   +    $build['link'] = [
   +      '#type' => 'link',
   +      '#title' => 'Add Node',
   +      '#url' => Url::fromRoute('node.add', ['node_type' => 'page']),
   

   No French jokes here?!? 😜 So much 🍷 or 🥖 opportunity!

2. +++ b/core/modules/ckeditor5/tests/src/FunctionalJavascript/CKEditor5DialogTest.php
   @@ -0,0 +1,73 @@
   +   * Tests if CKEditor 5 tooltips can be interacted with in dialogs.
   +   */
   +  public function testCKEditor5TooltipsInDialog() {
   

   🤔 Tool tips? I think you meant inputs?

3. +++ b/core/modules/ckeditor5/tests/src/FunctionalJavascript/CKEditor5DialogTest.php
   @@ -0,0 +1,73 @@
   +    $this->pressEditorButton('Link');
   +    $input = $assert_session->waitForElementVisible('css', '.ck-balloon-panel .ck-input-text');
   +    // Make sure the input field can have focus and we can type into it.
   +    $page->fillField($input->getAttribute('id'), '/ckeditor5_test/dialog');
   

   Seems pretty clear! 👍

I don't know what's going on, when I run the test, the extra library is not loaded so test fails :/ when I enable the page and browse it with umami theme, things work as expected.

1. i'll joke when that thing works :p
2. it's the focus inside tooltips inside dialogs that we're testing, renamed the method :)
3. i wish I could confirm that it works :p

Went with the approach in the related CKE4 issue for initialization of the fix, better than messing with file weights.

Funny thing I noticed is that the offcanvas didn't have focus issues, don't know what's different.

offcanvas didn't have focus issues, don't know what's different.

off-canvas has a CSS reset: misc/dialog/off-canvas.reset.css: {}.

… and it suspiciously also has this:

      # Add group setting to make sure this CSS load before any jQuery UI Dialog
      # CSS.
      misc/dialog/off-canvas.theme.css: { group: 200 }

🤔🤔🤔🤔

test still broken, still works when loaded outside a test. Do not know why the library alter is not taken into account during the tests.

no interdiff, changed a bunch of stuff, in how i added the js file fix so I'll start making interdiffs when things are working somewhat.

finally found why it didn't work in tests. A case of self own #3267204: library_info_alter can abort early if locale is not enabled.

Here is a failing patch (I'm just returning early in the JS file, that's why both patches have the same size).

For some reasons $content_area->getText() === $link_url returns false even if both strings look the same… so just checking it's not empty.

OMG!!!!!!!! Can't believe #3267204: library_info_alter can abort early if locale is not enabled struck again!

The failing patch's early return that you so cleverly added is being caught by babel sadly:

/var/www/html/core/modules/ckeditor5/js/ckeditor5.dialog.fix.es6.js
  11:3  error  Unreachable code  no-unreachable

✖ 1 problem (1 error, 0 warnings)

… so could you upload a true test-only patch? 🤞 😄

1. +++ b/core/modules/ckeditor5/js/ckeditor5.dialog.fix.es6.js
   @@ -0,0 +1,28 @@
   +      // In case the element is a CKEditor 5 Instance, do not change focus
   

   s/Instance/instance/

2. +++ b/core/modules/ckeditor5/tests/modules/ckeditor5_test/src/Controller/Ckeditor5DialogTestController.php
   @@ -0,0 +1,40 @@
   +class Ckeditor5DialogTestController {
   

   Übernit: s/Ckeditor/CKEditor/

3. +++ b/core/modules/ckeditor5/tests/src/FunctionalJavascript/CKEditor5DialogTest.php
   @@ -0,0 +1,84 @@
   + * Tests for CKEditor5 to ensure correct styling in off-canvas.
   

   Übernit: s/CKEditor5/CKEditor 5/

   Less über nit: it's not checking styling nor styling in off-canvas. This description needs to be updated 🤓

Failing patch, making a new one with doc fixes.

comments from #34 fixed

1. --- a/core/misc/normalize-fixes.css
   +++ b/core/misc/normalize-fixes.css
   
   @@ -20,6 +20,10 @@ input[type="search"]::-webkit-search-cancel-button {
   +.ui-dialog ~ .ck-body-wrapper {
   +  --ck-z-modal: 1261;
   +}
   

   Why is this in normalize-fixes.css? This is supposed to be a very low level CSS that simply addresses regressions caused by normalize.css. I assume that you wanted to load the variable as part of the first CSS files being loaded, but wouldn't there be a way to do that with the libraries system from CKEditor 5 module?

2. +++ b/core/modules/ckeditor5/ckeditor5.module
   @@ -440,6 +440,13 @@ function ckeditor5_library_info_alter(&$libraries, $extension) {
   +    // Fix the CKEditor 5 focus management in dialogs, we need this file to be
   +    // loaded after drupal.dialog.jquery-ui.js, changing the library declaration
   +    // is the most reliable way to do this.
   

   I would reword this as:

   Fix the CKEditor 5 focus management in dialogs. Modify the library declaration to ensure this file is always loaded after drupal.dialog.jquery-ui.js.
   

3. +++ b/core/modules/ckeditor5/js/ckeditor5.dialog.fix.es6.js
   @@ -0,0 +1,28 @@
   + * This file is directly added to the core/drupal.dialog library.
   

   Instead of describing how this file is loaded, I would include a high level explanation of the file. This could potentially be an additional comment but I'm not sure it's even necessary or preferred since if we change how this file is loaded, it would be unlikely that we would remember to update this. It's also relatively straight forward to search how a file is being loaded.

4. +++ b/core/modules/ckeditor5/js/ckeditor5.dialog.fix.es6.js
   @@ -0,0 +1,28 @@
   + * @todo remove in https://www.drupal.org/project/drupal/issues/2158943.
   

   Are we sure we can remove this in https://www.drupal.org/project/drupal/issues/2158943? We might have to override the modal dialog focus trap of the new implementation too. I would just omit this.

5. +++ b/core/modules/ckeditor5/js/ckeditor5.dialog.fix.es6.js
   @@ -0,0 +1,28 @@
   +    // Override core override of jQuery UI's `_focusTabbable()` so that
   +    // CKEditor 5 in modals can work as expected.
   ...
   +      // When the focused element is a CKEditor 5 instance, do not change the
   +      // focus management.
   

   I think this comment leaves some room for confusion. To me this comment implies that we would fallback to the jQuery UI default _focusTabbable implementation which isn't the case. What happens here is we essentially disable all jQuery UI focus trapping when inside CKEditor 5 since it implements its own focus trap when necessary.

1. created a new css file loaded alongside the js fix file.
2. updated docs
3. updated docs
4. removed
5. updated docs

Hopefully it's a bit clearer.

1. +++ b/core/modules/ckeditor5/ckeditor5.module
   @@ -441,10 +441,11 @@
   +    $libraries['drupal.dialog']['css']['component']['modules/ckeditor5/css/ckeditor5.dialog.fix.css'] = [];
   

   🤔 This location suggests the comment above applies to this too. But I don't think for the CSS the order matters at all? I think that could be documented in a comment too.

2. +++ b/core/modules/ckeditor5/js/ckeditor5.dialog.fix.es6.js
   @@ -1,8 +1,9 @@
   + * This file overrides the way jquery-ui focus trap works.
   

   s/jquery-ui/jQuery UI/

3. +++ b/core/modules/ckeditor5/js/ckeditor5.dialog.fix.es6.js
   @@ -1,8 +1,9 @@
   + * When a focus event is fired while a CKEditor 5 instance is focused, do not
   + * trap the focus and let CKEditor 5 manage that focus.
   

   👏 This is way clearer!

Thanks!

Committed 6d6ab58 and pushed to 10.0.x. Thanks!

We still need a 9.5.x patch for this.

9.5.x patch

so we get the deprecation because we load a node edit form in the dialog for the test. This node edit form has a date field that loads modernizr. Not sure how the rest of the tests pass since that date library is also deprecated 🤔

it's the collapse script that relies on core/modernizr and the deprecation notices is triggered by the fact that the replacement script is not used on the collapse library. Adding it to the library fixes the deprecation notice

+++ b/core/core.libraries.yml
@@ -491,6 +491,7 @@ drupal.collapse:
+    - core/drupal.touchevents-test

This is the only change compared to #43 👍

Looking at #3101922: Find replacement for Modernizr touchevent test and deprecate now from the angle of this issue, I'm wondering if we should remove the deprecation warning from the Modernizr.addTest since it doesn't make sense to trigger that for all uses of Modernizr, including those that don't require touchevents. For example, here we are adding dependency to the touchevents in a place where that isn't stricly required. Maybe we could open a follow-up and add a todo to remove core/drupal.touchevents-test as a dependency of core/drupal.collapse once we no longer trigger deprecations there?

I'd rather not have the dependency at all to fix the extra deprecation message. we can add the library in the test to work around that without changing the core library definition, that seem to work :) added a todo linked to #3269082: Remove HTML5 details collapse polyfill in the test controller.

DrupalCI failed.

I dislike the compile step :p

Patch doesn't apply any more since 17 Jun 2022 at 18:02 CEST.

Rerolled patch #55, please review it.

Unhappy TestBot

CKEditor 5 v35.0.1 breaks the test (and maybe the functionality) added in this issue. The committed test is being marked skipped on 10.1.x and 10.0.x for that reason. I filed #3301631: Regression with CKEditor 35.0.1 and modal dialogs to address that.

Patch from #61 + patch from #3301631-8: Regression with CKEditor 35.0.1 and modal dialogs to fix the regression

Committed 8969faf and pushed to 9.5.x and cherry-picked to 9.4.x. because CKEditor 5 is experimental. Thanks!

Maybe this is a problem with https://www.drupal.org/project/layout_paragraphs but this is still broken for me on Core 9.5.0-RC2

core-9.5-rc2--layout-paragraphs.mp4

After learning about ~ when using layout paragraphs my dom elements are not in the right order, so the css fixe does not apply. My .ck-body-wrapper comes before ui-dialog and the CSS requires it in that exact order https://www.w3schools.com/cssref/tryit.php?filename=trycss3_gen_sibling

.ui-dialog ~ .ck-body-wrapper {
  --ck-z-modal: 1261;
}

Not sure why the ~ selector is required, but for me this works too.

.ck-body-wrapper .ck {
  --ck-z-modal: 1261;
}
.ui-dialog ~ .ck-body-wrapper {
  --ck-z-modal: 1261;
}

Thank you @NicholasS your comment#69 works well with the properties table tooltip with modal Bootstrap 4

#69 Solved table tooltip issue for me too!

patch from #63 does nothing in 9.5 when the text field is within a layout_paragraphs.

I'm having this same problem using Layout Paragraphs. The text editor dialog box has a higher z-index than the CKEditor 5 balloon when I click on the "link" chain icon. The CSS suggestion in #69 works when applied to my Admin theme. I am weary of applying the patch since it has so many code changes. Is the patch the recommended way to go? Anyone know if the CK5 team is working on this for a future release?

It looks like several people are still having problems! I recommend creating a new issue documenting this and include steps to reproduce. It's incredibly rare that additional changes occur in an already closed issues, and on the occasions it happens it is within a few days of the issue being closed. If anyone's interested in seeing the issue addressed, a new issue should be created, it won't be on anyone's radar otherwise.

I see this problem in Layout Paragraphs when the Gin admin theme in version 8.x-3.0-rc1 is used. It sets the --ck-z-modal to 98 for all ck classes. Updating to 8.x-3.0-rc2 fixes the issue.

Per #75 this was fixed by #3331304: ckeditor5 balloon area not shown in modal for gin.

The patch at https://www.drupal.org/project/drupal/issues/3328425 works.

Thanks to @NicholasS,
patch in #69 works for me with Layout Paragraph.