Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Currently wysiwyg_get_css() assumes the CSS is local, but that's not necessarily the case in D7, where drupal_add_css() can take external CSS. The attached patch makes external CSS work, loading in the editor iframe when "Use theme CSS" is selected. Tested in TinyMCE.
It would be nice to have some way to do external CSS in D6 WYSIWYG, where drupal_add_css() isn't an option, but I'm not sure how that would work.
Comment | File | Size | Author |
---|---|---|---|
#5 | externalcss.patch | 755 bytes | sreynen |
externalcss.patch | 576 bytes | sreynen |
Comments
Comment #1
sreynen CreditAttribution: sreynen commentedOne way to get external CSS working in D6 is creating a wysiwyg_add_external_css() function to clone the functionality in D7 drupal_add_css(). Then anything that adds external CSS via drupal_set_html_head() in D6 could use wysiwyg_add_external_css() to make the CSS available in WYSIWYG. That doesn't seem like a great solution, but I can't think of anything better.
Comment #2
TwoDMaybe a nitpick, but I'd rather do the external check before
file_exists()
to completely avoid the disk access when it's not needed. ;)Before this can be committed, we need to make sure all editors can handle external stylesheets as well, since the code running here applies to all of them.
I've summarized the current level of stylesheet support in #901862-6: Editor does not load multiple CSS files and I could add an "External files" column there as well. I don't have time now to run the tests personally, but maybe someone else does.
Comment #3
sreynen CreditAttribution: sreynen commentedI can do tests. Should I bother continuing after I find one editor that doesn't support external CSS? Is the general approach for WYSIWYG to do conditional support for functionality in specific editors or to only support functionality that works everywhere?
Comment #4
TwoDGreat!
Please continue even if some editors fail the test. It's better to exactly which ones that don't support a feature rather than "at least one".
We try to support as many features as possible for each editor, but so far we've been limited by the profile configuration GUI which can't be customized for each editor (not all widgets available have an effect in all editors).
But now my point is more that this feature is implemented in wysiwyg.module, rather than in the editor-specific .inc files, so each .inc file must be able to "opt-in" on external stylesheet support. Though, if every editor appears to support, or at least won't take harm from, this featue that might not be needed now.
Comment #5
sreynen CreditAttribution: sreynen commentedTest results, all in Safari:
CKEditor: Works.
FCKeditor: Works.
jWYSIWYG: Doesn't work. Local CSS also doesn't load. External CSS has no impact.
markItUp: Unable to test. WYSIWYG doesn't load at all.
NicEdit: Works.
openWYSIWYG: Unable to test. I get "openWYSIWYG does not support your browser" message.
TinyMCE: Works.
Whizzywig: Doesn't work. Local CSS also doesn't load. External CSS has no impact.
WYMeditor: Doesn't work. Local CSS also doesn't load. External CSS has no impact.
YUI editor: Unable to test. WYSIWYG doesn't load at all. JS error: "TypeError: Result of expression 'this._buttonList' [null] is not an object."
So out of the 10 editors, 4 work, 3 don't work but seem to cause no problems, and 3 I couldn't test CSS because they didn't load at all.
Updated patch has the external check first.
Let me know if there's anything else I can do to move this forward.
Comment #6
TwoDWhizzywig needs the global JavaScript variable
cssFile
set to load a stylesheet. We don't currently set it, but can you try doing that by modifying the .js implementation file?WYMeditor only loads one file (the first one), so try specifying it using Editor CSS: Define CSS and CSS path: path-to-external-styleheet on admin/settings/wysiwyg/profile/#/edit or by overriding the value completely via hook_wysiwyg_editor_settings_alter().
If you get openWYSIWYG running in another browser, the current implementation overrides the stylesheets in the clientside implementation, so you might have to modify that too to test it.
YUI Editor needs at least one toolbar button enabled to not throw that error.
MarkItUp, WYMeditor and jWYSIWYG don't support stylesheets at all, so no surprise there. :/
Comment #7
sreynen CreditAttribution: sreynen commentedAdditional test results:
Whizzywig: I'm not sure which file you mean by "the .js implementation." I can hard-code external CSS in whizzywig.js and that works, but doesn't seem like a very relevant test. I don't see where CSS is being sent to editors in any of the JS files in the module.
WYMeditor: Does not work. External CSS has no impact.
openWYSIWYG: Does not work. External CSS has no impact.
YUI editor: Works.
So everything either works or has no impact. Nothing breaks with external CSS. Is that enough to call this RTBC, or do you want to try to fix all of the editors that don't handle external CSS?
Comment #8
TwoDI meant wysiwyg/editors/js/whizzywig-60.js (and the earlier versions too, but testing one is enough).
You'll notice that we set the global
window.buttonPath
variable before attaching the editor (so it can be different for different fields), the same thing would have to be done withwindow.cssFile
.Things not breaking because of this change is very positive. Fixing stylesheet handling (where the editor supports it but Wysiwyg doesn't) is a different issue, the importand part here is that editors won't crash or behave strange when they see [absolute] URLs from another domain.
Comment #9
sreynen CreditAttribution: sreynen commentedSetting window.cssFile to a specific external CSS URL works fine in whizzywig, though I was only able to do that with a hard-coded URL because I don't know how to get the theme CSS in that JS.
So I can confirm both that external CSS breaks nothing in the current whizzywig code, and also that it would break nothing in hypothetical future code that sets window.cssFile to a full external URL.
More generally, I can confirm the above patch breaks nothing in any editor and adds the desired functionality in some editors. It sounds like that's enough for RTBC.
Comment #10
sreynen CreditAttribution: sreynen commentedAnything I can do to move this forward? I need this to add WYSIWYG integration in @font-your-face #896024: Wysiwyg Integration.
Comment #11
TwoDI just posted a patch to #901862: Editor does not load multiple CSS files which complements your patch in #5 quite well and I agree on this patch's RTBC status.
When I get some more time, I'll test with @font-your-face as well.
Comment #12
pillarsdotnet CreditAttribution: pillarsdotnet commentedComment #13
kla2t CreditAttribution: kla2t commented#9: There is no need to set window.cssFile to a hard-coded URL. Only these lines have to be added in whizzywig-60.js after line 22:
- otherwise whizzywig.js cannot assign any value to its cssFile variable.
Might be worth to be reviewed!
Comment #14
sreynen CreditAttribution: sreynen commentedAs whizzywig does not need to accept external CSS for this patch to be useful, and this has been waiting for 9 months on a patch that has no apparent problems, can we please handle that in a separate issue?
Comment #15
sreynen CreditAttribution: sreynen commentedI'm moving this back to RTBC, as #13 is an additional feature. If there's anything I can do to move this forward, please let me know.
Comment #16
Anthony Pero CreditAttribution: Anthony Pero commentedBump.
Need this for @font-your-face #896024: Wysiwyg Integration.
Can this be rolled into the module yet?
Comment #17
sunSorry for the delay!
Thanks for reporting, reviewing, and testing! Committed to 7.x-2.x.
A new development snapshot will be available within the next 12 hours. This improvement will be available in the next official release.