Prevent styles from being removed when in Customizer preview with Standard mode #4553

pierlon · 2020-04-08T20:39:23Z

Summary

When previewing pages in the Customizer with Standard mode enabled, style tags are being removed, which can cause the page to look messed up. Although the style sanitizer is disabled when previewing pages in the Customizer, the AmpBoilerplate transformer will run and remove all existing style tags.

~~This PR resolves the issue by removing the AmpBoilerplate transformer when retrieving the Optimizer configuration.~~

An example with having a custom background CSS set:

Before

After

Fixes #4551

Checklist

My pull request is addressing an open issue (please create one otherwise).
My code is tested and passes existing tests.
My code follows the Engineering Guidelines (updates are often made to the guidelines, check it out periodically).

…ndard mode enabled

kienstra

Works well

Hi @pierlon,
Nice work. Like you mentioned, I also see that <style> elements are no longer stripped.

For example, the <style id="wp-custom-css"> from Additional CSS now appears in Standard mode:

...where it used to be stripped:

I don't have enough context on the optimizer to say if the fix is in the right place, but it works well.

…ard-mode-customizer-preview * 'develop' of github.com:ampproject/amp-wp: Omit Jetpack from being activated during PHPUnit test runs Mock Imgur embed tests Mock Facebook embed tests Standardize file and class names for embed handlers

…with Standard mode enabled" This reverts commit 9d0e2ad.

westonruter · 2020-04-09T05:08:42Z

includes/class-amp-theme-support.php

+		// Prevent styles from being removed when in Customizer preview and Standard mode is enabled.
+		if ( ! empty( $args['allow_dirty_styles'] ) && amp_is_canonical() ) {
+			$transformers = array_diff( $transformers, [ Optimizer\Transformer\AmpBoilerplate::class ] );
+		}


I think this isn't the right approach. Really what should be done is fixing AmpBoilerplate to not remove styles that are not for the boilerplate.

westonruter · 2020-04-09T05:10:51Z

lib/optimizer/src/Transformer/AmpBoilerplate.php

-                        if (! $headNode->hasAttribute(Attribute::AMP_CUSTOM)) {
+                        if ($headNode->hasAttribute($boilerplate)) {
                            $document->head->removeChild($headNode);
                        }
                        break;
                    case Tag::NOSCRIPT:
-                        $document->head->removeChild($headNode);
+                        $style = $headNode->getElementsByTagName('style')->item(0);
+                        if ($style instanceof DOMElement && $style->hasAttribute($boilerplate)) {
+                            $document->head->removeChild($headNode);
+                        }


@schlessera this fixes something I noticed before when first reviewing the Optimizer code. I thought at some point the logic of removing all styles other than the amp-custom could cause problems, and it seems my suspicion was correct. So now this logic removes exclusveily the AMP boilerplate styles only, leaving other styles intact.

I wasn't too happy with this logic either, but it is what the canonical Go optimizer package uses as well:
https://github.com/ampproject/amppackager/blob/releases/transformer/transformers/ampboilerplate.go#L24-L37

Maybe an issue and/or PR for the Go package would make sense as well?

Yes. But otherwise, you see the changes in this PR as correct and approved?

(Our needs are a bit different in that we often need to serve invalid AMP, whereas the Go version probably only ever needs to serve valid AMP. So that's perhaps why it is fine for Go use the logic it does now, but otherwise it seems more robust to do what this PR does now.)

One problem that I see is that if there's a mismatch between the type of AMP that the <html> tag states, and the boilerplate that is already included, the wrong one will stay in place.

So, if the document states it is AMP4EMAIL, but the AMP4ADS boilerplate was added (for whatever reason), the optimizer will only try to remove the AMP4EMAIL boilerplate (and fail), and then re-insert it, with the end result being that two boilerplates are in the document.

I was originally looping over all the possible boilerplates but then paired it down to just the identified one. You think it's best to check all?

Yes, I think that'd be safer. We should always keep in mind that the Optimizer is not at all coupled to the plugin, so we cannot make any assumptions about the actual HTML that gets served into the optimizer. For all we know, it could be a buggy Drupal extension or a Symfony middleware that hands it over to the Optimizer. So, while it's important to keep the needs of the plugin in mind, we also need to keep the Optimizer in a state where it will just work regardless of input.

How do you think of 6762054?

Approach looks good, just not happy with the naming (see code review comments).

westonruter · 2020-04-09T05:11:32Z

lib/optimizer/tests/Transformer/AmpBoilerplateTest.php

-                $expected('<html ⚡4ads>', $amp4AdsBoilerplate),
-            ],
+            'leaves out boilerplate' => $repeatTwice(
+                $htmlDocument('<html amp i-amphtml-no-boilerplate>'),


I noticed there weren't tests for the i-amphtml-no-boilerplate condition, so I've added a few here as well.

lib/optimizer/src/Transformer/AmpBoilerplate.php

Co-Authored-By: Alain Schlesser <alain.schlesser@gmail.com>

…ndard mode (#4553) * Prevent styles from being removed when in Customizer preview with Standard mode enabled * Revert "Prevent styles from being removed when in Customizer preview with Standard mode enabled" This reverts commit 9d0e2ad. * Re-work AmpBoilerplate to only remove boilerplate-specific styles * Remove now-unused AmpProject\DevMode * Improve phpdoc explaining refined behavior * Remove trailing comma from function args * Simplify logic in removeStyleAndNoscriptTags * Remove all boilerplates not just the identified one * Rename method to isBoilerplateStyle and improve docs Co-Authored-By: Alain Schlesser <alain.schlesser@gmail.com> Co-authored-by: Weston Ruter <westonruter@google.com> Co-authored-by: Alain Schlesser <alain.schlesser@gmail.com>

westonruter · 2020-04-09T16:10:34Z

Build for testing: amp.zip (v1.5.3-alpha-20200409T160641Z-5856b82a3)

…aching-reenable-button * 'develop' of github.com:ampproject/amp-wp: Restore unification of multi-page post content in Reader mode (#4547) Prevent styles from being removed when in Customizer preview with Standard mode (#4553) Omit Jetpack from being activated during PHPUnit test runs Use title case for Paired Browsing link in edit post screen (#4540) Ensure that validation query vars persist through redirects (#4544) Update dependency babel-jest to v25.2.6 (#4510) Update dependency css-loader to v3.5.0 (#4537) Update dependency autoprefixer to v9.7.6 (#4539) Add requirements to plugin file header (#4543) Force status code of validation responses to be 200 (#4533) Update optimizer test specs (#4527) Bump stable tag to 1.5.2 Cache response status and headers when fetching external stylesheets (#4509) Fix securing multi-line mustache templates (#4521) Add CSS monitoring time series to Site Health debugging info (#4519) Update hostname used for WordPress TV embeds to fix external HTTP requests (#4524) Mock Imgur embed tests Mock Facebook embed tests Standardize file and class names for embed handlers

GiNgesIo · 2020-04-12T11:45:59Z

The customizer preview is now working with the Version 1.5.3-alpha built. Thank you!
Will this be implemented in future updates so I can leave the AMP plugin auto update turned on?

I just noticed that enabling the AMP plugin makes the toolbar disappear even though I have "Show Toolbar when viewing site" still turned on.

Also the Smush Image Resize Detection is now (with your built) working in the customizer but not on the frontend.

Inspecting my website I noticed 3 errors:
[Error] Not allowed to load local resource: blob://nullhttps//paulreno.de/ww.js.map
[Error] Not allowed to request resource
[Error] Cannot load blob://nullhttps//paulreno.de/ww.js.map due to access control checks.

westonruter · 2020-04-12T16:18:56Z

@tinuslorvalds Thanks for testing. Yes, this fix will be part of 1.5.3 so you'll get it automatically when you update.

I just noticed that enabling the AMP plugin makes the toolbar disappear even though I have "Show Toolbar when viewing site" still turned on.

This is most likely due to a bug in Jetpack: Automattic/jetpack#15353.

Here’s a plugin you can install which will workaround that issue until the next version of Jetpack is released: https://gist.github.com/westonruter/d168c290d6c01c107da960d48fa3dad3

Installation instructions: https://gist.github.com/westonruter/6110fbc4bef0c4b8c021a112012f7e9c

Also the Smush Image Resize Detection is now (with your built) working in the customizer but not on the frontend.

There have been issues reported for Smush, for example: https://wordpress.org/support/topic/amp-integration-not-currently-working/

Apparently the issue is with Smush and not the AMP plugin. What is not working? Are images not showing up on AMP pages? In Standard mode there is no need for Smush's image lazy loading feature, since amp-img has that built-in.

Inspecting my website I noticed 3 errors:
[Error] Not allowed to load local resource: blob://nullhttps//paulreno.de/ww.js.map
[Error] Not allowed to request resource
[Error] Cannot load blob://nullhttps//paulreno.de/ww.js.map due to access control checks.

These are likely not related to the AMP plugin.

* tag '1.5.3': Bump 1.5.3 Bump version to 1.5.3-RC1 Fix handling of Mustache templates (#4583) Stub request based on test scenario (#4588) Update tests after block-library/style.css changes in Gutenberg 7.9 (#4579) Restrict doing plugin upgrade routine when not in admin (#4538) Add new accessibility sanitizer (#4535) Fix unit tests (#4564) Add button into Site Health to reenable CSS transient caching (#4522) Restore unification of multi-page post content in Reader mode (#4547) Prevent styles from being removed when in Customizer preview with Standard mode (#4553) Omit Jetpack from being activated during PHPUnit test runs (#4474) Mock Facebook embed tests (#4474) Mock Imgur embed tests (#4474) Use title case for Paired Browsing link in edit post screen (#4540) Ensure that validation query vars persist through redirects (#4544) Add requirements to plugin file header (#4543) Force status code of validation responses to be 200 (#4533) Update optimizer test specs (#4527) Bump 1.5.3-alpha

GiNgesIo · 2020-04-16T17:15:05Z

@westonruter thank you for your help.
Images are showing up on AMP pages but the Image Resize Detection is only working in the customizer preview. This is a feature that shows what images are being resized.

I am also getting a couple of suggestions from Google PageSpeed Insights:

Defer offscreen images: Consider lazy-loading offscreen and hidden images after all critical resources have finished loading to lower time to interactive.
Ensure that you are you using valid amp-img tags for your images which automatically lazy-load outside the first viewport.

Reduce the impact of third-party code: AMP 213 KB
Minimize main-thread work
Reduce JavaScript execution time: /v0.js(cdn.ampproject.org)

westonruter · 2020-04-16T17:44:58Z

Images are showing up on AMP pages but the Image Resize Detection is only working in the customizer preview. This is a feature that shows what images are being resized.

Most likely because the resize detection requires custom JS and the custom JS is currently only allowed in the Customizer preview. So this is working as expected. The image resize detection functionality would need to find an AMP-compatible alternative implementation.

Defer offscreen images: Consider lazy-loading offscreen and hidden images after all critical resources have finished loading to lower time to interactive. Ensure that you are you using valid amp-img tags for your images which automatically lazy-load outside the first viewport.

This is an issue with Lighthouse. See GoogleChrome/lighthouse#10471

Reduce the impact of third-party code: AMP 213 KB
Minimize main-thread work
Reduce JavaScript execution time: /v0.js(cdn.ampproject.org)

This would be an upstream issue for AMP itself.

There is a project board to track further performance improvements to the AMP runtime, see: https://github.com/ampproject/amphtml/projects/99

Please file an issue on the amphtml repo: https://github.com/ampproject/amphtml/issues

Make sure you include the URL to the AMP page that results in that Lighthouse audit. Then comment back here with a link to the issue you created.

Prevent styles from being removed when in Customizer preview with Sta…

9d0e2ad

…ndard mode enabled

pierlon requested review from schlessera and westonruter April 8, 2020 20:39

googlebot added the cla: yes Signed the Google CLA label Apr 8, 2020

pierlon requested a review from kienstra April 8, 2020 20:39

westonruter added this to the v1.5.3 milestone Apr 9, 2020

kienstra approved these changes Apr 9, 2020

View reviewed changes

westonruter added 3 commits April 8, 2020 21:05

Revert "Prevent styles from being removed when in Customizer preview …

b7d273a

…with Standard mode enabled" This reverts commit 9d0e2ad.

Re-work AmpBoilerplate to only remove boilerplate-specific styles

00dc3ce

westonruter reviewed Apr 9, 2020

View reviewed changes

westonruter added 2 commits April 8, 2020 22:29

Remove now-unused AmpProject\DevMode

b3fb078

Improve phpdoc explaining refined behavior

d8fc048

westonruter approved these changes Apr 9, 2020

View reviewed changes

westonruter added 3 commits April 8, 2020 22:36

Remove trailing comma from function args

ba90f19

Simplify logic in removeStyleAndNoscriptTags

4f81f56

Remove all boilerplates not just the identified one

6762054

schlessera requested changes Apr 9, 2020

View reviewed changes

lib/optimizer/src/Transformer/AmpBoilerplate.php Outdated Show resolved Hide resolved

lib/optimizer/src/Transformer/AmpBoilerplate.php Outdated Show resolved Hide resolved

lib/optimizer/src/Transformer/AmpBoilerplate.php Outdated Show resolved Hide resolved

Rename method to isBoilerplateStyle and improve docs

89105c8

Co-Authored-By: Alain Schlesser <alain.schlesser@gmail.com>

westonruter requested a review from schlessera April 9, 2020 15:48

schlessera approved these changes Apr 9, 2020

View reviewed changes

westonruter merged commit dba6055 into develop Apr 9, 2020

westonruter deleted the fix/standard-mode-customizer-preview branch April 9, 2020 15:59

westonruter mentioned this pull request Apr 9, 2020

Standard Mode Messes Up Customizer Preview #4551

Closed

westonruter mentioned this pull request Apr 11, 2020

Customizer doesn't load custom /template/style.css #4565

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prevent styles from being removed when in Customizer preview with Standard mode #4553

Prevent styles from being removed when in Customizer preview with Standard mode #4553

pierlon commented Apr 8, 2020 •

edited by westonruter

Loading

kienstra left a comment •

edited

Loading

westonruter Apr 9, 2020

westonruter Apr 9, 2020

schlessera Apr 9, 2020

westonruter Apr 9, 2020

westonruter Apr 9, 2020

schlessera Apr 9, 2020

westonruter Apr 9, 2020

schlessera Apr 9, 2020

westonruter Apr 9, 2020

schlessera Apr 9, 2020

westonruter Apr 9, 2020

westonruter commented Apr 9, 2020

GiNgesIo commented Apr 12, 2020

westonruter commented Apr 12, 2020

GiNgesIo commented Apr 16, 2020

westonruter commented Apr 16, 2020

Prevent styles from being removed when in Customizer preview with Standard mode #4553

Prevent styles from being removed when in Customizer preview with Standard mode #4553

Conversation

pierlon commented Apr 8, 2020 • edited by westonruter Loading

Summary

Checklist

kienstra left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

westonruter commented Apr 9, 2020

GiNgesIo commented Apr 12, 2020

westonruter commented Apr 12, 2020

GiNgesIo commented Apr 16, 2020

westonruter commented Apr 16, 2020

pierlon commented Apr 8, 2020 •

edited by westonruter

Loading

kienstra left a comment •

edited

Loading