It would be nice to be able to select which theme's css to apply when you opt to use the 'use css' setting for wysiwyg profile configuration.

The default behavior is to use the current theme, which is fine, unless you're using the admin theme for content editing. In that instance the current theme is the admin theme so content in the editor doesn't properly reflect how it will look on the front end.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

willvincent’s picture

Here's a patch that seems to work.. needs review.

I've supplied a git patch, and an older p0 format that will apply with older versions of drush make.

willvincent’s picture

Oops.. the previous patch doesn't handle stylesheets from a base theme if the selected theme is a sub theme.

These will handle that, but only a subtheme from a single base theme, not a sub theme of another sub theme.. that part would need more work.

TwoD’s picture

Thanks for the patch! Always nice to see feature requests with actual code! :)

There are already some ongoing discussions regarding themes and I'd like to get your opinion on those. In essence, pros/cons vs your approach, a kind of self-review since I don't have time to do it myself yet. Hope you don't mind. ;)
An alternate approach is discussed in #1017564: Allow user to dynamically change in-editor theme (no patch yet but a nice mockup).
#835682: %t (path_to_theme() ) returns modules/field (or whatever module is creating the wysiwyg) instead of the active theme. is most likely relevant to this discussion as well.

willvincent’s picture

No problem! As a module maintainer myself I always appreciate when people actually provide patches as well, so I generally try to return the favor whenever possible. :)

I looked at and commented on #1017564: Allow user to dynamically change in-editor theme

I'll try to read through #835682: %t (path_to_theme() ) returns modules/field (or whatever module is creating the wysiwyg) instead of the active theme. tomorrow.

fenstrat’s picture

The discussion in #835682: %t (path_to_theme() ) returns modules/field (or whatever module is creating the wysiwyg) instead of the active theme. is very useful, however it's really unrelated to this issue as it deals with the path to the theme when "Editor CSS" is set to "Define CSS", i.e. manually entering entering the path to specific stylesheet files.

The ideas in #1017564: Allow user to dynamically change in-editor theme are great, but require quite a bit of work to be fully functional.

I think the patch by willvincent here in #2 is the correct approach to allow setting which theme's css styles are loaded into the editor.

I've updated the patch:

  1. 1. Added wysiwyg_update_7201() to add the css_theme setting to existing profiles. This is needed when each editor loads theme css with wysiwyg_get_css($config['css_theme']) the css_theme key must be set for the profile.
  2. 2. Updated the stylesheet loading for a given theme to account for variations in stylesheet declarations. Still not sure if this is the best way, is there a simpler way to get all of a themes css?
    The method here loads any base_theme css (I've changed the key from 'base theme' to 'base_theme'), and then any theme css, but only if they're not for print. This also removes the assumption that stylesheets['all'] is always populated (e.g. seven places its css in stylesheets['screen'] leaving stylesheets['all'] empty). As mentioned, this loading doesn't account for sub themes of another sub theme.
  3. Added wysiwyg_get_css() parameter changes to TinyMCE and openWYSIWYG editor inc files as these were missed in the patch in #2.
TwoD’s picture

Thanks for the patches, arguments and detailed analysis!

@fenstrat, yes, #835682: %t (path_to_theme() ) returns modules/field (or whatever module is creating the wysiwyg) instead of the active theme. does not seem to affect this issue directly.

Wow, getting the correct stylesheets is getting complicated, but I guess that is to be expected considering how complex themes can be.

A couple of questions came into mind while reading the patch (haven't tested it yet).

  1. Preview mode might not show you what you expect anymore since it doesn't use the current theme's stylehseets. But again, preview wasn't that useful before when using an admin theme.
  2. The user might not realize which theme the styles applied in the editor come from at all, unless they inspect it with a developer tool. Can we do something about that?
  3. Maybe someone would like to keep the 'dynamic' nature of the "Use theme CSS" option, how about defaulting to a "User's them" option in the Theme list? Or add another option to the first list?
  4. Do we really need that file_exists() check in wysiwyg_get_css()? If we skip it we only risk requesting a stylesheet that doesn't exist. By skipping request for files Wysiwyg knows doesn't exist, we're hiding that fact from the admins/developers. (No entries in the 404 logs, no markup to indicate the files should exist.) It looks like drupal_get_css() and drupal_add_css() don't use file_exists(), so why should we?
  5. I'm starting to think it might be nice to have both this feature, and the one discussed in #835682: %t (path_to_theme() ) returns modules/field (or whatever module is creating the wysiwyg) instead of the active theme.. The theme we pick in profile configuration could then serve as the default theme for the switcher in the other issue's mockup, if the configuration allows it at all. That other issue would, as mentioned before, require quite a bit of work and AJAX magic to pull off, so I think it would be best to focus on this part first. (I see now that willvincent already thought of something like that in the other issue.)

Patch notes:

+++ b/wysiwyg.moduleundefined
@@ -602,28 +602,46 @@ function wysiwyg_get_editor_config($profile, $theme) {
  */
-function wysiwyg_get_css() {
+function wysiwyg_get_css($theme) {

Could we default to using the current theme from some global to perhaps avoid the update hook and/or not break backwards compatibility (in the odd case someone other than Wysiwyg actually uses the function)?

fenstrat’s picture

@TwoD, no worries, thanks for the feedback, sorry this fell off my radar for a while.

Addressing #6

  1. As you say, this will not work with preview mode. However this is nothing different from other custom Editor CSS settings. Without putting the preview in an iframe where the appropriate css could be loaded is there a way around this? Is it a deal breaker?
  2. Understand the user has no way of knowing what theme is loaded in the editor. Not really sure how to overcome this, looked at Drupal.wysiwygAttach but am unsure where a message could be added.
  3. I can see why someone might want to keep the 'dynamic' nature of the "Use theme CSS" option. So I've added the default value '' which means "Node admin theme". May do with user friendly name, but it does indicate what it does.
  4. Agreed, file_exists() check in wysiwyg_get_css() is gone.
  5. As mentioned, #835682: %t (path_to_theme() ) returns modules/field (or whatever module is creating the wysiwyg) instead of the active theme. doesn't really effect this issue.
  6. I've added a default value for the new $theme param to wysiwyg_get_css($theme = '') which will ensure backwards compatibility.
adamdicarlo’s picture

Status: Needs review » Needs work

This patch is not completely compatible with Omega subthemes -- if I select my Omega subtheme, its grid CSS does not get loaded in the WYSIWYG iframe. Probably due to the trickery involved in how Alpha/Omega manage CSS. I have no idea what the fix would be, unfortunately, but I figure I should reset to needs work.... :/

mstrelan’s picture

I would suggest allowing this to be set to "admin theme", "default theme", "node admin theme" and "other theme". That way if I were to export all the WYSIWYG settings I would probably have it set to "default theme" and could reuse the same settings on a new website. Sorry if this has already been brought up, I have just skimmed through the discussions and latest patch.

fenstrat’s picture

Status: Needs work » Needs review
FileSize
9.7 KB

The attached patch addresses the reviews in #8 and #9 and is based off current 7.x-2.x-dev.

  1. Better support for sub themes by correctly adding parent theme css (required changing $theme->info['base_theme'] to $theme->info['base theme'] because in the info key no uderscore is used, odd as up a level an undescore is used).
  2. Re #8: Now works with Omega sub themes. Unfortunatly to get omega's global.css included I had to look into the $theme->info['css'] as Omega adds its custom css to the css key in the info file, not the stylesheets key as other themes do.
    Also note that Omega grid css will not be loaded. As they're loaded with media queries basically I don't think it's worth including them.
  3. Re #9: Added additional theme options so the default theme can be set once, exported and used across multiple sites.
    Available options: Node admin theme (default), Admin theme, Default theme, Other (which contains a list of all available themes). I've key value for Admin theme is wysiwyg_theme_admin - "wysiwyg_theme_" prefix is to avoid clashes with other theme keys, better ideas to work around this most welcome.
  4. As TR notes in #6 4. there's little point in the file_exists() check when including stylesheets. So despite a recent commit in #1048300: Handle external CSS in wysiwyg_get_css() which keeps the file_exists() check and adds a work around for external css in the patch I've removed it all together.
Alan D.’s picture

I like, although I can not see many use cases for selecting non-default / admin themes.

These are some of the issues I found:

  • If you have multiple Editor profiles, the code will always return the same CSS based on the first editor call
  • it is not resettable in the tests, although I'm not sure if that is an issue atm.
  • no safeguards on the theme access
  • no checks that get_variable() actually return anything

The attached patch tries to resolve these in wysiwyg_get_css().

  $themes = list_themes();
  // Maybe we need drupal_theme_access() here too?
  if (empty($theme)) {
    // Seems draft, but this is how core does it.
    $theme = menu_get_custom_theme();
  }
  if ($theme == 'wysiwyg_theme_admin') {
    // This may not be set and user may not have access
    if ($admin_theme = variable_get('admin_theme') && drupal_theme_access($admin_theme)) {
      $theme = $admin_theme;
    }
  }
  // Fallback
  if (!$theme || !isset($themes[$theme])) {
    $theme = variable_get('theme_default', 'bartik');
  }
fenstrat’s picture

Thanks @Alan D.

  1. Multiple editors weren't handled, your addition of $files[$theme] handles it well.
  2. Use of drupal_static does indeed make it resettable for test (though there are no module tests atm). Also drupal_static is d7+, so will need to be changed back if backported to 6.x
  3. drupal_theme_access() is again only d7, and only checks if a theme is enabled (or is the admin theme), it doesn't check a users access to the theme. So not sure this is needed because if admin has enabled it as the WYSIWYG theme it had to be enabled in the first place. I've removed it in the attached patch.
  4. menu_get_custom_theme() "Gets the custom theme for the current page, if there is one.". If theme is passed in as an empty string (which it is when no value is set) then it should mimic the existing behaviour, which is use node admin theme if set, otherwise site default. I've reverted this.

I think this patch is close, would be great to get more reviews.

PieterDC’s picture

Manually applied patch to latest 7.x dev version and tested with CKeditor, which works.

fenstrat’s picture

@PieterDC Good to hear it worked for you, care to mark it as RTBC? Hopefully that'll get a few more eyes on it.

Yuri’s picture

The patch in #12 doesn't work any more because of latest dev release.
Please update and I'll test.

fenstrat’s picture

@Yuri Not sure why you couldn't apply it as it applied cleanly just with some offset/fuz for me.

Anyway, here's a reroll of #12.

Alan D.’s picture

I have not experienced this on my local install (I'm not running PHP 5.3), but this may throw notices as PHP's reset() expects a referenced array variable. This would also apply to the existing code.

So bits like this:

-        $settings['contentsCss'] = reset(wysiwyg_get_css());
+        $settings['contentsCss'] = reset(wysiwyg_get_css(isset($config['css_theme']) ? $config['css_theme'] : ''));

need updating to this

-        $settings['contentsCss'] = reset(wysiwyg_get_css());
+        $css_theme = wysiwyg_get_css(isset($config['css_theme']) ? $config['css_theme'] : ''));
+        $settings['contentsCss'] = reset($css_theme);

A couple of random examples.
#1188612: Strict warning: Only variables should be passed by reference in node_export()
#1513832: [crm_core_contact] Strict PHP Warnings

And I'm not sure if the Omega theme needs the plug (even though I do like the theme)

+    // Add any base theme css set in the 'css' key (e.g. Omega theme).

Maybe, "Collect and add CSS defined by any base themes."

Otherwise, applies nicely to the dev from 2012-06-26 (the last compatible dev version with #741606: Teaser splitter / text fields with summary support atm, and the styles are coming though nicely.

I wonder if the reset() issue should be moved to another issue, but this patch seems to cover the only two lines of code where this happens.

fenstrat’s picture

@Alan D. thanks for the review.

  1. reset() changes makes sense here, seeing as this patch is touching all instances of reset(). I also standardised the variable naming to $css_theme.
  2. Removed " (e.g. Omega theme)". Your suggestion of "Collect and add CSS defined by any base themes." doesn't make sense as that is already done above, rather this is additional check for values in the 'css' key is indeed specific to certain themes (Omega being one of those). But I've got no problems removing the reference to Omega in particular.

Close to RTBC?

Alan D.’s picture

Issue tags: +Usability

I thought it would have been ready much earlier. I only happened to remember about the other issue by chance after applying this to yet another site. :)

This is a massive increase in usability (aka, developers are likely to configure this now so styles actually come through)

fenstrat’s picture

.... so RTBC?

Alan D.’s picture

Status: Needs review » Reviewed & tested by the community

Lets see what sun or TwoD think.

jyee’s picture

Line 46 in patch #18 appears to have a typo for $css_thene which should be $css_theme.

fenstrat’s picture

Good catch @jyee! Still RTBC for when sun or TwoD get a chance to take a look at it.

TwoD’s picture

Status: Reviewed & tested by the community » Needs work

Thank you for the continued work on this issue!

I like the ideas but I think the implementation makes a couple of assumptions that could produce unexpected consequences.

For example, the old code was able to include any stylesheets which had been conditionally added using drupal_add_css(), while here we only get the files belonging to the theme itself. Any stylesheets not included in any .info files but added to the theme group by modules or the theme itself will be missed. Naturally, that was a lot easier to do with the old code because only the active theme could be used in the editor. If we could combine this method with fetching a file list from drupal_add_css(), but filter out already included files and those belonging to another theme, we'd be more backwards compatible and get the benefit of picking a theme and getting conditional stylesheets. (This may include having to duplicate a bit of core code to invoke hooks in the picked theme.)

The current code checks if the selected theme has a base theme and includes those stylesheets, but the base theme can also have a base theme - Omega's starterkit depends on Omega, which in turn depends on Alpha... The code may also add multiple stylesheets with the same filename [from different paths], which breaks overrides.

External stylesheets (added via drupal_add_css()) will be ignored after applying the patch.

The part fetching stylesheets added to the 'css' list in .info files - like Omega does - is interesting, but remember that those files are optional. We'd also have to inspect the theme's settings to know if to actually include each stylesheet. I'm not sure we'd be able to keep up with any changes made to themes using this method if we implement that logic ourselves. Perhaps time for a new "hook_wysiwyg_edititing_stylesheets()"? Putting the logic in other modules would allow for quicker adaptation when the theme changes, and a custom site-specific module could be used to more easily include any stylesheet. Using hook_wysiwyg_editor_settings_alter() for that is tricky since the settings have before that been converted to the structure used internally by each editor.

Because dealing with all the above can get a bit complex, I would like to simplify/minimize the code in the editor implementations and figure out which stylesheets to include just before calling the implementation's settings hook. That would also allow for caching the final list per theme if more than one editor is used. That would also make a nice place to add additional stylesheet overrides (Color module integration!). In here it wouldn't hurt so much to make a few file_exists() calls to determine whether an override actually overrides or removes a stylesheet from a parent theme.

I've already addressed most of the above points as part of a larger patch based on this one, and a couple others related to in-editor stylesheets. The use cases for the issues I aim to merge are slightly different, but I foresee them causing implementation conflicts and code duplication if not dealt with in one sweep. I've been busy with other projects so I've not had time to split out the parts relevant to this issue, but I'll post a patch when I can. If the changes start to become too interdependent, I'll make a new issue to merge all the other ones the patch aims to fix.

Sorry for lots of words and little code so far. ;)

clarke.hackworth@gmail.com’s picture

I had exactly this issue and found it very anonying. #22 worked for me, although I had to manually patch some files.

Elijah Lynn’s picture

Thanks everyone for the work on this so far. TwoD, how is the progress of the larger, overall patch coming?

Alan D.’s picture

Note that this will fix the 404's that constantly clog up the watchdog tables. One example is #1697996: Fetching Image css classes from theme stylesheet not working?.

Alan D.’s picture

Had to re-roll for v2.2 so here are two old patches to tie us over till TwoD gets back to this.

One was rolled against the latest dev version, the other against the tag 7.x-2.2 (i.e. release 7.x-2.2).

willvincent’s picture

#28 is working nicely for me.

Bernsch’s picture

Status: Needs work » Reviewed & tested by the community

#28 is work for me.

hefox’s picture

+++ b/wysiwyg.admin.inc
@@ -274,7 +275,30 @@ function wysiwyg_profile_form($form, &$form_state, $profile) {
+  $theme_list = array(
+    '' => t('Node admin theme'),
+    'wysiwyg_theme_admin' => t('Admin theme'),

This is odd language and likely even more confusing if someone isn't using admin theme.

+++ b/wysiwyg.admin.inc
@@ -274,7 +275,30 @@ function wysiwyg_profile_form($form, &$form_state, $profile) {
+    'Other' => array(),

Other should likely be in a t, e.g. t('Other') (same with usage below it)

+++ b/wysiwyg.admin.inc
@@ -274,7 +275,30 @@ function wysiwyg_profile_form($form, &$form_state, $profile) {
+    '#description' => t("Select which theme's CSS to apply to the editor.<br />Note: This is only applied when 'Editor CSS' field above is set to 'Use theme CSS'"),

Is the note needed considering using #states?

+++ b/wysiwyg.module
@@ -638,32 +638,77 @@ function wysiwyg_get_editor_config($profile, $theme) {
+ *   the current node admin theme.

I really find this 'node admin theme' confusing. Wysiwyg is used outside of node forms so really it's "Active theme," right?

+++ b/wysiwyg.module
@@ -638,32 +638,77 @@ function wysiwyg_get_editor_config($profile, $theme) {
+    $theme = variable_get('node_admin_theme') ? variable_get('admin_theme') : variable_get('theme_default', 'bartik');

...now I get it, but only by looking at the code

joegraduate’s picture

Re-rolled Alan D's patch in #28 against the latest dev version.

TwoD’s picture

Status: Reviewed & tested by the community » Needs work

@hefox has some very good points. When coming back to this issue now after not looking at it in a while, I'm having a hard time predicting which theme is actually going to be used, and why.

mstrelan’s picture

The checkbox to toggle the system variable node_admin_theme is labelled "Use the administration theme when editing or creating content". That's pretty difficult to translate to a setting for WYSIWYG. This is not necessarily the same as the active theme! WYSIWYG could be used for commenting on an entity, in which case the active theme would likely be the default theme. Alan's setting for node admin theme is precisely the theme used for node administration, as set at admin/appearance.

jhedstrom’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
769 bytes
10.43 KB

Here is a reroll of #32--a conflict with #1793704: Fully process local CSS file paths had to be resolved but I think I did that by using file_create_url() in the reroll. I also tried to address the UI concerns of #31.

jmuzz’s picture

Very nice feature. Works great for me and was not confused.

joegraduate’s picture

The attached patch is a re-roll of #35 which applies cleanly to the latest dev (7.x-2.x).

rooby’s picture

When I apply the patch in #37 I lose vertical scroll capabilities in the text area.

TwoD’s picture

Status: Needs review » Closed (duplicate)
Related issues: +#2289683: Select in-editor theme and get all stylesheets

Thank you for all your hard work on this!

There are a couple of limits to this approach in that it only looks for stylesheets in a few commonly used locations. A while ago I tested a different approach in #2289683: Select in-editor theme and get all stylesheets which was able to capture most, if not all, stylesheets from all themes I've tested so far (as long as the stylesheets are not applied to only specific pages, but then we're out of luck no matter what we do.)

Today I updated that patch with the selection code from this patch and I hope you are willing to try it instead.
I will give you all credit for the work put in here when that patch gets committed. (If we get verification it does indeed work for you, that'll be ASAP.)

Sadly, there's no "merged" status for patches here, so I'll have to mark this a duplicate...

@rooby, that likely happens because of the styles the selected theme applies to the body tag. Directly using a theme's stylesheets within the editor means the content will be styled as if placed directly below the body tag. Normally you'd have lots of other tags wrapping your content, which really can't be done inside the editor unless you hack it's base content template. Some editors do allow that, but it's a bit beyond the scope of this issue. (You could probably achieve it by also implementing hook_wysiwyg_editor_settings_alter())

rooby’s picture

Oh yeah, that makes sense. Thanks for the info.