Automattic / wp-calypso

The JavaScript and API powered WordPress.com
https://developer.wordpress.com
GNU General Public License v2.0
12.42k stars 1.99k forks source link

AMP: "Automatically fit text to container" broken on Simple sites #52020

Closed WunderBart closed 3 years ago

WunderBart commented 3 years ago

Steps to reproduce the behavior

  1. On a Simple site, go to the editor
  2. Add a paragraph block and type some text in
  3. Open the block settings and go to the AMP Settings section
  4. Check the Automatically fit text to container option
  5. Save the post and refresh the page
  6. See that the block is now broken and offers recovery.

Does this happen on simple or atomic sites or both?

Simple sites only (Gutenberg v10.3.2 + WP AMP v1.4.2)

Is there any console output or error text?

Content generated by `save` function:

<p value="<amp-fit-text layout=&quot;fixed-height&quot; min-font-size=&quot;6&quot; max-font-size=&quot;72&quot; height=&quot;80&quot;&gt;</amp-fit-text&gt;"></p>

Content retrieved from post body:

&lt;p value=&quot;&lt;amp-fit-text layout=&quot;fixed-height&quot; min-font-size=&quot;6&quot; max-font-size=&quot;72&quot; height=&quot;80&quot;&gt;asdasd</amp-fit-text>asdasd</p>

Demo

https://user-images.githubusercontent.com/1451471/115047662-05f58380-9ed9-11eb-8e63-7ca2604ca084.mp4

WunderBart commented 3 years ago

cc @gravityrail @claudiucelfilip maybe you could help with this one? 🙏

WunderBart commented 3 years ago

For comparison, this is the AMPed block's code on an Atomic site:

<!-- wp:paragraph {"ampFitText":true} -->
<amp-fit-text layout="fixed-height" min-font-size="6" max-font-size="72" height="80"><p>Test</p></amp-fit-text>
<!-- /wp:paragraph -->

...versus the same block on a Simple site:

<!-- wp:paragraph {"ampFitText":true} -->
<p value="<amp-fit-text layout=&quot;fixed-height&quot; min-font-size=&quot;6&quot; max-font-size=&quot;72&quot; height=&quot;80&quot;&gt;Test</amp-fit-text&gt;">Test</p>
<!-- /wp:paragraph -->
gravityrail commented 3 years ago

I don't currently have enough direct expertise with the Gutenberg amp integration to be much help here, hopefully somebody else is able to chip in with an obvious solution.

claudiucelfilip commented 3 years ago

The issue is caused by the value attribute added by WP AMP 1.4.2 when the Automatically fit text to container is enabled. This attribute pass through the KSES sanitizer properly which escapes the whole block. The escaped version of the content is save and won't match what the Gutenberg save function generates.

@WunderBart I opened D60408-code to address this issue. Similar to another situation handled in the plugin's code (regarding the inline style attr), this will bypass some of the sanitizing process and avoid escaping the whole block.

claudiucelfilip commented 3 years ago

D60408-code has been deployed 🎉

This fix only addresses the breaking block code, due to sanitisation. The actual auto-sizing functionality doesn't seem to work though on this version, and it seems to have on non-wpcom sites as well. We'll aim to address this by upgrading to WP AMP 2.0.5, where the functionality seems to work properly there. The aim is to fully rollout this new version next week (if nothing comes up). Until then, the new version can be forced by appending &amp-next=1 to the URL (on paged editor or published post).

claudiucelfilip commented 3 years ago

WP AMP 2.0.5 upgrade for Simple Sites has been deployed 🎉 : D60522-code The auto-sizing functionality should be working now on AMP.

WunderBart commented 3 years ago

WP AMP 2.0.5 upgrade for Simple Sites has been deployed 🎉 : D60522-code The auto-sizing functionality should be working now on AMP.

Tested on my Simple site - looks like it's working now! 🎉 Thanks!

May-06-2021 10-07-10