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.
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.
Comment | File | Size | Author |
---|---|---|---|
#37 | select_which_theme_s-1309040-37.patch | 10.46 KB | joegraduate |
#35 | wysiwyg-select-theme-css-1309040-35.patch | 10.43 KB | jhedstrom |
#35 | interdiff.txt | 769 bytes | jhedstrom |
#32 | wysiwyg-1309040-32-select-theme-styles.patch | 10.47 KB | joegraduate |
#28 | wysiwyg-1309040-28-select-theme-styles.patch | 10.78 KB | Alan D. |
Comments
Comment #1
willvincent CreditAttribution: willvincent commentedHere'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.
Comment #2
willvincent CreditAttribution: willvincent commentedOops.. 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.
Comment #3
TwoDThanks 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.
Comment #4
willvincent CreditAttribution: willvincent commentedNo 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.
Comment #5
fenstratThe 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.
Comment #6
TwoDThanks 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:
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)?
Comment #7
fenstrat@TwoD, no worries, thanks for the feedback, sorry this fell off my radar for a while.
Addressing #6
Drupal.wysiwygAttach
but am unsure where a message could be added.file_exists()
check inwysiwyg_get_css()
is gone.$theme
param towysiwyg_get_css($theme = '')
which will ensure backwards compatibility.Comment #8
adamdicarlo CreditAttribution: adamdicarlo commentedThis 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.... :/
Comment #9
mstrelan CreditAttribution: mstrelan commentedI 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.
Comment #10
fenstratThe 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.Comment #11
Alan D. CreditAttribution: Alan D. commentedI 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().
Comment #12
fenstratThanks @Alan D.
I think this patch is close, would be great to get more reviews.
Comment #13
PieterDCManually applied patch to latest 7.x dev version and tested with CKeditor, which works.
Comment #14
fenstrat@PieterDC Good to hear it worked for you, care to mark it as RTBC? Hopefully that'll get a few more eyes on it.
Comment #15
Yuri CreditAttribution: Yuri commentedThe patch in #12 doesn't work any more because of latest dev release.
Please update and I'll test.
Comment #16
fenstrat@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.
Comment #17
Alan D. CreditAttribution: Alan D. commentedI 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:
need updating to this
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)
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.
Comment #18
fenstrat@Alan D. thanks for the review.
Close to RTBC?
Comment #19
Alan D. CreditAttribution: Alan D. commentedI 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)
Comment #20
fenstrat.... so RTBC?
Comment #21
Alan D. CreditAttribution: Alan D. commentedLets see what sun or TwoD think.
Comment #22
jyee CreditAttribution: jyee commentedLine 46 in patch #18 appears to have a typo for $css_thene which should be $css_theme.
Comment #23
fenstratGood catch @jyee! Still RTBC for when sun or TwoD get a chance to take a look at it.
Comment #24
TwoDThank 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. ;)
Comment #25
clarke.hackworth@gmail.com CreditAttribution: clarke.hackworth@gmail.com commentedI had exactly this issue and found it very anonying. #22 worked for me, although I had to manually patch some files.
Comment #26
Elijah LynnThanks everyone for the work on this so far. TwoD, how is the progress of the larger, overall patch coming?
Comment #27
Alan D. CreditAttribution: Alan D. commentedNote 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?.
Comment #28
Alan D. CreditAttribution: Alan D. commentedHad 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).
Comment #29
willvincent CreditAttribution: willvincent commented#28 is working nicely for me.
Comment #30
Bernsch CreditAttribution: Bernsch commented#28 is work for me.
Comment #31
hefox CreditAttribution: hefox commentedThis is odd language and likely even more confusing if someone isn't using admin theme.
Other should likely be in a t, e.g. t('Other') (same with usage below it)
Is the note needed considering using #states?
I really find this 'node admin theme' confusing. Wysiwyg is used outside of node forms so really it's "Active theme," right?
...now I get it, but only by looking at the code
Comment #32
joegraduateRe-rolled Alan D's patch in #28 against the latest dev version.
Comment #33
TwoD@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.
Comment #34
mstrelan CreditAttribution: mstrelan commentedThe 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.Comment #35
jhedstromHere 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.Comment #36
jmuzz CreditAttribution: jmuzz commentedVery nice feature. Works great for me and was not confused.
Comment #37
joegraduateThe attached patch is a re-roll of #35 which applies cleanly to the latest dev (7.x-2.x).
Comment #38
rooby CreditAttribution: rooby commentedWhen I apply the patch in #37 I lose vertical scroll capabilities in the text area.
Comment #39
TwoDThank 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()
)Comment #40
rooby CreditAttribution: rooby commentedOh yeah, that makes sense. Thanks for the info.