🚀 🚀 🚀 amp-carousel ssr: allow render without JS 🚀 🚀 🚀 (#37646)

* amp-carousel ssr: allow render without JS * lint. scrollslide working * rcebulko feedback * further fixes * hyphen-case setProperty and remove Set * fix test * add an extra test * moar fix * solve lowercase vendor bug. * even moar test fixes (cherry picked from commit a50f171)
ampproject · Feb 18, 2022 · 3116a3c · 3116a3c
1 parent 4f48d4d
commit 3116a3c
Show file tree

Hide file tree

Showing 11 changed files with 138 additions and 113 deletions.
diff --git a/examples/compiler-hydration.html b/examples/compiler-hydration.html
@@ -5,6 +5,9 @@
   <title>Hydration Test</title>
   <meta name="viewport" content="width=device-width,minimum-scale=1,initial-scale=1">
   <script async custom-element="amp-fit-text" src="https://cdn.ampproject.org/v0/amp-fit-text-0.1.js"></script>
+  <!-- TODO: make lazy server work with .css files. -->
+  <link rel="stylesheet" href="/extensions/amp-carousel/0.1/amp-carousel.css" />
+  <link rel="stylesheet" href="/dist/v0.css" />
   <script async custom-element="amp-carousel" src="https://cdn.ampproject.org/v0/amp-carousel-0.1.js"></script>
   <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>
   <script async src="https://cdn.ampproject.org/v0.js"></script>
@@ -29,12 +32,20 @@
         Unlike most AMPHTML pages, this file's components should all be rendered.
     -->
 
-    <h1><code>amp-layout</code> with a 4:1 responsive ratio </h1>
-    <amp-layout layout="responsive" width="4" height="1" class="i-amphtml-layout-responsive i-amphtml-layout-size-defined" i-amphtml-layout="responsive" i-amphtml-ssr="">
+    <h1><code>client render: amp-layout</code> with a 4:1 responsive ratio </h1>
+    <amp-layout layout="responsive" width="4" height="1">
+        <svg viewBox="0 0 100 25">
+            <circle cx="50%" cy="50%" r="15%" stroke="black" stroke-width="3"></circle>
+            Sorry, your browser does not support inline SVG.
+        </svg>
+    </amp-layout>
+
+    <h1><code>server render: amp-layout</code> with a 4:1 responsive ratio </h1>
+    <amp-layout class="i-amphtml-layout-responsive i-amphtml-layout-size-defined i-amphtml-element" height=1 i-amphtml-layout=responsive i-amphtml-ssr layout=responsive width=4>
         <i-amphtml-sizer slot="i-amphtml-svc" style="padding-top: 25%;"></i-amphtml-sizer>
         <div class="i-amphtml-fill-content">
             <svg viewBox="0 0 100 25">
-                <circle cx="50%" cy="50%" r="15%" stroke="black" stroke-width="3"></circle>
+                <circle cx="50%" cy="50%" r="15%" stroke=black stroke-width=3></circle>
                 Sorry, your browser does not support inline SVG.
             </svg>
         </div>
@@ -58,48 +69,51 @@ <h1><code>amp-fit-text</code> with short text</h1>
 
     <h1><code>client render: carousel-0.1 type=slides</code></h1>
     <amp-carousel type="slides" width="400" height="300" controls loop>
-        <amp-img src="https://picsum.photos/400/300?img=1" width="400" height="300"></amp-img>
-        <amp-img src="https://picsum.photos/400/200?img=2" width="400" height="300"></amp-img>
-        <amp-img src="https://picsum.photos/400/200?img=3" width="400" height="300"></amp-img>
-        <amp-img src="https://picsum.photos/400/200?img=4" width="400" height="300"></amp-img>
-        <amp-img src="https://picsum.photos/400/200?img=5" width="400" height="300"></amp-img>
+        <img src="https://picsum.photos/400/300?img=1" width="400" height="300"></img>
+        <img src="https://picsum.photos/400/200?img=2" width="400" height="300"></img>
+        <img src="https://picsum.photos/400/200?img=3" width="400" height="300"></img>
+        <img src="https://picsum.photos/400/200?img=4" width="400" height="300"></img>
+        <img src="https://picsum.photos/400/200?img=5" width="400" height="300"></img>
     </amp-carousel>
 
     <h1><code>server render: carousel-0.1 type=slides</code></h1>
-    <amp-carousel class="i-amphtml-layout-fixed i-amphtml-layout-size-defined i-amphtml-slidescroll i-amphtml-carousel-has-controls" controls height=300 i-amphtml-layout=fixed i-amphtml-ssr loop style="width: 400px; height: 300px;" type=slides width=400>
-        <div aria-live=polite class="i-amphtml-slides-container i-amphtml-slidescroll-no-snap" tabindex="-1">
-            <div class="i-amphtml-slide-item"><amp-img class="amp-carousel-slide" height=300 src="https://picsum.photos/400/300?img=1" width=400></amp-img></div>
-            <div class="i-amphtml-slide-item"><amp-img class="amp-carousel-slide" height=300 src="https://picsum.photos/400/200?img=2" width=400></amp-img></div>
-            <div class="i-amphtml-slide-item"><amp-img class="amp-carousel-slide" height=300 src="https://picsum.photos/400/200?img=3" width=400></amp-img></div>
-            <div class="i-amphtml-slide-item"><amp-img class="amp-carousel-slide" height=300 src="https://picsum.photos/400/200?img=4" width=400></amp-img></div>
-            <div class="i-amphtml-slide-item"><amp-img class="amp-carousel-slide" height=300 src="https://picsum.photos/400/200?img=5" width=400></amp-img></div>
+    <amp-carousel class="i-amphtml-layout-fixed i-amphtml-layout-size-defined i-amphtml-slidescroll i-amphtml-carousel-has-controls i-amphtml-element" controls height="300" i-amphtml-layout="fixed" i-amphtml-ssr loop style="width: 400px; height: 300px" type="slides" width="400">
+        <div aria-live="polite" class="i-amphtml-slides-container i-amphtml-slidescroll-no-snap" tabindex="-1">
+            <div class="i-amphtml-slide-item i-amphtml-slide-item-show">
+                <img class="amp-carousel-slide" height="300" src="https://picsum.photos/400/300?img=1" width="400" />
+            </div>
+            <div class="i-amphtml-slide-item"><img class="amp-carousel-slide" height="300" src="https://picsum.photos/400/200?img=2" width="400"></div>
+            <div class="i-amphtml-slide-item"><img class="amp-carousel-slide" height="300" src="https://picsum.photos/400/200?img=3" width="400"></div>
+            <div class="i-amphtml-slide-item"><img class="amp-carousel-slide" height="300" src="https://picsum.photos/400/200?img=4" width="400"></div>
+            <div class="i-amphtml-slide-item"><img class="amp-carousel-slide" height="300" src="https://picsum.photos/400/200?img=5" width="400"></div>
         </div>
-        <div aria-disabled=false class="amp-carousel-button amp-carousel-button-prev" role=button tabindex=0 title="Previous item in carousel (5 of 5)"></div>
-        <div aria-disabled=false class="amp-carousel-button amp-carousel-button-next" role=button tabindex=0 title="Next item in carousel (2 of 5)"></div>
+        <div aria-disabled="false" class="amp-carousel-button amp-carousel-button-prev" role="button" tabindex="0" title="Previous item in carousel (5 of 5)"></div>
+        <div aria-disabled="false" class="amp-carousel-button amp-carousel-button-next" role="button" tabindex="0" title="Next item in carousel (2 of 5)"></div>
     </amp-carousel>
 
     <h1><code>client render: carousel-0.1 type=scrollable</code></h1>
     <amp-carousel width="300" height="100">
-        <amp-img src="https://picsum.photos/300/400?img=1" width="120" height="100"></amp-img>
-        <amp-img src="https://picsum.photos/300/400?img=2" width="120" height="100"></amp-img>
-        <amp-img src="https://picsum.photos/300/400?img=3" width="120" height="100"></amp-img>
-        <amp-img src="https://picsum.photos/300/400?img=4" width="120" height="100"></amp-img>
-        <amp-img src="https://picsum.photos/300/400?img=5" width="120" height="100"></amp-img>
-        <amp-img src="https://picsum.photos/300/400?img=6" width="120" height="100"></amp-img>
-        <amp-img src="https://picsum.photos/300/400?img=7" width="120" height="100"></amp-img>
+        <img src="https://picsum.photos/300/400?img=1" width="120" height="100">
+        <img src="https://picsum.photos/300/400?img=2" width="120" height="100">
+        <img src="https://picsum.photos/300/400?img=3" width="120" height="100">
+        <img src="https://picsum.photos/300/400?img=4" width="120" height="100">
+        <img src="https://picsum.photos/300/400?img=5" width="120" height="100">
+        <img src="https://picsum.photos/300/400?img=6" width="120" height="100">
+        <img src="https://picsum.photos/300/400?img=7" width="120" height="100">
     </amp-carousel>
 
     <h1><code>server render: carousel-0.1 type=scrollable</code></h1>
-    <amp-carousel class="i-amphtml-layout-fixed i-amphtml-layout-size-defined" height=100 i-amphtml-layout=fixed i-amphtml-ssr style="width: 300px; height: 100px;" width=300>
+    <amp-carousel class="i-amphtml-layout-fixed i-amphtml-layout-size-defined i-amphtml-element" height="100" i-amphtml-layout="fixed" i-amphtml-ssr style="width: 300px; height: 100px" width="300">
         <div class="i-amphtml-scrollable-carousel-container" tabindex="-1">
-            <amp-img class="amp-carousel-slide amp-scrollable-carousel-slide" height=100 src="https://picsum.photos/300/400?img=1" width=120></amp-img>
-            <amp-img class="amp-carousel-slide amp-scrollable-carousel-slide" height=100 src="https://picsum.photos/300/400?img=2" width=120></amp-img>
-            <amp-img class="amp-carousel-slide amp-scrollable-carousel-slide" height=100 src="https://picsum.photos/300/400?img=3" width=120></amp-img>
-            <amp-img class="amp-carousel-slide amp-scrollable-carousel-slide" height=100 src="https://picsum.photos/300/400?img=4" width=120></amp-img>
-            <amp-img class="amp-carousel-slide amp-scrollable-carousel-slide" height=100 src="https://picsum.photos/300/400?img=5" width=120></amp-img>
-            <amp-img class="amp-carousel-slide amp-scrollable-carousel-slide" height=100 src="https://picsum.photos/300/400?img=6" width=120></amp-img><amp-img class="amp-carousel-slide amp-scrollable-carousel-slide" height=100 src="https://picsum.photos/300/400?img=7" width=120></amp-img>
+          <img class="amp-carousel-slide amp-scrollable-carousel-slide" height="100" src="https://picsum.photos/300/400?img=1" width="120" />
+          <img class="amp-carousel-slide amp-scrollable-carousel-slide" height="100" src="https://picsum.photos/300/400?img=2" width="120" />
+          <img class="amp-carousel-slide amp-scrollable-carousel-slide" height="100" src="https://picsum.photos/300/400?img=3" width="120" />
+          <img class="amp-carousel-slide amp-scrollable-carousel-slide" height="100" src="https://picsum.photos/300/400?img=4" width="120" />
+          <img class="amp-carousel-slide amp-scrollable-carousel-slide" height="100" src="https://picsum.photos/300/400?img=5" width="120" />
+          <img class="amp-carousel-slide amp-scrollable-carousel-slide" height="100" src="https://picsum.photos/300/400?img=6" width="120" />
+          <img class="amp-carousel-slide amp-scrollable-carousel-slide" height="100" src="https://picsum.photos/300/400?img=7" width="120" />
         </div>
-        <div aria-disabled=true class="amp-carousel-button amp-carousel-button-prev amp-disabled" role=presentation tabindex="-1" title="Previous item in carousel"></div>
-        <div aria-disabled=false class="amp-carousel-button amp-carousel-button-next" role=presentation tabindex=0 title="Next item in carousel"></div>
+        <div aria-disabled="true" class="amp-carousel-button amp-carousel-button-prev amp-disabled" role="presentation" tabindex="-1" title="Previous item in carousel"></div>
+        <div aria-disabled="false" class="amp-carousel-button amp-carousel-button-next" role="presentation" tabindex="0" title="Next item in carousel"></div>
     </amp-carousel>
 </html>
diff --git a/extensions/amp-ad-network-adsense-impl/0.1/test/test-amp-ad-network-adsense-impl.js b/extensions/amp-ad-network-adsense-impl/0.1/test/test-amp-ad-network-adsense-impl.js
@@ -8,6 +8,7 @@ import {
   CONSENT_STRING_TYPE,
 } from '#core/constants/consent-state';
 import {addAttributesToElement, createElementWithAttributes} from '#core/dom';
+import {camelCaseToHyphenCase} from '#core/dom/style';
 import {utf8Decode, utf8Encode} from '#core/types/string/bytes';
 import {toWin} from '#core/window';
 
@@ -1536,7 +1537,12 @@ describes.realWin(
         };
         impl.iframe = {
           contentWindow: window,
-          style: {'visibility': 'hidden'},
+          style: {
+            'visibility': 'hidden',
+            setProperty: (name, value) => {
+              impl.iframe.style[camelCaseToHyphenCase(name)] = value;
+            },
+          },
         };
         win.postMessage('fill_sticky', '*');
         return renderPromise.then(() => {

diff --git a/extensions/amp-carousel/0.1/build-dom.js b/extensions/amp-carousel/0.1/build-dom.js
@@ -16,12 +16,14 @@ export const ClassNames = {
 
   // Generic
   SLIDE: 'amp-carousel-slide',
+  SHOW_SLIDE: 'amp-carousel-slide',
 
   // SlideScroll Carousel
   SLIDESCROLL_CAROUSEL: 'i-amphtml-slidescroll',
   SLIDE_WRAPPER: 'i-amphtml-slide-item',
   SLIDES_CONTAINER: 'i-amphtml-slides-container',
   SLIDES_CONTAINER_NOSNAP: 'i-amphtml-slidescroll-no-snap',
+  SLIDES_ITEM_SHOW: 'i-amphtml-slide-item-show',
 
   // Scrollable Carousel
   SCROLLABLE_CONTAINER: 'i-amphtml-scrollable-carousel-container',
@@ -229,6 +231,8 @@ function buildSlideScrollCarousel(element) {
     slidesContainer.appendChild(slideWrapper);
     slideWrappers.push(slideWrapper);
   });
+  // Initialize the first slide to be shown.
+  slideWrappers[0]?.classList.add(ClassNames.SLIDES_ITEM_SHOW);
 
   return {slidesContainer, slides, slideWrappers};
 }

diff --git a/extensions/amp-carousel/0.1/slidescroll.js b/extensions/amp-carousel/0.1/slidescroll.js
@@ -31,7 +31,6 @@ import {
 import {CarouselControls} from './carousel-controls';
 
 /** @const {string} */
-const SHOWN_CSS_CLASS = 'i-amphtml-slide-item-show';
 
 /** @const {number} */
 const NATIVE_SNAP_TIMEOUT = 200;
@@ -842,7 +841,7 @@ export class AmpSlideScroll extends AMP.BaseElement {
       if (this.shouldLoop_) {
         setStyle(this.slideWrappers_[showIndex], 'order', loopIndex + 1);
       }
-      this.slideWrappers_[showIndex].classList.add(SHOWN_CSS_CLASS);
+      this.slideWrappers_[showIndex].classList.add(ClassNames.SLIDES_ITEM_SHOW);
       const owners = Services.ownersForDoc(this.element);
       if (showIndex == newIndex) {
         owners.scheduleLayout(this.element, this.slides_[showIndex]);
@@ -929,17 +928,17 @@ export class AmpSlideScroll extends AMP.BaseElement {
   hideRestOfTheSlides_(indexArr) {
     const {noOfSlides_} = this;
     for (let i = 0; i < noOfSlides_; i++) {
-      if (!this.slideWrappers_[i].classList.contains(SHOWN_CSS_CLASS)) {
+      if (
+        !this.slideWrappers_[i].classList.contains(ClassNames.SLIDES_ITEM_SHOW)
+      ) {
         continue;
       }
       // Hide if not shown anymore
       if (!indexArr.includes(i)) {
         if (this.shouldLoop_) {
           setStyle(this.slideWrappers_[i], 'order', '');
         }
-        dev()
-          .assertElement(this.slideWrappers_[i])
-          .classList.remove(SHOWN_CSS_CLASS);
+        this.slideWrappers_[i].classList.remove(ClassNames.SLIDES_ITEM_SHOW);
         this.slides_[i].removeAttribute('aria-hidden');
       }
       // Pause if not the current slide

diff --git a/extensions/amp-fit-text/0.1/test/test-amp-fit-text.js b/extensions/amp-fit-text/0.1/test/test-amp-fit-text.js
@@ -2,7 +2,10 @@ import {createDocument as createWorkerDomDoc} from '@ampproject/worker-dom/dist/
 
 import {createElementWithAttributes} from '#core/dom';
 
-import {getDeterministicOuterHTML} from '#testing/helpers';
+import {
+  getDeterministicOuterHTML,
+  hypenCaseToCamelCase,
+} from '#testing/helpers';
 
 import {AmpFitText, calculateFontSize_, updateOverflow_} from '../amp-fit-text';
 import {buildDom} from '../build-dom';
@@ -244,7 +247,11 @@ describes.realWin('amp-fit-text updateOverflow', {}, (env) => {
     doc = win.document;
     classToggles = {};
     content = {
-      style: {},
+      style: {
+        setProperty(name, value) {
+          content.style[hypenCaseToCamelCase(name)] = value;
+        },
+      },
       classList: {
         toggle: (className, on) => {
           classToggles[className] = on;

diff --git a/src/compiler/builders.ts b/src/compiler/builders.ts
@@ -26,6 +26,7 @@ function wrap(buildDom: BuildDom): BuildDom {
     applyStaticLayout(element as AmpElement);
     buildDom(element);
     element.setAttribute('i-amphtml-ssr', '');
+    element.classList.add('i-amphtml-element');
   };
 }
 

diff --git a/src/core/dom/style.js b/src/core/dom/style.js
@@ -27,6 +27,23 @@ export function camelCaseToTitleCase(camelCase) {
   return camelCase.charAt(0).toUpperCase() + camelCase.slice(1);
 }
 
+/**
+ * @param {string} camelCase camel cased string
+ * @return {string} hyphen-cased string
+ */
+export function camelCaseToHyphenCase(camelCase) {
+  const hyphenated = camelCase.replace(
+    /[A-Z]/g,
+    (match) => '-' + match.toLowerCase()
+  );
+
+  // For o-foo or ms-foo, we need to convert to -o-foo and ms-foo
+  if (vendorPrefixes.some((prefix) => hyphenated.startsWith(prefix + '-'))) {
+    return `-${hyphenated}`;
+  }
+  return hyphenated;
+}
+
 /**
   Checks the style if a prefixed version of a property exists and returns
  * it or returns an empty string.
@@ -45,15 +62,6 @@ function getVendorJsPropertyName_(style, titleCase) {
   return '';
 }
 
-/**
- * A small set of curated properties that are valid with setProperty.
- * Within worker-dom, only `.setProperty` properly reflects into the attribute.
- * If we solve that, or decide to always use setProperty, this kludge can be removed.
- *
- * TODO(https://github.com/ampproject/worker-dom/issues/1134)
- */
-const SHOULD_USE_SET_PROPERTY = new Set(['width', 'height', 'order', 'hidden']);
-
 /**
  * Returns the possibly prefixed JavaScript property name of a style property
  * (ex. WebkitTransitionDuration) given a camelCase'd version of the property
@@ -101,7 +109,7 @@ export function setImportantStyles(element, styles) {
   const {style} = element;
   for (const k in styles) {
     style.setProperty(
-      getVendorJsPropertyName(style, k),
+      camelCaseToHyphenCase(getVendorJsPropertyName(style, k)),
       String(styles[k]),
       'important'
     );
@@ -126,11 +134,7 @@ export function setStyle(element, property, value, opt_units, opt_bypassCache) {
     return;
   }
   const styleValue = opt_units ? value + opt_units : value;
-  if (isVar(propertyName) || SHOULD_USE_SET_PROPERTY.has(propertyName)) {
-    element.style.setProperty(propertyName, styleValue);
-  } else {
-    /** @type {*} */ (element.style)[propertyName] = styleValue;
-  }
+  element.style.setProperty(camelCaseToHyphenCase(propertyName), styleValue);
 }
 
 /**

diff --git a/test/unit/core/dom/test-style.js b/test/unit/core/dom/test-style.js
@@ -24,9 +24,17 @@ describes.sandboxed('DOM - style helpers', {}, (env) => {
   });
 
   it('setStyle with vendor prefix', () => {
-    const element = {style: {WebkitTransitionDuration: ''}};
+    const element = {
+      style: {
+        WebkitTransitionDuration: '',
+        setProperty: (name, value) => {
+          element.style[name] = value;
+        },
+      },
+    };
+
     st.setStyle(element, 'transitionDuration', '1s', undefined, true);
-    expect(element.style.WebkitTransitionDuration).to.equal('1s');
+    expect(element.style['-webkit-transition-duration']).to.equal('1s');
   });
 
   it('setStyle with custom var', () => {
@@ -67,7 +75,7 @@ describes.sandboxed('DOM - style helpers', {}, (env) => {
       transitionDurationImportant: '1s',
     });
     expect(spy).to.have.been.calledWith(
-      'WebkitTransitionDurationImportant',
+      '-webkit-transition-duration-important',
       '1s',
       'important'
     );
@@ -95,6 +103,17 @@ describes.sandboxed('DOM - style helpers', {}, (env) => {
     expect(st.camelCaseToTitleCase(str)).to.equal('TheQuickBrownFox');
   });
 
+  it('camelCaseToHyphenCase', () => {
+    expect(st.camelCaseToHyphenCase('paddingTop')).to.equal('padding-top');
+    expect(st.camelCaseToHyphenCase('WebkitTransition')).to.equal(
+      '-webkit-transition'
+    );
+    expect(st.camelCaseToHyphenCase('webkitTransition')).to.equal(
+      '-webkit-transition'
+    );
+    expect(st.camelCaseToHyphenCase('oTransition')).to.equal('-o-transition');
+  });
+
   it('removeAlphaFromColor', () => {
     expect(st.removeAlphaFromColor('rgba(1, 1, 1, 0)')).to.equal(
       'rgba(1, 1, 1, 1)'