Avoid duplicate auto generated ids (#1251)

sebastianbenz · web-flow · commit 7a87b1fec09d · 2021-07-12T10:36:58.000+02:00
Fixes a problem in which the same auto-generated id is assigned to multiple elements. Fixes #1250
diff --git a/packages/optimizer/lib/transformers/ApplyCommonAttributes.js b/packages/optimizer/lib/transformers/ApplyCommonAttributes.js
@@ -164,13 +164,11 @@ class ApplyCommonAttributes {
     this.log = log;
     this.canRemoveBoilerplate = true;
     // node counter for id generation
-    this.counter = 0;
+    this.transformedNodesCounter = 0;
     // nodes to check for attributes
     this.nodesToTransform = [];
     // existing ids in the document
     this.ids = new Set();
-    // nodes that have been transformed
-    this.transformedNodes = [];
     this.attributeTransformations = {
       media: new MediaTransformer(),
       sizes: new SizesTransformer(),
@@ -201,19 +199,12 @@ class ApplyCommonAttributes {
    */
   apply() {
     for (const node of this.nodesToTransform) {
+      const id = this.getOrCreateId(node);
+      let nodeHasBeenTransformed = false;
       for (const [attribute, transformer] of Object.entries(this.attributeTransformations)) {
         if (hasAttribute(node, attribute)) {
           try {
-            const id = this.getOrCreateId(node);
-            const nodeHasBeenTransformed = transformer.transform(node, id);
-            this.transformedNodes.push(node);
-            if (nodeHasBeenTransformed && !node.attribs.id) {
-              // Only update id if it's needed...
-              node.attribs.id = id;
-            } else {
-              // Decrease counter otherwise
-              this.counter--;
-            }
+            nodeHasBeenTransformed = nodeHasBeenTransformed || transformer.transform(node, id);
           } catch (e) {
             this.log.debug(
               `Cannot remove boilerplate. Failed transforming ${attribute}="${node.attribs[attribute]}".`,
@@ -223,6 +214,11 @@ class ApplyCommonAttributes {
           }
         }
       }
+      if (nodeHasBeenTransformed) {
+        node.attribs.id = id;
+      } else {
+        this.transformedNodesCounter--;
+      }
     }
   }
 
@@ -247,7 +243,7 @@ class ApplyCommonAttributes {
       insertText(customStyles, '');
     }
     customStyles.children[0].data += styles;
-    for (const node of this.transformedNodes) {
+    for (const node of this.nodesToTransform) {
       for (const attribute of Object.keys(this.attributeTransformations)) {
         delete node.attribs[attribute];
       }
@@ -264,8 +260,8 @@ class ApplyCommonAttributes {
       return node.attribs.id;
     }
     node.attribs = node.attribs || [];
-    const id = ID_PREFIX + this.counter;
-    this.counter++;
+    const id = ID_PREFIX + this.transformedNodesCounter;
+    this.transformedNodesCounter++;
     if (this.ids.has(id)) {
       // generate a new id if this one already exists
       return this.getOrCreateId(node);
diff --git a/packages/optimizer/spec/transformers/valid/ServerSideRendering/converts_media_attribute_to_css/expected_output.html b/packages/optimizer/spec/transformers/valid/ServerSideRendering/converts_media_attribute_to_css/expected_output.html
@@ -5,7 +5,7 @@
 
 
   <link href="https://example.com/favicon.ico" rel="icon">
-  <style amp-custom>@media not all and (min-width: 650px){#i-amp-0,#my-image,#i-amp-4{display:none}}@media not screen and (min-width: 650px){#i-amp-1{display:none}}@media screen and (min-width: 650px){#i-amp-2{display:none}}@media (min-width: 650px){#i-amp-3,#i-amp-5,#i-amp-6{display:none}}</style>
+  <style amp-custom>@media not all and (min-width: 650px){#i-amp-0,#my-image,#i-amp-4{display:none}}@media not screen and (min-width: 650px){#i-amp-1{display:none}}@media screen and (min-width: 650px){#i-amp-2{display:none}}@media (min-width: 650px){#i-amp-3,#i-amp-5,#i-amp-6{display:none}}@media not all and (min-width: 999px){#i-amp-7{display:none}}</style>
 </head>
 <body>
   <!-- 1. empty -->
@@ -26,5 +26,7 @@
   <amp-img id="my-image" src="image7.jpg" height="355" width="466" class="i-amphtml-layout-fixed i-amphtml-layout-size-defined" style="width:466px;height:355px;" i-amphtml-layout="fixed"></amp-img>
   <!-- 8. avoids id conflicts -->
   <amp-img id="i-amp-4" src="image8.jpg" height="355" width="466" class="i-amphtml-layout-fixed i-amphtml-layout-size-defined" style="width:466px;height:355px;" i-amphtml-layout="fixed"></amp-img>
+  <!-- 9. media attribute empty sizes -->
+  <amp-img src="image2.jpg" height="355" width="466" class="i-amphtml-layout-fixed i-amphtml-layout-size-defined" style="width:466px;height:355px;" i-amphtml-layout="fixed" id="i-amp-7"></amp-img>
 </body>
 </html>
diff --git a/packages/optimizer/spec/transformers/valid/ServerSideRendering/converts_media_attribute_to_css/input.html b/packages/optimizer/spec/transformers/valid/ServerSideRendering/converts_media_attribute_to_css/input.html
@@ -27,5 +27,7 @@
     <amp-img id="my-image" media="(min-width: 650px)" src="image7.jpg" height="355" width="466"></amp-img>
     <!-- 8. avoids id conflicts -->
     <amp-img id="i-amp-4" media="(min-width: 650px)" src="image8.jpg" height="355" width="466"></amp-img>
+    <!-- 9. media attribute empty sizes -->
+    <amp-img media="(min-width: 999px)" sizes="" heights="" src="image2.jpg" height="355" width="466"></amp-img>
   </body>
 </html>
