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

Use of uniqid() generated CSS class names in WP 5.9 breaks parsed CSS transient caching #6925

Closed
westonruter opened this issue Feb 17, 2022 · 10 comments · Fixed by #6949
Closed
Assignees
Labels
Bug Something isn't working Changelogged Whether the issue/PR has been added to release notes. CSS P0 High priority
Milestone

Comments

@westonruter
Copy link
Member

westonruter commented Feb 17, 2022

Bug Description

See WordPress/gutenberg#38889 and the support forum topic.

I've created a mini plugin to fix the issue with layout container class names: https://gist.github.com/westonruter/71852bfeea22ebd7b570865a95b169d6

The same could be done for duotone and elements stylesheets as I've opened a PR to fix in Gutenberg: WordPress/gutenberg#38891 (disregarding the change to the responsive navigation block, which doesn't relate to CSS)

Note that the Duotone code is changing between WP 5.9 and 5.9.1, so two different versions may be needed.

We should probably apply these workarounds in 2.2.2 and potentially automatically re-enable CSS transient caching.

Expected Behaviour

Random strings should not be used to generate CSS class names so that parsed CSS transient caching is not automatically disabled.

Screenshots

No response

PHP Version

n/a

Plugin Version

2.2.1

AMP plugin template mode

Standard, Transitional, Reader

WordPress Version

5.9

Site Health

No response

Gutenberg Version

No response

OS(s) Affected

No response

Browser(s) Affected

No response

Device(s) Affected

No response

Acceptance Criteria

No response

Implementation Brief

No response

QA Testing Instructions

No response

Demo

No response

Changelog Entry

No response

@westonruter westonruter added Bug Something isn't working P0 High priority CSS labels Feb 17, 2022
@westonruter westonruter added this to the v2.2.2 milestone Feb 17, 2022
@delawski
Copy link
Collaborator

@westonruter I've been playing around with your plugin and was wondering if we really need to override the Gutenberg/Core filters in order to convert the unique IDs.

I've created this rough plugin that is a proof of concept for a different approach:
https://gist.github.com/delawski/31cf644d757b7068e8ebd2ac4c31c9ae

It first hooks into the render_block and searches for the specific class names like wp-container-62179f1feb01d. If found, it creates a new unique class handle using the same method as in your plugin (wp_unique_id). The mapping between the "old" and the new handles is also set.

Next, it hooks into the wp_enqueue_script (for block-based themes) or wp_footer (for classic themes) and looks for the same handles in the queued stylesheets. Every occurrence of the "old" handle in those inline stylesheets is replaced with the new one, based on the mapping we set up earlier.

Non-AMP AMP
Screenshot 2022-02-24 at 17 18 15 Screenshot 2022-02-24 at 17 18 26

Do you think that with that plugin the CSS transient caching would be re-enabled?

@westonruter
Copy link
Member Author

FYI: WordPress/gutenberg#38891 got labelled for backporting to a WP core minor release.

@westonruter
Copy link
Member Author

The fix was released as part of Gutenberg 12.7. It's not yet merged into core.

@westonruter
Copy link
Member Author

Do you think that with that plugin the CSS transient caching would be re-enabled?

I didn't see your question. But yes, this would prevent CSS transient caching from being disabled.

@westonruter westonruter assigned westonruter and unassigned delawski Mar 16, 2022
@westonruter
Copy link
Member Author

How to test this:

  1. On 2.2.1 with WordPress 5.9.1 active, run this WP-CLI command to disable CSS transient caching: wp eval "AMP_Options_Manager::update_option('amp_css_transient_monitor_disable_caching', true);"
  2. Go to Site Health and verify that the transient caching is disabled:
    image
  3. Create a post with the following content:
<!-- wp:image {"sizeSlug":"large","linkDestination":"none","style":{"color":{"duotone":["#8c00b7","#fcff41"]}}} -->
<figure class="wp-block-image size-large"><img src="https://amp-wp.org/wp-content/uploads/2018/11/Road-to-V1.0.jpg" alt=""/><figcaption>Wapuu</figcaption></figure>
<!-- /wp:image -->

<!-- wp:group {"backgroundColor":"foreground","textColor":"background"} -->
<div class="wp-block-group has-background-color has-foreground-background-color has-text-color has-background"><!-- wp:paragraph -->
<p>This is a paragraph.</p>
<!-- /wp:paragraph --></div>
<!-- /wp:group -->
  1. View the AMP version and you should see the markup as follows (notice the uniqid() in the class names:
<figure class="wp-duotone-6234bd477650a wp-block-image size-large">
	<img src="https://amp-wp.org/wp-content/uploads/2018/11/Road-to-V1.0.jpg" alt="" width="1544" height="500" class="amp-wp-enforced-sizes" decoding="async">
	<figcaption>Wapuu</figcaption>
</figure>
<div class="wp-container-6234bd4776eaa wp-block-group has-background-color has-foreground-background-color has-text-color has-background">
	<p>This is a paragraph.</p>
</div>
  1. Now upgrade to 2.2.2-alpha.
  2. Go to the Site Health screen and it should no longer say that CSS transient caching is disabled:
    image
  3. View the same AMP page again and the markup should now not utilize uniqid():
<figure class="wp-duotone-8 wp-block-image size-large">
	<img src="https://amp-wp.org/wp-content/uploads/2018/11/Road-to-V1.0.jpg" alt="" width="1544" height="500" class="amp-wp-enforced-sizes" decoding="async">
	<figcaption>Wapuu</figcaption>
</figure>
<div class="wp-container-9 wp-block-group has-background-color has-foreground-background-color has-text-color has-background">
	<p>This is a paragraph.</p>
</div>

@westonruter westonruter removed their assignment Mar 18, 2022
@pooja-muchandikar
Copy link

Followed the steps mentioned in this comment and found that the changes are reflecting in the markup but not on the Site health screen, with latest 2.2.2alpha Site health screen still shows transient caching is disabled

  1. Firstly, was able to reproduce the issue with AMP 2.2.1 and WordPress 5.9

Screenshot 2022-03-24 at 12 44 35 PM


  1. Next, executed step 5 as per this comment

  1. Step 6 from this comment is failing as it still shows transient caching is disabled ❌

image


  1. Step 7 from this comment is passed, the markup shows the expected result ✅

Screenshot 2022-03-24 at 12 44 19 PM

@maitreyie-chavan
Copy link
Collaborator

@dhaval-parekh please take a look at the issue reported in the QA notes

@westonruter
Copy link
Member Author

Just saw the fix was merged into the WordPress 5.9 branch: WordPress/wordpress-develop@f4837d7

So it should be part of WordPress 5.9.3.

@westonruter
Copy link
Member Author

5. Step 6 from this comment is failing as it still shows transient caching is disabled ❌

I'm not able to reproduce this issue. Parsed CSS transient caching is getting re-enabled for me:

unreproduced.mov

@pooja-muchandikar
Copy link

QA Passed ✅

Cross-checked the issue again with latest development build and seems its working fine. Parsed CSS transient caching is getting re-enabled.

Screenshot 2022-03-31 at 12 52 36 PM

@westonruter westonruter added the Changelogged Whether the issue/PR has been added to release notes. label Apr 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working Changelogged Whether the issue/PR has been added to release notes. CSS P0 High priority
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants