Download & Extend
Wysiwyg » Issues

Select which theme's CSS to use when choosing 'Use theme CSS' in settings

Posted by willvincent on October 13, 2011 at 7:51pm
Project:Wysiwyg
Version:7.x-2.x-dev
Component:User interface
Category:feature request
Priority:normal
Assigned:Unassigned
Status:needs work

Issue Summary

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.

Login or register to post comments

Comments

#1

Posted by willvincent on October 13, 2011 at 7:54pm
Status:active» needs review

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.

AttachmentSize
issue_1309040-Selectable_theme_for_use_theme_css_setting-p0.patch 5.83 KB
issue_1309040-Selectable_theme_for_use_theme_css_setting-p1.patch 6.34 KB

#2

Posted by willvincent on October 13, 2011 at 8:22pm

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.

AttachmentSize
issue_1309040-Selectable_theme_for_use_theme_css_setting-p0-2.patch 6.12 KB
issue_1309040-Selectable_theme_for_use_theme_css_setting-p1-2.patch 6.63 KB

#3

Posted by TwoD on October 14, 2011 at 8:32pm

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.

#4

Posted by willvincent on October 15, 2011 at 3:15pm

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.

#5

Posted by fenstrat on October 31, 2011 at 2:02pm

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.
AttachmentSize
1309040-5-select_theme_css.patch 9.01 KB

#6

Posted by TwoD on November 7, 2011 at 12:25am

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

#7

Posted by fenstrat on January 25, 2012 at 9:55am

@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.
AttachmentSize
1309040-7-select_theme_css.patch 8.6 KB
1309040-7-interdiff-5.txt 7.21 KB

#8

Posted by adamdicarlo on February 25, 2012 at 12:23am
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.... :/

#9

Posted by mstrelan on April 16, 2012 at 5:37am

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.

nobody click here