Permalink
Browse files

fix for CSS issue found by Catalin Silegeanu, thanks for bearing with…

… me ;-)
  • Loading branch information...
1 parent c753971 commit 4f4cc101c568f24526551953ace1471ef742be67 @futtta committed May 16, 2017
Showing with 5 additions and 1 deletion.
  1. +5 −1 classes/autoptimizeStyles.php
@@ -573,8 +573,11 @@ static function fixurls($file,$code) {
// switch all imports to the url() syntax
$code=preg_replace('#@import ("|\')(.+?)\.css.*("|\')#','@import url("${2}.css")',$code);
+
+ // avoid ASSETS_REGEX eating closing comment */ if it preceeds the semi-colon
+ $code_nocomments = preg_replace('#/\*.*\*/#Us','',$code);
- if( preg_match_all( self::ASSETS_REGEX, $code, $matches ) ) {
+ if( preg_match_all( self::ASSETS_REGEX, $code_nocomments, $matches ) ) {
$replace = array();
foreach($matches[1] as $k => $url) {
// Remove quotes
@@ -611,6 +614,7 @@ static function fixurls($file,$code) {
}
//Do the replacing here to avoid breaking URLs
$code = str_replace(array_keys($replace),array_values($replace),$code);
+ unset($code_nocomments);
}
return $code;
}

12 comments on commit 4f4cc10

Contributor

zytzagoo replied May 19, 2017

Can you please share some samples of css that breaks due to this?

Owner

futtta replied May 19, 2017

sure;

.ls-noskin .ls-nav-next {
    display: block;
    width: 36px !important;
    height: 36px !important;
    /*text-indent: -9999px;*/
    background: rgba(11,11,11,0.8) /* url("images/icons/slider-nav.png") no-repeat 0 0 */;
    transition: background-color .2s linear;
    font-size:35px;
    line-height:32px;
}
.flex-next i {
	left:-2px;
}

/* ----------------------------------------------------------------
    Slider Caption
-----------------------------------------------------------------*/
.slide-caption,
.rs-caption,
.nivo-caption {
    display: block;
    position: absolute;
    z-index: 8;
}

problem is the ASSET_REGEX "eats" the closing comment in

 background: rgba(11,11,11,0.8) /* url("images/icons/slider-nav.png") no-repeat 0 0 */;

and when the comments are removed (as part of the actual CSS minication) you loose the CSS between that line and the next */

I considered adapting the regex, but shy-ed away after a couple of attempts ;-)

Contributor

zytzagoo replied May 19, 2017

Having a hard time reproducing it fully for some reason on my end...
Using that input, example output file from AO contains:

.ls-noskin .ls-nav-next{display:block;width:36px !important;height:36px !important;background:rgba(11,11,11,0.8);transition:background-color .2s linear;font-size:35px;line-height:32px}.flex-next i{left:-2px}.slide-caption,.rs-caption,.nivo-caption{display:block;position:absolute;z-index:8}

Which looks fine (ie., nothing is eaten that shouldn't be?).

Tried also comparing that to results from http://csscompressor.com/ , http://www.cleancss.com/css-minify/ and https://cssminifier.com/ -- results are identical (apart for certain extra whitespace removal)

Contributor

zytzagoo replied May 19, 2017

I do notice a difference in highlighted matches when modifying the assets regex slightly:
https://regex101.com/r/kBk4RJ/1

but I'd like to be sure there's no other breakage/side-effects (if the change is needed at all!)
So, that's why I tried writing a test before even changing the regex, here's the test code/method:

    public function test_assets_regex_eating_closing_comment()
    {
        add_filter( 'autoptimize_filter_css_inlinesize', function( $bytes ) {
            return 4096;
        });

        $orig = <<<CSS
<style type="text/css">
.ls-noskin .ls-nav-next {
    display: block;
    width: 36px !important;
    height: 36px !important;
    /*text-indent: -9999px;*/
    background: rgba(11,11,11,0.8) /* url("images/icons/slider-nav.png") no-repeat 0 0 */;
    transition: background-color .2s linear;
    font-size:35px;
    line-height:32px;
}
.flex-next i {
	left:-2px;
}

/* ----------------------------------------------------------------
    Slider Caption
-----------------------------------------------------------------*/
.slide-caption,
.rs-caption,
.nivo-caption {
    display: block;
    position: absolute;
    z-index: 8;
}
</style>
CSS;
        $expected = '<style type="text/css" media="all">.ls-noskin .ls-nav-next{display:block;width:36px !important;height:36px !important;background:rgba(11,11,11,0.8);transition:background-color .2s linear;font-size:35px;line-height:32px}.flex-next i{left:-2px}.slide-caption,.rs-caption,.nivo-caption{display:block;position:absolute;z-index:8}</style><!--noptimize--><!-- Autoptimize found a problem with the HTML in your Theme, tag `title` missing --><!--/noptimize-->';

        $conf = autoptimizeConfig::instance();
        $options = [
            'justhead' => $conf->get('autoptimize_css_justhead'),
            'datauris' => $conf->get('autoptimize_css_datauris'),
            'defer' => $conf->get('autoptimize_css_defer'),
            'defer_inline' => $conf->get('autoptimize_css_defer_inline'),
            'inline' => $conf->get('autoptimize_css_inline'),
            'css_exclude' => $conf->get('autoptimize_css_exclude'),
            'cdn_url' => $conf->get('autoptimize_cdn_url'),
            'include_inline' => $conf->get('autoptimize_css_include_inline'),
            'nogooglefont' => $conf->get('autoptimize_css_nogooglefont')
        ];
        $instance = new autoptimizeStyles($orig);
        $instance->read($options);
        $instance->minify();
        $instance->cache();
        $actual = $instance->getcontent();

        $this->assertEquals($expected, $actual);
    }

This runs using "default" settings, so now I'm wondering if maybe one of the settings is different somewhere and if it could affect the output and/or code-paths used?

BTW, the test/code is ugly as hell and doing a shit ton of unrelated things, but I figured it's best to run through the entire "AO lifecycle" (same as it would happen when things get run inside autoptimize_end_buffering()) -- could that also be influencing the outcome somehow?

Ahem, so, when this runs, $expected equals $actual 🤷‍♂️ (again, without any changes to ASSETS_REGEX).

Can you confirm that the minified css output on your end is at least significantly different than mine?

Owner

futtta replied May 20, 2017

Contributor

zytzagoo replied May 22, 2017

I don't see any problems really... The differences are completely expected:
https://gist.github.com/zytzagoo/c472bea9229ff25078a7eaddf02804b6

Process was:

  • minify style.css manually by http://csscompressor.com/ online minifier and save to style.min.css
  • load that style.css and ran it through AO test, saving output in style.min.actual.css
  • pretty print both style.min.css and style.min.actual.css and do a diff on pretty printed files

There's some extra css comments with a backslash that don't seem to be fully/properly handled/stripped by AO (/*\*/), but it's not causing any real issues I can see from just messing with these text files?

I don't see ASSETS_REGEX as the cause of any of these issues/changes? I didn't have coffee yet though...

Owner

futtta replied May 22, 2017

re-tested in AO 2.2 pre the last commit, confirms the bug. I'll test with your fork to see if that makes the problem go away (some significant differences in autoptimizeStyles.php, so might well be ...).

Owner

futtta replied May 22, 2017

tested with your fork (test-branch), confirmed OK. so guess we'll have to dive into differences between our autoptimizeStyles.php ..

Owner

futtta replied May 22, 2017

replacing my legacy fixurls() with yours fixes the issue. given the isolate nature of fixurls() and given your test-suite, I feel inclined to adopt yours. so how confident are you? :-)

Contributor

zytzagoo replied May 23, 2017

go for it :)

if anything turns up, we'll fix it one way or another :)

Owner

futtta replied May 23, 2017

done :-)

Contributor

zytzagoo replied May 23, 2017

let the bugs start rolling :)

Please sign in to comment.