From 6a4d17b967efa06c70d00e26419fb958d65f0077 Mon Sep 17 00:00:00 2001 From: Brendan Kenny Date: Thu, 31 Jan 2019 11:58:04 -0800 Subject: [PATCH 1/6] core: optimize tap-targets audit --- lighthouse-core/audits/seo/tap-targets.js | 189 +++++++++++----------- lighthouse-core/lib/rect-helpers.js | 126 ++++++++------- 2 files changed, 167 insertions(+), 148 deletions(-) diff --git a/lighthouse-core/audits/seo/tap-targets.js b/lighthouse-core/audits/seo/tap-targets.js index 3a13b9369d86..04baa3a0c449 100644 --- a/lighthouse-core/audits/seo/tap-targets.js +++ b/lighthouse-core/audits/seo/tap-targets.js @@ -12,10 +12,12 @@ const Audit = require('../audit'); const ViewportAudit = require('../viewport'); const { + rectsTouchOrOverlap, getRectOverlapArea, getRectAtCenter, allRectsContainedWithinEachOther, getLargestRect, + getBoundingRectWithMinimimSize, } = require('../../lib/rect-helpers'); const {getTappableRectsFromClientRects} = require('../../lib/tappable-rects'); const i18n = require('../../lib/i18n/i18n.js'); @@ -47,6 +49,21 @@ const FINGER_SIZE_PX = 48; // to the finger area tapping on the intended element const MAX_ACCEPTABLE_OVERLAP_SCORE_RATIO = 0.25; +/** + * Returns a tap target augmented with a bounding rect for quick overlapping + * rejections. Rect contains all the client rects, with a width and height of at + * least FINGER_SIZE_PX. + * @param {LH.Artifacts.TapTarget[]} targets + * @return {BoundedTapTarget[]} + */ +function getBoundedTapTargets(targets) { + return targets.map(tapTarget => { + return { + tapTarget, + boundingRect: getBoundingRectWithMinimimSize(tapTarget.clientRects, FINGER_SIZE_PX), + }; + }); +} /** * @param {LH.Artifacts.Rect} cr @@ -57,130 +74,108 @@ function clientRectBelowMinimumSize(cr) { /** * A target is "too small" if none of its clientRects are at least the size of a finger. - * @param {LH.Artifacts.TapTarget[]} targets - * @returns {LH.Artifacts.TapTarget[]} + * @param {BoundedTapTarget[]} targets + * @returns {BoundedTapTarget[]} */ function getTooSmallTargets(targets) { return targets.filter(target => { - return target.clientRects.every(clientRectBelowMinimumSize); + return target.tapTarget.clientRects.every(clientRectBelowMinimumSize); }); } /** - * @param {LH.Artifacts.TapTarget[]} tooSmallTargets - * @param {LH.Artifacts.TapTarget[]} allTargets + * @param {BoundedTapTarget[]} tooSmallTargets + * @param {BoundedTapTarget[]} allTargets * @returns {TapTargetOverlapFailure[]} */ function getAllOverlapFailures(tooSmallTargets, allTargets) { /** @type {TapTargetOverlapFailure[]} */ - let failures = []; + const failures = []; tooSmallTargets.forEach(target => { - const overlappingTargets = getAllOverlapFailuresForTarget( - target, - allTargets - ); + // Convert client rects to unique tappable areas from a user's perspective + const tappableRects = getTappableRectsFromClientRects(target.tapTarget.clientRects); - failures = failures.concat(overlappingTargets); - }); + for (const maybeOverlappingTarget of allTargets) { + if (maybeOverlappingTarget === target) { + // Checking the same target with itself, skip. + continue; + } - return failures; -} + if (!rectsTouchOrOverlap(target.boundingRect, maybeOverlappingTarget.boundingRect)) { + // Bounding boxes (of at least FINGER_SIZE_PX) don't overlap, skip. + continue; + } -/** - * - * @param {LH.Artifacts.TapTarget} tapTarget - * @param {LH.Artifacts.TapTarget[]} allTapTargets - * @returns {TapTargetOverlapFailure[]} - */ -function getAllOverlapFailuresForTarget(tapTarget, allTapTargets) { - /** @type TapTargetOverlapFailure[] */ - const failures = []; + if (target.tapTarget.href === maybeOverlappingTarget.tapTarget.href) { + const isHttpOrHttpsLink = /https?:\/\//.test(target.tapTarget.href); + if (isHttpOrHttpsLink) { + // No overlap because same target action, skip. + continue; + } + } - for (const maybeOverlappingTarget of allTapTargets) { - if (maybeOverlappingTarget === tapTarget) { - // checking the same target with itself, skip - continue; - } + const maybeOverlappingRects = maybeOverlappingTarget.tapTarget.clientRects; + if (allRectsContainedWithinEachOther(tappableRects, maybeOverlappingRects)) { + // If one tap target is fully contained within the other that's + // probably intentional (e.g. an item with a delete button inside). + // We'll miss some problems because of this, but that's better + // than falsely reporting a failure. + return null; + } - const failure = getOverlapFailureForTargetPair(tapTarget, maybeOverlappingTarget); - if (failure) { - failures.push(failure); + const rectFailure = getOverlapFailureForTargetPair(tappableRects, maybeOverlappingRects); + if (rectFailure) { + failures.push({ + ...rectFailure, + tapTarget: target.tapTarget, + overlappingTarget: maybeOverlappingTarget.tapTarget, + }); + } } - } + }); return failures; } /** - * @param {LH.Artifacts.TapTarget} tapTarget - * @param {LH.Artifacts.TapTarget} maybeOverlappingTarget - * @returns {TapTargetOverlapFailure | null} + * @param {LH.Artifacts.Rect[]} tappableRects + * @param {LH.Artifacts.Rect[]} maybeOverlappingRects + * @returns {ClientRectOverlapFailure | null} */ -function getOverlapFailureForTargetPair(tapTarget, maybeOverlappingTarget) { - const isHttpOrHttpsLink = /https?:\/\//.test(tapTarget.href); - if (isHttpOrHttpsLink && tapTarget.href === maybeOverlappingTarget.href) { - // no overlap because same target action - return null; - } - - // Convert client rects to unique tappable areas from a user's perspective - const tappableRects = getTappableRectsFromClientRects(tapTarget.clientRects); - if (allRectsContainedWithinEachOther(tappableRects, maybeOverlappingTarget.clientRects)) { - // If one tap target is fully contained within the other that's - // probably intentional (e.g. an item with a delete button inside). - // We'll miss some problems because of this, but that's better - // than falsely reporting a failure. - return null; - } - - /** @type TapTargetOverlapFailure | null */ +function getOverlapFailureForTargetPair(tappableRects, maybeOverlappingRects) { + /** @type ClientRectOverlapFailure | null */ let greatestFailure = null; + for (const targetCR of tappableRects) { - for (const maybeOverlappingCR of maybeOverlappingTarget.clientRects) { - const failure = getOverlapFailureForClientRectPair(targetCR, maybeOverlappingCR); - if (failure) { - // only update our state if this was the biggest failure we've seen for this pair - if (!greatestFailure || - failure.overlapScoreRatio > greatestFailure.overlapScoreRatio) { - greatestFailure = { - ...failure, - tapTarget, - overlappingTarget: maybeOverlappingTarget, - }; - } + const fingerRect = getRectAtCenter(targetCR, FINGER_SIZE_PX); + // Score indicates how much of the finger area overlaps each target when the user + // taps on the center of targetCR + const tapTargetScore = getRectOverlapArea(fingerRect, targetCR); + + for (const maybeOverlappingCR of maybeOverlappingRects) { + const overlappingTargetScore = getRectOverlapArea(fingerRect, maybeOverlappingCR); + + const overlapScoreRatio = overlappingTargetScore / tapTargetScore; + if (overlapScoreRatio < MAX_ACCEPTABLE_OVERLAP_SCORE_RATIO) { + // low score means it's clear that the user tried to tap on the targetCR, + // rather than the other tap target client rect + continue; + } + + // only update our state if this was the biggest failure we've seen for this pair + if (!greatestFailure || overlapScoreRatio > greatestFailure.overlapScoreRatio) { + greatestFailure = { + overlapScoreRatio, + tapTargetScore, + overlappingTargetScore, + }; } } } return greatestFailure; } -/** - * @param {LH.Artifacts.Rect} targetCR - * @param {LH.Artifacts.Rect} maybeOverlappingCR - * @returns {ClientRectOverlapFailure | null} - */ -function getOverlapFailureForClientRectPair(targetCR, maybeOverlappingCR) { - const fingerRect = getRectAtCenter(targetCR, FINGER_SIZE_PX); - // Score indicates how much of the finger area overlaps each target when the user - // taps on the center of targetCR - const tapTargetScore = getRectOverlapArea(fingerRect, targetCR); - const maybeOverlappingScore = getRectOverlapArea(fingerRect, maybeOverlappingCR); - - const overlapScoreRatio = maybeOverlappingScore / tapTargetScore; - if (overlapScoreRatio < MAX_ACCEPTABLE_OVERLAP_SCORE_RATIO) { - // low score means it's clear that the user tried to tap on the targetCR, - // rather than the other tap target client rect - return null; - } - - return { - overlapScoreRatio, - tapTargetScore, - overlappingTargetScore: maybeOverlappingScore, - }; -} - /** * Only report one failure if two targets overlap each other * @param {TapTargetOverlapFailure[]} overlapFailures @@ -287,8 +282,11 @@ class TapTargets extends Audit { }; } - const tooSmallTargets = getTooSmallTargets(artifacts.TapTargets); - const overlapFailures = getAllOverlapFailures(tooSmallTargets, artifacts.TapTargets); + // Augment the targets with bounding rects for quick intersection testing. + const boundedTapTargets = getBoundedTapTargets(artifacts.TapTargets); + + const tooSmallTargets = getTooSmallTargets(boundedTapTargets); + const overlapFailures = getAllOverlapFailures(tooSmallTargets, boundedTapTargets); const overlapFailuresForDisplay = mergeSymmetricFailures(overlapFailures); const tableItems = getTableItems(overlapFailuresForDisplay); @@ -336,6 +334,11 @@ module.exports.UIStrings = UIStrings; overlappingTarget: LH.Artifacts.TapTarget; }} TapTargetOverlapFailure */ +/** @typedef {{ + boundingRect: LH.Artifacts.Rect; + tapTarget: LH.Artifacts.TapTarget; +}} BoundedTapTarget */ + /** @typedef {{ tapTarget: LH.Audit.DetailsRendererNodeDetailsJSON; overlappingTarget: LH.Audit.DetailsRendererNodeDetailsJSON; diff --git a/lighthouse-core/lib/rect-helpers.js b/lighthouse-core/lib/rect-helpers.js index 9deb75bbfaa9..72a6d436764c 100644 --- a/lighthouse-core/lib/rect-helpers.js +++ b/lighthouse-core/lib/rect-helpers.js @@ -9,48 +9,28 @@ * @param {LH.Artifacts.Rect} rect * @param {{x:number, y:number}} point */ -// We sometimes run this as a part of a gatherer script injected into the page, so prevent -// renaming the function for code coverage. -/* istanbul ignore next */ function rectContainsPoint(rect, {x, y}) { return rect.left <= x && rect.right >= x && rect.top <= y && rect.bottom >= y; } /** + * Returns whether rect2 is contained entirely within rect1; * @param {LH.Artifacts.Rect} rect1 * @param {LH.Artifacts.Rect} rect2 + * @return {boolean} */ // We sometimes run this as a part of a gatherer script injected into the page, so prevent // renaming the function for code coverage. /* istanbul ignore next */ function rectContains(rect1, rect2) { - return ( - // top left corner - rectContainsPoint(rect1, { - x: rect2.left, - y: rect2.top, - }) && - // top right corner - rectContainsPoint(rect1, { - x: rect2.right, - y: rect2.top, - }) && - // bottom left corner - rectContainsPoint(rect1, { - x: rect2.left, - y: rect2.bottom, - }) && - // bottom right corner - rectContainsPoint(rect1, { - x: rect2.right, - y: rect2.bottom, - }) - ); + return rect2.top >= rect1.top && + rect2.right <= rect1.right && + rect2.bottom <= rect1.bottom && + rect2.left >= rect1.left; } const rectContainsString = ` - ${rectContainsPoint.toString()} ${rectContains.toString()}; `; @@ -69,20 +49,24 @@ function filterOutTinyRects(rects) { * @returns {LH.Artifacts.Rect[]} */ function filterOutRectsContainedByOthers(rects) { - const rectsToKeep = new Set(rects); + const rectsToKeep = []; for (const rect of rects) { + let contained = false; for (const possiblyContainingRect of rects) { if (rect === possiblyContainingRect) continue; - if (!rectsToKeep.has(possiblyContainingRect)) continue; if (rectContains(possiblyContainingRect, rect)) { - rectsToKeep.delete(rect); + contained = true; break; } } + + if (!contained) { + rectsToKeep.push(rect); + } } - return Array.from(rectsToKeep); + return rectsToKeep; } /** @@ -110,6 +94,56 @@ function rectsTouchOrOverlap(rectA, rectB) { ); } +/** + * Returns a bounding rect for all the passed in rects, with a width and height + * of at least `minimumSize`. + * @param {LH.Artifacts.Rect[]} rects + * @param {number} minimumSize + * @return {LH.Artifacts.Rect} + */ +function getBoundingRectWithMinimimSize(rects, minimumSize) { + if (rects.length === 0) { + throw new Error('No rects to take bounds of'); + } + + let left = Number.MAX_VALUE; + let right = -Number.MAX_VALUE; + let top = -Number.MAX_VALUE; + let bottom = Number.MAX_VALUE; + for (const rect of rects) { + left = Math.min(left, rect.left); + right = Math.max(right, rect.right); + top = Math.min(top, rect.top); + bottom = Math.max(bottom, rect.bottom); + } + + let width = right - left; + let height = bottom - top; + + // If too small in width or height, expand from the midpoint to minimumSize. + if (width < minimumSize) { + const midX = (left + right) / 2; + left = midX - minimumSize / 2; + right = midX + minimumSize / 2; + width = minimumSize; + } + if (height < minimumSize) { + const midY = (top + bottom) / 2; + top = midY - minimumSize / 2; + bottom = midY + minimumSize / 2; + height = minimumSize; + } + + return { + left, + right, + top, + bottom, + width, + height, + }; +} + /** * @param {LH.Artifacts.Rect} rectA * @param {LH.Artifacts.Rect} rectB @@ -161,32 +195,15 @@ function addRectTopAndBottom({x, y, width, height}) { * @param {LH.Artifacts.Rect} rect1 * @param {LH.Artifacts.Rect} rect2 */ -function getRectXOverlap(rect1, rect2) { +function getRectOverlapArea(rect1, rect2) { // https://stackoverflow.com/a/9325084/1290545 - return Math.max( - 0, - Math.min(rect1.right, rect2.right) - Math.max(rect1.left, rect2.left) - ); -} + const rectYOverlap = Math.min(rect1.bottom, rect2.bottom) - Math.max(rect1.top, rect2.top); + if (rectYOverlap <= 0) return 0; -/** - * @param {LH.Artifacts.Rect} rect1 - * @param {LH.Artifacts.Rect} rect2 - */ -function getRectYOverlap(rect1, rect2) { - // https://stackoverflow.com/a/9325084/1290545 - return Math.max( - 0, - Math.min(rect1.bottom, rect2.bottom) - Math.max(rect1.top, rect2.top) - ); -} + const rectXOverlap = Math.min(rect1.right, rect2.right) - Math.max(rect1.left, rect2.left); + if (rectXOverlap <= 0) return 0; -/** - * @param {LH.Artifacts.Rect} rect1 - * @param {LH.Artifacts.Rect} rect2 - */ -function getRectOverlapArea(rect1, rect2) { - return getRectXOverlap(rect1, rect2) * getRectYOverlap(rect1, rect2); + return rectXOverlap * rectYOverlap; } /** @@ -244,13 +261,12 @@ module.exports = { rectContainsString, addRectWidthAndHeight, addRectTopAndBottom, - getRectXOverlap, - getRectYOverlap, getRectOverlapArea, getRectAtCenter, getLargestRect, getRectCenterPoint, getBoundingRect, + getBoundingRectWithMinimimSize, rectsTouchOrOverlap, allRectsContainedWithinEachOther, filterOutRectsContainedByOthers, From ba83e7dbdfac9928ea421ca0195e2119e17f8df1 Mon Sep 17 00:00:00 2001 From: Brendan Kenny Date: Thu, 31 Jan 2019 20:19:55 -0800 Subject: [PATCH 2/6] fix tests. linear now, old way was fine --- lighthouse-core/lib/rect-helpers.js | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/lighthouse-core/lib/rect-helpers.js b/lighthouse-core/lib/rect-helpers.js index 72a6d436764c..034219a4ebc9 100644 --- a/lighthouse-core/lib/rect-helpers.js +++ b/lighthouse-core/lib/rect-helpers.js @@ -49,24 +49,20 @@ function filterOutTinyRects(rects) { * @returns {LH.Artifacts.Rect[]} */ function filterOutRectsContainedByOthers(rects) { - const rectsToKeep = []; + const rectsToKeep = new Set(rects); for (const rect of rects) { - let contained = false; for (const possiblyContainingRect of rects) { if (rect === possiblyContainingRect) continue; + if (!rectsToKeep.has(possiblyContainingRect)) continue; if (rectContains(possiblyContainingRect, rect)) { - contained = true; + rectsToKeep.delete(rect); break; } } - - if (!contained) { - rectsToKeep.push(rect); - } } - return rectsToKeep; + return Array.from(rectsToKeep); } /** From 17b694a227c7791a141b35c4e802acd5866bd73b Mon Sep 17 00:00:00 2001 From: Brendan Kenny Date: Fri, 1 Feb 2019 11:05:13 -0800 Subject: [PATCH 3/6] getBoundingRectWithPadding --- lighthouse-core/audits/seo/tap-targets.js | 11 ++++---- lighthouse-core/lib/rect-helpers.js | 34 ++++++++--------------- 2 files changed, 17 insertions(+), 28 deletions(-) diff --git a/lighthouse-core/audits/seo/tap-targets.js b/lighthouse-core/audits/seo/tap-targets.js index 04baa3a0c449..0fc690a6b1c7 100644 --- a/lighthouse-core/audits/seo/tap-targets.js +++ b/lighthouse-core/audits/seo/tap-targets.js @@ -17,7 +17,7 @@ const { getRectAtCenter, allRectsContainedWithinEachOther, getLargestRect, - getBoundingRectWithMinimimSize, + getBoundingRectWithPadding, } = require('../../lib/rect-helpers'); const {getTappableRectsFromClientRects} = require('../../lib/tappable-rects'); const i18n = require('../../lib/i18n/i18n.js'); @@ -51,8 +51,7 @@ const MAX_ACCEPTABLE_OVERLAP_SCORE_RATIO = 0.25; /** * Returns a tap target augmented with a bounding rect for quick overlapping - * rejections. Rect contains all the client rects, with a width and height of at - * least FINGER_SIZE_PX. + * rejections. Rect contains all the client rects, padded to half FINGER_SIZE_PX. * @param {LH.Artifacts.TapTarget[]} targets * @return {BoundedTapTarget[]} */ @@ -60,7 +59,7 @@ function getBoundedTapTargets(targets) { return targets.map(tapTarget => { return { tapTarget, - boundingRect: getBoundingRectWithMinimimSize(tapTarget.clientRects, FINGER_SIZE_PX), + boundingRect: getBoundingRectWithPadding(tapTarget.clientRects, FINGER_SIZE_PX), }; }); } @@ -103,7 +102,7 @@ function getAllOverlapFailures(tooSmallTargets, allTargets) { } if (!rectsTouchOrOverlap(target.boundingRect, maybeOverlappingTarget.boundingRect)) { - // Bounding boxes (of at least FINGER_SIZE_PX) don't overlap, skip. + // Bounding boxes (padded with half FINGER_SIZE_PX) don't overlap, skip. continue; } @@ -282,7 +281,7 @@ class TapTargets extends Audit { }; } - // Augment the targets with bounding rects for quick intersection testing. + // Augment the targets with padded bounding rects for quick intersection testing. const boundedTapTargets = getBoundedTapTargets(artifacts.TapTargets); const tooSmallTargets = getTooSmallTargets(boundedTapTargets); diff --git a/lighthouse-core/lib/rect-helpers.js b/lighthouse-core/lib/rect-helpers.js index 034219a4ebc9..1476d97f8028 100644 --- a/lighthouse-core/lib/rect-helpers.js +++ b/lighthouse-core/lib/rect-helpers.js @@ -91,13 +91,13 @@ function rectsTouchOrOverlap(rectA, rectB) { } /** - * Returns a bounding rect for all the passed in rects, with a width and height - * of at least `minimumSize`. + * Returns a bounding rect for all the passed in rects, with padded with half of + * `minimumSize` on all sides. * @param {LH.Artifacts.Rect[]} rects * @param {number} minimumSize * @return {LH.Artifacts.Rect} */ -function getBoundingRectWithMinimimSize(rects, minimumSize) { +function getBoundingRectWithPadding(rects, minimumSize) { if (rects.length === 0) { throw new Error('No rects to take bounds of'); } @@ -113,30 +113,20 @@ function getBoundingRectWithMinimimSize(rects, minimumSize) { bottom = Math.max(bottom, rect.bottom); } - let width = right - left; - let height = bottom - top; - - // If too small in width or height, expand from the midpoint to minimumSize. - if (width < minimumSize) { - const midX = (left + right) / 2; - left = midX - minimumSize / 2; - right = midX + minimumSize / 2; - width = minimumSize; - } - if (height < minimumSize) { - const midY = (top + bottom) / 2; - top = midY - minimumSize / 2; - bottom = midY + minimumSize / 2; - height = minimumSize; - } + // Pad on all sides. + const halfMinSize = minimumSize / 2; + left -= halfMinSize; + right += halfMinSize; + top -= halfMinSize; + bottom += halfMinSize; return { left, right, top, bottom, - width, - height, + width: right - left, + height: bottom - top, }; } @@ -262,7 +252,7 @@ module.exports = { getLargestRect, getRectCenterPoint, getBoundingRect, - getBoundingRectWithMinimimSize, + getBoundingRectWithPadding, rectsTouchOrOverlap, allRectsContainedWithinEachOther, filterOutRectsContainedByOthers, From 3234df46202173a1b4ad2f8c97af04d3febfc065 Mon Sep 17 00:00:00 2001 From: Brendan Kenny Date: Fri, 1 Feb 2019 11:24:51 -0800 Subject: [PATCH 4/6] fix bounding rect; tests --- lighthouse-core/lib/rect-helpers.js | 4 +-- lighthouse-core/test/lib/rect-helpers-test.js | 32 +++++++++++++++++++ 2 files changed, 34 insertions(+), 2 deletions(-) diff --git a/lighthouse-core/lib/rect-helpers.js b/lighthouse-core/lib/rect-helpers.js index 1476d97f8028..5bae4486e8ea 100644 --- a/lighthouse-core/lib/rect-helpers.js +++ b/lighthouse-core/lib/rect-helpers.js @@ -104,8 +104,8 @@ function getBoundingRectWithPadding(rects, minimumSize) { let left = Number.MAX_VALUE; let right = -Number.MAX_VALUE; - let top = -Number.MAX_VALUE; - let bottom = Number.MAX_VALUE; + let top = Number.MAX_VALUE; + let bottom = -Number.MAX_VALUE; for (const rect of rects) { left = Math.min(left, rect.left); right = Math.max(right, rect.right); diff --git a/lighthouse-core/test/lib/rect-helpers-test.js b/lighthouse-core/test/lib/rect-helpers-test.js index 22c0a19be04e..82d001686515 100644 --- a/lighthouse-core/test/lib/rect-helpers-test.js +++ b/lighthouse-core/test/lib/rect-helpers-test.js @@ -13,6 +13,7 @@ const { getLargestRect, getRectAtCenter, allRectsContainedWithinEachOther, + getBoundingRectWithPadding, } = require('../../lib/rect-helpers'); describe('Rect Helpers', () => { @@ -84,4 +85,35 @@ describe('Rect Helpers', () => { expect(allRectsContainedWithinEachOther([rect1], [rect2])).toBe(false); }); }); + + describe('#getBoundingRectWithPadding', () => { + it('throws an error if no rects are passed in', () => { + expect(() => getBoundingRectWithPadding([])).toThrow('No rects'); + }); + + it('pads rect with half minimum size on all sides', () => { + const rect = addRectTopAndBottom({x: 0, y: 0, width: 0, height: 0}); + const minimumSize = 20; + const expectedPaddedBounds = addRectTopAndBottom({x: -10, y: -10, width: 20, height: 20}); + + expect(getBoundingRectWithPadding([rect], minimumSize)).toEqual(expectedPaddedBounds); + }); + + it('pads the bounding box of two rects with half of the minimum size', () => { + const rect1 = addRectTopAndBottom({x: 0, y: 0, width: 10, height: 10}); + const rect2 = addRectTopAndBottom({x: 50, y: 50, width: 10, height: 10}); // in the middle somewhere + const rect3 = addRectTopAndBottom({x: 100, y: 100, width: 10, height: 10}); + + const minimumSize = 20; + const expectedPaddedBounds = addRectTopAndBottom({ + x: -minimumSize / 2, + y: -minimumSize / 2, + width: 110 + minimumSize, + height: 110 + minimumSize, + }); + + const rects = [rect1, rect2, rect3]; + expect(getBoundingRectWithPadding(rects, minimumSize)).toEqual(expectedPaddedBounds); + }); + }); }); From bbf0c5a36d95e6482a755999331ce11c8d35b6fb Mon Sep 17 00:00:00 2001 From: Brendan Kenny Date: Fri, 1 Feb 2019 12:06:40 -0800 Subject: [PATCH 5/6] continue --- lighthouse-core/audits/seo/tap-targets.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lighthouse-core/audits/seo/tap-targets.js b/lighthouse-core/audits/seo/tap-targets.js index 0fc690a6b1c7..8ac7a139e54c 100644 --- a/lighthouse-core/audits/seo/tap-targets.js +++ b/lighthouse-core/audits/seo/tap-targets.js @@ -120,7 +120,7 @@ function getAllOverlapFailures(tooSmallTargets, allTargets) { // probably intentional (e.g. an item with a delete button inside). // We'll miss some problems because of this, but that's better // than falsely reporting a failure. - return null; + continue; } const rectFailure = getOverlapFailureForTargetPair(tappableRects, maybeOverlappingRects); From 2fbf6e042b6eb2e923bc4462cac1eb99d75754be Mon Sep 17 00:00:00 2001 From: Brendan Kenny Date: Mon, 4 Feb 2019 12:54:47 -0800 Subject: [PATCH 6/6] paddedBoundsRect --- lighthouse-core/audits/seo/tap-targets.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/lighthouse-core/audits/seo/tap-targets.js b/lighthouse-core/audits/seo/tap-targets.js index 8ac7a139e54c..b82e51fdb8bf 100644 --- a/lighthouse-core/audits/seo/tap-targets.js +++ b/lighthouse-core/audits/seo/tap-targets.js @@ -36,7 +36,7 @@ const UIStrings = { /** Label of a table column that identifies a tap target (like a link or button) that overlaps with another tap target. */ overlappingTargetHeader: 'Overlapping Target', /** Explanatory message stating that there was a failure in an audit caused by the viewport meta tag not being optimized for mobile screens, which caused tap targets like buttons and links to be too small to tap on. */ - /* eslint-disable max-len */ + /* eslint-disable-next-line max-len */ explanationViewportMetaNotOptimized: 'Tap targets are too small because there\'s no viewport meta tag optimized for mobile screens', /** Explanatory message stating that a certain percentage of the tap targets (like buttons and links) on the page are of an appropriately large size. */ displayValue: '{decimalProportion, number, percent} appropriately sized tap targets', @@ -59,7 +59,7 @@ function getBoundedTapTargets(targets) { return targets.map(tapTarget => { return { tapTarget, - boundingRect: getBoundingRectWithPadding(tapTarget.clientRects, FINGER_SIZE_PX), + paddedBoundsRect: getBoundingRectWithPadding(tapTarget.clientRects, FINGER_SIZE_PX), }; }); } @@ -101,7 +101,7 @@ function getAllOverlapFailures(tooSmallTargets, allTargets) { continue; } - if (!rectsTouchOrOverlap(target.boundingRect, maybeOverlappingTarget.boundingRect)) { + if (!rectsTouchOrOverlap(target.paddedBoundsRect, maybeOverlappingTarget.paddedBoundsRect)) { // Bounding boxes (padded with half FINGER_SIZE_PX) don't overlap, skip. continue; } @@ -334,7 +334,7 @@ module.exports.UIStrings = UIStrings; }} TapTargetOverlapFailure */ /** @typedef {{ - boundingRect: LH.Artifacts.Rect; + paddedBoundsRect: LH.Artifacts.Rect; tapTarget: LH.Artifacts.TapTarget; }} BoundedTapTarget */