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.

CommentFileSizeAuthor
#5 externalcss.patch755 bytessreynen
externalcss.patch576 bytessreynen
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sreynen’s picture

One 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.

TwoD’s picture

Maybe 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.

sreynen’s picture

I 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?

TwoD’s picture

Great!

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.

sreynen’s picture

FileSize
755 bytes

Test 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.

TwoD’s picture

Whizzywig 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. :/

sreynen’s picture

Additional 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?

TwoD’s picture

I 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 with window.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.

sreynen’s picture

Status: Needs review » Reviewed & tested by the community

Setting 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.

sreynen’s picture

Anything I can do to move this forward? I need this to add WYSIWYG integration in @font-your-face #896024: Wysiwyg Integration.

TwoD’s picture

I 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.

pillarsdotnet’s picture

kla2t’s picture

Status: Reviewed & tested by the community » Needs review

#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:

  // Assign the external css file path, if available.
  if (settings.externalCSS) {
    window.cssFile = settings.externalCSS;
  }

- otherwise whizzywig.js cannot assign any value to its cssFile variable.

Might be worth to be reviewed!

sreynen’s picture

As 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?

sreynen’s picture

Status: Needs review » Reviewed & tested by the community

I'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.

Anthony Pero’s picture

Bump.

Need this for @font-your-face #896024: Wysiwyg Integration.

Can this be rolled into the module yet?

sun’s picture

Version: 7.x-2.0 » 7.x-2.x-dev
Status: Reviewed & tested by the community » Fixed

Sorry 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.

Status: Fixed » Closed (fixed)

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