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

Divi 4.10+ compatibility improvement #4320

Closed
4 tasks
piotrbak opened this issue Sep 2, 2021 · 3 comments · Fixed by #4377
Closed
4 tasks

Divi 4.10+ compatibility improvement #4320

piotrbak opened this issue Sep 2, 2021 · 3 comments · Fixed by #4377
Assignees
Labels
3rd party compatibility Issues related to 3rd party compatibility like theme, plugin or hosting effort: [S] 1-2 days of estimated development time module: combine CSS 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 Sep 2, 2021

Before submitting an issue please check that you’ve completed the following steps:

  • Made sure you’re on the latest version
  • Used the search feature to ensure that the bug hasn’t been reported before

Describe the bug
When we have specific settings enabled, we'll change the order of the styles. Which might cause the layout problems. Problematic setup:

  1. WP Rocket's CSS Combination enabled
  2. WP Rocket's RUCSS disabled
  3. Divi's Load Dynamic Stylesheet Inline enabled

This is happening because we're not combining internal styles <style></style>. This is how it works for a long time, however, Divi 4.10+, for any reason, is adding some important styles to CSS files and some others (that could be related to the same elements) to internal styles.

To Reproduce
Steps to reproduce the behavior:

  1. Enable settings as described above
  2. Check the source of the cached page
  3. The combined CSS file is at the very top
  4. Check the source of the not cached page
  5. Divi CSS files are below the internal styles - the order is changed!

Expected behavior
We should preserve Divi's CSS order. We could:

  1. Disable their feature when RUCSS is disabled + Combine CSS is enabled
  2. Disable our Combine CSS when their option is enabled + RUCSS is disabled
  3. Exclude their files from combination when RUCSS is disabled

Screenshots
If applicable, add screenshots to help explain your problem.

Additional context
Add any other context about the problem here.

Backlog Grooming (for WP Media dev team use only)

  • Reproduce the problem
  • Identify the root cause
  • Scope a solution
  • Estimate the effort
@piotrbak piotrbak added type: enhancement Improvements that slightly enhance existing functionality and are fast to implement 3rd party compatibility Issues related to 3rd party compatibility like theme, plugin or hosting priority: high Issues which should be resolved as quickly as possible module: combine CSS labels Sep 2, 2021
@mostafa-hisham mostafa-hisham added GROOMING IN PROGRESS Use this label when the issue is currently being groomed. and removed needs: grooming labels Sep 21, 2021
@mostafa-hisham
Copy link
Contributor

mostafa-hisham commented Sep 21, 2021

Reproduce the issue ✔️

Identify the root cause ✅

Divi CSS files order is changed when we use combined CSS
in some cases, it will be above the Divi inline style which is not the correct order

Scope a solution ✅

I think the third solution that @piotrbak suggested is the better solution
we need to exclude all Divi CSS files which are stored in /wp-content/et-cache/ From being Compiend.

  1. In inc/ThirdParty/Themes/Divi.php
    add an event rocket_combine_css_excluded_external when Rucss is disabled
		if ( ! (bool) $this->options->get( 'remove_unused_css', 0 ) ) {
			$events['rocket_combine_css_excluded_external'] = 'exclude_divi_css_from_combine';
		}
  1. In inc/ThirdParty/Themes/Divi.php add the function to exclude Divi CSS files
public function exclude_divi_css_from_combine( $exclude_css ) {
		$wp_content  = wp_parse_url( content_url( '/' ), PHP_URL_PATH );
		$exclude_css [] = $wp_content."et-cache/(.*).css";
		return $exclude_css;
	}

  1. Create tests

Estimate the effort ✅

Effort [S]

@mostafa-hisham mostafa-hisham added effort: [S] 1-2 days of estimated development time and removed GROOMING IN PROGRESS Use this label when the issue is currently being groomed. labels Sep 21, 2021
@remyperona
Copy link
Contributor

@mostafa-hisham wouldn't checking for the value of the remove_unused_css value be enough, instead of injecting the whole controller?

@mostafa-hisham
Copy link
Contributor

mostafa-hisham commented Sep 21, 2021

@Tabrisrp
You are right and I updated the grooming to use options

In UsedCSS::is_allowed() have other conditions not only if the rucss is enabled or not
that's why i injected the controller

@mostafa-hisham mostafa-hisham self-assigned this Sep 23, 2021
@iCaspar iCaspar added this to the 3.10.1 milestone Sep 27, 2021
remyperona added a commit that referenced this issue Oct 5, 2021
Co-authored-by: mostafa-hisham <mostafa-hisham@users.noreply.github.com>
Co-authored-by: Rémy Perona <remperona@gmail.com>
@remyperona remyperona mentioned this issue Oct 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3rd party compatibility Issues related to 3rd party compatibility like theme, plugin or hosting effort: [S] 1-2 days of estimated development time module: combine CSS 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.

4 participants