danyj commented 9 years ago

Is there any particular reason why you are adding css files inside the body tag. I do understand js but CSS should not be there

ghost commented 9 years ago

WordPress also adds css in body.

This happens because options html is rendered late when the page <head> was already generated, and those options include their js and css. We don't know a better solution, because we can't know inside <head> which options will be displayed in the page later.

Including css in body has a downside: Some page elements are displayed without design then after the css from body was loaded, the styles applies, this make the page to "glitch" on load (for e.g. you have 2 div one under another, the the styles applies and make them float). We "fixed" this by adding opacity:0 to div that wraps options html, and show a $.fadeIn() effect on document ready. (You can observe that when refresh the Theme Settings page)

danyj commented 9 years ago

I know what the downsides are thus I am asking the question. No matter what WP does we both know that css files should be inside the head. And those are CSS files which are not dynamic, so no matter what the head contains they should be there and not inside the body.

And I was talking about frontend not admin which is bigger concern than options side.

ghost commented 9 years ago

And I was talking about frontend not admin which is bigger concern than options side.

Where did you find css files included by unyson in body on frontend?

danyj commented 9 years ago

Any shortcode use prints them out , even on your demo my demo on twentyfifteen by using your instructions from #152 capture

your demo capturexx

rockalife commented 9 years ago

Hi @danyj,

The styles are loaded in the body because the wp_enqueue_style function was called after the html for the <head> element was generated. Let me explain on the example of shortcodes. When WordPress renders some page from top to bottom and reaches the content with some shortcodes in it eventually the handlers (php functions) that were registered with those shortcodes will be executed. By default, somewhere in those functions, the shortcode's static.php file is required and so functions like wp_enqueue_style or wp_enqueue_script will be called. The benefit of this kind of separation is that the shortcodes are self contained, with all of their static files inside of them thus making them somehow easier to understand, debug and maintain plus their static files will be loaded only when needed (when the shortcode is rendered), but the drawback is that those functions are called after the <head> element was already rendered, so all the files are loaded in the body.

I hope that you understood what I was trying to explain.

Now the good news is that it does not have to be this way, you can customize your theme in whatever way that works for you. I'll give you some tips on how to do this:

Put all the styles of your theme (including those of the shortcodes) into one big styles.css and load it on every page in the frontend from functions.php or any other file that is loaded on every time. Make sure that the stylesheets are being loaded in <head>
If you have customized some shortcode that comes with the framework rewrite it's static.php in your theme so that it is empty and put your custom styles into your styles.css.
If you have created shortcodes of you own then simply don't create a static.php file for them and just put those styles into your styles.css

Feel free to let us know what you think.

danyj commented 9 years ago

@rockalife Bud, I think I posted a valid bug. Not sure what the way I would construct my theme has to do with it. Your css files and inline css are loading in a wrong place. Simple as that. This has nothing to do with my theme and I did not say that files from my theme are loading in the body. Just because wp_enqueue_style is loading before your methods does not mean that you cant read the body and place the css where it belongs. You can create a method that fires before body render , get your css and move those up.

ghost commented 9 years ago

@rockalife This fixes the problem. Rendering the shortcodes (post content) in the wp_enqueue_scripts action includes all static.php and enqueue the styles inside <head>.

The only "problem" is that the post content shortcodes will be rendered twice.

What do you think?

rockalife commented 9 years ago

@moldcraft,

Another problem is that the users who do something similar to the steps that I have described in the list above and put all their css into one file (thus reducing the number of http requests) and load the file in the <head> element would get a useless render of the shortcodes on every page.

I did not test how expensive is that extra render, but something just feels wrong about this, it's kind of hacky. I'll think about it and if I won't figure something better out then I'll leave your solution. Thanks for the help by the way.

ghost commented 9 years ago

The fix is included in v1.2.7

Lisa-Williams commented 9 years ago

EDITED

@moldcraft

We don't know a better solution, because we can't know inside <head> which options will be displayed in the page later.

Yes you can. And you are already using it. PHP output buffering. While rendering header.php, start output buffering, and also render an empty <style>OPTION_STYLES_HERE<style> tag into <head> While rendering the body of the page (whatever-page.php) concatenate the option specific CSS in one variable. (Just one variable for all options added to the page.) While rendering footer.php, at the very end, get the output buffer into a variable content and stop buffering. Then use str_replace to replace the string OPTION_STYLES_HERE in variable content with the contents of the variable holding the CSS for all options added to the page. And send this variable content to the browser.

Just out of curiosity. Why would you want to know exactly which options will be displayed on the page, and then load only their CSS in the <body>?

When all CSS rules for all options would be loaded in <head> that file would be cached by the browser. And on each subsequent page access the browser would load the styles from the browser cache. Not from the server. So there is only a penalty when a visitor (site owner) accesses the website for the very first time. After that it's full speed.

But I might be missing something here.

ghost commented 9 years ago

@Lisa-Williams

// file: header.php
do_action('admin_enqueue_scripts');
// here I don't know what scripts and styles will be enqueued below (in page content)

// file: whatever-page.php

// Extension #1
if ($something) {
    wp_enqueue_style('style-1', ...);
}

// Extension #2
if ($something_else) {
    wp_enqueue_style('style-7', ...);
}
if ($whatever) {
    wp_enqueue_style('style-23', ...);
}

I have some questions about the solution with output buffering:

If page content/html is big (for e.g. scratch theme demo options), how much it will increase php memory consumption?
If some fatal error will happen while rendering page content, the error will be shown or the user will see a white/empty page?
How to collect only framework styles and move them to <head> (without making enqueue through custom functions) ?

P.S. This is not a priority for us now. If you want, investigate this problem and if you will find a solution, make a pull request.

Lisa-Williams commented 9 years ago

Yes it will use more memory (the variables). How much I do not know. But memory_get_usage (true) will. This could be a show stopper.
It's possible to specify an output callback function like this ob_start("ob_errorCheck") for the other ob_* functions to use, for example ob_end_flush() (flush and clean functions only). When executing $content = ob_end_flush(); it will pass the output buffer through the error checking callback and $content will either contain the output_buffer or an error message.

    ob_start("ob_errorCheck");

    function ob_errorCheck($obBuffer)
    {
        // In case the web server (Apache) changed the working directory when calling this callback. Put it back to what it was.
        //chdir(dirname($_SERVER['SCRIPT_FILENAME']))

        $error = error_get_last();

        if ($error && $error["type"] == E_USER_ERROR || $error["type"] == E_ERROR) {
            return ini_get("error_prepend_string") .
                "\nAn error occurred: $error[message] in $error[file] on line $error[line]\n" .
                ini_get("error_append_string");
            //return FALSE; // Silence is golden.
            //return ""; // Silence is golden.
        }

        // No errors.
        return $obBuffer;
    }

I can't without a custom function. An other option: Start output buffering. But render the body first, with regular wp_enqueue_style() calls for the conditional options. And at the end of rendering the body save the output buffer in a variable $body_content = ob_get_contents(); ob_end_clean(); (which stops buffering). The styles for the options in the body will have been enqueued in the 2 arrays $wp_styles->queue and $wp_styles->to_do. Then render <head> (without output buffering). wp_head() will also find the enqueued styles of the body. Then echo $body_content. And last render the footer (without output buffering).

Yes this (memory consumption) definitely needs some investigation first.

danyj commented 9 years ago

And here again reminder why this should not be done

151 2Fwebd just released his theme on TF and html is not validating because you have the style inside the body. If style is to be used there which it should not it should also use property='stylesheet'

http://validator.w3.org/check?verbose=1&uri=http://2f-design.fr/themes/starry/

But this is not acceptable http://2f-design.fr/themes/starry/wp-content/plugins/unyson/framework/extensions/shortcodes/shortcodes/section/static/css/styles.css?ver=4.1

one line of layout hack should really be somewhere else.

This is options framework and it should provide options for us to build themes , not force us to build themes the way someone else intended.

What you could do is give us the option to consolidate all framework CSS into one file which in turn can eliminate all further css loading issues. As far as I can see 99% of those css files are not dynamic so this should not be that big of an issue.

ghost commented 9 years ago

his theme on TF and html is not validating because you have the style inside the body

Fixed https://github.com/ThemeFuse/Unyson/issues/151#issuecomment-71656840

This is options framework and it should provide options for us to build themes , not force us to build themes the way someone else intended.

This is the way we organized files to be convenient for developing. Like that, all shortcodes are in their own directory with everything they need without affecting any other file outside it.

You can disable any shortcode styles/scripts by overwriting its static.php with an empty file in the theme, and place all your styles in a single style.css in theme root.

What you could do is give us the option to consolidate all framework CSS into one file which in turn can eliminate all further css loading issues

I think this is the job of cache/combine/minify plugins to do that.

Or you can do the combine manyally using this script and disable all shortcodes styles and scripts.

As far as I can see 99% of those css files are not dynamic so this should not be that big of an issue.

If you use on the page only one shortcode, only its style/script will be included. If you have on the page 10 shortcodes, more styles will be included. So it's dynamic.

But again, manually you can combine and move all styles and scripts to one file and disable other files inclusion.

I don't know a way to consolidate all framework CSS automatically.

danyj commented 9 years ago

what you could get away with is something we did in our joomla framework and that is inline css where is needed. we achieved that with a global inline css var and trough out the framework we keep on adding to it if needed so everything stays dynamic, no css files are being used and the css stays in head. With your current approach you are sacrificing page file requests for one css line which turns out to be an overload.

I am still studding your framework and will see if more can be done.

Yes you are right about compression plugin and we might port this to WP https://github.com/youjoomla/jcompress But you should consider using inline css for 1 css line instead of a file. Or simply create global framework css file and just add everything inside. I browsed trough extensions and the only "large" file is the grid css , other than that you have just few lines spread trough out multiple files.

When I say dynamic I mean that none of the CSS properties are being changed depending on a option. They are always as you have written them in css file. 1 CSS file of 10KB is much better than 10 requests for 10 1KB files .

Now if these were LESS files and we had a compiler , than your approach would be reasonable and we would still end up having one compiled CSS file.

danyj commented 9 years ago

You can consolidate all your css with this compiler https://github.com/oyejorge/less.php we are using it for our LESS and CSS files Check it here http://joomlatemplates.youjoomla.info/eximium/ we compiled over 10 css files plus BS and FA http://joomlatemplates.youjoomla.info/eximium/templates/eximium/css_compiled/template-blue.css

And here is what I was saying about dynamic CSS trough one variable https://www.uploady.com/#!/download/HljVRslfZf3/ylyPDkeMbWF~_24Z

ghost commented 9 years ago

Thank you for sharing the info.

we achieved that with a global inline css var and trough out the framework we keep on adding to it

Wordpress users know that css files are included with wp_enqueue_style() and js with wp_enqueue_script(), that's all. If we will add some custom functions to include js/css this will be confusing. We want to stick to the default wordpress behavior and keep things simple.

If you are and advanced developer and want to optimize your theme, you can easy do the one global variable trick:

// file: {theme}/int/hooks.php

global $theme_inline_css = array();

function _action_theme_print_inline_styles() {
    global $theme_inline_css;
    echo '<style type="text/css" >'. implode("\n\n", $theme_inline_css) .'</style>';
}
add_action( 'wp_print_styles', '_action_theme_print_inline_styles' );

// file: {theme}/int/static.php

function _action_theme_enqueue_small_style() {
    global $theme_inline_css;
    $theme_inline_css[] = '.hello_world { color: green; }';
}
add_action( 'wp_enqueue_scripts', '_action_theme_enqueue_small_style' );

Or another solution is to include style contents in href="" as base64 data uri.

You can test this code in your theme functions.php

/**
 * Try to find file path by its uri and read file contents
 * @param string $file_uri
 * @return bool|string false or string file contents
 */
function fw_read_file_by_uri($file_uri) {
    static $base = null;

    if ($base === null) {
        $base = array();
        $base['dir'] = WP_CONTENT_DIR;
        $base['uri'] = ltrim(content_url(), '/');
        $base['uri_prefix_regex'] = '/^'. preg_quote($base['uri'], '/') .'/';
    }

    $file_rel_path = preg_replace($base['uri_prefix_regex'], '', $file_uri);

    if ($base['uri'] === $file_rel_path) {
        // the file is not inside base dir
        return false;
    }

    $file_path = $base['dir'] .'/'. $file_rel_path;

    if (!file_exists($file_path)) {
        return false;
    }

    return file_get_contents($file_path);
}

/**
 * Try to convert 
 * href="http://.../style.css" to href="data:text/css;base64,..."
 * 
 * @param string $href
 * @return string
 */
function fw_stylesheet_href_to_data_uri($href) {
    $stylesheet_contents = fw_read_file_by_uri($href);

    if ($stylesheet_contents === false) {
        return $href;
    }

    $dir_uri = dirname($href);

    /**
     * Replace relative 'url(img/bg.png)' 
     * with full 'url(http://site.com/assets/img/bg.png)'
     *
     * Do not touch if url starts with:
     * - 'https://'
     * - 'http://'
     * - '/' (also matches '//')
     * - '#' (for css property: "behavior: url(#behaveBinObject)")
     * - 'data:'
     */
    $stylesheet_contents = preg_replace(
        '/url\s*\((?!\s*[\'"]?(?:\/|data\:|\#|(?:https?:)?\/\/))\s*([\'"])?/', 
        'url($1'. $dir_uri .'/', 
        $stylesheet_contents
    );

    return 'data:text/css;base64,'. base64_encode($stylesheet_contents);
}

/**
 * @param string $tag <link href='...' ... />
 * @param string $handle
 * @return mixed
 */
function _filter_theme_css_href_to_data_uri($tag, $handle) {
    /**
     * @var WP_Styles $wp_styles
     */
    global $wp_styles;

    $style = $wp_styles->query($handle);

    if (!$style) {
        return $tag;
    }

    return preg_replace(
        '/href=["\'].*?["\']/', 
        'href="'. fw_stylesheet_href_to_data_uri($style->src) .'"', 
        $tag
    );
}
add_filter('style_loader_tag', '_filter_theme_css_href_to_data_uri', 99, 2);

Of course I did this for demo, it's not good to use this solution for large css files. But you can do the encode for styles which is known that are small

wp_enqueue_style(
    'my-small-style',
    fw_stylesheet_href_to_data_uri('http://.../small-style.css')
);

Also the base64_encode() function, it's not allowed in themes (it won't pass theme-check).

I think we will begin to use both global variable and href="data:uri" when it's relevant (for small css). But a general solution to consolidate all css files (including large files) I don't know.

Lisa-Williams commented 9 years ago

@moldcraft

Also the base64_encode() function, it's not allowed in themes (it won't pass theme-check).

I'm 100% sure that I've seen at least 3 demos in Themeforest's Top 10 WP selling themes, which very sparsely make use of base64 encoded images in CSS files. These themes are in the Top 10 month after month. It varies per theme from 1 -3 usages. And the count fluctuates. A theme might have 1 usage this week, 2 or 3 usages the next week/month. So is it usage of the base64_encode() function that won't pass the check, or usage of base64 data, or both?

sergiubagrin commented 9 years ago

Hi @Lisa-Williams

So is it usage of the base64_encode() function that won't pass the check, or usage of base64 data, or both?

base64_encode() function is not allowed. A lot of themes on themeforest use base64_encode. If you need an encode solution best of all avoid base64_encode in your Theme

Thanks

ghost commented 9 years ago

@Lisa-Williams base64 data in css is ok, base64_encode() php function is not

You can simply test that:

Take any theme
Open any php file and place in it base64_encode('');
Create zip and upload it on http://themecheck.org/
You will see this error

That's all. No one cares if that function usage is safe or does something bad, they just see "Critical alert" and report it as bug and you must remove it.

ghost commented 9 years ago

The same solution (as above) but without base64.

You can test it in your theme functions.php

/**
 * Try to find file path by its uri and read the file contents
 * @param string $file_uri
 * @return bool|string false or string - the file contents
 */
function fw_read_file_by_uri($file_uri) {
    static $base = null;

    if ($base === null) {
        $base = array();
        $base['dir'] = WP_CONTENT_DIR;
        $base['uri'] = ltrim(content_url(), '/');
        $base['uri_prefix_regex'] = '/^'. preg_quote($base['uri'], '/') .'/';
    }

    $file_rel_path = preg_replace($base['uri_prefix_regex'], '', $file_uri);

    if ($base['uri'] === $file_rel_path) {
        // the file is not inside base dir
        return false;
    }

    $file_path = $base['dir'] .'/'. $file_rel_path;

    if (!file_exists($file_path)) {
        return false;
    }

    return file_get_contents($file_path);
}

/**
 * Make stylesheet contents (portable) independent of directory location
 * For e.g. replace relative paths 'url(img/bg.png)' with full paths 'url(http://site.com/assets/img/bg.png)'
 *
 * @param string $href 'http://.../style.css'
 * @param null|string $contents If not specified, will try to read from $href
 * @return bool|string false - on failure; string - stylesheet contents
 */
function fw_make_stylesheet_portable($href, $contents = null) {
    if (is_null($contents)) {
        $contents = fw_read_file_by_uri($href);

        if ($contents === false) {
            return false;
        }
    }

    $dir_uri = dirname($href);

    /**
     * Replace relative 'url(img/bg.png)'
     * with full 'url(http://site.com/assets/img/bg.png)'
     *
     * Do not touch if url starts with:
     * - 'https://'
     * - 'http://'
     * - '/' (also matches '//')
     * - '#' (for css property: "behavior: url(#behaveBinObject)")
     * - 'data:'
     */
    $contents = preg_replace(
        '/url\s*\((?!\s*[\'"]?(?:\/|data\:|\#|(?:https?:)?\/\/))\s*([\'"])?/',
        'url($1'. $dir_uri .'/',
        $contents
    );

    return $contents;
}

/**
 * @param string $tag <link href='...' ... />
 * @param string $handle
 * @return mixed
 */
function _filter_theme_css_href_to_data_uri($tag, $handle) {
    /**
     * @var WP_Styles $wp_styles
     */
    global $wp_styles;

    $style = $wp_styles->query($handle);

    if (!$style) {
        return $tag;
    }

    $contents = fw_make_stylesheet_portable($style->src);

    if (!$contents) {
        return $tag;
    }

    // remove new lines and big spaces
    $contents = preg_replace('/\s+/', ' ', $contents);

    return preg_replace(
        '/href=["\'].*?["\']/',
        'href="data:text/css,'. esc_attr($contents) .'"',
        $tag
    );
}
add_filter('style_loader_tag', '_filter_theme_css_href_to_data_uri', 99, 2);

Lisa-Williams commented 9 years ago

Looks like I have to swap out base64_encode() and base64_decode() then. :( One of my own plugins to be included with the theme, exchanges binary data through an RSS/Atom/XML feed (consuming and generating) by base64 encoding it.

Better to discover this now. Thanks!

sergiubagrin commented 9 years ago

One of my own plugins to be included with the theme, exchanges binary data through an RSS/Atom/XML feed (consuming and generating) by base64 encoding it.

You can use base64_encode() and base64_decode() in plugins, it is not recomended to use it in themes.

ghost commented 9 years ago

@sergiubagrin You can use base64_encode() and base64_decode() in plugins

No, you can't.

Yesterday I checked unyson with plugin-check and it showed me this:

WARNING: Found base64_encode in the file unyson/framework/helpers/general.php. base64_encode() is not allowed. Line 1111: return 'data:text/css;base64,'. base64_encode($stylesheet_contents);

so I changed the function and removed base64_encode()

Lisa-Williams commented 9 years ago

@sergiubagrin @moldcraft I'm already testing with ascii85/base85 now. It's more efficient. 25% overhead instead of 33%. According to Wikipedia it's used in Postscript and PDF. This one: https://github.com/scottchiefbaker/php-base85 (He's also linking to a version written in C and implemented as a PHP extension in the Readme.) ascii85/base85 might be short-lived though, until the plugin and theme checkers catch up.

ThemeFuse / Unyson

CSS files inside body ? #153

151 2Fwebd just released his theme on TF and html is not validating because you have the style inside the body. If style is to be used there which it should not it should also use property='stylesheet'