Posted by willvincent on October 13, 2011 at 7:51pm
18 followers
| Project: | Wysiwyg |
| Version: | 7.x-2.x-dev |
| Component: | User interface |
| Category: | feature request |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | needs work |
| Issue tags: | Usability |
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.
Comments
#1
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.
#2
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.
#3
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
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
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:
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.
#6
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).
file_exists()check inwysiwyg_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 likedrupal_get_css()anddrupal_add_css()don't usefile_exists(), so why should we?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
@TwoD, no worries, thanks for the feedback, sorry this fell off my radar for a while.
Addressing #6
Drupal.wysiwygAttachbut am unsure where a message could be added.file_exists()check inwysiwyg_get_css()is gone.$themeparam towysiwyg_get_css($theme = '')which will ensure backwards compatibility.#8
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
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.
#10
The attached patch addresses the reviews in #8 and #9 and is based off current 7.x-2.x-dev.
$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).$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.
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.
file_exists()check when including stylesheets. So despite a recent commit in #1048300: Handle external CSS in wysiwyg_get_css() which keeps thefile_exists()check and adds a work around for external css in the patch I've removed it all together.#11
I like, although I can not see many use cases for selecting non-default / admin themes.
These are some of the issues I found:
The attached patch tries to resolve these in wysiwyg_get_css().
<?php$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');
}
?>
#12
Thanks @Alan D.
I think this patch is close, would be great to get more reviews.
#13
Manually applied patch to latest 7.x dev version and tested with CKeditor, which works.
#14
@PieterDC Good to hear it worked for you, care to mark it as RTBC? Hopefully that'll get a few more eyes on it.
#15
The patch in #12 doesn't work any more because of latest dev release.
Please update and I'll test.
#16
@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.
#17
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:
<?php- $settings['contentsCss'] = reset(wysiwyg_get_css());
+ $settings['contentsCss'] = reset(wysiwyg_get_css(isset($config['css_theme']) ? $config['css_theme'] : ''));
?>
need updating to this
<?php- $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.
#18
@Alan D. thanks for the review.
Close to RTBC?
#19
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)
#20
.... so RTBC?
#21
Lets see what sun or TwoD think.
#22
Line 46 in patch #18 appears to have a typo for $css_thene which should be $css_theme.
#23
Good catch @jyee! Still RTBC for when sun or TwoD get a chance to take a look at it.
#24
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. ;)
#25
I had exactly this issue and found it very anonying. #22 worked for me, although I had to manually patch some files.
#26
Thanks everyone for the work on this so far. TwoD, how is the progress of the larger, overall patch coming?
#27
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?.
#28
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).