🐛 compiler.js buildDoms: ensure behavior matches browser (#37671)

* compiler.js buildDoms: ensure behavior matches browser * document the hack * fix test, also update compiler-hydration with latest. * fix more broken unit tests * pr feedback updates
ampproject · Feb 15, 2022 · b875822 · b875822
1 parent 4bb5570
commit b875822
Show file tree

Hide file tree

Showing 11 changed files with 228 additions and 30 deletions.
diff --git a/examples/compiler-hydration.html b/examples/compiler-hydration.html
@@ -66,16 +66,16 @@ <h1><code>client render: carousel-0.1 type=slides</code></h1>
     </amp-carousel>
 
     <h1><code>server render: carousel-0.1 type=slides</code></h1>
-    <amp-carousel type="slides" width="400" height="300" controls="" loop="" i-amphtml-ssr style="position: relative;" class="i-amphtml-slidescroll i-amphtml-carousel-has-controls">
-        <div tabindex="-1" class="i-amphtml-slides-container i-amphtml-slidescroll-no-snap" aria-live="polite">
-            <div class="i-amphtml-slide-item"><amp-img src="https://picsum.photos/400/300?img=1" width="400" height="300" data-slide-id="slide-id" style="display: inline;" class="amp-carousel-slide"></amp-img></div>
-            <div class="i-amphtml-slide-item"><amp-img src="https://picsum.photos/400/300?img=2" width="400" height="300" style="display: inline;" class="amp-carousel-slide"></amp-img></div>
-            <div class="i-amphtml-slide-item"><amp-img src="https://picsum.photos/400/300?img=3" width="400" height="300" style="display: inline;" class="amp-carousel-slide"></amp-img></div>
-            <div class="i-amphtml-slide-item"><amp-img src="https://picsum.photos/400/300?img=4" width="400" height="300" style="display: inline;" class="amp-carousel-slide"></amp-img></div>
-            <div class="i-amphtml-slide-item"><amp-img src="https://picsum.photos/400/300?img=5" width="400" height="300" style="display: inline;" class="amp-carousel-slide"></amp-img></div>
+    <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>
         </div>
-        <div tabindex="0" class="amp-carousel-button amp-carousel-button-prev" role="button" title="Previous item in carousel (5 of 5)" aria-disabled="false"></div>
-        <div tabindex="0" class="amp-carousel-button amp-carousel-button-next" role="button" title="Next item in carousel (2 of 5)" aria-disabled="false"></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>
@@ -90,17 +90,16 @@ <h1><code>client render: carousel-0.1 type=scrollable</code></h1>
     </amp-carousel>
 
     <h1><code>server render: carousel-0.1 type=scrollable</code></h1>
-    <amp-carousel width="300" height="100" i-amphtml-ssr>
+    <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>
         <div class="i-amphtml-scrollable-carousel-container" tabindex="-1">
-            <amp-img src="https://picsum.photos/300/400?img=1" width="120" height="100" id="img-0" style="width: 120px; height: 100px;" class="amp-carousel-slide amp-scrollable-carousel-slide"></amp-img>
-            <amp-img src="https://picsum.photos/300/400?img=2" width="120" height="100" id="img-1" style="width: 120px; height: 100px;" class="amp-carousel-slide amp-scrollable-carousel-slide"></amp-img>
-            <amp-img src="https://picsum.photos/300/400?img=3" width="120" height="100" id="img-2" style="width: 120px; height: 100px;" class="amp-carousel-slide amp-scrollable-carousel-slide"></amp-img>
-            <amp-img src="https://picsum.photos/300/400?img=4" width="120" height="100" id="img-3" style="width: 120px; height: 100px;" class="amp-carousel-slide amp-scrollable-carousel-slide"></amp-img>
-            <amp-img src="https://picsum.photos/300/400?img=5" width="120" height="100" id="img-4" style="width: 120px; height: 100px;" class="amp-carousel-slide amp-scrollable-carousel-slide"></amp-img>
-            <amp-img src="https://picsum.photos/300/400?img=6" width="120" height="100" id="img-5" style="width: 120px; height: 100px;" class="amp-carousel-slide amp-scrollable-carousel-slide"></amp-img>
-            <amp-img src="https://picsum.photos/300/400?img=7" width="120" height="100" id="img-6" style="width: 120px; height: 100px;" class="amp-carousel-slide amp-scrollable-carousel-slide"></amp-img>
+            <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>
         </div>
-        <div tabindex="-1" class="amp-carousel-button amp-carousel-button-prev amp-disabled" role="presentation" title="Previous item in carousel" aria-disabled="true"></div>
-        <div tabindex="0" class="amp-carousel-button amp-carousel-button-next" role="presentation" title="Next item in carousel" aria-disabled="false"></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
@@ -458,7 +458,7 @@ describes.realWin(
         impl.iframe = {
           contentWindow: window,
           nodeType: 1,
-          style: {},
+          style: {setProperty: () => {}},
         };
         impl.element.setAttribute('data-ad-client', 'ca-adsense');
 

diff --git a/extensions/amp-carousel/0.1/test/test-scrollable-carousel.js b/extensions/amp-carousel/0.1/test/test-scrollable-carousel.js
@@ -1,4 +1,6 @@
 import '../amp-carousel';
+import {createDocument as createWorkerDomDoc} from '@ampproject/worker-dom/dist/server-lib.mjs';
+
 import {ActionTrust_Enum} from '#core/constants/action-constants';
 import {createElementWithAttributes} from '#core/dom';
 import {whenUpgradedToCustomElement} from '#core/dom/amp-element-helpers';
@@ -8,6 +10,8 @@ import {ActionService} from '#service/action-impl';
 
 import {user} from '#utils/log';
 
+import {getDeterministicOuterHTML} from '#testing/helpers';
+
 import {buildDom} from '../build-dom';
 import {AmpScrollableCarousel} from '../scrollable-carousel';
 
@@ -39,7 +43,7 @@ describes.realWin(
       schedulePreloadSpy = env.sandbox.spy(owners, 'schedulePreload');
     });
 
-    function getAmpScrollableCarousel(addToDom = true) {
+    function getAmpScrollableCarousel(addToDom = true, doc = env.win.document) {
       const imgUrl =
         'https://lh3.googleusercontent.com/5rcQ32ml8E5ONp9f9-' +
         'Rf78IofLb9QjS5_0mqsY1zEFc=w300-h200-no';
@@ -50,13 +54,13 @@ describes.realWin(
 
       const slideCount = 7;
       for (let i = 0; i < slideCount; i++) {
-        const img = document.createElement('amp-img');
+        const img = doc.createElement('amp-img');
         img.setAttribute('src', imgUrl);
         img.setAttribute('width', '120');
         img.setAttribute('height', '100');
-        img.style.width = '120px';
-        img.style.height = '100px';
-        img.id = 'img-' + i;
+        img.style.setProperty('width', '120px');
+        img.style.setProperty('height', '100px');
+        img.setAttribute('id', `img-${i}`);
         carouselElement.appendChild(img);
       }
 
@@ -251,6 +255,24 @@ describes.realWin(
         expect(el2.outerHTML).equal(el1.outerHTML);
       });
 
+      it('buildDom should behave same in browser and in WorkerDOM', async () => {
+        const browserCarousel = await getAmpScrollableCarousel(
+          /* addToDom */ false,
+          env.win.doc
+        );
+        const workerCarousel = await getAmpScrollableCarousel(
+          /* addToDom */ false,
+          createWorkerDomDoc()
+        );
+
+        buildDom(browserCarousel);
+        buildDom(workerCarousel);
+
+        const browserHtml = getDeterministicOuterHTML(browserCarousel);
+        const workerDomHtml = getDeterministicOuterHTML(workerCarousel);
+        expect(workerDomHtml).equal(browserHtml);
+      });
+
       it('buildCallback should assign ivars even when server rendered', async () => {
         const el1 = await getAmpScrollableCarousel(/* addToDom */ false);
         buildDom(el1);

diff --git a/extensions/amp-carousel/0.1/test/test-slidescroll.js b/extensions/amp-carousel/0.1/test/test-slidescroll.js
@@ -1,4 +1,6 @@
 import '../amp-carousel';
+import {createDocument as createWorkerDomDoc} from '@ampproject/worker-dom/dist/server-lib.mjs';
+
 import {ActionTrust_Enum} from '#core/constants/action-constants';
 import {createElementWithAttributes} from '#core/dom';
 import {whenUpgradedToCustomElement} from '#core/dom/amp-element-helpers';
@@ -8,6 +10,7 @@ import {ActionService} from '#service/action-impl';
 
 import {user} from '#utils/log';
 
+import {getDeterministicOuterHTML} from '#testing/helpers';
 import {installResizeObserverStub} from '#testing/resize-observer-stub';
 
 import {buildDom} from '../build-dom';
@@ -39,14 +42,16 @@ describes.realWin(
      * @param {boolean=} opt_attachToDom
      * @param {boolean=} opt_hasAutoplay
      * @param {boolean=} opt_autoplayLoops
+     * @param {Document=} opt_doc
      * @return {Element}
      */
     function getAmpSlideScroll(
       opt_hasLooping,
       opt_slideCount = 5,
       opt_attachToDom = true,
       opt_hasAutoplay = false,
-      opt_autoplayLoops
+      opt_autoplayLoops,
+      doc = env.win.document
     ) {
       const imgUrl =
         'https://lh3.googleusercontent.com/5rcQ32ml8E5ONp9f9-' +
@@ -55,7 +60,7 @@ describes.realWin(
       ampSlideScroll.setAttribute('type', 'slides');
       ampSlideScroll.setAttribute('width', '400');
       ampSlideScroll.setAttribute('height', '300');
-      ampSlideScroll.style.position = 'relative';
+      ampSlideScroll.style.setProperty('position', 'relative');
       ampSlideScroll.setAttribute('controls', '');
       if (opt_hasLooping) {
         ampSlideScroll.setAttribute('loop', '');
@@ -74,7 +79,7 @@ describes.realWin(
         img.setAttribute('width', '400');
         img.setAttribute('height', '300');
         // See https://github.com/ampproject/amphtml/issues/3989
-        img.style.display = 'inline';
+        img.style.setProperty('display', 'inline');
         if (i == 0) {
           img.setAttribute('data-slide-id', 'slide-id');
         }
@@ -1489,6 +1494,28 @@ describes.realWin(
         expect(el2.outerHTML).equal(el1.outerHTML);
       });
 
+      it('buildDom should behave same in browser and in WorkerDOM', async () => {
+        const browserCarousel = await getAmpSlideScroll(
+          /* hasLooping */ true,
+          /* slideCount */ undefined,
+          /* attachToDom */ false
+        );
+        const workerCarousel = await getAmpSlideScroll(
+          /* hasLooping */ true,
+          /* slideCount */ undefined,
+          /* attachToDom */ false,
+          /* opt_hasAutoPlay */ undefined,
+          /* opt_autoplayLoops */ undefined,
+          createWorkerDomDoc()
+        );
+        buildDom(browserCarousel);
+        buildDom(workerCarousel);
+
+        const browserHtml = getDeterministicOuterHTML(browserCarousel);
+        const workerDomHtml = getDeterministicOuterHTML(workerCarousel);
+        expect(workerDomHtml).equal(browserHtml);
+      });
+
       it('buildCallback should assign ivars even when server rendered', async () => {
         const el1 = await getAmpSlideScroll(
           /* hasLooping */ true,

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
@@ -1,5 +1,9 @@
+import {createDocument as createWorkerDomDoc} from '@ampproject/worker-dom/dist/server-lib.mjs';
+
 import {createElementWithAttributes} from '#core/dom';
 
+import {getDeterministicOuterHTML} from '#testing/helpers';
+
 import {AmpFitText, calculateFontSize_, updateOverflow_} from '../amp-fit-text';
 import {buildDom} from '../build-dom';
 
@@ -53,6 +57,28 @@ describes.realWin(
       expect(fitText1.outerHTML).to.equal(fitText2.outerHTML);
     });
 
+    it('buildDom should behave same in browser and in WorkerDOM', async () => {
+      const browserFitText = createElementWithAttributes(doc, 'amp-fit-text', {
+        width: '111px',
+        height: '222px',
+      });
+      const workerFitText = createElementWithAttributes(
+        createWorkerDomDoc(),
+        'amp-fit-text',
+        {
+          width: '111px',
+          height: '222px',
+        }
+      );
+
+      buildDom(browserFitText);
+      buildDom(workerFitText);
+
+      const browserHtml = getDeterministicOuterHTML(browserFitText);
+      const workerHtml = getDeterministicOuterHTML(workerFitText);
+      expect(workerHtml).to.equal(browserHtml);
+    });
+
     it('buildCallback should assign ivars even when server rendered', async () => {
       const fitText = createElementWithAttributes(doc, 'amp-fit-text', {
         width: '111px',

diff --git a/src/core/dom/style.js b/src/core/dom/style.js
@@ -45,6 +45,15 @@ 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
@@ -60,6 +69,7 @@ export function getVendorJsPropertyName(style, camelCase, opt_bypassCache) {
     // CSS vars are returned as is.
     return camelCase;
   }
+
   if (!propertyNameCache) {
     propertyNameCache = map();
   }
@@ -116,7 +126,7 @@ export function setStyle(element, property, value, opt_units, opt_bypassCache) {
     return;
   }
   const styleValue = opt_units ? value + opt_units : value;
-  if (isVar(propertyName)) {
+  if (isVar(propertyName) || SHOULD_USE_SET_PROPERTY.has(propertyName)) {
     element.style.setProperty(propertyName, styleValue);
   } else {
     /** @type {*} */ (element.style)[propertyName] = styleValue;

diff --git a/test/integration/test-video-manager.js b/test/integration/test-video-manager.js
@@ -356,6 +356,9 @@ describes.sandboxed
           width: null,
           height: null,
           opacity: null,
+          setProperty(name, value) {
+            video.style[name] = value;
+          },
         },
         muted: null,
         playsinline: null,

diff --git a/test/unit/builtins/test-amp-layout.js b/test/unit/builtins/test-amp-layout.js
@@ -1,9 +1,13 @@
+import {createDocument as createWorkerDomDoc} from '@ampproject/worker-dom/dist/server-lib.mjs';
+
 import {AmpLayout} from '#builtins/amp-layout/amp-layout';
 import {buildDom} from '#builtins/amp-layout/build-dom';
 
 import {createElementWithAttributes} from '#core/dom';
 import {Layout_Enum} from '#core/dom/layout';
 
+import {getDeterministicOuterHTML} from '#testing/helpers';
+
 describes.realWin('amp-layout', {amp: true}, (env) => {
   async function getAmpLayout(attrs, innerHTML) {
     const {win} = env;
@@ -60,6 +64,32 @@ describes.realWin('amp-layout', {amp: true}, (env) => {
     expect(layout1.outerHTML).to.equal(layout2.outerHTML);
   });
 
+  it('buildDom should result in the same outerHTML in WorkerDOM and Browser', () => {
+    const browserAmpLayout = createElementWithAttributes(
+      env.win.document,
+      'amp-layout',
+      {
+        width: 100,
+        height: 100,
+      }
+    );
+    const workerDomAmpLayout = createElementWithAttributes(
+      createWorkerDomDoc(),
+      'amp-layout',
+      {
+        width: 100,
+        height: 100,
+      }
+    );
+
+    buildDom(browserAmpLayout);
+    buildDom(workerDomAmpLayout);
+
+    const browserHtml = getDeterministicOuterHTML(browserAmpLayout);
+    const workerDomHtml = getDeterministicOuterHTML(workerDomAmpLayout);
+    expect(browserHtml).to.equal(workerDomHtml);
+  });
+
   it('buildDom does not modify server rendered elements', () => {
     const layout = createElementWithAttributes(env.win.document, 'amp-layout');
     buildDom(layout);

diff --git a/test/unit/compiler/test-builders.js b/test/unit/compiler/test-builders.js
@@ -1,7 +1,11 @@
+import {createDocument as createWorkerDomDoc} from '@ampproject/worker-dom/dist/server-lib.mjs';
+
 import {getBuilders} from '#compiler/builders';
 
 import {createElementWithAttributes} from '#core/dom';
 
+import {getDeterministicOuterHTML} from '#testing/helpers';
+
 describes.fakeWin('getBuilders', {}, (env) => {
   let doc;
   beforeEach(() => (doc = env.win.document));
@@ -52,5 +56,27 @@ describes.fakeWin('getBuilders', {}, (env) => {
       builders.noop(elem);
       expect(elem.getAttribute('i-amphtml-layout')).equal('fixed');
     });
+
+    it('wrapper should behave same in browser as in WorkerDOM', () => {
+      const browserDiv = createElementWithAttributes(doc, 'div', {
+        height: 100,
+        width: 100,
+      });
+      const workerDomDiv = createElementWithAttributes(
+        createWorkerDomDoc(),
+        'div',
+        {
+          height: 100,
+          width: 100,
+        }
+      );
+
+      builders.noop(browserDiv);
+      builders.noop(workerDomDiv);
+
+      const browserHtml = getDeterministicOuterHTML(browserDiv);
+      const workerHtml = getDeterministicOuterHTML(workerDomDiv);
+      expect(workerHtml).equal(browserHtml);
+    });
   });
 });
diff --git a/test/unit/test-fixed-layer.js b/test/unit/test-fixed-layer.js
@@ -274,7 +274,7 @@ describes.sandboxed('FixedLayer', {}, (env) => {
             elem.style[privProp] = `${value} !${priority}`;
           } else if (
             elem.style[privProp] ||
-            !endsWith(elem.computedStyle[prop], '!important')
+            !elem.computedStyle[prop]?.endsWith('!important')
           ) {
             if (
               prop === 'transition' &&