Allow ckeditor_stylesheets to refer to external URL, for e.g. webfonts

even javascript if the library contained that

We specifically don't want that. That's why it's not using libraries.

@reekris for #3 at Drupal 8.1 there is a new CKEditorPluginCssInterface. Checkout #2645100: CKEditorPluginCssInterface: Allow CKEditor plugins to add CSS to iframe CKEditor instances.

Clarifying issue title.

I think the case could be made for this. But it's definitely something that's pretty rarely necessary. As long as this has test coverage, I'm fine with it.

Thanks for feedback on this Wim.

I'd have to disagree that this is a minor/rare situation, however.

If you're using externally-hosted webfonts in your front end theme (I'd wager the majority of sites do) then content editors need to see those same font styles in CKEditor for consistency. Otherwise it is only quasi-WYSIWYG.

I agree fonts are quite common. But loading fonts from a 3rd party is always a bad idea. An additional point of failure (and actually a single point of failure for tens of thousands of sites). Guaranteed worse performance, because more TCP/IP connections.
Whenever possible, self-host your font(s). And subset them.

But, like I said: . As long as this has test coverage, I'm fine with it.

This would just require modifications to _ckeditor_theme_css(), so that it detects when the specified value is not actually a path. i.e. use parse_url() to detect whether it's an absolute or protocol-relative URL, and if that's the case, don't prefix it with the theme path. That's it.

#9: woot! Happy to provide reviews.

Here's the basic fix as requested in #8

I'll have to look into the tests next. I guess CKEditorLoadingTest::testLoading is a good starting point? (I'm new to this testing thing :) )

Switching to NR.

#11: That looks perfect :)

Now all we need is some test coverage. I'd add a new testExternalStylesheets method to CKEditorLoadingTest, and let first let it install theme A which has an absolute external URL, then let it install theme B, which has a root-relative external URL and finally let it install theme C, which has a relative URL (a path), to test the original case

Test only patch. Should fail!

And the full patch :)

EDIT: Oh snap, saw some nits already in the info.yml files but waiting for Wim's review first :)

#15: Thanks, that looks great!

1. +++ b/core/modules/ckeditor/src/Tests/CKEditorLoadingTest.php
   @@ -194,6 +194,35 @@ protected function testLoadingWithoutInternalButtons() {
   +    \Drupal::service('theme_handler')->install(['test_ckeditor_stylesheets_external']);
   

   Rather than repeating \Drupal::service('theme_handler') six times, let's store it in a variable.

2. +++ b/core/modules/ckeditor/src/Tests/CKEditorLoadingTest.php
   @@ -194,6 +194,35 @@ protected function testLoadingWithoutInternalButtons() {
   +    // Case 2: Install theme which has an external root-relative CSS URL.
   

   This is protocol-relative, not root-relative.

3. +++ b/core/modules/system/tests/themes/test_ckeditor_stylesheets_external/test_ckeditor_stylesheets_external.info.yml
   @@ -0,0 +1,9 @@
   +name: Test External CKEditor stylesheets
   ...
   +description: 'A theme that uses an external webfont stylesheet.'
   
   +++ b/core/modules/system/tests/themes/test_ckeditor_stylesheets_external_root_relative/test_ckeditor_stylesheets_external_root_relative.info.yml
   @@ -0,0 +1,9 @@
   +name: Test External CKEditor stylesheets
   ...
   +description: 'A theme that uses an external webfont stylesheet.'
   
   +++ b/core/modules/system/tests/themes/test_ckeditor_stylesheets_relative/test_ckeditor_stylesheets_relative.info.yml
   @@ -0,0 +1,9 @@
   +name: Test External CKEditor stylesheets
   ...
   +description: 'A theme that uses an external webfont stylesheet.'
   

   These are all identical names. Let's give them corresponding names.

4. +++ b/core/modules/system/tests/themes/test_ckeditor_stylesheets_external/test_ckeditor_stylesheets_external.info.yml
   --- /dev/null
   +++ b/core/modules/system/tests/themes/test_ckeditor_stylesheets_external_root_relative/test_ckeditor_stylesheets_external_root_relative.info.yml
   

   s/root/protocol/

#16:

+++ b/core/modules/ckeditor/ckeditor.module
@@ -86,7 +87,12 @@ function _ckeditor_theme_css($theme = NULL) {
       foreach ($css as $key => $path) {

Let's rename $path to $url.

Other remarks:

1. We need to update hook_ckeditor_css_alter()'s documentation in ckeditor.api.php.
2. We need a change record.
3. The documentation at https://www.drupal.org/developing/api/8/ckeditor is sufficient already.

hi,

i am a newbie , this is my first path ever. uploading my first path here. i have just incorporated point #2 from comment #15 .

#19: Thanks! Can you please provide an interdiff? See https://www.drupal.org/documentation/git/interdiff.

Uploading a new patch with inter-diff.
Thanks for your reply.

Thanks! Great first patch :)

That means #18.2 is now solved, which leaves #18.1, #18.3 and #18.4, as well as the remarks at the bottom of #18.

Uploaded my first patch ever thanks @snehi and @jagjit for help.
Renamed $path to $url as mentioned for #16.

Thank you!

Temporarily assigning to me to work on it.

@avinashm: thanks, that also looks good :)

Here is the updated patch as per #18

Note for #18-4: Couldn't apply s/root/protocol due to test_ckeditor_stylesheets_external_protocol_relative is over the maximum allowed length of 50 characters I changed it to test_ckeditor_stylesheets_protocol_relative.

Here is the change record draft too: https://www.drupal.org/node/2717985

Perfect!

CR looks great (I made only minor changes).

I think this is ready.

Nice tests! Committed 0dd68ab and pushed to 8.2.x. Thanks!

Isn't this more of a bug and therefore eligible for 8.1.x too?

It wasn't a bug; it was intentional that we only supported files in the theme originally. But you could argue that was an oversight. This is extremely low risk, so I'd be fine with committing it to 8.1 too.

@Wim Leers but if I'm reading the code right on 8.1.x atm using an external url will result in 404s. Since we're just doing $css[$key] = $theme_path . '/' . $path;. If we'd chosen to error on an external URL I'd agree.

Good point. Though I don't know if it'd 404, I don't know how a browser would parse something like /themes/foobar/http://example.com/webfont.css, and I'd suspect there might be differences between browsers.

Only committing to 8.2 to be on the safe side is fine for me.

Discussed with @catch and we decided that fixing this in 8.1.x was ok.

Committed d8ee7e2 and pushed to 8.1.x. Thanks!

FYI: #3099662: Allow ckeditor_stylesheets to refer to a Drupal root URL