Update CKEditor to 49.1.0

This is a 12.0.0 rc blocker.

Some test failures in ckeditor5 JS tests so I guess this needs a bit of work.

The only remaining failing test is

ime: 00:07.980, Memory: 8.00 MB
    
    CKEditor5Height (Drupal\Tests\ckeditor5\FunctionalJavascript\CKEditor5Height)
     ✘ C k editor 5 height
       ┐
       ├ Failed asserting that 637 is less than 625.
       │
       │ /builds/core/modules/ckeditor5/tests/src/FunctionalJavascript/CKEditor5HeightTest.php:62
       ┴
    
    FAILURES!
    Tests: 1, Assertions: 12, Failures: 1.

Relevant test code:

    // Test the max height of the editor is less that the window height.
    $body = \str_repeat('<p>Llamas are cute.</p>', 100);
    $node = $this->drupalCreateNode([
      'body' => $body,
    ]);
    $this->drupalGet($node->toUrl('edit-form'));
    $this->assertLessThan($this->getWindowHeight(), $this->getEditorHeight());

I think this is happening because something changed CKEditor's styles, and there's a margin now above the first line. See below, with the new version test output on the right:

I haven't looked into what sets the expectation the window height should be greater than the editor height.

The other failing tests that were resolved:

• Adjust performance test script byte numbers
• Had to update tests to close the balloon that is shown on a media object in the editor before the "Source" button could be pressed
• In addition, on one test, the Source button had to pressed again to revert to rich text editing in order for the changes there to be reflected in the editor. This however seems only an issue within the test, and when I tried this manually, I did not have to switch back to rich text editing for my source changes to be retained when I saved, but someone else should probably also validate

Something I discovered in testing is that balloon tool tips now have white backgrounds, so the text can't be read. I haven't looked at what rules need to be changed to fix this yet.
HEAD

MR

Also, seems like this is a preexisting issue, but clicking in editor outside the drupal-media element does not close the balloon like it does for other elements such as table or image. This might be worth a follow up.

Going to take a break from this for now, will see if I can revisit some time over weekend. Others are welcome to jump in.

I also ran into an unrelated Media Library issue while testing, so I opened #3634443: media-library.ui.js added to page before jQuery.

All tests are green now.

tl;dr: This is ready for review now.

First, a correction:

Also, seems like this is a preexisting issue, but clicking in editor outside the drupal-media element does not close the balloon like it does for other elements such as table or image. This might be worth a follow up.

While the balloon does close for the table element, it does not close for image.

About CKEditor5HeightTest:
Traced the origin to #3273755: CKEditor 5 should not grow to infinite height:

  /* Set the max-height to not grow beyond the height of the viewport (minus
   * any toolbars. */
  max-height: calc(100vh - var(--drupal-displace-offset-top, 0px) - var(--drupal-displace-offset-bottom, 0px) - 20px);

Since the test was failing after the update to v49 with the editor height being 637 and the window height being 625, I changed the 20px in the CSS rule to 35px to cover the difference. Note that in CKEditor5HeightTest.php, there are comments documenting expected pixel dimensions for the height. These might be out of date now, but I think we can adjust them in a follow up if need be.

About the tooltips.
Tracked this to a combination of things.
Previously in v48.5.1, the relevant CSS rules were:
https://github.com/ckeditor/ckeditor5/blob/1867f7a292a98e5740873b5f42f20...

.ck.ck-balloon-panel {
  background: var(--ck-color-panel-background);
}

and
https://github.com/ckeditor/ckeditor5/blob/1867f7a292a98e5740873b5f42f20...

.ck.ck-balloon-panel.ck-tooltip {
  --ck-color-panel-background: var(--ck-color-tooltip-background);
}

In 49.0.1, the relevant rules look like this:
https://github.com/ckeditor/ckeditor5/blob/b8cd0da471427176f4cb2c79be503...

.ck.ck-balloon-panel {
  background: var(--ck-color-panel-background, var(--ck-balloon-panel-background-color));
}

and this
https://github.com/ckeditor/ckeditor5/blob/b8cd0da471427176f4cb2c79be503...

.ck.ck-balloon-panel.ck-tooltip {
  --ck-balloon-panel-background-color: var(--ck-color-tooltip-background, var(--ck-tooltip-background-color));
}

The intent is that --ck-balloon-panel-background-color property should set the tooltip balloon bg color, but the use of --ck-color-panel-background is preserved for backwards compatibility.

Because of that BC layer, the rules in default_admin's ckeditor5.pcss.css and variables-color-semantic.pcss.css now set the background for the tooltip to white when the theme is in light mode.

.ck {
   --ck-color-panel-background: var(--admin-color-bg-raised);
}

  --admin-color-bg-raised: light-dark(var(--admin-color-white), #47474c);

With v48.5.1, the default_admin theme .ck override for --ck-color-panel-background was superseded by the value being set in a more specific selector .ck.ck-balloon-panel.ck-tooltip, but the latter was removed in v49.

To fix this, I added a css rule in default_admin to remove the BC layer:

.ck.ck-balloon-panel {
  background: var(--ck-balloon-panel-background-color);
}

Here are screenshots of what the tooltips with v49 looked like before and after the above CSS change:
Before
Light

Dark

After
Light

Dark

I think this is good enough to address the regression here. There can be a follow up if desired to made the tooltip colors respect light/dark mode, and/or to check whether --ck-color-panel-background should be used/overridden anymore in default_admin.

Created the follow up: #3634547: [PP-1] Make CKEditor balloon tooltips respect Admin light or dark mode

Bumping priority since this is a blocker.

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

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

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

Merge conflict was in the performance test numbers and has been resolved.

Okay let me know if there are steps I'm missing.

Applied the MR on a ddev setup
Did a git add core, so I have everything in the MR staged
ddev ssh and cd into core (following the steps from https://www.drupal.org/about/core/policies/core-dependency-policies-and-...)
Ran yarn run vendor-update = no change
Ran yarn run build:ckeditor5 = no change
Ran yarn run build:ckeditor5-types = which made core/modules/ckeditor5/js/build/ckeditor5.types.jsdoc not sure if that's good or bad?

Searched core/core.libraries.yml and found 1 instance that still needs to be replaced and that's the parent ckeditor5 which still says 48.5.1

For the manual testing never know exactly what to target so I kinda just test the main buttons and of course media library. But question, looking at the MR I see some changes were needed for the tests. Should we do a CR or something as this may be another ckeditor update that breaks contrib modules?

Searched core/core.libraries.yml and found 1 instance that still needs to be replaced and that's the parent ckeditor5 which still says 48.5.1

Not seeing this in the MR.

core/modules/ckeditor5/js/build/ckeditor5.types.jsdoc

Yes, changes are expected and are in the MR.

Should we do a CR or something as this may be another ckeditor update that breaks contrib modules?

No, I don't think so. Changes are specific to these tests. See #10 and #11.

It's not in the MR but I still see this locally with the MR applied

Not sure what happened on your local, but the change to 49.0.1 in that line is in the MR.
https://git.drupalcode.org/project/drupal/-/merge_requests/17633/diffs?f...

I just double-checked my local version of the branch, and 48.5.1 does not appear anywhere

I also switched local branch to main and applied the MR diff downloaded from Gitlab, and the change is applied:

@smustgrave, re-reading #23, I think you might need to do yarn install first, so that your node_modules are updated.

Weird maybe curl didn't pull the full diff down. I switched to the branch though and it's all good. Did the same steps #23 so going to mark.

Updated the MR to 49.1.0.

Reviewed the new release notes and seems to be around AI improvements. Which makes me concerned about the future of ckeditor and how much AI is taking over it.

Committed and pushed acfffb4307a to main and db863809e63 to 12.0.x. Thanks!