background image data-uri broken #87

Closed
futtta opened this Issue Mar 3, 2017 · 15 comments

Comments

Projects
None yet
3 participants
Owner

futtta commented Mar 3, 2017

just got a mail;

When i use data uri for the background PNGs (i am using sprites!) and change the size with the filter (e.g here 40960) i get with the normal AO this code in the CSS, which works fine:

background:url(data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAASLCAYAAABUcdckAAABW0lEQVRYhe3Yy0pCURSA4aVmZVlSJEVRiJIUSVHYTcvOJKFB0KTLE5iTBj1JD9AsCHqFZr5FA+kBiube3Z1/FyIZCA7apPsHv6lrexabgyIiIfcjUUjBudhsIh7wgg/8MGpuHJvJ9IPXK6CXQa+Fx9w4NrMppWa6YBvIfnuZuDU3js1kebiEY0hCDMIQMDOUzWjZbFb9MY7jKMnlckre3j+UPD0+KLm/iSh5vQvChDL9o9gM1PUlZsH0hDYDJVInStJX16r/18I9jJKXYjEvz4VC/P+ij6AP0y/pw/yg2Wx20Gg02qnX6+3UarUOqtVqO5VKpZ1yudxBqVT6Rn/H4OY+BAeOINPiEA4gDSnYh13YgW1IwhZswgasQwLWYBVWIA7LEIMoRGAJFr/uIZd5mINZCLduqWmYghBMQhDGYQwCMALD4Ich8IG3RS9/9GTSe0ouzk572ZfuN/AnoLQRuSmfDyMAAAAASUVORK5CYII=)0 -767px repeat-x;
*background: url(mhtml:https://www.all4hardware4u.de/wp-content/cache/autoptimize/autoptimize_191ca98368e126a8daadba43b0bdcb22.txt!2)0 -767px repeat-x;

In the new AO this:

background:url(data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAASLCAYAAABUcdckAAABW0lEQVRYhe3Yy0pCURSA4aVmZVlSJEVRiJIUSVHYTcvOJKFB0KTLE5iTBj1JD9AsCHqFZr5FA+kBiube3Z1/FyIZCA7apPsHv6lrexabgyIiIfcjUUjBudhsIh7wgg/8MGpuHJvJ9IPXK6CXQa+Fx9w4NrMppWa6YBvIfnuZuDU3js1kebiEY0hCDMIQMDOUzWjZbFb9MY7jKMnlckre3j+UPD0+KLm/iSh5vQvChDL9o9gM1PUlZsH0hDYDJVInStJX16r/18I9jJKXYjEvz4VC/P+ij6AP0y/pw/yg2Wx20Gg02qnX6+3UarUOqtVqO5VKpZ1yudxBqVT6Rn/H4OY+BAeOINPiEA4gDSnYh13YgW1IwhZswgasQwLWYBVWIA7LEIMoRGAJFr/uIZd5mINZCLduqWmYghBMQhDGYQwCMALD4Ich8IG3RS9/9GTSe0ouzk572ZfuN/AnoLQRuSmfDyMAAAAASUVORK5CYII=);
*url(mhtml:https://www.all4hardware4u.de/wp-content/cache/autoptimize/autoptimize_ea7236f3adccbcdb29a144faa6852724.txt!18);_url(https://www.all4hardware4u.de/wp-content/themes/all4hardware4u/images/shadow.png);0 -767px repeat-x;

So here is missing the "0 -767px repeat-x" behind the datauri to work correctly with the sprite PNG

this is very likely due to the assets_regex @zytzagoo and @dtbaker, so I'm wondering if we need to change the regex, or switch back to a proper regex for the background images-stuff.

Contributor

zytzagoo commented Mar 3, 2017

Any chance to also get the original css for that snippet (ie., before these various transformations are applied?)

Or if they could test my fork too when they get the chance and see if the output is exactly the same?

Owner

futtta commented Mar 3, 2017 edited

sure;
background: url(https://www.all4hardware4u.de/wp-content/themes/all4hardware4u/images/shadow.png) 0 -767px repeat-x;

the problem as I understand it is that ASSETS_REGEX only captures url() but in order to ensure 0 -767px repeat-x; can be repeated for each "version" of the inlined image, you need to capture more then just url()

Contributor

zytzagoo commented Mar 3, 2017

Multiple "versions" of inlined images are there to support ancient IE versions only as I understand it?
(assuming that due to "star-background" rule being used/set, which applies only to IE7 and less if I remember correctly?)

All that mhtml stuff is only for those browsers or?

Owner

futtta commented Mar 3, 2017 edited

mhtml-stuff & co is for backwards compat with old IE's indeed (the .txt-file in 2nd pos and the normal link in 3rd)

but the fact that for each "version" we indeed need background: to be added at the beginning and the sprite-CSS 0 -767px repeat-x; at the end (in this example) is the actual issue at hand. hence my assumption we'll probably need to use (a variation of) the old regex which captures more then just url()

Owner

futtta commented Mar 3, 2017

rolling back to the original regex indeed fixed the issue, but we're back at square one as we then don't "harvest" all url's if there's more then one ..

Contributor

zytzagoo commented Mar 3, 2017

Matching and differentiating between multiple background images and all the other shorthand/longhand variations in a single regex sounds quite tricky, if not impossible even.

Experimenting with this currently: https://regex101.com/r/QfMBO9/1

It might work (for now), but is it good enough?

Owner

futtta commented Mar 3, 2017

I'm afraid you'll also need to capture what's in front of url(, as that part (i.e. background- or background:\s*) has to be duplicated as well ...

@zytzagoo zytzagoo added a commit to zytzagoo/autoptimize that referenced this issue Mar 3, 2017

@zytzagoo zytzagoo WARNING: modified ASSETS_REGEX and some other bits, looking into brea…
…kage described in futtta#87

Currently drops mhtml support, but other stuff appears to still work, but needs way more testing
b4376e9
Contributor

zytzagoo commented Mar 3, 2017

Well, it depends, I might be on to something, but I don't have more time to test and screw around with it.

My fork now has a commit in which the following theme css (testing manually, because datauris and test env and other code don't go along well :/) doesn't quite break:

.shadow { background:url(img/shadow.png) top center; }

With a filter:

add_filter( 'autoptimize_filter_css_datauri_maxsize', function(){
    return 40096;
});

(and a big shadow.png in 2017 child theme's img folder) results in:

.shadow{background:url(data:image/png;base64,...) top center}

So that's definitely a good start, and perhaps I manage to get mhtml support back in too... Who knows.
If not, my fork will live without mhtml support for now :)

Owner

futtta commented Mar 3, 2017

If not, my fork will live without mhtml support for now :)

that's a luxury that I obviously don't have (well, not without alienating at least a part of my userbase which I prefer not to do) :-)

Contributor

zytzagoo commented Mar 3, 2017

are there really that many people/sites out there that really need ie8/7/6 support for datauri'd images? or am I missing some other breakage?

why even support that in 2017? for free? worst case, those users can stay on an older version?

Owner

futtta commented Mar 3, 2017

yeah, true ... don't know. don't like regressions I guess. but I'll (re-)consider :-)

Owner

futtta commented Mar 4, 2017

conclusion after a good nights sleep; keeping that cruft (both code and output CSS) in there makes no sense any more.

@zytzagoo zytzagoo added a commit to zytzagoo/autoptimize that referenced this issue Mar 6, 2017

@zytzagoo zytzagoo improved handling of inlined and multiple bg images and sprites
ref: futtta#87

* adds two new filters for overriding/shortcircuiting image datauri inlining logic
* sort replacements array by key length in desc order (in fixurls() and rewrite_assets())
* adds crude tests for inlining and sprites etc. by using the new filters
a39edcb
Owner

futtta commented Mar 8, 2017

should be fixed with afc802b, will ask original reporter to confirm.

Owner

futtta commented Mar 9, 2017

confirmed fixed by Harry :-)

futtta closed this Mar 9, 2017

Yes confirmed;) Great work!!

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