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

Prevent sanitization of Customizer preview styles #4977

Merged
merged 5 commits into from
Jul 4, 2020

Conversation

pierlon
Copy link
Contributor

@pierlon pierlon commented Jul 2, 2020

Summary

When the site is being previewed in the Customizer:

  • Adds the data-ampdevmode attribute to scripts so that they are ignored by sanitizers
  • Adds the data-ampdevmode attribute to all dependencies of the customizer-preview style handle, including itself
  • Removes references of the allow_dirty_styles and allow_dirty_scripts sanitizer args from the codebase as they are no longer being used anywhere

Fixes #4804.

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).

@googlebot googlebot added the cla: yes Signed the Google CLA label Jul 2, 2020
@pierlon pierlon requested review from westonruter and schlessera July 2, 2020 23:51
@pierlon
Copy link
Contributor Author

pierlon commented Jul 2, 2020

@westonruter Do you think this PR also include the prevention of CSS tree shaking when the sanititization argument allow_dirty_styles is true?

@github-actions
Copy link
Contributor

github-actions bot commented Jul 2, 2020

Plugin builds for 9dd7976 are ready 🛎️!

includes/class-amp-theme-support.php Outdated Show resolved Hide resolved
includes/class-amp-theme-support.php Outdated Show resolved Hide resolved
includes/templates/class-amp-content-sanitizer.php Outdated Show resolved Hide resolved
@pierlon pierlon requested a review from westonruter July 3, 2020 03:48
Copy link
Member

@westonruter westonruter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something else we need to address is how the Customizer preview handles (or doesn't currently) the addition of a header video in Twenty Seventeen. Currently a YouTube video just fails to show up, and an MP4 is positioned in the wrong spot. We need to make sure that the header_video and external_header_video settings get a transport assigned of refresh as opposed to postMessage to deal with this. I'll address this as part of #4984.

@westonruter westonruter added this to the v1.6 milestone Jul 4, 2020
@westonruter
Copy link
Member

(E2E tests failing is a known issue to be fixed by #4980.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelogged Whether the issue/PR has been added to release notes. cla: yes Signed the Google CLA
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Header image fails to load in Customizer for Twenty Seventeen theme in Standard mode
3 participants