Skip to main content ↑ ↓
Proposed Fix For Variants CSS issues Started by scripple · · Read 15275 times 0 Members and 1 Guest are viewing this topic. previous topic - next topic

Proposed Fix For Variants CSS issues

So I've been campaigning for fixes to the variant system not loading the css files for the editor.  I finally decided to do something about it.  I've rewritten loadCSSFile with the following changes.

New input parameter 'variant'.  If set to true (the default) and $context['theme_variant'] is set loadCSSFile attempts to load the css file from the variant directory.  If the file does not exist there it falls back to base directory of the default theme.  (If you're up for this patch this can be fixed to fall back to the base of the current theme and then the base of the default theme.)

Otherwise the behavior is what it already was supposed to be.

However for this to work well I suggest we drop the _variant name from css files in the variant theme directory.  It's redundant and requires a bunch of unnecessary name mangling.  The different directory is sufficient to make it unique.

As it turns out index.css is loaded before $context['theme_variant'] is set so it automatically loads from the top level directory with no other changes.  But to make it less fragile the variant input parameter can be set to false for loading index.css and anything else that you want to always load a base copy of.  And to not have the base files loaded twice for the things you intend to have live beside the base set fallback = false.

So _light/index_light.css would become just _light/index.css and the index css calls would be
loadCSSFile('index.css', variant = false) <= load the main index.css
loadCSSFile('index.css', fallback = false) <= Only call this if there are variants.  Will load a _light/index.css for example but not reload the main index.css if it doesn' t exist.

This would also let the hack for loading variant files for admin.  If a variant is set it would automatically load the one from the variant otherwise it would fall back to the default.  So people who don't want to theme the admin don't have to have an admin file in their variant directories anymore.

This also lets any css file (other than the sceditor iframe which has already been handled separately) be overwritten by a variant while falling back to the base if present.  (Currently only for the default as there needs to be an outer fallback for non-default themes.  I can write that if you're willing to go forward with this.)  (And yes, I updated the cache hash so multiple variants can be in use at once.)

And as an aside, while doing this I found a few bugs in loadCSSFile.  The check for fallback = false doesn't actually work if you set fallback=false.  And if you pass in multiple file names to loadCSSFile in one call then the first one that triggers fallback will automatically cause all following files to be loaded from the default theme whether they exist or not.

So what do you think?  Is this change acceptable?

Re: Proposed Fix For Variants CSS issues

Reply #1 –

Here's the modified function.
Code: [Select]
/**
 * Add a CSS file for output later
 *
 * @param mixed $filenames string or array of filenames to work on
 * @param mixed[] $params = array()
 *         Keys are the following:
 *         - ['local'] (true/false): define if the file is local
 *         - ['fallback'] (true/false): if false  will attempt to load the file
 *             from the default theme if not found in the current theme
 *         - ['variant'] (true/false): if true will attempt to load the file
 *             from the theme variant falling back to the default theme if not available
 *         - ['stale'] (true/false/string): if true or null, use cache stale, 
 *             false do not, or used a supplied string
 * @param string $id optional id to use in html id=""
 */
function loadCSSFile($filenames, $params = array(), $id = '')
{
	global $settings, $context, $db_show_debug;

	if (empty($filenames))
		return;

	if (!is_array($filenames))
		$filenames = array($filenames);

	// Static values for all these settings
	$params['stale'] = (!isset($params['stale']) || $params['stale'] === true) ? CACHE_STALE : (is_string($params['stale']) ? ($params['stale'] = $params['stale'][0] === '?' ? $params['stale'] : '?' . $params['stale']) : '');
	$params['fallback'] = (isset($params['fallback']) && ($params['fallback'] === false)) ? false : true;
	if ((isset($params['variant']) && ($params['variant'] === false)) || empty($context['theme_variant']))
	{
		$params['variant'] = false;
		$params['cache_dir'] = $settings['theme_dir'];
		$params['dir'] = $settings['theme_dir'] . '/css/';
		$params['url'] = $settings['theme_url'] . '/css/';
	}
	else
	{
		$params['variant'] = true;
		$params['cache_dir'] = $settings['theme_dir'] . $context['theme_variant'];
		$params['dir'] = $settings['theme_dir'] . '/css/' . $context['theme_variant'] . '/';
		$params['url'] = $settings['theme_url'] . '/css/' . $context['theme_variant_url'];
	}

	// Whoa ... we've done this before yes?
	$cache_name = 'load_css_' . md5($params['cache_dir'] . implode('_', $filenames));
	if (($temp = cache_get_data($cache_name, 600)) !== null)
	{
		if (empty($context['css_files']))
			$context['css_files'] = array();
		$context['css_files'] += $temp;
	}
	else
	{
		$this_build = array();
		// All the files in this group use the parameters as defined above
		foreach ($filenames as $filename)
		{
			// Account for shorthand like admin.css?xyz11 filenames
			$has_cache_staler = strpos($filename, '.css?');
			$params['basename'] = $has_cache_staler ? substr($filename, 0, $has_cache_staler + 4) : $filename;
			$this_id = empty($id) ? strtr(basename($filename), '?', '_') : $id;

			// Is this a local file?
			if (substr($filename, 0, 4) !== 'http' || !empty($params['local']))
			{
				$params['local'] = true;

				// Fallback if needed?
				if ($params['fallback'] && ($params['variant'] || ($settings['theme_dir'] !== $settings['default_theme_dir'])) && !file_exists($params['dir'] . $filename))
				{
					// Fallback if we are not already in the default theme
					if (file_exists($settings['default_theme_dir'] . '/css/' . $filename))
					{
						$filename = $settings['default_theme_url'] . '/css/' . $filename . ($has_cache_staler ? '' : $params['stale']);
					}
					else
						$filename = false;
				}
				else
					$filename = $params['url'] . $filename . ($has_cache_staler ? '' : $params['stale']);
			}

			// Add it to the array for use in the template
			if (!empty($filename))
				$this_build[$this_id] = $context['css_files'][$this_id] = array('filename' => $filename, 'options' => $params);

			if ($db_show_debug === true)
				$context['debug']['sheets'][] = $params['basename'] . (!empty($params['url']) ? '(' . basename($params['url']) . ')' : '');

			// Save this build
			cache_put_data($cache_name, $this_build, 600);
		}
	}
}

And yes this would require a few changes elsewhere in Load.php to drop the _variant part of the file names.

Re: Proposed Fix For Variants CSS issues

Reply #2 –

This is sounding good. Will have to see what the other deranged wombats think.
Master of Expletives: Now with improved family f@&king friendliness! :D

Sources code: making easy front end changes difficult since 1873. :P

Re: Proposed Fix For Variants CSS issues

Reply #3 –

And amusingly enough since the variants and name mangling are currently added outside of calls to loadCSSFile this actually works fine with the current structure.  I'm still for simplifying it though and dropping all the manipulation outside of loadCSSFile.

Re: Proposed Fix For Variants CSS issues

Reply #4 –

I didn't read that one until now several hours ago[1] because I thought it was one I already read and was keeping unread to remember something... lol

Anyway, I posted this issue before reading this topic:
https://github.com/elkarte/Elkarte/issues/1525
that is somehow related to your second post.

I'm looking at that part of the code now, I agree on moving variants to loadCSSFile, though there is a problem (as usual): loadCSSFile may be called before the variants are set (and I'm not only talking about the "core", but addons too), so we'd have to separate the "adding" from the "processing" (intended as the code that checks if the file exists or not), moving the processing much further, ideally just after the point I moved the loading of custom.css.

I already found another couple of bugs in the two functions loadCSSFile and loadJavascriptFile, and in the combine class and the templating, I fixed them (see https://github.com/emanuele45/Dialogo/compare/master...fivefixes ), but add some more code now sounds somehow risky.

Additionally, I'm a bit concerned by the fallback ability, that one has a meaning only for non-variant files, because I feel it's unlikely two themes will have compatible variants (for example even only in respect to the name, let alone the css) and I'm not sure how it works in real-life cases.

I'd say this is food for 1.1, and much needed. ;D

Another thing I would consider is to simplify the stuff: for example "force" themes to have at least one variant[2], it's just a matter of keep some files into a specific directory.
Dunno, looks like there are tons of alternatives, even too many... :-\
1 Actually I started writing that post yesterday, but never managed to finish it the way I wanted it to look like, so I'm posting it even though it's messy and it doesn't express exactly what I'd like to say↵
2 Do not I already proposed somewhere that starting from 1.1, for the users point of view should be "de facto" themes. What I mean is that in the profile page "pick a theme" two variants should be presented like two themes in order to give them much more visibility. Then "under the hood" everything else will remain the same.↵
Bugs creator.
Features destroyer.
Template killer.

Re: Proposed Fix For Variants CSS issues

Reply #5 –

Actually... you are right scripple: it may not be as tricky as I thought.
Bugs creator.
Features destroyer.
Template killer.

Re: Proposed Fix For Variants CSS issues

Reply #6 –

Changing it to fallback only from variants to the current theme base would be trivial.  That's really what it's doing now all that would be needed is drop default_ prefixes from the fallback and replace them with the current stuff.

The calls before the variant is established I thought was  a feature.  :))  But as long as user_id is set before the first call it wouldn't be too hard to make it lookup the desired variant itself if the variant flag is set.  And the variant flag could still be set false to force loading of theme base files like index.css. 

In the existing system any calls made before variant is set right now obviously get the base theme files.  They still do with this in place.  Actually I saw index.css and fontawesome.css get loaded before the variant was set every time.  It works w/o issue happily finding the base version.

Really this isn't that big of a change.  I've been running it on my test forum for a couple days now and the existing hardcodes all pass though the same way they would before.  I'd have to look at your minimizer to see if it needs a little tweaking or not.  And if you're married to the  vairant/file_variant naming it's not really hard to fix that here if we assume css files will come in without the variant already appended.  (And that assumption means that that existing calls to variant files will still work.)  I still think the variant/file_variant is redundant.   Like it came about before it was decided to use directories and everything lived in one directory.

I don't find this very frightening myself.   loadCSSFile already needs tweaking (won't have time to look at your changes this morning) and certainly outside the minimizer this is easy to test.  (Haven't looked at that yet either.) print statements from loadcss actually make it pretty easy to see what's being requested and what's being loaded.

Really this is kind of doing the hardcoded tweaks like the ones for admin non-wiz sceditor without requiring hard coded external tweaks.  There's basically no difference between you setting the directory before you call loadcss or having loadcss do it for you in terms of results.  And in case there was some edge case where you didn't want the variant added by default that's why I gave it a flag to force disable that feature on a call.

Re: Proposed Fix For Variants CSS issues

Reply #7 –

I committed this:
https://github.com/emanuele45/Dialogo/commit/26f9192d6547d253156fa230d03ffcd652152475
(with your name because the code changes are a variation of yours, if you don't want just let me know ;)).

I think in this function there is still a bug somewhere (not exactly sure where and if it actually is a but, just a feeling), .
Bugs creator.
Features destroyer.
Template killer.

Re: Proposed Fix For Variants CSS issues

Reply #8 –

If left this way yes please take my name off the submit.

I think this is the wrong behavior regarding variants and fallback.  You've made it default to not searching variants and to falling back to the default theme.  It should default to searching variants and fall back to the base of the current theme.   The point is to make variants basically be able to overwrite any css by default (anywhere loadcss or a template style sheet is given) but not have to override anything the variant doesn't need to.  By making the variant call explicit and not letting it fall back to the base theme it defeats the purpose.  Now again every variant search will have to be explicitly coded in all of Elk and all extensions for variants to be able to change things.  And if someone makes a new theme with variants they'll be required to have variant files for everything that makes a variant enabled call because otherwise it will not fallback and the load will fail since you disable fallback if a variant search is made.

To me a variant falling back to base is much more important and natural than a completely separate theme falling back to default theme.  But I guess if you're going to fall back from a theme to the default then there really should be two levels of fallback.  Variant => Base => Default Base. 

If a whole theme can fall back on the default certainly the themes minor variants should be able to as well.  Please, let's do two levels of fallback or drop the fallback to the default theme.  But we have to bring the variant fallback by default back as that's the whole point of this change.  So atwho, or sceditor, or admin, or anything anyone adds can all be themed in a variant if appropriate or ignored if not and safely fall back to the base of that theme.

And a minor bug.  When you construct the final filename when not falling back you mix url and dir settings.

Code: [Select]
$filename = $settings['theme_url'] . '/' . $params['subdir'] . $variant_dir . '/' . $filename . $cache_staler;

I think that should be variant_url and a leading slash not trailing since the variant urls get the trailing slash already. 
Last Edit: April 07, 2014, 11:16:16 pm by scripple

Re: Proposed Fix For Variants CSS issues

Reply #9 –

I agree with Scripple here about the levels of fallback.
Master of Expletives: Now with improved family f@&king friendliness! :D

Sources code: making easy front end changes difficult since 1873. :P

Re: Proposed Fix For Variants CSS issues

Reply #10 –

Thinking a bit more, since I know emanuele is a little worried about falling back from another theme's variant all the way to the base of the default theme let me try to say simply why there's no reason to worry.  The only reason it would fall back that far is the person making the theme didn't change things enough in either their theme base directory or their variant to create the file.  So it's obviously not a problem for them that it falls back.  If the theme or variant was somehow so different that they had to change whatever is missing, well then it wouldn't be missing would it?  And the fallback would never happen.

Really unless there's a bug in the final version this won't break things.  If the variant doesn't have the file it will load the same one it's been loading all along.  If there is a bug in the final version, it will show up really fast as suddenly several js or css files don't get loaded and everything is broken. 

And you'll be able to rollback some of the "check for variant and load it if it exists stuff" you just put in.  Everything can be handled by a default call to loadCSSFile or loadJavascript.  The only special one is the iframe styling on the sceditor.  And I'd say to be sure the default theme top level index.css gets loaded once and only once no matter any future code shuffling that the calls to loadCSSFile for the base and variant index.css files should set variant=false on the base call and fallback=false on the variant call.  Everything else should let variants do their thing.  And once you're happy it hasn't caused the end of Elk we can rename the files in the variants dropping the _variant part of the file name and drop all the special check and load variant items. 

And once this is place even mods can be styled by variants without work by the mod author.  That way when Ant finally gets so frustrated with the word filter and writes the badwords mod that injects random bad words in everyone's profiles and signatures I will be able to create a file in my dark variant's css directory called badwords.css and @include his original but fix the coloring to work on the dark background.  And I don't have to go whine to him about making a dark theme variant of his mod.

Re: Proposed Fix For Variants CSS issues

Reply #11 –

I can just post naughty images of words. :D
Master of Expletives: Now with improved family f@&king friendliness! :D

Sources code: making easy front end changes difficult since 1873. :P

Re: Proposed Fix For Variants CSS issues

Reply #12 –

At the moment I can't read and understand everything so forgive me if I write something that has already been discussed.

The code I committed doesn't really do any decision, it follows the instructions.
Fallback was always meant to be fallback from a theme to default. If the idea is to change that meaning, then I missed it.
I'm starting from the idea (maybe wrong) that each call to loadCSSFile loads one file. In the sense that loadCSSFile('index.css') loads 1 css and not the variant css, and then the general css, I'm not sure if this is a common ground or you have in mind something different. :)

That said, it's not that I'm worried about falling back from a variant to the "general" and then the default, is that I see no meaning in doing so, at least the way the theme is currently structured.
The current structure is:
"general" (current index.css) or non-variant gives the structure,
 variant gives the colors.
With that subdivision in mind, if we fallback from variant to "general" we get something completely different than what we are aiming at.
What's the point of loading something that gives structure when we want colors?

Then fallback from variant to default. Following your logic the natural flow of fallbacks should be my-theme-variant => my-theme-non-variant => default-theme-variant => default-theme-non-variant.
Fine: you theme has variant: my_nice_holidays.
How can I fallback from that to dark? Or maybe it was in your variant you wanted to fallback to besocial?
It's not that I'm worried about falling back to a "default" variant, is that it's technically impossible because unless you in the theme declare a "dependency" Elk cannot know where you want to fallback. Of course you can say: "just fallback to the same name", fine, what if tomorrow I want to have two black variants in my themes? black and black2? balck2 will never be able to automatically fallback to the default variant.

P.S.
There are just questions, not attacks or a kind of defense. ;)
Bugs creator.
Features destroyer.
Template killer.

Re: Proposed Fix For Variants CSS issues

Reply #13 –

I'm not saying anything about going back to default theme variants for exactly the reason you stated.  Not likely to have the name.  And in fact I'm fine with falling back no farther than the base directory of the active theme.  It's actually what we discussed a few posts up before your version.

Two big changes you made I disagree with.

1)  You made the default setting, if variant isn't set, to be variant=false.  It should be the other way.  Default to variant=true unless the user specifically sets it false.

2) You did this:  if (variant == true) fallback = false.  Variant should have no effect on fallback.  Fallback should default to true like it always has and be independent of variant.

Fallback should happen from variant to the base of the current theme.  For example besocial and light both have happily shared many js and css files.  No reason to prevent that.  (And since this now handles css and js it's very important to fallback to the theme base directory.)

I'm not personally worried about falling back all the way to the default theme, but you had it in there before this mod and put it in again so I assumed you want it.

So my wish:

Code: ("Make variant default to true") [Select]
if (empty($context['theme_variant']) || (isset($params['variant']) && ((bool) $params['variant'] === false)))
{
	$variant_dir = '';
	$variant_url = '';
}
else
{
	$variant_dir = '/' . $context['theme_variant'];
	$variant_url = $context['theme_variant_url'];
}

And don't make fallback related to variant at all
Code: [Select]
/ * REMOVE THIS
// Always ignore fallback if using a variant
if (isset($params['variant']) && ((bool) $params['variant'] === true))
	$fallback =  false;
*/

// Of course no fallback if it is unwanted
/* Dropped else here as the top condition is gone
elseif (isset($params['fallback']) && ((bool) $params['fallback'] === false))
*/
if (isset($params['fallback']) && ((bool) $params['fallback'] === false))
	$fallback = false;
// In any other case: fallback to default theme
else
	$fallback = true;

And fall back to the current theme not the default
Code: [Select]
					if (file_exists($settings['theme_dir'] . '/' . $params['subdir'] . $filename))
 					{
						$filename = $settings['theme_url'] . '/' . $params['subdir'] . $filename . $cache_staler;
						$params['dir'] = $settings['theme_dir'] . '/' . $params['subdir'];
// Doesn't the URL need the subdir too????
 						$params['url'] = $settings['theme_url'];

 					}

These were hastily typed in the editor.  There are likely bugs or typos but hopefully it makes it clearer what I'm aiming for.

Re: Proposed Fix For Variants CSS issues

Reply #14 –

Actually with this doing js it might still be good to fall back to the default theme base directory if the current theme base directory doesn't have the file.  But no, never fallback to default them variants.  Again remember this now works for javascript not just css.  And a lot of variants and theme probably won't use custom javascript.