core(perf): speed up tap-target's isVisible() (#9056)

GoogleChrome · Jun 5, 2019 · 43896af · 43896af
1 parent 952ae02
commit 43896af
Show file tree

Hide file tree

Showing 4 changed files with 57 additions and 32 deletions.
diff --git a/lighthouse-cli/test/fixtures/seo/seo-tap-targets.html b/lighthouse-cli/test/fixtures/seo/seo-tap-targets.html
@@ -159,6 +159,24 @@ <h1>SEO Tap targets</h1>
 
     <br/><br/>
 
+    <!-- even if a tap target's size is 0x0px it can still fail if overflowing content is visible -->
+    <div>
+      <a
+        data-gathered-target="zero-width-tap-target-with-overflowing-child-content"
+        style="display: block; width: 0; white-space: nowrap">
+        <!-- TODO: having the span should not be necessary to cause a failure here, but
+             right now we don't try to get the client rects of children that are text nodes -->
+        <span style="display: inline-block; width: 110px; height: 18px">zero width target</span>
+      </a>
+      <a
+        data-gathered-target="passing-tap-target-next-to-zero-width-target"
+        style="display: block; width: 110px; height: 100px;background: #aaa;">
+        passing target
+      </a>
+    </div>
+
+    <br/><br/>
+
     <!-- Should not fail if the two links have the same link target -->
     <div>
       <a

diff --git a/lighthouse-cli/test/smokehouse/seo/expectations.js b/lighthouse-cli/test/smokehouse/seo/expectations.js
@@ -62,6 +62,13 @@ const expectedGatheredTapTargets = [
   {
     snippet: /large-enough-tap-target-next-to-too-small-tap-target/,
   },
+  {
+    snippet: /zero-width-tap-target-with-overflowing-child-content/,
+    shouldFail: true,
+  },
+  {
+    snippet: /passing-tap-target-next-to-zero-width-target/,
+  },
   {
     snippet: /links-with-same-link-target-1/,
   },
@@ -251,6 +258,30 @@ module.exports = [
           })(),
           details: {
             items: [
+              {
+                'tapTarget': {
+                  'type': 'node',
+                  /* eslint-disable max-len */
+                  'snippet': '<a data-gathered-target="zero-width-tap-target-with-overflowing-child-content" style="display: block; width: 0; white-space: nowrap">\n        <!-- TODO: having the span should not be necessary to cause a failure here, but\n             right now we don\'t try to get the client rects of children that …',
+                  'path': '2,HTML,1,BODY,14,DIV,0,A',
+                  'selector': 'body > div > a',
+                  'nodeLabel': 'zero width target',
+                },
+                'overlappingTarget': {
+                  'type': 'node',
+                  /* eslint-disable max-len */
+                  'snippet': '<a data-gathered-target="passing-tap-target-next-to-zero-width-target" style="display: block; width: 110px; height: 100px;background: #aaa;">\n        passing target\n      </a>',
+                  'path': '2,HTML,1,BODY,14,DIV,1,A',
+                  'selector': 'body > div > a',
+                  'nodeLabel': 'passing target',
+                },
+                'tapTargetScore': 864,
+                'overlappingTargetScore': 720,
+                'overlapScoreRatio': 0.8333333333333334,
+                'size': '110x18',
+                'width': 110,
+                'height': 18,
+              },
               {
                 'tapTarget': {
                   'type': 'node',
@@ -276,11 +307,13 @@ module.exports = [
                   'type': 'node',
                   'path': '2,HTML,1,BODY,3,DIV,24,A',
                   'selector': 'body > div > a',
+                  'nodeLabel': 'left',
                 },
                 'overlappingTarget': {
                   'type': 'node',
                   'path': '2,HTML,1,BODY,3,DIV,25,A',
                   'selector': 'body > div > a',
+                  'nodeLabel': 'right',
                 },
                 'tapTargetScore': 1920,
                 'overlappingTargetScore': 560,

diff --git a/lighthouse-core/gather/gatherers/seo/tap-targets.js b/lighthouse-core/gather/gatherers/seo/tap-targets.js
@@ -37,38 +37,12 @@ const TARGET_SELECTORS = [
 const tapTargetsSelector = TARGET_SELECTORS.join(',');
 
 /**
- * @param {Element} element
+ * @param {HTMLElement} element
  * @returns {boolean}
  */
 /* istanbul ignore next */
 function elementIsVisible(element) {
-  const {overflowX, overflowY, display, visibility} = getComputedStyle(element);
-
-  if (
-    display === 'none' ||
-    (visibility === 'collapse' && ['TR', 'TBODY', 'COL', 'COLGROUP'].includes(element.tagName))
-  ) {
-    // Element not displayed
-    return false;
-  }
-
-  // only for block and inline-block, since clientWidth/Height are always 0 for inline elements
-  if (display === 'block' || display === 'inline-block') {
-    // if height/width is 0 and no overflow in that direction then
-    // there's no content that the user can see and tap on
-    if ((element.clientWidth === 0 && overflowX === 'hidden') ||
-        (element.clientHeight === 0 && overflowY === 'hidden')) {
-      return false;
-    }
-  }
-
-  const parent = element.parentElement;
-  if (parent && parent.tagName !== 'BODY') {
-    // if a parent is invisible then the current element is also invisible
-    return elementIsVisible(parent);
-  }
-
-  return true;
+  return !!(element.offsetWidth || element.offsetHeight || element.getClientRects().length);
 }
 
 /**
@@ -238,7 +212,7 @@ function gatherTapTargets() {
   // Capture element positions relative to the top of the page
   window.scrollTo(0, 0);
 
-  /** @type {Element[]} */
+  /** @type {HTMLElement[]} */
   // @ts-ignore - getElementsInDocument put into scope via stringification
   const tapTargetElements = getElementsInDocument(tapTargetsSelector);
 

diff --git a/lighthouse-core/lib/page-functions.js b/lighthouse-core/lib/page-functions.js
@@ -80,15 +80,15 @@ function checkTimeSinceLastLongTask() {
 /**
  * @param {string=} selector Optional simple CSS selector to filter nodes on.
  *     Combinators are not supported.
- * @return {Array<Element>}
+ * @return {Array<HTMLElement>}
  */
 /* istanbul ignore next */
 function getElementsInDocument(selector) {
   const realMatchesFn = window.__ElementMatches || window.Element.prototype.matches;
-  /** @type {Array<Element>} */
+  /** @type {Array<HTMLElement>} */
   const results = [];
 
-  /** @param {NodeListOf<Element>} nodes */
+  /** @param {NodeListOf<HTMLElement>} nodes */
   const _findAllElements = nodes => {
     for (let i = 0, el; el = nodes[i]; ++i) {
       if (!selector || realMatchesFn.call(el, selector)) {