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

Already on GitHub? Sign in to your account

Added support for multiple background images and CDN #81

Merged
merged 2 commits into from Feb 20, 2017

Conversation

Projects
None yet
4 participants
Contributor

dtbaker commented Feb 4, 2017

Here is the alternate version as mentioned here: #80 (comment)

This uses a single regex with minimal code changes.

Contributor

dtbaker commented Feb 4, 2017

@zytzagoo will your ASSETS_REGEX be making it into master any time soon? That looks like the regex to use.

Contributor

zytzagoo commented Feb 4, 2017

@dtbaker that's completely up to @futtta to decide :)

Personally, I'd be more than thrilled to get my fork fully merged upstream at some point, since I have 20+ sites using it in production.

But it would require a lot of work and testing to be sure nothing's really broken (since I changed some stuff, moved code around, changed certain method visibility so it can be tested more easily, used a different coding style, etc. -- it's not [currently, at least] a clean/simple merge).

It works for me and the sites I've built, but who knows if it'll work everywhere. I try to keep it in sync with upstream and even created a (rather crude) unit test suite/setup, but the plugin as a whole does a million different things with a lot of options, so... it's complicated :)

If you've got the will & energy to try running the fork on the verge-blog-demo you mentioned, that might help in some way?

But, again, @futtta is the boss here... and merging this "easy" PR is a welcome improvement for everyone if it doesn't break anything. And it might even get my regex into master indirectly if you update your PR to contain it :)

Contributor

dtbaker commented Feb 4, 2017

Cheers @zytzagoo - I'll give your fork a whirl and update the regex in my PR

Owner

futtta commented Feb 5, 2017 edited

Great! :-)

Regarding the PR; @dtbaker can you indeed update your code to have @zytzagoo's assets_regex? I'll be happy to merge this!

The bigger picture; @zytzagoo 's fork is great and I try to follow what happens there, but given AO is currently active on over 200000 websites, I'm simply too scared to try to merge. Micro-changes (like this one) to fix something that is clearly broken on the other hand are a go ;-)

Owner

futtta commented Feb 10, 2017

pinging @dtbaker: can you indeed update your code to have zytzagoo's assets_regex? I'll be happy to merge this!

Contributor

dtbaker commented Feb 10, 2017

There we go @futtta I've moved @zytzagoo's regex into my commit: 90d5285

Cheers.

Owner

futtta commented Feb 10, 2017

thanks @dtbaker !

See you also use the assets_regex in fixurls() as @zytzagoo does, but that discards at least one fix that went into the fixurl-regex for a bug reported back in 2015. you can compare this regex101 test with the assets_regex vs the same test with the legacy one (which isn't perfect either as it has the same problem as the one that got you into this in the first place ;-) ).

So possible approaches:

  • we don't use assets_regex in fixurls (sticking with the legacy one)
  • we improve assets_regex so those weird CSS clipping edge-cases don't break (my preferred solution)

@zytzagoo zytzagoo added a commit to zytzagoo/autoptimize that referenced this pull request Feb 10, 2017

@zytzagoo zytzagoo updates ASSETS_REGEX to support an old fix done to upstream's autopti…
…mizeStyles::fixurls() + minor whitespace trim

Hopefully doesn't break anything else... See more details here:
futtta#81 (comment)
futtta@99ea204
db7e1bb
Contributor

zytzagoo commented Feb 10, 2017

Can we create a minimal test case(s) which would demonstrate the issues above?

i.e., from reading that thread, the idea is that urls beginning with '#...' should not be matched/touched at all?

Could that cause some weirdness in case cdn replacement needs to be done?

Owner

futtta commented Feb 10, 2017

well, the small block in the linked regex101-test could be a start;

section.clipped.clippedTop {clip-path:url("#clipPolygonTop")}
section.clipped.clippedBottom {clip-path:url("#clipPolygonBottom")}

.myimg {background-image: url("images/under-left-leaf.png"), url("images/over-blue-bird.png"), url("images/under-top.png"), url("images/bg-top-grunge.png");}

what weirdness are you expecting for CDN replacement?

Contributor

zytzagoo commented Feb 10, 2017

yeah its a start, but whats the expected outcome in that snippet, based on various options?

re: cdn, it might be expected from a user POV that all urls/assets should be replaced/pointed to the cdn url, including those that only start with a hash?

am on mobile now, will look into it some more when i get the chance.

Owner

futtta commented Feb 10, 2017

expected outcome after fixurls():

section.clipped.clippedTop {clip-path:url("#clipPolygonTop")}
section.clipped.clippedBottom {clip-path:url("#clipPolygonBottom")}

.myimg {background-image: url("//blog.url/wp-content/themes/atheme/images/under-left-leaf.png"), url("//blog.url/wp-content/themes/atheme/images/over-blue-bird.png"), url("//blog.url/wp-content/themes/atheme/images/under-top.png"), url("//blog.url/wp-content/themes/atheme/images/bg-top-grunge.png");}

so #-stuff is left untouched (as not a URL, clearly)
none #-stuff is considered a normal relative URL and is turned into an absolute one

regarding CDN: a clip-path:url("#xyz") is not an asset, it can't be cdn'ed :-)

Contributor

zytzagoo commented Feb 10, 2017

Sweet, should be great then with the new regex.

I'll try to add some tests for this to my fork next week (if not earlier).

Owner

futtta commented Feb 18, 2017

All good with that commit & tests @zytzagoo? if so, can you update the PR so I can merge @dtbaker ? :-)

Contributor

zytzagoo commented Feb 18, 2017

looks good to me

Contributor

zytzagoo commented Feb 18, 2017

test passes with the updated regex: zytzagoo/autoptimize@b6cb280

Owner

futtta commented Feb 18, 2017

super, let's update the & merge this micro-monster of a PR @dtbaker ;-)

@futtta futtta merged commit 0a13cd5 into futtta:master Feb 20, 2017

1 check passed

Scrutinizer 2 new issues
Details
Owner

futtta commented Feb 20, 2017 edited

this just in; when you have SVG embedded in CSS (yeah) but instead of base64'ing one encodes some characters, then you can have this;

fill='url(%23c)'

which again should not be "fixurl-ed". this update of the regex fixes that,

url\s*\(\s*(?!["\']?data:)(?![\'|\"]?[\#|\%])([^)]+)\s*\)

I've added a working example of CSS that uses this approach and the test-cases discussed earlier in this regex101 test and all seems OK.

So what to you guys think? "GO"? :-)

@zytzagoo zytzagoo added a commit to zytzagoo/autoptimize that referenced this pull request Feb 20, 2017

@zytzagoo zytzagoo update ASSETS_REGEX and test_fixurls_with_hash_only_urls() to confirm…
… it doesn't break with svg containing some encoded chars:

see: futtta#81 (comment)
https://regex101.com/r/42L4J3/2
6c4d372
Contributor

zytzagoo commented Feb 20, 2017

That's some crazy stuff right there :)
Did you go hunting for that edge-case or just ran into it by chance somewhere? :)

https://travis-ci.org/zytzagoo/autoptimize/builds/203498887 is green, so I guess it's fine [until some other weirdness comes along :)]

Owner

futtta commented Feb 20, 2017

no, crazy is coming to me all by itself, seems like I attract crazy ;-)

I don't think speeding a website is crazy stuff :) By using smart encoding of inline svg instead of base64 I saved about 3kB on my css file. It is two usual data packets! :) Instead of saying thank you and incorporating that idea (not mine, I admit) into autoptimize paid version, you call people weirdos. Eh, it is just Internet, full of wolves :)

Owner

futtta commented Feb 20, 2017

it's crazy nonetheless, but good crazy ;-)

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