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

Add sanitizer for ensuring AMP compatibility for core themes #1060

Closed
postphotos opened this issue Apr 5, 2018 · 6 comments · Fixed by #1074
Closed

Add sanitizer for ensuring AMP compatibility for core themes #1060

postphotos opened this issue Apr 5, 2018 · 6 comments · Fixed by #1074
Milestone

Comments

@postphotos
Copy link
Contributor

As as a WordPress user of a core theme, I expect the AMP to render theme content without the inability to render what comes out of the box.

From @westonruter :

AC1: Add custom sanitizer for hardcoded html, like svg, to ensure core themes render AMP support properly.

Related #1036. From yesterday's backlog groom: twentyseventeen renders SVG that breaks AMP, and the answer isn't "get rid of SVG in the other codebase," it's a question: "How does AMP work with all of these core themes?"

@westonruter westonruter changed the title Add sanitization for hardcoded HTML Add sanitizer for ensuring AMP compatibility for core themes Apr 5, 2018
@westonruter
Copy link
Member

Some aspects for how this could look:

  • Add an admin screen to enable amp theme support, either native or paired, with paired allowing you to select which templates would be served as AMP (e.g. single and page).
  • This admin screen could incorporate results from the validation tool to let the user know how “AMP ready” they are. Validation tool could also be spawned from here, see [Compatibility Tool] Better tooling for evaluating plugins #1007.
  • This screen could also have a way for you you to enable amp theme support just for the current logged-in user so that you can temporarily see how things are working before turning it on for everyone.
  • Aside: The above could also integrate with A/B testing tools to compare how AMP performs vs non-AMP.
  • Once amp theme support is enabled, and a core-bundled theme is active (or is a parent theme of what is active) then a new sanitizer should be added to amp_content_sanitizers called something like AMP_Core_Theme_Sanitizer within which any tweaks needed to ensure the core themes work with AMP would be applied, such as replacing no-svg with svg in Twenty Seventeen.

@kienstra
Copy link
Contributor

kienstra commented Apr 9, 2018

Thanks, Question About Including In Design For #1006

Hi @westonruter,
Those are some great ideas, especially the report for how “AMP ready” a theme is.

Would it help if they considered your points here for the design of issue #1006?

@westonruter
Copy link
Member

Yeah, it's all related 😄 My bullet points here are also closely related to #1003 (comment)

@westonruter
Copy link
Member

I suggest adding the mu-plugin described here for this issue: #1007 (comment)

With that in place, one can then check out each core theme, and in one tab have ?amp_forced in the URL and in another tab have it absent. So then one can go through a theme and easily find the discrepancies. Once the discrepancies are found (e.g. no-svg in Twenty Seventeen) and listed out, we can then work on a core theme sanitizer to fix them up.

@kienstra
Copy link
Contributor

kienstra commented Jun 5, 2018

Test Steps

Hi @csossi,
Could you please test this?

  1. Go to 5-10 posts or pages on https://paired-ampconfdemo.pantheonsite.io/
  2. Also open the equivalent AMP page by appending ?amp or &amp to the URL
  3. Compare the AMP version of them to the non-AMP version
  4. Expected: there's no obvious styling issue, like with the header image or featured image
  5. Expected: commenting works for logged-in and logged-out users (example URL)

@csossi
Copy link

csossi commented Sep 18, 2018

Verified in QA

@csossi csossi removed their assignment Sep 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants