I know that it is possible to load custom css to all ckeditor instances by specifying 'ckeditor_stylesheets' in my theme.
The problem I encountered with this was that I needed to load an external css (containing webfonts). This does not work since I cannot define an external css file like I do when defining a library, which is:
font:
version: 1.0
css:
base:
//cloud.webtype.com/css/XXXXXXXXXXXXX.css: { type: external }
One way to fix this would be to make it possible to pass in an object to ckeditor_stylesheets instead of a list of files. But wouldn't it be even nicer if we could attach libraries to be loaded instead? Then we could attach internal and external css and even javascript if the library contained that. Something like:
ckeditor_libraries:
- theme/font
Comment | File | Size | Author |
---|---|---|---|
#26 | 2682723-26.patch | 6.02 KB | thpoul |
#26 | interdiff-2682723-23-26.txt | 5.95 KB | thpoul |
#23 | interdiff.txt | 802 bytes | avinashm |
#23 | 2682723-23.patch | 5.44 KB | avinashm |
#21 | interdiff.txt | 827 bytes | jagjitsingh |
Comments
Comment #2
Wim LeersWe specifically don't want that. That's why it's not using libraries.
Comment #3
reekris CreditAttribution: reekris commentedOh I see :)
But what about the possibility of adding css in 'object form' then to make it possible to attach external css?
Or maybe a check could be added to the css path so that it supports absolute urls? Now it prefixes each css with my theme path even if it's an absolute url to an external resource
My current workaround is to use the hook for adding css which is working fine. Except that it seems it needs to be added in a module and not a theme. So I have a custom module only containig this hook and referencing css in my theme, which doesn't feel right.
Comment #4
thpoul CreditAttribution: thpoul at Pixual commented@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.Comment #5
reekris CreditAttribution: reekris commented@thpoul thanks for that link!
Although it seems for my use case a ckeditor plugin seems to be something more complex than what I want. I'm not adding any custom button or other functionality to my ckeditor, I simply wish to add external css to the ckeditor iframe without having to create a custom module to implement the hook.
Since adding libraries is not supported by design, I would say that adding support for external css under the
ckeditor_stylesheets
key eould be a good solution.But maybe I'm just nitpicking, the hook solution I went with is working fine so its actually not that big a deal :)
Comment #6
Wim LeersClarifying 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.
Comment #7
breezeweb CreditAttribution: breezeweb commentedThanks 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.
Comment #8
Wim LeersI 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:
This would just require modifications to
_ckeditor_theme_css()
, so that it detects when the specified value is not actually a path. i.e. useparse_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.Comment #9
tstoecklerComment #10
Wim Leers#9: woot! Happy to provide reviews.
Comment #11
maggo CreditAttribution: maggo at UEBERBIT GmbH commentedHere'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 :) )
Comment #12
thpoul CreditAttribution: thpoul at Pixual commentedSwitching to NR.
Comment #13
Wim Leers#11: That looks perfect :)
Now all we need is some test coverage. I'd add a new
testExternalStylesheets
method toCKEditorLoadingTest
, 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 caseComment #14
avinashm CreditAttribution: avinashm commentedComment #15
thpoul CreditAttribution: thpoul at Pixual commentedTest only patch. Should fail!
Comment #16
thpoul CreditAttribution: thpoul at Pixual commentedAnd the full patch :)
EDIT: Oh snap, saw some nits already in the info.yml files but waiting for Wim's review first :)
Comment #18
Wim Leers#15: Thanks, that looks great!
Rather than repeating
\Drupal::service('theme_handler')
six times, let's store it in a variable.This is protocol-relative, not root-relative.
These are all identical names. Let's give them corresponding names.
s/root/protocol/
#16:
Let's rename
$path
to$url
.Other remarks:
hook_ckeditor_css_alter()
's documentation inckeditor.api.php
.Comment #19
jagjitsingh CreditAttribution: jagjitsingh as a volunteer and at Publicis Sapient for Publicis Sapient commentedhi,
i am a newbie , this is my first path ever. uploading my first path here. i have just incorporated point #2 from comment #15 .
Comment #20
Wim Leers#19: Thanks! Can you please provide an interdiff? See https://www.drupal.org/documentation/git/interdiff.
Comment #21
jagjitsingh CreditAttribution: jagjitsingh as a volunteer and for Publicis Sapient commentedUploading a new patch with inter-diff.
Thanks for your reply.
Comment #22
Wim LeersThanks! 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.
Comment #23
avinashm CreditAttribution: avinashm as a volunteer and at Publicis Sapient for Publicis Sapient commentedUploaded my first patch ever thanks @snehi and @jagjit for help.
Renamed $path to $url as mentioned for #16.
Comment #24
thpoul CreditAttribution: thpoul at Pixual commentedThank you!
Temporarily assigning to me to work on it.
Comment #25
Wim Leers@avinashm: thanks, that also looks good :)
Comment #26
thpoul CreditAttribution: thpoul at Pixual commentedHere 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 totest_ckeditor_stylesheets_protocol_relative
.Comment #27
thpoul CreditAttribution: thpoul at Pixual commentedHere is the change record draft too: https://www.drupal.org/node/2717985
Comment #28
Wim LeersPerfect!
CR looks great (I made only minor changes).
I think this is ready.
Comment #30
alexpottNice 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?
Comment #32
Wim LeersIt 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.
Comment #33
alexpott@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.Comment #34
Wim LeersGood 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.
Comment #35
alexpottDiscussed with @catch and we decided that fixing this in 8.1.x was ok.
Committed d8ee7e2 and pushed to 8.1.x. Thanks!
Comment #38
jibranFYI: #3099662: Allow ckeditor_stylesheets to refer to a Drupal root URL