Problem/Motivation

The ckeditor_stylesheets property in a theme's info.yml makes it possible to CKEditor contents with CSS from the front-end theme. More info on ckeditor_stylesheets use here.

This also needs to be supported in the CKEditor 5 module, but accounting for the fact that it can't be a 1:1 equivalent due to CKEditor 5 not being style fenced by an <iframe>).

Steps to reproduce

Proposed resolution

  • Add a new ckeditor5-stylesheets theme info property for adding default theme CSS files to CKEditor 5 editor instances. It's provided as a new property as CKEditor 5 is not encapsulated in an <iframe> and functions differently as a result.
  • ckeditor_stylesheets loads CSS that site editors rely on for their editing experience. This can range from minor UI improvements to styles essential to representing the content they are editing. Because ckeditor5-stylesheets is an entirely different property from ckeditor_stylesheets, switching to CKEditor 5 (be it an optional switch in Drupal 9 or as part of a Drupal 10 upgrade) will remove these styles entirely. This can result in significant UI regressions. If there is ckeditor_stylesheets config without corresponding ckeditor5-stylesheets config (even if ckeditor5-stylesheets is just opt-out by setting it to FALSE). This is a potentially significant regression that may not get the attention it deserves if only present in a change record. The extensive validation process when switching from CKeditor to CKeditor 5 may also lead to the impression that all considerations are accounted for despite the CSS not being part of that process. There should be additional means of preventing this regression. Two options come to mind:
    • add a warning in the text format edit form and non-public facing CKEditor5 instances to alert people to the fact that CKEditor 4 has custom styling configured that is not applied to CKEditor 5. The text format warning is present to alert site admins and will contain more information. The CKEditor 5 instance warning is present to alert content editors as to why their UI changed, and the information needed to report the issue (this is particularly needed for scenarios where the site admin is not in-house)
    • Somehow making this part of the deprecation check process.

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#76 9-10-differences.txt680 bytesWim Leers
#76 3194084-76-D10.patch33.73 KBWim Leers
#76 3194084-76-D9.patch33.72 KBWim Leers
#65 Screen Shot 2022-02-11 at 11.24.08 AM.png139.35 KBbnjmnm
#65 interdiff_63-65.txt12.01 KBbnjmnm
#65 3194084-65.patch30.4 KBbnjmnm
#63 interdiff_59-63.txt2.47 KBbnjmnm
#63 3194084-63.patch29.98 KBbnjmnm
#59 3194084-59.patch30.15 KBbnjmnm
#59 Screen Shot 2022-02-10 at 3.08.14 PM.png187.46 KBbnjmnm
#51 3194084-50.patch19.94 KBhooroomoo
#49 interdiff_47-49.txt8 KBbnjmnm
#49 3194084-49.patch31.67 KBbnjmnm
#47 interdiff_39-47.txt17.03 KBbnjmnm
#47 3194084-47.patch27.53 KBbnjmnm
#39 interdiff.txt4.44 KBlauriii
#39 3194084-39.patch11.4 KBlauriii
#36 interdiff.txt3.84 KBlauriii
#36 3194084-36.patch11.88 KBlauriii
#33 3194084-33.patch11.8 KBlauriii
#22 3194084-22.patch22.16 KBbnjmnm
#20 3194084-20.patch25.12 KBbnjmnm
#3 3194084-3.patch812 bytesPooja Ganjage

Issue fork drupal-3194084

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:

Issue fork ckeditor5-3194084

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
Support from Acquia helps fund testing for Drupal Acquia logo

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

From what I can tell by reading the CKEditor 5 docs, adding editor-only stylesheets is now part of the build process, not something easily done with a prebuild library loading

My first thought is to dynamically load versions of the stylesheet styles with a .ck-content prefix, triggered by CKEditor's ready event similar to how I did it for CKEditor styles in off-canvas dialogs.
core/modules/ckeditor/js/ckeditor.off-canvas-css-reset.es6.js

Perhaps there's a more elegant way? At the very least, this approach is familiar to me.

Pooja Ganjage’s picture

Pooja Ganjage CreditAttribution: Pooja Ganjage at Asentech LLC commented
FileSize
3194084-3.patch812 bytes

Hi,

Creating a patch for this issue.

Please review the patch.

Thanks.

Pooja Ganjage’s picture

Pooja Ganjage CreditAttribution: Pooja Ganjage at Asentech LLC commented
Status: Active » Needs review
bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
CreditAttribution: bnjmnm at Acquia commented
Status: Needs review » Active

A few things with #3

  • This was assigned to me and comment #2 points out that we haven't decided on a solution
  • @@ -5,3 +5,13 @@ package: CKEditor5
 core_version_requirement: ^9
 dependencies:
   - drupal:editor
+ckeditor_stylesheets:
+  - js/build/ckeditor5-dll.js: { preprocess: false, minified: true }
+  - js/build/editor-classic.js: { preprocess: false, minified: true }
+  - js/build/essentials.js: { preprocess: false, minified: true }
+  - js/build/heading.js: { preprocess: false, minified: true }
+  - js/build/basic-styles.js: { preprocess: false, minified: true }
+  - js/build/block-quote.js: { preprocess: false, minified: true }
+  - js/build/link.js: { preprocess: false, minified: true }
+  - js/build/list.js: { preprocess: false, minified: true }
+  - js/ckeditor5.js: {}

    I'm not sure what this patch was intended to do, but there are a few reasons it's not applicable to this issue

    • ckeditor_stylesheets is a property used by themes, not modules. When a front end theme specifies ckeditor_stylesheets, the styles in those sheets should also be present in CKEditor. This is how it currently works in CKEditor4 and should continue to work this way in 5. For example, the CSS files speficied in Bartik's ckeditor_stylesheets should have their styles applied to CKEditor when Bartik is the front end theme.
    • ckeditor_stylesheets is for loading css files. These are all Javascript files.
    • The loading of these JS files is already defined in ckeditor5.libraries.yml

If you'd like to continue with the issue, feel free to weigh in on the feasability of the solution proposed in #2. Additional patches can start from scratch, no need to diff from #3

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Version: » 1.0.x-dev

My first thought is to dynamically load versions of the stylesheet styles with a .ck-content prefix […]
Perhaps there's a more elegant way? At the very least, this approach is familiar to me.

This seems like the only viable approach to me too 🤓

I think a rough prototype should prove the viability of this. bartik.info.yml includes:

ckeditor_stylesheets:
  - css/base/elements.css
  - css/components/captions.css
  - css/components/table.css
  - css/components/text-formatted.css
  - css/classy/components/media-embed-error.css

That should at least show up for the <a> tag for example.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

A caveat: some tags used to make sense in the CKEditor 4 era, but won't in the CKEditor 5 era.

  1. With CKEditor 4, only <iframe> instances needed this, not contentEditable ones (because then the live CSS already applies). I think with CKEditor 5, we need it always. Needs to be confirmed.
  2. With CKEditor 4, the <iframe> instances necessarily also have <html> and <body> tags. You can see this in core/themes/bartik/css/base/elements.css for example:
    html {
  height: 100%;
}
body {
  min-height: 100%;
  word-wrap: break-word;
  font-family: Georgia, "Times New Roman", Times, serif;
  font-size: 87.5%;
  line-height: 1.5;
}

    How to make this work in both CKEditor 4 and 5?

    I propose:

    1. Ignore html
    2. Map body to .ck-content
Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Title: Support ckeditor_stylesheets setting in theme info » Support ckeditor_stylesheets setting in theme info, allow CSS to work for both CKEditor 4 and 5
Related issues: +#1890502: WYSIWYG: Add CKEditor module to core

I was going to link to the core issue that added ckeditor_stylesheets to the Drupal 8 core ckeditor module but apparently it already was included in the very first commit: #1890502: WYSIWYG: Add CKEditor module to core.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Related issues: +#3205565: Add language of parts plugin

FYI: #3205565: Add language of parts plugin is the first to port in-CKEditor-instance styling from Drupal 8/CKEditor 4. it confirms that #7 is possible and AFAICT confirms @bnjmnm's proposal in #2 👍.

bnjmnm opened merge request !52

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
CreditAttribution: bnjmnm at Acquia commented
Status: Active » Needs review

I got this working. Also made it evidient that the off-canvas CSS solution does not account for media queries, but it works here so shouldn't be difficult to apply that solution to it as well.

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
CreditAttribution: bnjmnm at Acquia commented

Note that The approach in the merge request may be kind of resource heavy, but the generated CSS won’t change all
That often so it’s probably worth finding a way to cache this with local storage or something similar.

edited to add: with the current approach, I'm not sure how to cache without potentially serving stale CSS as the CSS paths do not include cache busting args. This may not be that noticeable a hit based on some crude timing tests. On my local install, the CKEditor instance takes ~30ms to initialize. this added CSS takes ~1.5ms. This will obviously differ depending on system, but perhaps it's not enough of an increase to the existing initialization time to be a concern?

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
CreditAttribution: bnjmnm at Acquia commented
Assigned: bnjmnm » Unassigned
Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Needs review » Needs work
  1. There's still unaddressed feedback from @lauriii at https://git.drupalcode.org/project/ckeditor5/-/merge_requests/52/diffs#n... and https://git.drupalcode.org/project/ckeditor5/-/merge_requests/52/diffs#n....
  2. CKEditor 4 explicitly supported external stylesheets: \Drupal\Tests\ckeditor\Kernel\CKEditorTest::testExternalStylesheets(). Is that expected to still work? Yes, holy crap! 🤯
  3. with the current approach, I'm not sure how to cache without potentially serving stale CSS as the CSS paths do not include cache busting args. This may not be that noticeable a hit based on some crude timing tests. On my local install, the CKEditor instance takes ~30ms to initialize. this added CSS takes ~1.5ms. This will obviously differ depending on system, but perhaps it's not enough of an increase to the existing initialization time to be a concern?

    It is thanks to this magic that my previous point was a non-concern!

    I think the best/most elegant solution here is to:

    1. cache in localStorage
    2. use the cache only if the CSS file that needs to be transformed actually was retrieved from the browser cache, by doing something like
      performance.getEntriesByName('https://example.com/example.css')[0].transferSize === 0
Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Issue tags: +stable blocker

This looks pretty far long already, which is very encouraging 👍

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

Note: classy does this:

ckeditor_stylesheets:
  - css/components/media-embed-error.css

\Drupal\Tests\media\FunctionalJavascript\CKEditorIntegrationTest::testErrorMessages() is specifically asserting the presence of that CSS:

$assert_session->responseContains('classy/css/components/media-embed-error.css');

This needed to be commented out in #3206522: Add FunctionalJavascript test coverage for media library — it has a @todo pointing here.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Project: CKEditor 5 » Drupal core
Version: 1.0.x-dev » 9.3.x-dev
Component: Code » ckeditor5.module
Assigned: Unassigned » bnjmnm
Priority: Normal » Major
Issue tags: +CSS, +JavaScript

We'll need to redo the MR from scratch as we move from the ckeditor5 project to the drupal project.

But … since the current MR is already dating back to June, it pretty much would need a rewrite anyway. Hence: almost no extra work necessary 🤓

Re-assigning to @bnjmnm because he knows this by far the best!

Wim Leers closed merge request !52

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
CreditAttribution: bnjmnm at Acquia commented
Status: Needs work » Needs review
FileSize
3194084-20.patch25.12 KB

Gitlab wasn't fully cooperating with the switch from contrib to core, but I wanted to get my progress on the issue before I took the time to figure it out. Here are the changes in classic patch form.

Status: Needs review » Needs work

The last submitted patch, 20: 3194084-20.patch, failed testing. View results

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
CreditAttribution: bnjmnm at Acquia commented
Status: Needs work » Needs review
FileSize
3194084-22.patch22.16 KB

The test failures in #20 were due to accidentally deleted files. They're brought back here. No interdiff since the only change is not deleting a directory in HEAD that shouldn't have been deleted.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Needs review » Needs work 
  1. +++ b/core/modules/ckeditor5/ckeditor5.module
@@ -364,6 +372,47 @@ function ckeditor5_library_info_alter(&$libraries, $extension) {
+ * Retrieves the default theme's CKEditor stylesheets.

    🤔 Should we specify that this is for both CKEditor 4 and 5?

    Or … should we change the theme *.info.yml property to ckeditor5_stylesheets instead?

    The CSS will have to be different, so that does feel appropriate. 

  2. +++ b/core/modules/ckeditor5/tests/modules/ckeditor5_test/ckeditor5_test.libraries.yml
@@ -7,4 +7,4 @@ layercake:
-    - core/ckeditor5
+    - ckeditor5/ckeditor5

    🐛 This library does not exist? core/ckeditor5 was correct. 

  3. +++ b/core/modules/ckeditor5/tests/src/FunctionalJavascript/AddedStylesheetsTest.php
@@ -0,0 +1,103 @@
+ * @group ckeditor

    🐛 s/ckeditor/ckeditor5/ 

  4. +++ b/core/modules/ckeditor5/tests/src/FunctionalJavascript/AddedStylesheetsTest.php
@@ -0,0 +1,103 @@
+  public function testCkeditorStylesheets($default_theme, $expected_css) {
...
+  public function providerTestCkeditorStylesheets() {

    Nit: we used fooCKEditorBar everywhere else, not fooCkeditorBar — because it is a given name/abbreviation. 

  5. +++ b/core/modules/ckeditor5/tests/src/FunctionalJavascript/AddedStylesheetsTest.php
@@ -0,0 +1,103 @@
+      // This includes confirming that nested and other uses of at-rules work.
+      // This which may not be immediately apparent if looking at the CSS on a
+      // single line, so here are the prettified expected styles:
+      /*
+        .ck-editor .a-selector-for-testing,
+        .ck-editor .a-second-selector-for-testing {
+          font-size: 800px;
+        }
+
+        @media (min-width: 31.25rem) {
+          .ck-editor #cool-id {
+            background: rgb(10, 110, 180);
+          }
+          @supports (display: flex) {
+            .ck-editor #nice-flex {
+              color: red;
+            }
+          }
+        }
+
+        @supports not (display: grid) {
+          .ck-editor div#no-grid {
+            float: right;
+          }
+        }
+       */
+      'test_ckeditor5_stylesheets_relative' => [
+        'test_ckeditor5_stylesheets_relative',
+        '.ck-editor .a-selector-for-testing, .ck-editor .a-second-selector-for-testing { font-size: 800px; } @media (min-width: 31.25rem) { .ck-editor #cool-id { background: rgb(10, 110, 180); } @supports (display: flex) { .ck-editor #nice-flex { color: red; } }  } @supports not (display: grid) { .ck-editor div#no-grid { float: right; } }',
+      ],

    Why not use https://www.php.net/manual/en/language.types.string.php#language.types.s... or https://www.php.net/manual/en/language.types.string.php#language.types.s... syntax instead? Then we would not have to repeat this in a comment! 🤓

    (And then we could use it for the other test cases too!)

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented

Or … should we change the theme *.info.yml property to ckeditor5_stylesheets instead?

The CSS will have to be different, so that does feel appropriate.

It doesn't necessarily have to be different because the CSS could be really generic, like changing the font-family for native HTML elements. However, since we can't guarantee that the CSS will work same as it did with CKEditor 4, it might be safer to deprecate ckeditor_stylesheets and replace it with ckeditor5_stylesheets.

I'm wondering if that could potentially allow us to also remove the assumption that the CSS is only applied for the CKEditor instance. We could make it the themers responsibility to appropriately scope their CSS for CKEditor. That would be less complex to implement and less complex for themers to debug.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

like changing the font-family for native HTML elements

That's a good example. But it's still crucially different CSS:

  • In CKEditor 4, it'd have been loaded into an <iframe> and therefore your CSS selector for that could've been * { font: …; }
  • In CKEditor 5, the CSS is loaded in the context of the Drupal page's theme, and therefore your CSS selector must be prefixed with .ck-content, so: .ck-content * { font: …; }

it might be safer to deprecate

The CKEditor (4) module is known to go away already; this is only a natural consequence of that. An explicit deprecation might be good to give people advance warning though, and I suspect this is what you mean?

I'm wondering if that could potentially allow us to also remove the assumption that the CSS is only applied for the CKEditor instance. We could make it the themers responsibility to appropriately scope their CSS for CKEditor. That would be less complex to implement and less complex for themers to debug.

Yes, see above! That's definitely going to happen, there's no way for us around that.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

D'oh — @bnjmnm's magic is actually what removes that distinction and makes it happen automatically — so I'm completely wrong! 🙈

I guess the conversation in #23 + #24 + #25 leads to a question: do we want to make the pre-existing CKEditor 4-targeting CSS automagically work in CKEditor 5? Or do we want to ask theme maintainers to provide a CKEditor 5 alternative?

Right now we're only asking module maintainers to make changes. The state of the patch in this issue means theme maintainers won't have to do something like that. But the direction #23 + #24 + #25 would take us in, would force them to also make changes.

So:

  1. I think the current approach is fine. There is a certain level of magic though. That is the cost with this approach.
  2. But we could consider adding a specific ckeditor5_stylesheets property too, which requires CSS to be written for CKEditor 5, and which does not have the magic. This can be a nice-to-have follow-up that is not stable-blocking.
lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented

Discussed this with @bnjmnm and @Wim Leers to decide path forward here. We agreed that we would test the current implementation with Olivero and Bartik to determine the path forward.

I did some manual testing on both of the themes. Unfortunately, I don't think the current implementation is really doing what we expect it to do. For example, neither of the themes are able to change the font-family used in the editor because the property is targeting body and html elements. I also tested some custom styles for built in HTML elements, and in most cases CKEditor has more specific CSS that overrides the customizations.

There's also a high risk that generic CSS overriding CKEditor styles will accidentally cause usability and/or accessibility issues. For example, Olivero is changing the link color to something that leads into inaccessible color combinations with CKEditor styles. Some of this could be made to work, but I think there's a high risk that CSS that is not built to integrate with CKEditor will have unwanted side-effects.

Based on this, I'm leaning towards the simplest approach where we provide new key in theme info.yml, and the theme maintainers are responsible for scoping CSS selectors properly. This is best from expectation management perspective because it seems like coming up with CSS processing that is able to take into account all edge cases could be expensive to implement and maintain.

One of the use cases that @bnjmnm raised was that it's important that CSS components defined in a theme, must continue to work in CKEditor. I think this isn't necessarily at risk with CKEditor 5, even if we don't automate scoping the CSS. The custom components shouldn't be styling HTML elements, and should simply use specific enough class names that don't collide with out admin UI. CKEditor also doesn't ship with styles for these custom elements meaning that specificity issues are less likely to occur. This means that themers could pretty safely load their CSS for CSS components to be rendered in the CKEditor without prefixing every CSS class with a CKEditor class.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

#27: If I try to summarize your concerns:

  1. Your key concern is the "general look and feel" of the front end theme: that CSS would need to be written specifically for CKEditor 5, to override CKEditor 5's default styles.
  2. You're not concerned about components provided by themes, because those already have a higher CSS selector specificity, meaning that CKEditor 5's default styles won't win (because they're not as specific).

Is that a fair summary?

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented

I think the summary would be that:

  1. Most generic CSS will have to be written specifically for CKEditor to make it compatible with CKEditor styles because of selector specificity and color conflicts
  2. Custom CSS components probably won't suffer from these issues as much since the CKEditor styles shouldn't be touching them for the most part

And with these I'm trying to make a point that we probably won't gain much from the extra complexity that is involved with automatically prefixing all selectors.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

👍

That's clear to me now.

Really curious to hear @bnjmnm's thoughts 🤓

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented

Discussed with @bnjmnm and he has some reservations on the potential impact to people upgrading from CKE 4 to CKE 5. He said that he would like to see how the CR for this change could look like, and wanted to make sure that we invest enough time into figuring out how the changed behavior can be communicated to users.

Based on that, I'll start working on a CR draft.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Assigned: bnjmnm » lauriii

Per #31.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented
Status: Needs work » Needs review
FileSize
3194084-33.patch11.8 KB

No interdiff since this is patch for the new approach.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented
Assigned: lauriii » Unassigned
Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Needs review » Needs work 
  1. +++ b/core/modules/ckeditor5/ckeditor5.libraries.yml
@@ -28,6 +28,12 @@ drupal.ckeditor5:
+# Library used for dynamically loading CKEditor 5 stylesheets from the default
+# front end theme.
+drupal.ckeditor5.stylesheets:
+  version: VERSION

    I didn't know such an empty asset library definition was possible/permitted!

    Can we add an @see to point to the code where we populate this definition? 

  2. +++ b/core/modules/ckeditor5/ckeditor5.module
@@ -343,6 +344,19 @@ function ckeditor5_library_info_alter(&$libraries, $extension) {
+    $css = _ckeditor5_theme_css();
+    $libraries['drupal.ckeditor5.stylesheets'] = [
+      'css' => [
+        'theme' => array_map(function () {
+          return [];
+        }, array_flip($css)),
+      ],
+    ];

    Woah, what's going on here! 🤔

    It'd help if it were documented what the structure of the return value of the helper method was. 

  3. +++ b/core/modules/ckeditor5/ckeditor5.module
@@ -396,3 +410,41 @@ function ckeditor5_config_schema_info_alter(&$definitions) {
+    if (isset($info['ckeditor5-stylesheets'])) {
+      $css = $info['ckeditor5-stylesheets'];

    🐛 ckeditor5_stylesheets (underscore, not dash) — for consistency with ckeditor_stylesheets

    Ahhh! No! This is better, because it makes CKE5 consistent with all other theme info properties! It was the CKE4 theme property that was inconsistent before! 🙈

    👏 

  4. +++ b/core/modules/ckeditor5/tests/src/Functional/AddedStylesheetsTest.php
@@ -0,0 +1,91 @@
+    'editor',
+    'ckeditor5',

    🤓 We don't need editor if we're specifying ckeditor5. 

  5. +++ b/core/modules/ckeditor5/tests/src/Functional/AddedStylesheetsTest.php
@@ -0,0 +1,91 @@
+  protected function setUp(): void {

    Missing {@inheritdoc}. 

  6. +++ b/core/modules/ckeditor5/tests/src/Functional/AddedStylesheetsTest.php
@@ -0,0 +1,91 @@
+    $filtered_html_format = FilterFormat::create([
+      'format' => 'basic_html',
+      'name' => 'Basic HTML',
+      'weight' => 0,
+      'filters' => [],
+      'roles' => [RoleInterface::AUTHENTICATED_ID],
+    ]);
+    $filtered_html_format->save();
+    $this->editor = Editor::create([
+      'format' => 'basic_html',
+      'editor' => 'ckeditor5',
+      'settings' => [
+        'toolbar' => [
+          'items' => [],
+        ],
+      ],
+    ]);

    🤓

    1. We don't need weight: 0
    2. Let's name this something other than basic_html because this clearly does not look like the "Basic HTML" text format we all know.
  7. +++ b/core/modules/ckeditor5/tests/src/Kernel/CKEditor5StylesheetsTest.php
@@ -0,0 +1,66 @@
+   * Tests loading of theme's CKEditor stylesheets defined in the .info file.

    s/CKEditor/CKEditor 5/ 

  8. +++ b/core/modules/ckeditor5/tests/src/Kernel/CKEditor5StylesheetsTest.php
@@ -0,0 +1,66 @@
+    // Case 1: Install theme which has an absolute external CSS URL.
...
+    // Case 2: Install theme which has an external protocol-relative CSS URL.
...
+    // Case 3: Install theme which has a relative CSS URL.
...
+    // Case 4: Install theme which has a Drupal root CSS URL.

    Can we change this to use a @dataProvider instead for better legibility?

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented
Status: Needs work » Needs review
FileSize
3194084-36.patch11.88 KB
interdiff.txt3.84 KB

This should address all except #35.8.

#35.8: This was directly copied from \Drupal\Tests\ckeditor\Kernel\CKEditorTest. I can refactor this if think that would be helpful but I wanted to just provide equal test coverage to what we had before.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

Thanks, looking good! (Except the hunk before the last one, that looks like an odd change.)

I can refactor this if think that would be helpful

If we can spend 5 minutes now to refactor this using a data provider, it'll make this test easier to maintain and understand for the next decade 🤓

Status: Needs review » Needs work

The last submitted patch, 36: 3194084-36.patch, failed testing. View results

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented
Status: Needs work » Needs review
FileSize
3194084-39.patch11.4 KB
interdiff.txt4.44 KB

Except the hunk before the last one, that looks like an odd change.

There's only one case so I thought it made more sense to remove it from there.

If we can spend 5 minutes now to refactor this using a data provider, it'll make this test easier to maintain and understand for the next decade 🤓

Sure! I thought what I had there would be the simplest possible change but not worth arguing whether we should do it or not since it literally takes 5 minutes to do that. 😅

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

Looks good to. me! Curious what @bnjmnm thinks 🤓

Status: Needs review » Needs work

The last submitted patch, 39: 3194084-39.patch, failed testing. View results

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

HUH …

  1x: Temporary work-around until https://www.drupal.org/project/drupal/issues/3196689 lands.
    1x in AddedStylesheetsTest::testCkeditorStylesheets from Drupal\Tests\ckeditor5\Functional

caused by

drupal.ckeditor5.quickedit-temporary-work-around:
  deprecated: "Temporary work-around until https://www.drupal.org/project/drupal/issues/3196689 lands."
  css:
    theme:
      css/quickedit-override.css: {}

in ckeditor5.libraries.yml

🤔

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
CreditAttribution: bnjmnm at Acquia commented
Title: Support ckeditor_stylesheets setting in theme info, allow CSS to work for both CKEditor 4 and 5 » Support functionality equivalent to ckeditor_stylesheets
Issue summary: View changes

I'd like to figure out a way to gently alert content editors about this change. I'm concerned the people most impacted by this may not be the ones reading the change record.
Ideally I'd like something associated with individual user accounts that can be opted out of indefinitely once they've seen it, and there's specific criteria for it appearing at all so it doesn't show up for users that never used CKEditor 4 or for themes without ckeditor_stylesheets. I'm also aware that may be an unreasonable amount of conditional logic for a little help message so I'll think of some options and invite others to do the same.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented

I'd like to figure out a way to gently alert content editors about this change. I'm concerned the people most impacted by this may not be the ones reading the change record.

If we want to provide a warning to users about this change, I think we could trigger it based on ckeditor_stylesheets existing in the default themes info.yml without ckeditor5-stylesheets. If we go with that approach, we need to decide whether it should be visible on the text format form and/or on pages that are using CKEditor 5.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

If we go with that approach, we need to decide whether it should be visible on the text format form

Ohhh, this is a really great point!

I think we could and should add a validation constraint that checks whether the default front-end theme contains ckeditor_stylesheets but does not contain ckeditor5_stylesheets. We can then warn the user about that. (And for ensuring it's a warning, not an error, search core/modules/ckeditor5 for addWarning — there's a precedent for that.)

Perhaps in addition to that, we could implement hook_requirements() to add a warning on the status report too?

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
CreditAttribution: bnjmnm at Acquia commented
Issue tags: +Needs followup

Discussed the message with @lauriii and @Wim Leers and we agreed this could be triggered by the presence of an active ckeditor_stylesheets without an equivalent ckeditor5-stylesheets and the message will be visible on any fields using CKEditor 5 as well as the text format form. We'll begin with the underlying logic then iterate on the message language to ensure it helps more than complicates.

We also need to create followups for any core theme use of ckeditor_stylesheets as they will need equivalent ckeditor5-stylesheets, though they may result in somewhat different appearances, due on variables such as CKEditor 5's built-in stying providing a better experience.

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
CreditAttribution: bnjmnm at Acquia commented
Status: Needs work » Needs review
FileSize
3194084-47.patch27.53 KB
interdiff_39-47.txt17.03 KB

This adds the logic for the warnings, as well has hastily-written warning messages that will undoubtedly improve after they are discussed.

Test coverage for the warnings appearing was added, but should probably be expanded to ensure warnings aren't presented when they shouldn't be.

The error examined in #42 will likely still occur.

Status: Needs review » Needs work

The last submitted patch, 47: 3194084-47.patch, failed testing. View results

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
CreditAttribution: bnjmnm at Acquia commented
Status: Needs work » Needs review
FileSize
3194084-49.patch31.67 KB
interdiff_47-49.txt8 KB

Added test coverage to ensure there aren't false positives.

Also addressed the fail mentioned in #42 - there's test coverage for that solution too.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Needs review » Needs work 
  1. +++ b/core/modules/ckeditor5/ckeditor5.libraries.yml
@@ -88,6 +96,14 @@ drupal.ckeditor5.filter.admin:
+      ckeditor4_stylesheets: []
+
 
 admin:

    🤓 One empty line too many. 

  2. +++ b/core/modules/ckeditor5/ckeditor5.module
@@ -343,6 +344,57 @@ function ckeditor5_library_info_alter(&$libraries, $extension) {
+    // - We are on an admin page.

    🤓 s/page/route/ 

  3. +++ b/core/modules/ckeditor5/ckeditor5.module
@@ -343,6 +344,57 @@ function ckeditor5_library_info_alter(&$libraries, $extension) {
+    // If the following are all true, we check if any ckeditor_stylesheets are
+    // being added to the default theme. If so, then a warning is generated to

    Suggestion: if any stylesheets are being added by default theme (by detecting the presence of ckeditor_stylesheets). 

  4. +++ b/core/modules/ckeditor5/ckeditor5.module
@@ -343,6 +344,57 @@ function ckeditor5_library_info_alter(&$libraries, $extension) {
+    if (\Drupal::service('router.admin_context')->isAdminRoute() && empty($css) && !$opt_out_ckeditor5_stylesheets) {

    🤔 Does this actually work? AFAICT hook_library_info_alter() is called only once, and the result is cached.

    See \Drupal\Core\Asset\LibraryDiscoveryParser::parseLibraryInfo(), which invokes this.

    I think we need to move this logic to hook_page_attachments() instead. 

  5. +++ b/core/modules/ckeditor5/ckeditor5.module
@@ -343,6 +344,57 @@ function ckeditor5_library_info_alter(&$libraries, $extension) {
+    $moduleHandler = \Drupal::service('module_handler');

    🤓 Nit: s/$moduleHandler/$module_handler/ 

  6. +++ b/core/modules/ckeditor5/js/ckeditor5.es6.js
@@ -354,6 +354,23 @@
+            'There are CKEditor 4 stylesheets that may need CKeditor 5 equivalents.',

    🤓 s/CKeditor/CKEditor/

    🤔 And … should we make this a link to the (yet to be created) change record? 

  7. +++ b/core/modules/ckeditor5/js/ckeditor5.filter.admin.es6.js
@@ -111,6 +111,113 @@
+              'There are CKEditor 4 stylesheets without equivalents: @stylesheets',

    Same link remark as above. 

  8. +++ b/core/modules/ckeditor5/js/ckeditor5.filter.admin.es6.js
@@ -111,6 +111,113 @@
+              // if the IE11 warning is needed.

    🤔 This is not the IE11 warning?

hooroomoo’s picture

hooroomoo
they/them
CreditAttribution: hooroomoo at Acquia commented
FileSize
3194084-50.patch19.94 KB
hooroomoo’s picture

hooroomoo
they/them
CreditAttribution: hooroomoo at Acquia commented

I wasn't sure how to address feedback #4, 6, 7, 8 from comment #50 so that still needs work but the patch has the other fixes.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented 
  1. +++ b/core/modules/ckeditor5/ckeditor5.module
@@ -343,6 +344,58 @@ function ckeditor5_library_info_alter(&$libraries, $extension) {
+    // - We are on an admin route.

    Why are we checking the admin route? CKEditor could be rendered on the front-end too, why are we not rendering the message then? This includes the node form when "Use the administration theme when editing or creating content" is unchecked. 

  2. +++ b/core/modules/ckeditor5/ckeditor5.module
@@ -343,6 +344,58 @@ function ckeditor5_library_info_alter(&$libraries, $extension) {
+    // - No CKEditor 5 stylesheets were discovered.

    Shouldn't we be checking if the ckeditor5-stylesheets key is present in the info.yml? This way if the stylesheets are not needed with CKEditor 5, an empty value could be used for removing the warning. 

  3. +++ b/core/modules/ckeditor5/js/ckeditor5.es6.js
@@ -364,6 +364,23 @@
+            'There are CKEditor 4 stylesheets that may need CKEditor 5 equivalents.',

    This message is potentially confusing to non-technical users since they might not understand what's the impact of this. I think we need some explanation of that here. 

  4. +++ b/core/modules/ckeditor5/js/ckeditor5.filter.admin.js
@@ -69,6 +69,58 @@
+          selectMessages.add(Drupal.t('There are CKEditor 4 stylesheets without equivalents: @stylesheets', {

    I think there's potential for confusion with the current message because it could be implying that all of these stylesheets need an equivalent in CKEditor 5 for the message to not appear. 

  5. +++ b/core/modules/ckeditor5/js/ckeditor5.filter.admin.js
@@ -69,6 +69,58 @@
+            '@stylesheets': drupalSettings.ckeditor5.ckeditor4_stylesheets.join(', ')

    What's the benefit of rendering the stylesheet names in the message? I think it would be more important to explain which theme the message is about and have a link to the change record for instructions.

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
CreditAttribution: bnjmnm at Acquia commented

#53.1

Why are we checking the admin route? CKEditor could be rendered on the front-end too, why are we not rendering the message then? This includes the node form when "Use the administration theme when editing or creating content" is unchecked.

I suspected there's was no perfect existing solution, so I thought admin route might be a "good enough" for a few reasons:

  • The message is only relevant to people who are (or are coworkers of) someone able to update the site themes. Visitors to comment forms, for example, shouldn't see this warning.
  • The visitors that shouldn't see the message might still be authenticated, but are very unlikely to be on an admin route
  • Users with "Use the administration theme when editing or creating content" on the node form not seeing the warning is a tradeoff for erring in the direction not making the message public facing. There's still a message in the text format. Using this feature potentially eliminates the need to add a custom stylesheet anyway, as the default theme CSS is loaded and the editor is no longer in an iframe.
  • Admin route is very obvious criteria
  • This message is quite helpful, but not essential. the sites that see it benefit, the ones that don't do not suffer, they just miss out on a nice courtesy.
  • In the less-likely scenario of the message being seen on a public-facing page, it's still pretty innocuous
  • It took ~30 seconds to implement so if it's the kind of thing people have strong feeings about doing differently, it doesn't undo a significant amount of work.

That's my thought process.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy at Acquia commented
Issue tags: +Needs issue summary update

The issue summary does not explain why the ckeditor5 stylesheets must exist (even if ckeditor4 ones existed). Are we assuming the theme is styling some CK compoennts specifically that they also want to style for CK5? Will something break if this does not happen or will things look funny? I don't recall a scenario where we did a transitional warning like this when a theme did not implement something we thought may break the UX.

I totally agree displaying a warning about a theme component missing to site end users is not a good idea. I don't understand yet the severity of the situation.

+++ b/core/modules/ckeditor5/js/ckeditor5.filter.admin.es6.js
@@ -111,6 +111,113 @@
+            Drupal.t(
+              'There are CKEditor 4 stylesheets without equivalents: @stylesheets',
+              {
+                '@stylesheets':
+                  drupalSettings.ckeditor5.ckeditor4_stylesheets.join(', '),
+              },
+            ),
+            {
+              type: 'warning',
+            },

What's the impact and what to do about this? A changelog link or docs page link would be useful if we consider this warning is desired due to the impact, so those seeing it have a next step to take. Without that its just "ok, why is that a problem and what now?".

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
CreditAttribution: bnjmnm at Acquia commented
Issue summary: View changes
Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy at Acquia commented 
+++ b/core/modules/ckeditor5/js/ckeditor5.es6.js
@@ -364,6 +364,23 @@
+            'There are CKEditor 4 stylesheets that may need CKEditor 5 equivalents.',

+++ b/core/modules/ckeditor5/js/ckeditor5.filter.admin.es6.js
@@ -111,6 +111,113 @@
+              'There are CKEditor 4 stylesheets without equivalents: @stylesheets',

+++ b/core/modules/ckeditor5/js/ckeditor5.js
@@ -223,6 +223,15 @@ function _arrayLikeToArray(arr, len) { if (len == null || len > arr.length) len
+        editorMessages.add(Drupal.t('There are CKEditor 4 stylesheets that may need CKEditor 5 equivalents.'), {

I think there are 3 of these instances of the warning in the patch. The UI impact is not clear, two of the direct CK warnings would show on the page's error messages when CK is shown (on an admin route, given that that is when the data is added only). Contrary to the error message, the stylesheets are not paired up, meaning the equivalence of stylesheets is not checked (and I don't think they could be). Also given the big difference with CK4 to CK5, I am not sure "equivalent" is the right approach to suggest even?

For me I think the errors would be best shown on the CKEditor configuration screen, but it would ideally cycle through and be collected/shown for all enabled themes. Then you would get an overview of what might break. If the text format configuration screen is where we aim to make ckeditor configuration foolproof, that would be the best / standard place to warn admins about the lack of theme support as well, no?

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy at Acquia commented
Issue tags: -Needs issue summary update

Also, thanks for updating the issue summary :)

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
CreditAttribution: bnjmnm at Acquia commented
Status: Needs work » Needs review
FileSize
Screen Shot 2022-02-10 at 3.08.14 PM.png187.46 KB
3194084-59.patch30.15 KB

This incorporates the feedback from #57. The message checks all themes and distinguishes between the primary theme and base theme. Note that if the primary theme has ckeditor5-stylesheets, the base themes are not checked as it could be configured with ckeditor5-stylesheets: false, which could mean "I'm not bothering with CKEditor 5 stylesheets in general"

Here's what it looks like when using Gin, to demonstrate the message accounting for primary and base themes - and also reporting for default/admin.

If this approach seems good, the test coverage should probably expand a bit to include base theme scenarios.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy at Acquia commented

I think this is great. The text itself is a bit convoluted, but it may need to be that way due to what it tries to convey.

Status: Needs review » Needs work

The last submitted patch, 59: 3194084-59.patch, failed testing. View results

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented

Should we just provide list of all enabled themes that have ckeditor_stylesheets without explaining which theme they are? That would probably make sense in most cases since usually people wouldn't have too many themes installed. That way we could simplify the messages to something like:

Bartik, Seven and Claro have ckeditor_stylesheets configured, but there is no corresponding ckeditor5-stylesheets configuration.
bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
CreditAttribution: bnjmnm at Acquia commented
Status: Needs work » Needs review
FileSize
3194084-63.patch29.98 KB
interdiff_59-63.txt2.47 KB

This is to address the test failures, we'll still need to discuss the suggestion in #62, though

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy at Acquia commented

#62 sounds like a good suggestion :)

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
CreditAttribution: bnjmnm at Acquia commented
FileSize
3194084-65.patch30.4 KB
interdiff_63-65.txt12.01 KB
Screen Shot 2022-02-11 at 11.24.08 AM.png139.35 KB

Incorporates the suggestion in #62 and expands test coverage of the warnining to include base theme use cases.

bnjmnm opened merge request !1813

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
CreditAttribution: bnjmnm at Acquia commented

I opened an MR in order to use the Gitlab UI for some walkthroughs, with the same changes as #65. We can continue using the patch workflow for this issue and I'll take care of anything that gets out of sync.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Needs review » Needs work

Looking great 😊

Posted a few nits and a few deeper questions.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Related issues: +#3265230: Refactor ie11.filter.warnings.es6.js to simpler structure & other improvements

This looks way simpler now! :D

BTW, already RTBC'd #3265230: Refactor ie11.filter.warnings.es6.js to simpler structure & other improvements, thanks for splitting it off from this one! 😊

Yay clear scopes :)

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

Analysis

The message always gets set by ckeditor5_form_filter_format_form_alter().

The problem is that when first accessing /admin/config/content/formats/manage/basic_html (i.e. GET), _update_ckeditor5_html_filter() does not run, which means the message is not consumed. Therefore it is still available by the time \Drupal\Core\Render\Element\StatusMessages::renderMessages() runs.

Because 

    $form['real_time_validation_errors_location'] = [
      '#type' => 'status_messages',
      '#id' => 'ckeditor5-realtime-validation-messages-container',
      '#attributes' => [
        'data-drupal-messages' => TRUE,
      ],
    ];

has been changed to use '#type' => 'status_messages',, it has the same placeholder as \Drupal\system\Plugin\Block\SystemMessagesBlock::build(), so when \Drupal\Core\Render\Element\StatusMessages::renderMessages() runs, it replaces both placeholders. Voila: this is why it appears twice!

Solution

We basically want to make this CKEditor 5-specific message to appear in a specific location, and not in the default messages location. Confirmed this with @bnjmnm.

So we need to prevent the placeholdering mechanism from getting in the way. We can do that by:

  1. continuing to use '#type' => 'status_messages', and then resetting the global state (well, the session metadata, so sort of global state) that contains the accumulated messages, then forcefully rendering it (to prevent it from still becoming a placeholder and hence delaying the rendering and hence still containing all messages), and then restoring the global state. This is implemented in 1f7c4e17cd997bfe27f06e567581ef3e95a73709.
  2. instead use '#theme' => 'status_messages',, which is what '#type' => 'status_messages', eventually ends up calling anyway, but we'd call it directly. This is fine: it is a template that is not marked @internal. The significant benefit is that we then don't need to mess with global state nor do we need to do early rendering anymore: we can just pass an explicit list of messages! This is implemented in d8eddd7e89de5dd76ed9170bc78099053facba5e.

IMHO that second solution just makes it way simpler to understand, less risky, etc. I think the diff shows this clearly too. Hopefully you agree 😊

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
CreditAttribution: bnjmnm at Acquia commented
Status: Needs work » Needs review

Had to move some code around in library_info_alter due to a condition causing it to return early. Filed an issue for the bug #3267204: library_info_alter can abort early if locale is not enabled, and worked around it by having the code here run before the bug happens.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Needs review » Needs work
  • 👍 Thoroughly tested the "inform the site builder" UX in the admin UI in Bartik and Claro. Works great 😄
  • 🐛 Changing the default theme does not cause ckeditor5_library_info_alter() to run again, because modifying system.theme config does not invalidate the library_info cache tag. Installing a new (theme) extension causes all caches to be wiped, which is why this is not noticed by the AddedStylesheetsTest test.
    Solution: do something like \Drupal\color\EventSubscriber\ColorConfigCacheInvalidator to invalidate the library_info cache tag whenever the default key-value pair in system.theme is modified. Functional test coverage similar to that in \Drupal\Tests\color\Functional\ColorTest::testOverrideAndResetScheme() would then be able to prove that this works correctly.

Kicking back to Needs work for that second point, even though I was very close to RTBC'ing — it's only when I started to actually test the functionality this is adding — sorry! 😬

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
CreditAttribution: bnjmnm at Acquia commented
Status: Needs work » Needs review
Issue tags: -Needs followup
Related issues: +#3267434: [PP-1] Add Umami ckeditor5-stylesheets config, +#3267436: Add Claro ckeditor5-stylesheets config, +#3267438: Add Classy ckeditor5-stylesheets config, +#3267439: Add Olivero ckeditor5-stylesheets config, +#3267440: Add ckeditor5-stylesheets config, +#3267435: [PP-1] Add Bartik ckeditor5-stylesheets config, +#3267443: [PP-1] Add starterkit ckeditor5-stylesheets config

Addressed #72 and created followups

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

Aha, not currently mergeable — probably because of 🐛 This is unrelated to this issue — probably a merge gone wrong, because this landed upstream in #3264727: [drupalMedia|drupalImage] Allow removing data-align in the UI, and making an image inline yesterday 😄.

a49af1b2049d50db5a1aaab5a87f39bf88ae47c6 should fix that.

P.S.: I had to disable the core pre-commit hooks to be able to merge this, it was causing the merge commit to fail like described here: https://iffgit.fz-juelich.de/fleur/fleur/-/issues/164 🤯

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Needs review » Reviewed & tested by the community

I made only trivial changes, so I can still RTBC this:

  • Comment nits.
  • Mark a service private
  • Merge upstream
  • Remove one injected service that is unused

Review

Re-tested the problem I found in #72. Works perfectly now! 👍

Really subtle problem with test coverage that took a lot of time to figure out😬

That is, until I made the service private. The test coverage looks solid, but … it somehow always passes, even if I remove the new cache tag deleting event subscriber! 😱

It's as if it's wiping the caches in the test when in a non-test Drupal, it is not wiping the caches. Putting a breakpoint on each of the three $this->drupalGet('node/add/article'); calls in the test plus one in ckeditor5_library_info_alter() actually proves this! 🤯 Holy shit — what's going on here?!

Well … turns out this happened because we were retrieving node/add/article in the default theme rather than the admin theme. Fix: set node.settings's use_admin_theme to TRUE.

Except that STILL did not fix it! It was still using the front-end theme. After some debugging, I realized the test user must have the view the administration theme permission. Added that.

Now the test fails as expected 👍

That's what you should see in 7441307e5b0f5d40cdd5eb74c142db6dc6d06a14: tests should fail. The fix is then simple: make the event subscriber service public again (that was my mistake, but it's only thanks to that mistake that we discovered the test coverage was not actually testing what it should be testing 🙃). That's what you see in fc4876049ca1bd17af63599cf38dfb7170bc2ee7.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
FileSize
3194084-76-D9.patch33.72 KB
3194084-76-D10.patch33.73 KB
9-10-differences.txt680 bytes

Patches for all 3 branches.

FYI: the -D9 patch should also apply to 9.3, but won't yet, because there are several commits still to be cherry-picked to 9.3.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 76: 3194084-76-D10.patch, failed testing. View results

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Needs work » Reviewed & tested by the community

D10 failed, but 100% unrelated random fail:

Testing Drupal\KernelTests\Core\Entity\EntityAutocompleteTest
.F                                                                  2 / 2 (100%)

Time: 00:02.756, Memory: 4.00 MB

There was 1 failure:

1) Drupal\KernelTests\Core\Entity\EntityAutocompleteTest::testSelectionSettingsHandling
Non-existent selection settings key throws an exception.

/var/www/html/core/tests/Drupal/KernelTests/Core/Entity/EntityAutocompleteTest.php:111
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestResult.php:726

Re-testing.

  • lauriii committed 2846170 on 10.0.x 
    Issue #3194084 by bnjmnm, Wim Leers, lauriii, hooroomoo, Gábor Hojtsy:...

  • lauriii committed 9a4c530 on 9.4.x 
    Issue #3194084 by bnjmnm, Wim Leers, lauriii, hooroomoo, Gábor Hojtsy:...
lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented
Version: 9.4.x-dev » 9.3.x-dev

Committed 2846170 and pushed to 10.0.x. Committed the 9.x patch to 9.4.x. Thanks!

Leaving open for 9.3.x backport once the test results come back.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented
Status: Reviewed & tested by the community » Patch (to be ported)

Seems like we need new patch for 9.3.x

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Patch (to be ported) » Reviewed & tested by the community

@lauriii AFAICT that's only because #3260032: CKEditor 5 adds ie11.user.warnings library to every page, triggering a FOUC even for anonymous users has not yet been cherry-picked to 9.3.x 🤓😅

  • bnjmnm committed 9c7aa8a on 9.3.x authored by lauriii 
    Issue #3194084 by bnjmnm, Wim Leers, lauriii, hooroomoo, Gábor Hojtsy:...
bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
CreditAttribution: bnjmnm at Acquia commented
Status: Reviewed & tested by the community » Fixed

@Wim Leers was correct in #83. Cherry picking that to 9.3.x, made it possible to cherry pick this to 9.3.x.

Status: Fixed » Closed (fixed)

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