Add AJAX command to add style sheets to CKEditor instances

Interesting! More detailed feedback to follow. But this definitely needs functional JS tests.

Allow add_css commands to explicitly set the document context in which the CSS is applied. It should default to window.document to maintain backwards compatibility and normal behavior, but calling code will now be able to add CSS directly to iframe CKEditor instances.

I don't understand why we'd modify the existing add_css AJAX command. That seems unnecessarily risky.

Additionally, create a new add_css_ckeditor command

That'd make much more sense to me.

… but it looks like you'd need to duplicate quite a bit from add_css in this new add_css_ckeditor command. So if @nod_ and other AJAX framework maintainers are okay with it, then I am too.

I'd rename add_css_ckeditor to add_css_ckeditor_iframe.

@import hack for IEs in CORE maybe outdated. In this case, we could consider using CKeditor's API.
http://docs.ckeditor.com/#!/api/CKEDITOR.dom.document-method-appendStyle...

I'd rename add_css_ckeditor to add_css_ckeditor_iframe.

Quickedit using `contenteditable`

NW for tests.

This patch is looking great! :) However, one key question: what would call this AJAX command and when?

1. +++ b/core/modules/ckeditor/js/ckeditor.js
   @@ -276,4 +276,48 @@
   +  Drupal.AjaxCommands.prototype.ckeditor_add_style = function (ajax, response, status) {
   

   We shouldn't support this. Drupal core doesn't support inline styles. This should not either.

   (Inline styles also come with significant security risks.)

   This does not need further security review; loading stylesheets in CKEditor is not something new, Drupal already does this. The difference is that here it needs to happen dynamically.

2. +++ b/core/modules/ckeditor/js/ckeditor.js
   @@ -276,4 +276,48 @@
   +        editor.document.appendStyleSheet(url);
   

   Can you add a @see to CKEditor's documentation?

3. +++ b/core/modules/ckeditor/src/Ajax/AddStyleSheetCommand.php
   @@ -0,0 +1,34 @@
   +class AddStyleSheetCommand implements CommandInterface {
   

   Incomplete docs: class docs, property docs, method docs.

4. +++ b/core/modules/ckeditor/tests/src/FunctionalJavascript/AjaxCssTest.php
   @@ -0,0 +1,42 @@
   +    $this->getSession()->getPage()->pressButton('Add CSS');
   ...
   +    $condition = 'CKEDITOR.instances[“edit-body-0-value”].window.$.getComputedStyle(CKEDITOR.instances[“edit-body-0-value”].document.$.body).color === “rgb(255, 0, 0)”';
   

   Nice :)

5. +++ b/core/tests/Drupal/FunctionalJavascriptTests/JavascriptTestBase.php
   @@ -61,6 +61,11 @@ protected function tearDown() {
   +  protected function awaitAjax() {
   +    // Wait up to 5 seconds for AJAX stuff to finish.
   +    return $this->getSession()->wait(5000, '(typeof(jQuery)==“undefined” || (0 === jQuery.active && 0 === jQuery(\':animated\').length))');
   +  }
   

   You're modifying JavascriptTestBase: you're expanding its API. This will need to be documented.

Finally, can you also upload a test-only patch? That should fail, and will hence prove that the test is testing what it should be testing.

1. +++ b/core/modules/ckeditor/tests/modules/css/test.css
   @@ -0,0 +1,3 @@
   +    color: red;
   

   little space problems, 2 instead 4 spaces

2. +++ b/core/tests/Drupal/FunctionalJavascriptTests/JavascriptTestBase.php
   @@ -49,7 +49,7 @@ protected function initMink() {
   -    $result = $this->getSession()->wait(5000, '(typeof(jQuery)=="undefined" || (0 === jQuery.active && 0 === jQuery(\':animated\').length))');
   +    $result = $this->awaitAjax();
   

   This isn't related to current issue scope. Can you open a new issue?

   Also it's a `assertWaitOnAjaxRequest()` in JSWebAssert class

I think, once this is in core, Lightning will use this command by altering Entity Embed's AJAX responses, converting any add_css commands so that the styles will be attached in CKEditor rather than the main browser window.

You'd need it in the "main window" too, because it's possible CKEditor is being used in-place, in which case it lives in the "main window" rather than in an <iframe>.

Can you now post a failing and passing patch at the same time? :)

AJAX command to add style sheets to a CKEditor instance.

iframe instance

I'm somewhat concerned that the test isn't an actual integration test. In reality, it's a bit more difficult to let the server know the DOM ID of the CKEditor instance. In this test, it's just hardcoded.

Finally, I'd like to see test coverage of an inline CKEditor instance too, and test the behavior there. Looking at http://docs.ckeditor.com/#!/api/CKEDITOR.dom.document, I don't know if this will work for both inline and iframe instances.

I'll see what I can do about making it work with an inline editor instance, assuming core supports that.

That's what Quick Edit uses.

But if the CKEditor module always instantiates its editors as iframes, I'm not sure what benefit it would confer to check that it works with inlines as well.

Because it impacts what the server side must do: whether sending only this new AJAX command is sufficient (regardless of CKEditor instance type), or whether the server must be know when to also send the already existing CSS AJAX command.

i.e. I want us to understand the need + use cases + how to use it before adding new code. I'm the one who has to maintain it, after all.

Note that you don't have to test with Quick Edit. That'd tie the test to too many modules. I just meant that you'll find examples of how to create an inline CKE instance there.

I'd do:

1. a controller, that returns a render array that renders a CKE iframe instance and a CKE inline instance
2. let the JavascriptTestBase test add A.css to the iframe instance, and check that it doesn't exist in the main page
3. let the JavascriptTestBase test add B.css to the inline instance, and check that it exists on the main page, but not in the iframe instance

Something like that.

Such awesome test coverage! Thank you :)

1. +++ b/core/modules/ckeditor/js/ckeditor.js
   @@ -304,4 +304,30 @@
   +     * Command to add style sheets to a CKEditor instance.
   +     *
   
   +++ b/core/modules/ckeditor/src/Ajax/AddStyleSheetCommand.php
   @@ -0,0 +1,64 @@
   + * AJAX command to add style sheets to a CKEditor instance.
   

   Add a second line here: Works for both iframe and inline CKEditor instances.

2. +++ b/core/modules/ckeditor/js/ckeditor.js
   @@ -304,4 +304,30 @@
   +      if (editor) {
   +        response.stylesheets.forEach(function (url) {
   +          editor.document.appendStyleSheet(url);
   +        });
   +        }
   

   Indentation error here (on the last quoted line).

3. +++ b/core/modules/ckeditor/tests/modules/js/ajax-css.js
   @@ -0,0 +1,20 @@
   +(function (Drupal, ckeditor, editorSettings, $) {
   +
   +    'use strict';
   +
   +    Drupal.behaviors.ajaxCssForm = {
   +
   +        attach: function (context) {
   +            // Initialize an inline CKEditor on the #edit-inline element if it
   +            // isn't editable already.
   +            $(context)
   +                .find('#edit-inline')
   +                .not('[contenteditable]')
   +                .each(function () {
   +                    ckeditor.attachInlineEditor(this, editorSettings.formats.basic_html);
   +                });
   +        }
   +
   +    };
   +
   +})(Drupal, Drupal.editors.ckeditor, drupalSettings.editor, jQuery);
   

   4-space indentation.

   Run this through eslint, and you'll get all complaints that a committer will push back against as well.

4. +++ b/core/modules/ckeditor/tests/modules/src/Form/AjaxCssForm.php
   @@ -0,0 +1,107 @@
   +    $form['inline'] = [
   ...
   +    $form['iframe'] = [
   

   Either rename these to (inline|iframe)_cke_instance, or add a comment such as

   // Create both an inline and an iframe CKEditor instance, so that we can test against both.
   

5. +++ b/core/modules/ckeditor/tests/modules/src/Form/AjaxCssForm.php
   @@ -0,0 +1,107 @@
   +      '#children' => $this->t('Here be dragons.'),
   ...
   +      '#default_value' => $this->t('Here be llamas.'),
   

   :)

6. +++ b/core/modules/ckeditor/tests/modules/src/Form/AjaxCssForm.php
   @@ -0,0 +1,107 @@
   +        '#value' => $this->t('Add inline CSS'),
   ...
   +        '#value' => $this->t('Add iFrame CSS'),
   

   I'd rename these to Add CSS to (inline|iframe) CKEditor instance

7. +++ b/core/modules/ckeditor/tests/modules/src/Form/AjaxCssForm.php
   @@ -0,0 +1,107 @@
   +   * Generates an AJAX response to add CSS to an editor instance.
   

   s/an editor/a CKEditor Text Editor/

8. +++ b/core/modules/ckeditor/tests/modules/src/Form/AjaxCssForm.php
   @@ -0,0 +1,107 @@
   +   *   The editor instance ID.
   

   s/editor/Text Editor/

9. +++ b/core/modules/ckeditor/tests/modules/src/Form/AjaxCssForm.php
   @@ -0,0 +1,107 @@
   +   * @return AjaxResponse
   ...
   +   * @return AjaxResponse
   ...
   +   * @return AjaxResponse
   

   Needs FQCN.

10. +++ b/core/modules/ckeditor/tests/modules/src/Form/AjaxCssForm.php
    @@ -0,0 +1,107 @@
    +      $base_url . '/' . drupal_get_path('module', 'ckeditor_test') . '/css/test.css',
    

    Use file_url_transform_relative(file_create_url(drupal_get_path('module', 'ckeditor_test') . '/css/test.css')).

11. +++ b/core/modules/ckeditor/tests/modules/src/Form/AjaxCssForm.php
    @@ -0,0 +1,107 @@
    +   * Handles the AJAX request to add CSS to the iFrame editor.
    
    +++ b/core/modules/ckeditor/tests/src/FunctionalJavascript/AjaxCssTest.php
    @@ -0,0 +1,79 @@
    +    // but not the iFrame.
    ...
    +    // Add the iFrame CSS and assert that the style is applied to the iFrame,
    

    SUPERNIT: I've never seen "iframe" written this way before. (But apparently there's a few occurrences in Drupal core.)

    Can you change it to just "iframe"? That's consistent with the majority of Drupal core, and the rest of the CKEditor module.

12. +++ b/core/modules/ckeditor/tests/src/FunctionalJavascript/AjaxCssTest.php
    @@ -0,0 +1,79 @@
    +  protected $profile = 'standard';
    

    Please no! This will install everything under the sun. This makes this test super slow.

    It's not even necessary anymore with your updated test I think :)

13. +++ b/core/modules/ckeditor/tests/src/FunctionalJavascript/AjaxCssTest.php
    @@ -0,0 +1,79 @@
    +  public static $modules = ['ckeditor_test'];
    

    You will need to add ckeditor here if you remove standard per the previous point.

14. +++ b/core/modules/ckeditor/tests/src/FunctionalJavascript/AjaxCssTest.php
    @@ -0,0 +1,79 @@
    +    $account = User::load(1);
    +    $account->setPassword('admin');
    +    $account->save();
    +    $account->passRaw = 'admin';
    +    $this->drupalLogin($account);
    

    Why user 1? This should not be necessary.

15. +++ b/core/modules/ckeditor/tests/src/FunctionalJavascript/AjaxCssTest.php
    @@ -0,0 +1,79 @@
    +  protected function getEditorStyle($instance_id, $attribute) {
    +    $instance = 'CKEDITOR.instances["' . $instance_id . '"]';
    +    $js = sprintf(
    ...
    +      $instance,
    +      $instance,
    +      $attribute
    +    );
    +    return $this->getSession()->evaluateScript($js);
    +  }
    

    WOAH MAD SCIENTIST LEVEL 9000

    I had to read this five times to make sense of it. It's effectively just evaluating this JS:

    CKEDITOR.instances['edit-inline'].window.$.getComputedStyle(CKEDITOR.instances['edit-inline'].document.$.body).color
    

    Which makes sense. Except the first part. I first thought it was using http://docs.ckeditor.com/#!/api/CKEDITOR.dom.element-method-getComputedS..., but it's not. It's not using CKE's API, but the DOM API: https://developer.mozilla.org/en-US/docs/Web/API/Window/getComputedStyle.

    It's using the CKE instance to get to the window… but that's not necessary. You can simplify %s.window.$.getComputedStyle to just window.getComputedStyle.

    And then you might as well simplify this entire thing to just:

      protected function getEditorStyle($instance_id, $attribute) {
        $js = "window.getComputedStyle(CKEDITOR.instances['$instance_id'].document.$.body).$attribute";
        return $this->getSession()->evaluateScript($js);
      }
    

Once the nits and the simplification are done, this is RTBC.

Superb! Thank you :)

1. +++ b/core/modules/ckeditor/tests/modules/src/Form/AjaxCssForm.php
   @@ -64,30 +66,29 @@
   +    $url = file_create_url($url);
   +    $url = file_url_transform_relative($url);
   

   Nit: please combine these on one line. Makes for better greppability if we need to change that pattern in the future (it was introduced in #1494670: References to CSS, JS, and similar files should be root-relative URLs: avoids mixed content warnings & fewer bytes to send.

   Does not block commit.

2. +++ b/core/modules/ckeditor/tests/src/FunctionalJavascript/AjaxCssTest.php
   @@ -13,23 +14,31 @@
   +  protected function setUp() {
   +    parent::setUp();
   +
   +    FilterFormat::create([
   +      'format' => 'test_format',
   +      'name' => $this->randomMachineName(),
   +    ])->save();
   +
   +    Editor::create([
   +      'editor' => 'ckeditor',
   +      'format' => 'test_format',
   +    ])->save();
   +
   +    user_role_grant_permissions('anonymous', ['use text format test_format']);
   +  }
   

   Perfect :)

The one thing this still needs is a CR to inform people of this new API, this new capability. Does not need to block commit though: I vouch I will create the CR if @phenaproxima does not find the time.

No point in re-RTBC'ing until #2828045: Tests failing with unrelated „Translation: The French translation is not available” is fixed.

#2828438: Exception when adding tab to a node managed by content moderation broke HEAD, and caused these failures. It's currently being re-tested.

1. +++ b/core/modules/ckeditor/js/ckeditor.js
   @@ -3,7 +3,7 @@
   -(function (Drupal, debounce, CKEDITOR, $, displace) {
   +(function (Drupal, debounce, CKEDITOR, $, displace, AjaxCommands) {
   
   @@ -304,4 +304,33 @@
   -})(Drupal, Drupal.debounce, CKEDITOR, jQuery, Drupal.displace);
   +  if (AjaxCommands) {
   ...
   +    AjaxCommands.prototype.ckeditor_add_stylesheet = function (ajax, response, status) {
   

   I don't think we need to add the extra argument AjaxCommands. We already pass in Drupal so instead of AjaxCommands.prototype.ckeditor_add_stylesheet we can just do Drupal.AjaxCommands.prototype.ckeditor_add_stylesheet. Then there's no if and we're more BC and forward compatible. Plus ckeditor_add_stylesheet should be camel case to be consistent with the rest of ajax commands.

2. +++ b/core/modules/ckeditor/tests/modules/css/test.css
   --- /dev/null
   +++ b/core/modules/ckeditor/tests/modules/js/ajax-css.js
   
   +++ b/core/modules/ckeditor/tests/modules/js/ajax-css.js
   +++ b/core/modules/ckeditor/tests/modules/js/ajax-css.js
   @@ -0,0 +1,19 @@
   
   @@ -0,0 +1,19 @@
   +(function (Drupal, ckeditor, editorSettings, $) {
   

   FILE: ...sk/dev/drupal/core/modules/ckeditor/tests/modules/js/ajax-css.js
   ----------------------------------------------------------------------
   FOUND 1 ERROR AFFECTING 1 LINE
   ----------------------------------------------------------------------
    1 | ERROR | [x] Missing file doc comment
   ----------------------------------------------------------------------
   PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
   ----------------------------------------------------------------------
   

3. +++ b/core/modules/ckeditor/tests/modules/src/Form/AjaxCssForm.php
   @@ -0,0 +1,108 @@
   +
   +class AjaxCssForm extends FormBase {
   

   FILE: ...pal/core/modules/ckeditor/tests/modules/src/Form/AjaxCssForm.php
   ----------------------------------------------------------------------
   FOUND 1 ERROR AFFECTING 1 LINE
   ----------------------------------------------------------------------
    10 | ERROR | [x] Missing class doc comment
   ----------------------------------------------------------------------
   PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
   ----------------------------------------------------------------------
   

4. +++ b/core/modules/ckeditor/tests/src/FunctionalJavascript/AjaxCssTest.php
   @@ -0,0 +1,86 @@
   +/**
   + * @group ckeditor
   + */
   +class AjaxCssTest extends JavascriptTestBase {
   

   FILE: .../modules/ckeditor/tests/src/FunctionalJavascript/AjaxCssTest.php
   ----------------------------------------------------------------------
   FOUND 1 ERROR AFFECTING 1 LINE
   ----------------------------------------------------------------------
    9 | ERROR | Missing short description in doc comment
   ----------------------------------------------------------------------
   

   This should say what is being tested.

5. It'd be great to see a comment form nod_ or droplet on the issue.

#46.1: there will still be an if, because the Drupal object alone doesn't guarantee that Drupal.AjaxCommands will exist. I'm fine with what you proposed, but the current patch is actually consistent with the rest of that JS file: Drupal.displace is also explicitly injected.

#46.5: I'd like that too, but after 1.5 month of waiting I don't think it's reasonable to block this on their review.

Keeping at NW for the nits in 2+3+4.

Re #46.1 then an awful lot of existing JS has this weakness. I agree that the approach of passing Drupal.ajaxCommands in the most recent patch is consistent with this file. That said there is no if around code using displace and debounce. I thought that the recommendation in #46.1 might make it easier for anyone swapping out this JS but after thinking about it I'm not sure.

#48: the difference is that most JS that adds an AJAX command depends on the drupal/drupal.ajax library. That's not the case here! The drupal/drupal.ckeditor library does NOT depend on drupal/drupal.ajax, because it does not need it. But it aims to support this when necessary. Hence the condition.

This is also exactly why there is no "if" around displace and debounce: because those are dependencies.

Committed 6fa5085 and pushed to 8.3.x. Thanks!

Thanks for explaining @Wim Leers in #49.