Skip to content
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

Get rid of "Combine CSS files" option #5909

Closed
piotrbak opened this issue May 8, 2023 · 7 comments · Fixed by #5911
Closed

Get rid of "Combine CSS files" option #5909

piotrbak opened this issue May 8, 2023 · 7 comments · Fixed by #5911
Assignees
Labels
effort: [M] 3-5 days of estimated development time epics 🔥 module: combine CSS needs: documentation Issues which need to create or update a documentation priority: high Issues which should be resolved as quickly as possible type: enhancement Improvements that slightly enhance existing functionality and are fast to implement
Milestone

Comments

@piotrbak
Copy link
Contributor

piotrbak commented May 8, 2023

Is your feature request related to a problem? Please describe.
HTTP/1 is outdated protocol and CSS Combination is only useful in this specific circumstance for performance purposes. It's also good in terms of browser caching, as the single files will be saved and shared between all pages.

Describe the solution you'd like
The further details related to this improvement are available here.

@piotrbak piotrbak added type: enhancement Improvements that slightly enhance existing functionality and are fast to implement priority: high Issues which should be resolved as quickly as possible needs: grooming epics 🔥 module: combine CSS labels May 8, 2023
@piotrbak piotrbak added this to the 3.15 milestone May 8, 2023
@CrochetFeve0251 CrochetFeve0251 added GROOMING IN PROGRESS Use this label when the issue is currently being groomed. and removed needs: grooming labels May 9, 2023
@CrochetFeve0251
Copy link
Contributor

CrochetFeve0251 commented May 9, 2023

Scope a solution

The first step will be to get rid of the actual settings section.
For that we will have to remove inside Page.php from the line 682 to 705.

We will have to remove the logic inside Minify CSS:

  • Inside CSS we will have to change the process logic from:
if ( $this->options->get( 'minify_css' ) && $this->options->get( 'minify_concatenate_css' ) ) {
 $this->set_processor_type( new Combine( $this->options, $assets_local_cache ) );
} elseif ( $this->options->get( 'minify_css' ) && ! $this->options->get( 'minify_concatenate_css' ) ) {
 $this->set_processor_type( new Minify( $this->options, $assets_local_cache ) );
}

to :

 if ( $this->options->get( 'minify_css' ) ) {
 $this->set_processor_type( new Minify( $this->options, $assets_local_cache ) );
}
  • We will have to delete the Combine class.

Then we will have to remove everywhere this code is used in the core logic:

  • Inside the RUCSS/Settings class remove the method maybe_disable_combine_css
  • Inside Support/Data remove line 33 the index.
  • Inside DeactivationIntent we will have to remove line 135.
  • Inside Upgrader we will have to remove line 109.

Then we will have to remove it inside 3rd parties:

  • Inside Autoptimize we will need to remove lines 43 and 92
  • Inside Divi we will remove the method exclude_css_from_combine
  • Inside Cloudflare remove the reference at line 81

To clean assets from wpr on update, we will clear the cache.

Finally we will have to make sure all tests pass.

Estimate effort

Effort M

@CrochetFeve0251 CrochetFeve0251 added effort: [M] 3-5 days of estimated development time and removed GROOMING IN PROGRESS Use this label when the issue is currently being groomed. labels May 9, 2023
@piotrbak piotrbak added the needs: documentation Issues which need to create or update a documentation label May 15, 2023
@rorytassell
Copy link

The combine CSS feature actually helps with SEO.

Allowing GoogleBot to crawl less CSS/JS files is ALWAYS a good thing. While minifying files is good for performance, you still have a similar amount of individual CSS files (i.e. http requests) which GoogleBot (or bing etc) will need to crawl, thus using precious crawl budget.

Not sure who signed this off, or why it got worked/released so quickly considering there are plenty of other bug fixes and features missing.

@scottstanford
Copy link

Agreed with @rorydesign - the combine CSS feature is essential (and ultimately could be disabled in the UI if someone doesn't want to use it). Our site structure uses a dynamic set of small css files, but combining them allowed for much better performance and page load times.

With this feature removed, we are getting much longer page loads and have had to rollback to a previous version.

@DahmaniAdame
Copy link
Contributor

Many factors motivated the change:

  • HTTP2 is now the norm in most websites
  • PageSpeed deprecating the related recommendation (the former Combine External CSS/JS)
  • High risk of causing trouble, like when other layers of page cache are involved
  • Regarding reducing the number of requests, browser cache will sort that out for field results as all assets will be requested once and then cached, and using Remove Unused CSS will have a similar effect as combining by drastically reducing the number of requests related to CSS files
  • CSS control is getting more granular, and delivering a combined and unoptimized file goes against that

With this feature removed, we are getting much longer page loads and have had to rollback to a previous version.

@scottstanford would you mind sending me the audit results from https://www.webpagetest.org/ to adame@wp-media.me for combined vs. no combined?

I'll be curious to see the impact on your specific use case.

While minifying files is good for performance, you still have a similar amount of individual CSS files (i.e. http requests) which GoogleBot (or bing etc) will need to crawl, thus using precious crawl budget.

@rorydesign using Remove Unused CSS will have a similar effect.

We understand that this change won't be to the taste of some of our users. We apologize for the inconvenience, but it was a necessary step to take on our side.

There is room for reconsideration if there is a drastic penalty for the vast majority of our users.

@aharonyltd
Copy link

Bad decision by wp rocket team
Some sites load CSS files one after the other and cause CLS issues
Not in every site we can use the CSS removal feature because it still doesn't work smoothly.

I improve the performance of websites, I won the first place in the competition to improve the speeds of Cloudways WooCommerce websites

I suggest you bring back the Combine CSS feature

There are sites where I need to install another plugin that will do this action

Secondly, you did something that sometimes things don't work well with the CSS
There are such paths that are added:
https://wordpress-505400-1873104.cloudwaysapps.com/wp-content/cache/background-css/wordpress-505400-1873104.cloudwaysapps.com/wp-content/cache/min/1/wp-content/plugins/ shortcodes-ultimate/includes/css/shortcodes.css?ver=1694525856&wpr_t=1694525856

This also caused the broken loading of some icons and fonts on the sites that I am improving the speed of

@vmanthos
Copy link
Contributor

This also caused the broken loading of some icons and fonts on the sites

@piotrbak FYI, I checked this 👆 with lazyload for CSS background images and it's working fine. Icons are also displayed so this might be coming from Remove Unused CSS if that's enabled.

@piotrbak
Copy link
Contributor Author

Thank you for the feedback, it's very apprieciated. Please remember that we use GitHub for development purposes only.

If you experience any performance decrease because of the lack of CSS Combination feature, please reach our Support Team with real data or example, we'll be more than happy to analyze that and find a proper solution. Each decision made is preceded by research and benchmarks.

@aharonyltd I'm sorry to hear you are experiencing problem with new feature. Please reach our Support Team, we'll investigate the problem and fix it very soon.

@wp-media wp-media locked and limited conversation to collaborators Sep 13, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
effort: [M] 3-5 days of estimated development time epics 🔥 module: combine CSS needs: documentation Issues which need to create or update a documentation priority: high Issues which should be resolved as quickly as possible type: enhancement Improvements that slightly enhance existing functionality and are fast to implement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants