New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[2.1.99] CSS path rewriting incorrect & other issues #94

Closed
pbowyer opened this Issue May 9, 2017 · 19 comments

Comments

3 participants
@pbowyer

pbowyer commented May 9, 2017

I've been testing the master branch as at 6c69cca against version 2.1 and have found a few differences in the CSS minification that cause issues on our site.

In all tests the CSS options were set as below. When changing plugin versions I removed the plugin folder entirely and installed the new version, and manually cleared cache/autoptimize/{js,css}/* to ensure they were empty.

image

To diff the minified files, I ran them through http://unminify.com/ before comparing.

Issues 1 & 2 are important and affect the look of the website; the others minor changes that I spotted.

1. Paths outside the theme directory incorrectly rewritten

This site is a direct port of a Drupal site, and some assets/CSS files are in their original locations. Before that was fine; now URL paths are incorrectly rewritten:

- background:url(//example.com/modules/system/../../misc/tree.png) no-repeat 11px center
+ background:url(//example.com/wp-content/modules/system/../../misc/tree.png) no-repeat -11px center

The CSS file is located outside the theme directory, in this case in /modules/system.

2. Some image positions are changed

Set to zero:

- background:url(//example.com/sites/all/themes/example/css/../images/tab-left.png) no-repeat left -76px
+ background:url(//example.com/wp-content/sites/all/themes/example/css/../images/tab-left.png) no-repeat left 0

Changed:

- background:url(//example.com/sites/all/themes/example/css/../images/tab-secondary.png) repeat-x left -56px
+ background:url(//example.com/wp-content/sites/all/themes/example/css/../images/tab-secondary.png) repeat-x left top

Flipped

ul.secondary a:hover,ul.secondary a:focus
{
-	background:url(//example.com/sites/all/themes/example/css/../images/tab-secondary.png) repeat-x left bottom
+	background:url(//example.com/wp-content/sites/all/themes/example/css/../images/tab-secondary.png) repeat-x left top
}

Inverted units:

- background:url(//example.com/modules/system/../../misc/tree.png) no-repeat 11px center
+ background:url(//example.com/wp-content/modules/system/../../misc/tree.png) no-repeat -11px center

3. Extraneous units added in certain places

I can't find a pattern in this, as often the new version is more succinct. However occasionally it changes 0 to 0px as shown below:

-	-webkit-box-shadow:-32px 0 0 -31px rgba(255,255,255,1);
-	-moz-box-shadow:-32px 0 0 -31px rgba(255,255,255,1);
-	box-shadow:-32px 0 0 -31px rgba(255,255,255,1)
+	-webkit-box-shadow:-32px 0px 0px -31px rgba(255,255,255,1);
+	-moz-box-shadow:-32px 0px 0px -31px rgba(255,255,255,1);
+	box-shadow:-32px 0px 0px -31px rgba(255,255,255,1)

And to %:

-	background:linear-gradient(to bottom,#fefefe 0,#dddcda 100%);
+	background:linear-gradient(to bottom,#fefefe 0%,#dddcda 100%);

(Yes I did run the old version through the CSS Validator, just to check they were valid!)

4. Retains empty rules

These legacy stylesheets contain empty rules (no idea why). Before, they were stripped; now Autoptimize leaves them in:

+ #footer .section {
+ }

@pbowyer pbowyer changed the title from [2.1.99] CSS path rewriting incorrect & other errors to [2.1.99] CSS path rewriting incorrect & other issues May 9, 2017

@futtta

This comment has been minimized.

Show comment
Hide comment
@futtta

futtta May 9, 2017

Owner

great work @pbowyer !

some quick initial feedback:
(1) is most certainly a bug in my code, will look into that tomorrow
(2)-(4) might (or might not) be regressions in the CSS minifier AO uses (YUI CSS minifier PHP port)

Now for that potential CSS minifier issue, could you change L202 in autoptimize.php from

@include(AUTOPTIMIZE_PLUGIN_DIR.'classes/external/php/yui-php-cssmin-2.4.8-p10/cssmin.php');
to
@include(AUTOPTIMIZE_PLUGIN_DIR.'classes/external/php/yui-php-cssmin-2.4.8-4_fgo.php');

This will load the version used in AO 2.1 and I would expect this might "fix" some of these ...

Owner

futtta commented May 9, 2017

great work @pbowyer !

some quick initial feedback:
(1) is most certainly a bug in my code, will look into that tomorrow
(2)-(4) might (or might not) be regressions in the CSS minifier AO uses (YUI CSS minifier PHP port)

Now for that potential CSS minifier issue, could you change L202 in autoptimize.php from

@include(AUTOPTIMIZE_PLUGIN_DIR.'classes/external/php/yui-php-cssmin-2.4.8-p10/cssmin.php');
to
@include(AUTOPTIMIZE_PLUGIN_DIR.'classes/external/php/yui-php-cssmin-2.4.8-4_fgo.php');

This will load the version used in AO 2.1 and I would expect this might "fix" some of these ...

@pbowyer

This comment has been minimized.

Show comment
Hide comment
@pbowyer

pbowyer May 10, 2017

@futta Thanks, I've tested with the previous CSS minifier and the results are:
(1) Still present
(2) Diffing 3-way (v2.1 CSS, new w new minifier, new w old minifier) I am a bit confused whether there are the same or fewer occurrences of this bug; these are the only ones I saw when comparing to the v2.1 CSS:

- background: transparent url(//example.com/modules/system/../../misc/throbber.gif) no-repeat 0 -18px;
+background: transparent url(//example.com/wp-content/modules/system/../../misc/throbber.gif) no-repeat 0 -18px no-repeat 0 -18px;
- background: url(//example.com/sites/all/modules/ctools/css/../images/status-active.gif) right center no-repeat
+ background: url(//example.com/wp-content/sites/all/modules/ctools/css/../images/status-active.gif) center center no-repeat
- background: url(//example.com/modules/system/../../misc/tree.png) no-repeat 11px center
+ background: url(//example.com/wp-content/modules/system/../../misc/tree.png) no-repeat -11px center

(3) No longer present

(4) Still present

Thanks!

pbowyer commented May 10, 2017

@futta Thanks, I've tested with the previous CSS minifier and the results are:
(1) Still present
(2) Diffing 3-way (v2.1 CSS, new w new minifier, new w old minifier) I am a bit confused whether there are the same or fewer occurrences of this bug; these are the only ones I saw when comparing to the v2.1 CSS:

- background: transparent url(//example.com/modules/system/../../misc/throbber.gif) no-repeat 0 -18px;
+background: transparent url(//example.com/wp-content/modules/system/../../misc/throbber.gif) no-repeat 0 -18px no-repeat 0 -18px;
- background: url(//example.com/sites/all/modules/ctools/css/../images/status-active.gif) right center no-repeat
+ background: url(//example.com/wp-content/sites/all/modules/ctools/css/../images/status-active.gif) center center no-repeat
- background: url(//example.com/modules/system/../../misc/tree.png) no-repeat 11px center
+ background: url(//example.com/wp-content/modules/system/../../misc/tree.png) no-repeat -11px center

(3) No longer present

(4) Still present

Thanks!

futtta added a commit that referenced this issue May 12, 2017

@futtta

This comment has been minimized.

Show comment
Hide comment
@futtta

futtta May 12, 2017

Owner

I just committed a fix (well, rollback actually) for (1) @pbowyer , could you test?

cfr. c753971

Owner

futtta commented May 12, 2017

I just committed a fix (well, rollback actually) for (1) @pbowyer , could you test?

cfr. c753971

@futtta

This comment has been minimized.

Show comment
Hide comment
@futtta

futtta May 17, 2017

Owner

@pbowyer can you for (2) provide the original un-optimized CSS as well?

Owner

futtta commented May 17, 2017

@pbowyer can you for (2) provide the original un-optimized CSS as well?

@zytzagoo

This comment has been minimized.

Show comment
Hide comment
@zytzagoo

zytzagoo May 19, 2017

Collaborator

Running a test using this code:

    public function test_some_stuff_for_issue_94()
    {
        $orig = <<<CSS
<style type="text/css">
.rule {
	webkit-box-shadow:-32px 0 0 -31px rgba(255,255,255,1);
	-moz-box-shadow:-32px 0 0 -31px rgba(255,255,255,1);
	box-shadow:-32px 0 0 -31px rgba(255,255,255,1)
}
.rule2 {
    background:linear-gradient(to bottom,#fefefe 0,#dddcda 100%);
}
#empty .thing {
}
</style>
CSS;
        $expected = '<style type="text/css" media="all">.rule{webkit-box-shadow:-32px 0 0 -31px rgba(255,255,255,1);-moz-box-shadow:-32px 0 0 -31px rgba(255,255,255,1);box-shadow:-32px 0 0 -31px rgba(255,255,255,1)}.rule2{background:linear-gradient(to bottom,#fefefe 0,#dddcda 100%)}</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);
    }

I cannot get the behavior described in (4), that is, #empty .thing rule is removed properly.

Also, I don't see the issue described under (3), but that might be related to the original un-optimized css (having it would certainly help a lot with determining what's going on).

BTW, the test above is running against yui-php-cssmin-2.4.8-p10 minifier (so, newer version, not the old one from 2.1)

Collaborator

zytzagoo commented May 19, 2017

Running a test using this code:

    public function test_some_stuff_for_issue_94()
    {
        $orig = <<<CSS
<style type="text/css">
.rule {
	webkit-box-shadow:-32px 0 0 -31px rgba(255,255,255,1);
	-moz-box-shadow:-32px 0 0 -31px rgba(255,255,255,1);
	box-shadow:-32px 0 0 -31px rgba(255,255,255,1)
}
.rule2 {
    background:linear-gradient(to bottom,#fefefe 0,#dddcda 100%);
}
#empty .thing {
}
</style>
CSS;
        $expected = '<style type="text/css" media="all">.rule{webkit-box-shadow:-32px 0 0 -31px rgba(255,255,255,1);-moz-box-shadow:-32px 0 0 -31px rgba(255,255,255,1);box-shadow:-32px 0 0 -31px rgba(255,255,255,1)}.rule2{background:linear-gradient(to bottom,#fefefe 0,#dddcda 100%)}</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);
    }

I cannot get the behavior described in (4), that is, #empty .thing rule is removed properly.

Also, I don't see the issue described under (3), but that might be related to the original un-optimized css (having it would certainly help a lot with determining what's going on).

BTW, the test above is running against yui-php-cssmin-2.4.8-p10 minifier (so, newer version, not the old one from 2.1)

@pbowyer

This comment has been minimized.

Show comment
Hide comment
@pbowyer

pbowyer May 22, 2017

@futtta Sorry been busy, will test this week. Can you provide an email address I can send the unoptimized CSS to? (about 15 files)

pbowyer commented May 22, 2017

@futtta Sorry been busy, will test this week. Can you provide an email address I can send the unoptimized CSS to? (about 15 files)

@futtta

This comment has been minimized.

Show comment
Hide comment
@futtta

futtta May 22, 2017

Owner

sure; futtta-at-gmail-dot-com

Owner

futtta commented May 22, 2017

sure; futtta-at-gmail-dot-com

@futtta

This comment has been minimized.

Show comment
Hide comment
@futtta

futtta May 23, 2017

Owner

commit 0199624 might fix (2) @pbowyer

Owner

futtta commented May 23, 2017

commit 0199624 might fix (2) @pbowyer

@futtta

This comment has been minimized.

Show comment
Hide comment
@futtta

futtta Jun 1, 2017

Owner

any news @pbowyer ?

Owner

futtta commented Jun 1, 2017

any news @pbowyer ?

@futtta

This comment has been minimized.

Show comment
Hide comment
@futtta

futtta Jun 11, 2017

Owner

@pbowyer I'm planning to push at AO 2.2 next Sunday, so speak now or be silent forever ;-)

Owner

futtta commented Jun 11, 2017

@pbowyer I'm planning to push at AO 2.2 next Sunday, so speak now or be silent forever ;-)

@pbowyer

This comment has been minimized.

Show comment
Hide comment
@pbowyer

pbowyer Jun 14, 2017

Hi @futtta just back from holiday... will see what I can do once I've caught up with work!

pbowyer commented Jun 14, 2017

Hi @futtta just back from holiday... will see what I can do once I've caught up with work!

@pbowyer

This comment has been minimized.

Show comment
Hide comment
@pbowyer

pbowyer Jun 15, 2017

Hi @futtta, I have tested and can confirm that 1 & 2 are sorted. Thank you!

The site has 58 separate CSS files spread across ~20 directories which is why providing you with a copy of the unoptimised CSS proved to be somewhat challenging. I have now used https://github.com/futtta/autoptimize#filter-autoptimize_css_do_minify and aggregated into one file, which I have attached.
autoptimize_726bd8251005170296a27661ac9200de.css.txt

pbowyer commented Jun 15, 2017

Hi @futtta, I have tested and can confirm that 1 & 2 are sorted. Thank you!

The site has 58 separate CSS files spread across ~20 directories which is why providing you with a copy of the unoptimised CSS proved to be somewhat challenging. I have now used https://github.com/futtta/autoptimize#filter-autoptimize_css_do_minify and aggregated into one file, which I have attached.
autoptimize_726bd8251005170296a27661ac9200de.css.txt

@futtta

This comment has been minimized.

Show comment
Hide comment
@futtta

futtta Jun 16, 2017

Owner

OK, so (3) is still present (but is due to upstream changes because of bugs, going to let that be) and then we have (4). will look into that now.

Owner

futtta commented Jun 16, 2017

OK, so (3) is still present (but is due to upstream changes because of bugs, going to let that be) and then we have (4). will look into that now.

@futtta

This comment has been minimized.

Show comment
Hide comment
@futtta

futtta Jun 16, 2017

Owner

tested (4) by extracting that legacy file ("Page Background Styling") and enqueuing is, empty rules are not retained here?

Owner

futtta commented Jun 16, 2017

tested (4) by extracting that legacy file ("Page Background Styling") and enqueuing is, empty rules are not retained here?

@pbowyer

This comment has been minimized.

Show comment
Hide comment
@pbowyer

pbowyer Jun 16, 2017

(3) isn't worth the effort to fix, the impact on file size is negligible. (4) is weird, I can reproduce it; if you enqueue the entire aggregated file do you not experience it?

pbowyer commented Jun 16, 2017

(3) isn't worth the effort to fix, the impact on file size is negligible. (4) is weird, I can reproduce it; if you enqueue the entire aggregated file do you not experience it?

@futtta

This comment has been minimized.

Show comment
Hide comment
@futtta

futtta Jun 16, 2017

Owner

Well, I enqueued just the file that has the empty rules (having "Page Background Styling" in the opening comment block), which is how it is added at your side as well (the aggregated file is what AO generates)?

ah, but I might now what is happening; is that file a xyz.min.css or xyz-min.css? because in that case AO WILL indeed considers it minified already and will just do very light optimizations ..

Owner

futtta commented Jun 16, 2017

Well, I enqueued just the file that has the empty rules (having "Page Background Styling" in the opening comment block), which is how it is added at your side as well (the aggregated file is what AO generates)?

ah, but I might now what is happening; is that file a xyz.min.css or xyz-min.css? because in that case AO WILL indeed considers it minified already and will just do very light optimizations ..

@pbowyer

This comment has been minimized.

Show comment
Hide comment
@pbowyer

pbowyer Jun 16, 2017

ah, but I might now what is happening; is that file a xyz.min.css or xyz-min.css?

It's not, actually - it's called /sites/all/themes/examplecom/css/page-backgrounds.css.

I'm happy if (4) is a 'wontfix' task as it's a rare occurrence I suspect.

pbowyer commented Jun 16, 2017

ah, but I might now what is happening; is that file a xyz.min.css or xyz-min.css?

It's not, actually - it's called /sites/all/themes/examplecom/css/page-backgrounds.css.

I'm happy if (4) is a 'wontfix' task as it's a rare occurrence I suspect.

@pbowyer

This comment has been minimized.

Show comment
Hide comment
@pbowyer

pbowyer Jun 16, 2017

Here's the exact file, because when I include just it, it is left intact, and nothing is stripped out.

page-backgrounds.css.txt

Raw HTML tag in page when Autoptimize is disabled:

<link rel="stylesheet" href="http://example.com/sites/all/themes/ebecs2013/css/page-backgrounds.css">

pbowyer commented Jun 16, 2017

Here's the exact file, because when I include just it, it is left intact, and nothing is stripped out.

page-backgrounds.css.txt

Raw HTML tag in page when Autoptimize is disabled:

<link rel="stylesheet" href="http://example.com/sites/all/themes/ebecs2013/css/page-backgrounds.css">
@futtta

This comment has been minimized.

Show comment
Hide comment
@futtta

futtta Jun 16, 2017

Owner

tried with that stylesheet, still not reproducible I'm afraid .. indeed going to consider "wontfix"

Owner

futtta commented Jun 16, 2017

tried with that stylesheet, still not reproducible I'm afraid .. indeed going to consider "wontfix"

@futtta futtta closed this Jun 25, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment