PX:ignore invinsible elements (#1173)

ampproject · Mar 22, 2021 · bbd7787 · bbd7787
1 parent 3f81e71
commit bbd7787
Show file tree

Hide file tree

Showing 3 changed files with 63 additions and 17 deletions.
diff --git a/packages/page-experience/lib/PageDataGatherer.js b/packages/page-experience/lib/PageDataGatherer.js
@@ -88,13 +88,26 @@ class PageAnalyzer {
       /* global document, window */
 
       /**
-       * Returns true if the given element is in the first viewport.
+       * Returns true if the given element is visible
        *
-       * @param {Element} el
+       * @param {Element} elem
        * @return {boolean}
        */
-      const isElementInViewport = (el) => {
-        const rect = el.getBoundingClientRect();
+      const isVisible = (elem) => {
+        return !!(elem.offsetWidth || elem.offsetHeight || elem.getClientRects().length);
+      };
+
+      /**
+       * Returns true if the given element is visible and in the first viewport.
+       *
+       * @param {Element} elem
+       * @return {boolean}
+       */
+      const isCriticalElement = (elem) => {
+        const rect = elem.getBoundingClientRect();
+        if (!isVisible(elem)) {
+          return false;
+        }
         return (
           rect.top <= (window.innerHeight || document.documentElement.clientHeight) &&
           rect.left <= (window.innerWidth || document.documentElement.clientWidth)
@@ -167,7 +180,7 @@ class PageAnalyzer {
           if (!font) {
             return;
           }
-          if (isElementInViewport(node)) {
+          if (isCriticalElement(node)) {
             criticalFonts.add(font);
           } else {
             nonCriticalFonts.add(font);
@@ -187,7 +200,7 @@ class PageAnalyzer {
        */
       const collectInitialIframes = () => {
         return [...document.querySelectorAll('amp-iframe')]
-          .filter(isElementInViewport)
+          .filter(isCriticalElement)
           .map((i) => i.getAttribute('src'));
       };
 
@@ -197,17 +210,15 @@ class PageAnalyzer {
        * @return {Array<Object>} object containing the image's src, layout, width and height values
        */
       const collectInitialAmpImg = () => {
-        return [...document.querySelectorAll('amp-img')]
-          .filter(isElementInViewport)
-          .map((ampImg) => {
-            return {
-              src: ampImg.getAttribute('src'),
-              dataHero: ampImg.hasAttribute('data-hero'),
-              layout: ampImg.getAttribute('layout'),
-              width: ampImg.getAttribute('width'),
-              height: ampImg.getAttribute('height'),
-            };
-          });
+        return [...document.querySelectorAll('amp-img')].filter(isCriticalElement).map((ampImg) => {
+          return {
+            src: ampImg.getAttribute('src'),
+            dataHero: ampImg.hasAttribute('data-hero'),
+            layout: ampImg.getAttribute('layout'),
+            width: ampImg.getAttribute('width'),
+            height: ampImg.getAttribute('height'),
+          };
+        });
       };
 
       return {

diff --git a/packages/page-experience/test-data/checks/heroImages/ignore-hidden-images.html b/packages/page-experience/test-data/checks/heroImages/ignore-hidden-images.html
@@ -0,0 +1,32 @@
+<!DOCTYPE html>
+<html ⚡>
+<head>
+  <meta charset="utf-8" />
+  <title>My AMP Page</title>
+  <link rel="canonical" href="self.html" />
+  <meta name="viewport" content="width=device-width" />
+  <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>
+  <script async custom-element="amp-sidebar" src="https://cdn.ampproject.org/v0/amp-sidebar-0.1.js"></script>
+
+  <style amp-custom>
+        .sizer {
+          height: 100vh;
+        }
+        h1 {
+          font-family: "Open Sans";
+        }
+
+    </style>
+</head>
+<body>
+  <amp-sidebar id="sidebar" class="sample-sidebar" layout="nodisplay" side="left">
+    <amp-img width="200" height="200" layout="responsive" src="https://unsplash.it/400/400">
+    </amp-img>
+     <h1>Hello AMPHTML World!</h1>
+  </amp-sidebar>
+  <button class="hamburger" on="tap:sidebar.toggle">Toggle</button>
+  <div class="sizer"></div>
+  <h1>Hello AMPHTML World!</h1>
+</body>
+</html>
diff --git a/packages/page-experience/test-data/checks/heroImages/ignore-hidden-images.json b/packages/page-experience/test-data/checks/heroImages/ignore-hidden-images.json
@@ -0,0 +1,3 @@
+{
+  "details": {}
+}