🐛[amp-tiktok] Fix CLS bug. (ampproject#35850)

* [`amp-tiktok`] Fix CLS issue caused by promise failing to resolve. * [`amp-tiktok`] Add Tests for CLS fix. * [`amp-tiktok`] Update unit tests to cover cls fix case. * [`amp-tiktok`] Remove trailing whitespaces.
BroadBandyDev · Sep 9, 2021 · dd34d57 · dd34d57
1 parent f10b622
commit dd34d57
Show file tree

Hide file tree

Showing 4 changed files with 92 additions and 22 deletions.
diff --git a/examples/amp-tiktok-cls-example.html b/examples/amp-tiktok-cls-example.html
@@ -0,0 +1,47 @@
+<!DOCTYPE html>
+<html ⚡ lang="en">
+  <head>
+    <meta charset="utf-8" />
+    <title>amp-tiktok example</title>
+    <link rel="canonical" href="amps.html" />
+    <meta
+      name="viewport"
+      content="width=device-width,minimum-scale=1,initial-scale=1"
+    />
+    <!-- prettier-ignore -->
+    <style amp-boilerplate>body{-webkit-animation:-amp-start 8s steps(1,end) 0s 1 normal both;-moz-animation:-amp-start 8s steps(1,end) 0s 1 normal both;-ms-animation:-amp-start 8s steps(1,end) 0s 1 normal both;animation:-amp-start 8s steps(1,end) 0s 1 normal both}@-webkit-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-moz-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-ms-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-o-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}</style><noscript><style amp-boilerplate>body{-webkit-animation:none;-moz-animation:none;-ms-animation:none;animation:none}</style></noscript>
+    <style amp-custom>
+      amp-tiktok {
+        color: red;
+        border-style: solid;
+      }
+    </style>
+    <script
+      async
+      custom-element="amp-tiktok"
+      src="https://cdn.ampproject.org/v0/amp-tiktok-0.1.js"
+    ></script>
+    <script async src="https://cdn.ampproject.org/v0.js"></script>
+  </head>
+  <body>
+    <h2>Should not show CLS because `height` attr is equal to the final height of the iframe</h2>
+    <amp-tiktok height="721" layout="fixed-height" data-src="https://www.tiktok.com/@countingprimes/video/6988237085899574533"></amp-tiktok>
+    </break>
+
+    <h2>Does show cls because the initial height is larger than the final size of the iframe</h2>
+    <amp-tiktok
+      width="700"
+      height="800"
+      data-src="https://www.tiktok.com/@scout2015/video/6943753342808034566"
+    ></amp-tiktok>
+    <p>
+      Lorem ipsum dolor sit amet, consectetur adipiscing elit, sed do eiusmod
+      tempor incididunt ut labore et dolore magna aliqua. Ut enim ad minim
+      veniam, quis nostrud exercitation ullamco laboris nisi ut aliquip ex ea
+      commodo consequat. Duis aute irure dolor in reprehenderit in voluptate
+      velit esse cillum dolore eu fugiat nulla pariatur. Excepteur sint occaecat
+      cupidatat non proident, sunt in culpa qui officia deserunt mollit anim id
+      est laborum.
+    </p>
+  </body>
+</html>
diff --git a/extensions/amp-tiktok/0.1/amp-tiktok.css b/extensions/amp-tiktok/0.1/amp-tiktok.css
@@ -1,28 +1,29 @@
-
-
 .i-amphtml-tiktok-centered {
-    height: 100%;
-    left: 50%;
-    width: 325px;
-    transform: translateX(-50%);
-    position: absolute;
+  height: 100%;
+  left: 50%;
+  width: 325px;
+  transform: translateX(-50%);
+  position: absolute;
 }
 
 .i-amphtml-tiktok-unresolved {
-    position: fixed;
-    opacity: 0;
-    pointer-events: none;
-    width: 325px;
-    height: 500px;
+  position: fixed;
+  opacity: 0;
+  pointer-events: none;
+  width: 325px;
+  /* This size is ok because the iframe is temporarily `position: fixed;`
+    1000px was chosen because TikTok requires a large height in order to send
+    the height messages to the iframe*/
+  height: 1000px;
 }
 
 .i-amphtml-tiktok-placeholder-image {
-    height: 578px;
-    top: 1px;
-    border-radius: 8px 8px 0px 0px;
+  height: 578px;
+  top: 1px;
+  border-radius: 8px 8px 0px 0px;
 }
 
-.i-amphtml-tiktok-placeholder-image-container{
-    height: 100%;
-    background: rgba(220, 220, 220, 0.6);
+.i-amphtml-tiktok-placeholder-image-container {
+  height: 100%;
+  background: rgba(220, 220, 220, 0.6);
 }
diff --git a/extensions/amp-tiktok/0.1/amp-tiktok.js b/extensions/amp-tiktok/0.1/amp-tiktok.js
@@ -197,6 +197,9 @@ export class AmpTiktok extends AMP.BaseElement {
       this.handleTiktokMessages_.bind(this)
     );
 
+    const {promise, resolve} = new Deferred();
+    this.resolveReceivedFirstMessage_ = resolve;
+
     Promise.resolve(this.oEmbedResponsePromise_).then((data) => {
       if (data?.['title']) {
         iframe.title = `TikTok: ${data['title']}`;
@@ -205,13 +208,10 @@ export class AmpTiktok extends AMP.BaseElement {
 
     this.element.appendChild(iframe);
     return this.loadPromise(iframe).then(() => {
-      const {promise, resolve} = new Deferred();
-
-      this.resolveRecievedFirstMessage_ = resolve;
       Services.timerFor(this.win)
         .timeoutPromise(1000, promise)
         .catch(() => {
-          // If no resize messages are recieved the fallback is to
+          // If no resize messages are received the fallback is to
           // resize to the fallbackHeight value.
           this.resizeOuter_(this.fallbackHeight_);
           setStyles(this.iframe_, {

diff --git a/extensions/amp-tiktok/0.1/test/test-amp-tiktok.js b/extensions/amp-tiktok/0.1/test/test-amp-tiktok.js
@@ -101,6 +101,28 @@ describes.realWin(
       expect(computedStyle(win, playerIframe).height).to.equal('775.25px');
     });
 
+    it('should resolve messages received before load', async () => {
+      const tiktok = await getTiktokBuildOnly({height: '600'});
+      const impl = await tiktok.getImpl();
+
+      // ensure that loadPromise is never resolved
+      env.sandbox.stub(impl, 'loadPromise').returns(new Promise(() => {}));
+      impl.layoutCallback();
+
+      // ensure that resolver is set after layoutCallback
+      expect(impl.resolveReceivedFirstMessage_).to.be.ok;
+      const firstMessageStub = env.sandbox.stub(
+        impl,
+        'resolveReceivedFirstMessage_'
+      );
+      impl.handleTiktokMessages_({
+        origin: 'https://www.tiktok.com',
+        source: tiktok.querySelector('iframe').contentWindow,
+        data: JSON.stringify({height: 555}),
+      });
+      expect(firstMessageStub).to.be.calledOnce;
+    });
+
     it('renders placeholder', async () => {
       const videoSrc =
         'https://www.tiktok.com/@scout2015/video/6948210747285441798';