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

Disabled tree shaking in Customizer causes stylesheet exclusions #6787

Closed
westonruter opened this issue Dec 14, 2021 · 2 comments · Fixed by #6788
Closed

Disabled tree shaking in Customizer causes stylesheet exclusions #6787

westonruter opened this issue Dec 14, 2021 · 2 comments · Fixed by #6788
Labels
Bug Something isn't working Changelogged Whether the issue/PR has been added to release notes. P0 High priority Sandboxing Experiment
Milestone

Comments

@westonruter
Copy link
Member

Bug Description

I discovered a regression when testing Twenty Fifteen as a Reader theme. When I opened the AMP Customizer, I saw the theme without any styling:

image

I see the same thing if I view the changeset preview link (https://wordpress-stable.lndo.site/hello-world/?amp=1&customize_changeset_uuid=9c4fa0b0-3e3a-48a3-92a6-90b2812802e6)

If I validate that changeset preview I see the stylesheets as follows:

image

Note that two are being excluded. No tree shaking is being performed:

image

It is expected that tree-shaking is disabled in the Customizer preview, as Customizer scripts required dynamic style rules. What is not expected, however, is that stylesheets are getting excluded.

I can also reproduce this issue in Twenty-Twenty One and Twenty Twenty.

It does not happen in Standard mode because excessive CSS is kept by default in Standard mode.

Expected Behaviour

Excessive CSS should be kept by default in the Customizer preview.

Screenshots

No response

PHP Version

No response

Plugin Version

2.2

AMP plugin template mode

Transitional, Reader

WordPress Version

No response

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 labels Dec 14, 2021
@westonruter westonruter added this to the v2.2 milestone Dec 14, 2021
@westonruter westonruter self-assigned this Dec 14, 2021
@westonruter
Copy link
Member Author

The regression was introduced in #6546 in which the allow_excessive_css arg was added to AMP_Style_Sanitizer to be a more explicit opt-in to keeping excessive CSS, instead of using skip_tree_shaking to imply that excessive CSS should be kept:

image

So the only thing we need to do is:

--- a/includes/amp-helper-functions.php
+++ b/includes/amp-helper-functions.php
@@ -1560,7 +1560,8 @@ function amp_get_content_sanitizers( $post = null ) {
 		],
 		AMP_Block_Sanitizer::class             => [], // Note: Block sanitizer must come after embed / media sanitizers since its logic is using the already sanitized content.
 		AMP_Style_Sanitizer::class             => [
-			'skip_tree_shaking' => is_customize_preview(),
+			'skip_tree_shaking'   => is_customize_preview(),
+			'allow_excessive_css' => is_customize_preview(),
 		],
 		AMP_Meta_Sanitizer::class              => [],
 		AMP_Layout_Sanitizer::class            => [],

@westonruter
Copy link
Member Author

QA Passed per #6788 (comment)

@westonruter westonruter added the Changelogged Whether the issue/PR has been added to release notes. label Dec 15, 2021
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. P0 High priority Sandboxing Experiment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant