From 8aa6457dedb9eb589e8aa919c09f046fe8d3d231 Mon Sep 17 00:00:00 2001 From: Matt Zeunert Date: Sun, 2 Dec 2018 10:09:05 +0000 Subject: [PATCH 01/11] Client rect functions --- lighthouse-core/lib/client-rect-functions.js | 311 ++++++++++++++++++ .../test/lib/client-rect-functions-test.js | 245 ++++++++++++++ types/artifacts.d.ts | 9 + 3 files changed, 565 insertions(+) create mode 100644 lighthouse-core/lib/client-rect-functions.js create mode 100644 lighthouse-core/test/lib/client-rect-functions-test.js diff --git a/lighthouse-core/lib/client-rect-functions.js b/lighthouse-core/lib/client-rect-functions.js new file mode 100644 index 000000000000..8a6785cb1c11 --- /dev/null +++ b/lighthouse-core/lib/client-rect-functions.js @@ -0,0 +1,311 @@ +/** + * @license Copyright 2018 Google Inc. All Rights Reserved. + * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. You may obtain a copy of the License at http://www.apache.org/licenses/LICENSE-2.0 + * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License. + */ +'use strict'; + +/** + * @param {LH.Artifacts.ClientRect} cr + * @param {{x:number, y:number}} point + */ +/* istanbul ignore next */ +function rectContainsPoint(cr, {x, y}) { + return cr.left <= x && cr.right >= x && cr.top <= y && cr.bottom >= y; +} + +/** + * @param {LH.Artifacts.ClientRect} cr1 + * @param {LH.Artifacts.ClientRect} cr2 + */ +/* istanbul ignore next */ +function rectContains(cr1, cr2) { + const topLeft = { + x: cr2.left, + y: cr2.top, + }; + const topRight = { + x: cr2.right, + y: cr2.top, + }; + const bottomLeft = { + x: cr2.left, + y: cr2.bottom, + }; + const bottomRight = { + x: cr2.right, + y: cr2.bottom, + }; + return ( + rectContainsPoint(cr1, topLeft) && + rectContainsPoint(cr1, topRight) && + rectContainsPoint(cr1, bottomLeft) && + rectContainsPoint(cr1, bottomRight) + ); +} + +/** + * Merge client rects together. This may result in a larger overall size than that of the individual client rects. + * @param {LH.Artifacts.ClientRect[]} clientRects + */ +function simplifyClientRects(clientRects) { + clientRects = filterOutTinyClientRects(clientRects); + clientRects = filterOutClientRectsContainedByOthers(clientRects); + clientRects = mergeTouchingClientRects(clientRects); + return clientRects; +} + +/** + * @param {LH.Artifacts.ClientRect[]} clientRects + * @returns {LH.Artifacts.ClientRect[]} + */ +function filterOutTinyClientRects(clientRects) { + // 1x1px rect shouldn't be reason to treat the rect as something the user should tap on. + // Often they're made invisble in some obscure way anyway, and only exist for e.g. accessibiliity. + const nonTinyClientRects = clientRects.filter( + rect => rect.width > 1 && rect.height > 1 + ); + if (nonTinyClientRects.length > 0) { + return nonTinyClientRects; + } + return clientRects; +} + +const rectContainsString = ` + ${rectContainsPoint.toString()} + ${rectContains.toString()}; +`; + +/** + * @param {LH.Artifacts.ClientRect[]} clientRects + * @returns {LH.Artifacts.ClientRect[]} + */ +function filterOutClientRectsContainedByOthers(clientRects) { + const filteredOutRects = new Set(); + return clientRects.filter(cr => { + for (const possiblyContainingRect of clientRects) { + if (possiblyContainingRect === cr) { + continue; + } + if (filteredOutRects.has(possiblyContainingRect)) { + // Can't contain anything because we removed it from the list + continue; + } + if (rectContains(possiblyContainingRect, cr)) { + filteredOutRects.add(cr); + return false; + } + } + return true; + }); +} + +/** + * @param {number} a + * @param {number} b + */ +function almostEqual(a, b) { + // Sometimes a child will reach out of the parent by + // 1px or 2px, so be somewhat tolerant for merging + return Math.abs(a - b) <= 2; +} + +/** + * @param {LH.Artifacts.ClientRect} rect + */ +function getRectCenterPoint(rect) { + return { + x: rect.left + rect.width / 2, + y: rect.top + rect.height / 2, + }; +} + +/** + * @param {LH.Artifacts.ClientRect} crA + * @param {LH.Artifacts.ClientRect} crB + * @returns {boolean} + */ +function clientRectsTouch(crA, crB) { + // https://stackoverflow.com/questions/2752349/fast-rectangle-to-rectangle-intersection + return ( + crA.left <= crB.right && + crB.left <= crA.right && + crA.top <= crB.bottom && + crB.top <= crA.bottom + ); +} + +/** + * @param {LH.Artifacts.ClientRect[]} clientRects + * @returns {LH.Artifacts.ClientRect[]} + */ +function mergeTouchingClientRects(clientRects) { + for (let i = 0; i < clientRects.length; i++) { + for (let j = i + 1; j < clientRects.length; j++) { + const crA = clientRects[i]; + const crB = clientRects[j]; + + /** + * Examples of what we want to merge: + * + * AAABBB + * + * AAA + * AAA + * BBBBB + */ + const rectsLineUpHorizontally = + almostEqual(crA.top, crB.top) || almostEqual(crA.bottom, crB.bottom); + const rectsLineUpVertically = + almostEqual(crA.left, crB.left) || almostEqual(crA.right, crB.right); + const canMerge = + clientRectsTouch(crA, crB) && + (rectsLineUpHorizontally || rectsLineUpVertically); + + + if (canMerge) { + // create rect that contains both crA and crB + const left = Math.min(crA.left, crB.left); + const right = Math.max(crA.right, crB.right); + const top = Math.min(crA.top, crB.top); + const bottom = Math.max(crA.bottom, crB.bottom); + const replacementClientRect = addRectWidthAndHeight({ + left, + right, + top, + bottom, + }); + + const mergedRectCenter = getRectCenterPoint(replacementClientRect); + if ( + !( + rectContainsPoint(crA, mergedRectCenter) || + rectContainsPoint(crB, mergedRectCenter) + ) + ) { + // Don't merge because the new shape is too different from the + // merged rects, and tapping in the middle wouldn't actually hit + // either rect + continue; + } + + clientRects = clientRects.filter(cr => cr !== crA && cr !== crB); + clientRects.push(replacementClientRect); + + return mergeTouchingClientRects(clientRects); + } + } + } + + return clientRects; +} + +/** + * @param {{left:number, top:number, right:number, bottom: number}} rect + * @return {LH.Artifacts.ClientRect} + */ +function addRectWidthAndHeight({left, top, right, bottom}) { + return { + left, + top, + right, + bottom, + width: right - left, + height: bottom - top, + }; +} + +/** + * @param {LH.Artifacts.ClientRect} rect1 + * @param {LH.Artifacts.ClientRect} rect2 + */ +function getRectXOverlap(rect1, rect2) { + // https:// stackoverflow.com/a/9325084/1290545 + return Math.max( + 0, + Math.min(rect1.right, rect2.right) - Math.max(rect1.left, rect2.left) + ); +} + +/** + * @param {LH.Artifacts.ClientRect} rect1 + * @param {LH.Artifacts.ClientRect} 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) + ); +} + +/** + * @param {LH.Artifacts.ClientRect} rect1 + * @param {LH.Artifacts.ClientRect} rect2 + */ +function getRectOverlapArea(rect1, rect2) { + return getRectXOverlap(rect1, rect2) * getRectYOverlap(rect1, rect2); +} + + +/** + * @param {LH.Artifacts.ClientRect} clientRect + * @param {number} fingerSize + */ +function getFingerAtCenter(clientRect, fingerSize) { + return addRectWidthAndHeight({ + left: clientRect.left + clientRect.width / 2 - fingerSize / 2, + top: clientRect.top + clientRect.height / 2 - fingerSize / 2, + right: clientRect.right - clientRect.width / 2 + fingerSize / 2, + bottom: clientRect.bottom - clientRect.height / 2 + fingerSize / 2, + }); +} + +/** + * @param {LH.Artifacts.ClientRect} cr + */ +function getClientRectArea(cr) { + return cr.width * cr.height; +} + +/** + * @param {LH.Artifacts.ClientRect[]} clientRects + */ +function getLargestClientRect(clientRects) { + let largestCr = clientRects[0]; + for (const cr of clientRects) { + if (getClientRectArea(cr) > getClientRectArea(largestCr)) { + largestCr = cr; + } + } + return largestCr; +} + +/** + * + * @param {LH.Artifacts.ClientRect[]} crListA + * @param {LH.Artifacts.ClientRect[]} crListB + */ +function allClientRectsContainedWithinEachOther(crListA, crListB) { + for (const crA of crListA) { + for (const crB of crListB) { + if (!rectContains(crA, crB) && !rectContains(crB, crA)) { + return false; + } + } + } + return true; +} + +module.exports = { + rectContains, + rectContainsString, + simplifyClientRects, + addRectWidthAndHeight, + getRectXOverlap, + getRectYOverlap, + getRectOverlapArea, + getFingerAtCenter, + getLargestClientRect, + allClientRectsContainedWithinEachOther, +}; diff --git a/lighthouse-core/test/lib/client-rect-functions-test.js b/lighthouse-core/test/lib/client-rect-functions-test.js new file mode 100644 index 000000000000..d3bcee72d9e4 --- /dev/null +++ b/lighthouse-core/test/lib/client-rect-functions-test.js @@ -0,0 +1,245 @@ +/** + * @license Copyright 2018 Google Inc. All Rights Reserved. + * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. You may obtain a copy of the License at http://www.apache.org/licenses/LICENSE-2.0 + * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License. + */ +'use strict'; + +/* eslint-env jest */ + +const {simplifyClientRects, getRectOverlapArea} = require('../../lib/client-rect-functions'); +const assert = require('assert'); + +function makeClientRect({x, y, width, height}) { + return { + left: x, + top: y, + right: x + width, + bottom: y + height, + width, + height, + }; +} + +describe('simplifyClientRects', () => { + it('Merges rects if a smaller rect is inside a larger one', () => { + const res = simplifyClientRects([ + makeClientRect({ + x: 10, + y: 10, + width: 100, + height: 10, + }), + makeClientRect({ + x: 10, + y: 10, + width: 50, + height: 10, + }), + ]); + assert.deepEqual(res, [ + makeClientRect({ + x: 10, + y: 10, + width: 100, + height: 10, + }), + ]); + }); + it('Merges two horizontally adjacent client rects', () => { + const res = simplifyClientRects([ + makeClientRect({ + x: 10, + y: 10, + width: 100, + height: 10, + }), + makeClientRect({ + x: 110, + y: 10, + width: 100, + height: 10, + }), + ]); + assert.deepEqual(res, [ + makeClientRect({ + x: 10, + y: 10, + width: 200, + height: 10, + }), + ]); + }); + + it('Merges three horizontally adjacent client rects', () => { + const res = simplifyClientRects([ + makeClientRect({ + x: 10, + y: 10, + width: 100, + height: 10, + }), + makeClientRect({ + x: 110, + y: 10, + width: 100, + height: 10, + }), + makeClientRect({ + x: 210, + y: 10, + width: 100, + height: 10, + }), + ]); + assert.deepEqual(res, [ + makeClientRect({ + x: 10, + y: 10, + width: 300, + height: 10, + }), + ]); + }); + + it('Merges two vertically adjacent client rects even if one is wider than the other', () => { + // We do this because to fix issues with children (e.g. images) inside links. + // The link itself might be small, so if we put a finger on it directly then it's + // likely to overlap with something. + const res = simplifyClientRects([ + makeClientRect({ + x: 10, + y: 10, + width: 100, + height: 10, + }), + makeClientRect({ + x: 10, + y: 15, + width: 200, + height: 15, + }), + ]); + assert.deepEqual(res, [ + makeClientRect({ + x: 10, + y: 10, + width: 200, + height: 20, + }), + ]); + }); + + it('Does not merge if the center of the merged rect wouldn\'t be in the original rects', () => { + const res = simplifyClientRects([ + makeClientRect({ + x: 10, + y: 10, + width: 10, + height: 100, + }), + makeClientRect({ + x: 10, + y: 10, + width: 200, + height: 10, + }), + ]); + assert.equal(res.length, 2); + }); + + it('Merges two horizontally adjacent client rects that don\'t line up exactly', () => { + // 2px difference is ok, often there are cases where an image is a 1px or 2px out of the main link client rect + const res = simplifyClientRects([ + makeClientRect({ + x: 10, + y: 10, + width: 100, + height: 10, + }), + makeClientRect({ + x: 110, + y: 12, + width: 100, + height: 10, + }), + ]); + assert.deepEqual(res, [ + makeClientRect({ + x: 10, + y: 10, + width: 200, + height: 12, + }), + ]); + }); + + it('Merges two identical client rects into one', () => { + const res = simplifyClientRects([ + makeClientRect({ + x: 10, + y: 10, + width: 10, + height: 10, + }), + makeClientRect({ + x: 10, + y: 10, + width: 10, + height: 10, + }), + ]); + assert.deepEqual(res, [ + makeClientRect({ + x: 10, + y: 10, + width: 10, + height: 10, + }), + ]); + }); + + it('Removes tiny 1x1px client rects', () => { + const res = simplifyClientRects([ + makeClientRect({ + x: 10, + y: 10, + width: 100, + height: 100, + }), + makeClientRect({ + x: 5, + y: 5, + width: 1, + height: 1, + }), + ]); + assert.deepEqual(res, [ + makeClientRect({ + x: 10, + y: 10, + width: 100, + height: 100, + }), + ]); + }); +}); + +describe('getRectOverlapArea', () => { + it('Works in a basic example', () => { + const overlapArea = getRectOverlapArea( + makeClientRect({ + x: 0, + y: 0, + width: 10, + height: 10, + }), makeClientRect({ + x: 8, + y: 6, + width: 10, + height: 10, + }) + ); + expect(overlapArea).toBe(8); + }); +}); diff --git a/types/artifacts.d.ts b/types/artifacts.d.ts index aebaa284d310..1789d83a06b7 100644 --- a/types/artifacts.d.ts +++ b/types/artifacts.d.ts @@ -278,6 +278,15 @@ declare global { }; } + export interface ClientRect { + width: number; + height: number; + top: number; + right: number; + bottom: number; + left: number; + } + export interface ViewportDimensions { innerWidth: number; innerHeight: number; From 5ad1ab2fd3ebd8f24ab1c65611a4a585ba753503 Mon Sep 17 00:00:00 2001 From: Matt Zeunert Date: Wed, 5 Dec 2018 18:19:42 +0000 Subject: [PATCH 02/11] Address review feedback --- lighthouse-core/lib/client-rect-functions.js | 27 ++++++++++++-------- 1 file changed, 17 insertions(+), 10 deletions(-) diff --git a/lighthouse-core/lib/client-rect-functions.js b/lighthouse-core/lib/client-rect-functions.js index 8a6785cb1c11..616e8bc1cb24 100644 --- a/lighthouse-core/lib/client-rect-functions.js +++ b/lighthouse-core/lib/client-rect-functions.js @@ -9,6 +9,8 @@ * @param {LH.Artifacts.ClientRect} cr * @param {{x:number, y:number}} point */ +// We sometimes run this as a part of a gatherer script injected into the page, prevent +// renaming the function for code coverage. /* istanbul ignore next */ function rectContainsPoint(cr, {x, y}) { return cr.left <= x && cr.right >= x && cr.top <= y && cr.bottom >= y; @@ -18,6 +20,8 @@ function rectContainsPoint(cr, {x, y}) { * @param {LH.Artifacts.ClientRect} cr1 * @param {LH.Artifacts.ClientRect} cr2 */ +// We sometimes run this as a part of a gatherer script injected into the page, prevent +// renaming the function for code coverage. /* istanbul ignore next */ function rectContains(cr1, cr2) { const topLeft = { @@ -45,7 +49,8 @@ function rectContains(cr1, cr2) { } /** - * Merge client rects together. This may result in a larger overall size than that of the individual client rects. + * Merge client rects together and remove small ones. This may result in a larger overall + * size than that of the individual client rects. * @param {LH.Artifacts.ClientRect[]} clientRects */ function simplifyClientRects(clientRects) { @@ -65,10 +70,12 @@ function filterOutTinyClientRects(clientRects) { const nonTinyClientRects = clientRects.filter( rect => rect.width > 1 && rect.height > 1 ); - if (nonTinyClientRects.length > 0) { - return nonTinyClientRects; + if (nonTinyClientRects.length === 0) { + // If all client rects are tiny don't remove them, so later in the code we don't + // need to deal with elements that don't have client rects. + return clientRects; } - return clientRects; + return nonTinyClientRects; } const rectContainsString = ` @@ -125,7 +132,7 @@ function getRectCenterPoint(rect) { * @param {LH.Artifacts.ClientRect} crB * @returns {boolean} */ -function clientRectsTouch(crA, crB) { +function clientRectsTouchOrOverlap(crA, crB) { // https://stackoverflow.com/questions/2752349/fast-rectangle-to-rectangle-intersection return ( crA.left <= crB.right && @@ -159,7 +166,7 @@ function mergeTouchingClientRects(clientRects) { const rectsLineUpVertically = almostEqual(crA.left, crB.left) || almostEqual(crA.right, crB.right); const canMerge = - clientRectsTouch(crA, crB) && + clientRectsTouchOrOverlap(crA, crB) && (rectsLineUpHorizontally || rectsLineUpVertically); @@ -220,7 +227,7 @@ function addRectWidthAndHeight({left, top, right, bottom}) { * @param {LH.Artifacts.ClientRect} rect2 */ function getRectXOverlap(rect1, rect2) { - // https:// stackoverflow.com/a/9325084/1290545 + // https://stackoverflow.com/a/9325084/1290545 return Math.max( 0, Math.min(rect1.right, rect2.right) - Math.max(rect1.left, rect2.left) @@ -232,7 +239,7 @@ function getRectXOverlap(rect1, rect2) { * @param {LH.Artifacts.ClientRect} rect2 */ function getRectYOverlap(rect1, rect2) { - // https:// stackoverflow.com/a/9325084/1290545 + // https://stackoverflow.com/a/9325084/1290545 return Math.max( 0, Math.min(rect1.bottom, rect2.bottom) - Math.max(rect1.top, rect2.top) @@ -252,7 +259,7 @@ function getRectOverlapArea(rect1, rect2) { * @param {LH.Artifacts.ClientRect} clientRect * @param {number} fingerSize */ -function getFingerAtCenter(clientRect, fingerSize) { +function getRectAtCenter(clientRect, fingerSize) { return addRectWidthAndHeight({ left: clientRect.left + clientRect.width / 2 - fingerSize / 2, top: clientRect.top + clientRect.height / 2 - fingerSize / 2, @@ -305,7 +312,7 @@ module.exports = { getRectXOverlap, getRectYOverlap, getRectOverlapArea, - getFingerAtCenter, + getRectAtCenter, getLargestClientRect, allClientRectsContainedWithinEachOther, }; From c2221feec527de01371019f4d0885484cadf4944 Mon Sep 17 00:00:00 2001 From: Matt Zeunert Date: Fri, 7 Dec 2018 09:17:43 +0000 Subject: [PATCH 03/11] Clean up filterOutClientRectsContainedByOthers --- lighthouse-core/lib/client-rect-functions.js | 23 +++++++-------- .../test/lib/client-rect-functions-test.js | 28 +++++++++++++++++-- 2 files changed, 36 insertions(+), 15 deletions(-) diff --git a/lighthouse-core/lib/client-rect-functions.js b/lighthouse-core/lib/client-rect-functions.js index 616e8bc1cb24..852fe2aff8d6 100644 --- a/lighthouse-core/lib/client-rect-functions.js +++ b/lighthouse-core/lib/client-rect-functions.js @@ -88,23 +88,20 @@ const rectContainsString = ` * @returns {LH.Artifacts.ClientRect[]} */ function filterOutClientRectsContainedByOthers(clientRects) { - const filteredOutRects = new Set(); - return clientRects.filter(cr => { + const rectsToKeep = new Set(clientRects); + + for (const cr of clientRects) { for (const possiblyContainingRect of clientRects) { - if (possiblyContainingRect === cr) { - continue; - } - if (filteredOutRects.has(possiblyContainingRect)) { - // Can't contain anything because we removed it from the list - continue; - } + if (cr === possiblyContainingRect) continue; + if (!rectsToKeep.has(possiblyContainingRect)) continue; if (rectContains(possiblyContainingRect, cr)) { - filteredOutRects.add(cr); - return false; + rectsToKeep.delete(cr); + break; } } - return true; - }); + } + + return Array.from(rectsToKeep); } /** diff --git a/lighthouse-core/test/lib/client-rect-functions-test.js b/lighthouse-core/test/lib/client-rect-functions-test.js index d3bcee72d9e4..b90b02a32ee8 100644 --- a/lighthouse-core/test/lib/client-rect-functions-test.js +++ b/lighthouse-core/test/lib/client-rect-functions-test.js @@ -22,20 +22,44 @@ function makeClientRect({x, y, width, height}) { } describe('simplifyClientRects', () => { + it('Merges rects if a smaller rect is inside a larger one', () => { + const containingRect = makeClientRect({ + x: 10, + y: 10, + width: 100, + height: 10, + }); + const containedRect = makeClientRect({ + x: 10, + y: 10, + width: 50, + height: 10, + }); + + assert.deepEqual(simplifyClientRects([ + containingRect, + containedRect, + ]), [containingRect]); + assert.deepEqual(simplifyClientRects([ + containedRect, + containingRect, + ]), [containingRect]); + }); it('Merges rects if a smaller rect is inside a larger one', () => { const res = simplifyClientRects([ makeClientRect({ x: 10, y: 10, - width: 100, + width: 50, height: 10, }), makeClientRect({ x: 10, y: 10, - width: 50, + width: 100, height: 10, }), + ]); assert.deepEqual(res, [ makeClientRect({ From 7c78888474151fb1252334daf9617383bcfb6817 Mon Sep 17 00:00:00 2001 From: Matt Zeunert Date: Fri, 7 Dec 2018 09:21:50 +0000 Subject: [PATCH 04/11] Add comments --- lighthouse-core/lib/client-rect-functions.js | 3 +++ 1 file changed, 3 insertions(+) diff --git a/lighthouse-core/lib/client-rect-functions.js b/lighthouse-core/lib/client-rect-functions.js index 852fe2aff8d6..a3872bc41281 100644 --- a/lighthouse-core/lib/client-rect-functions.js +++ b/lighthouse-core/lib/client-rect-functions.js @@ -193,9 +193,12 @@ function mergeTouchingClientRects(clientRects) { continue; } + // Replace client rects with merged version clientRects = clientRects.filter(cr => cr !== crA && cr !== crB); clientRects.push(replacementClientRect); + // Start over so we don't have to handle complexity introduced by array mutation. + // Client rect ararys rarely contain more than 5 rects, so starting again doesn't cause perf issues. return mergeTouchingClientRects(clientRects); } } From c1a044b99d44eea73f8bd9ac628cb8c47eb25b9f Mon Sep 17 00:00:00 2001 From: Matt Zeunert Date: Fri, 7 Dec 2018 09:25:53 +0000 Subject: [PATCH 05/11] Inline rects --- lighthouse-core/lib/client-rect-functions.js | 44 ++++++++++---------- 1 file changed, 22 insertions(+), 22 deletions(-) diff --git a/lighthouse-core/lib/client-rect-functions.js b/lighthouse-core/lib/client-rect-functions.js index a3872bc41281..64ed8b4825f3 100644 --- a/lighthouse-core/lib/client-rect-functions.js +++ b/lighthouse-core/lib/client-rect-functions.js @@ -9,7 +9,7 @@ * @param {LH.Artifacts.ClientRect} cr * @param {{x:number, y:number}} point */ -// We sometimes run this as a part of a gatherer script injected into the page, prevent +// 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(cr, {x, y}) { @@ -20,31 +20,31 @@ function rectContainsPoint(cr, {x, y}) { * @param {LH.Artifacts.ClientRect} cr1 * @param {LH.Artifacts.ClientRect} cr2 */ -// We sometimes run this as a part of a gatherer script injected into the page, prevent +// 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(cr1, cr2) { - const topLeft = { - x: cr2.left, - y: cr2.top, - }; - const topRight = { - x: cr2.right, - y: cr2.top, - }; - const bottomLeft = { - x: cr2.left, - y: cr2.bottom, - }; - const bottomRight = { - x: cr2.right, - y: cr2.bottom, - }; return ( - rectContainsPoint(cr1, topLeft) && - rectContainsPoint(cr1, topRight) && - rectContainsPoint(cr1, bottomLeft) && - rectContainsPoint(cr1, bottomRight) + // top left corner + rectContainsPoint(cr1, { + x: cr2.left, + y: cr2.top, + }) && + // top right corner + rectContainsPoint(cr1, { + x: cr2.right, + y: cr2.top, + }) && + // bottom left corner + rectContainsPoint(cr1, { + x: cr2.left, + y: cr2.bottom, + }) && + // bottom right corner + rectContainsPoint(cr1, { + x: cr2.right, + y: cr2.bottom, + }) ); } From 95000ab0ff2f3ba85870bbadde4e7c85bf5aa63b Mon Sep 17 00:00:00 2001 From: Matt Zeunert Date: Fri, 7 Dec 2018 09:27:55 +0000 Subject: [PATCH 06/11] Update client-rect-functions.js --- lighthouse-core/lib/client-rect-functions.js | 31 ++++++++++++-------- 1 file changed, 19 insertions(+), 12 deletions(-) diff --git a/lighthouse-core/lib/client-rect-functions.js b/lighthouse-core/lib/client-rect-functions.js index 64ed8b4825f3..f058ecae0388 100644 --- a/lighthouse-core/lib/client-rect-functions.js +++ b/lighthouse-core/lib/client-rect-functions.js @@ -139,6 +139,23 @@ function clientRectsTouchOrOverlap(crA, crB) { ); } +/** + * @param {LH.Artifacts.ClientRect} crA + * @param {LH.Artifacts.ClientRect} crB + */ +function getBoundingRect(crA, crB) { + const left = Math.min(crA.left, crB.left); + const right = Math.max(crA.right, crB.right); + const top = Math.min(crA.top, crB.top); + const bottom = Math.max(crA.bottom, crB.bottom); + return addRectWidthAndHeight({ + left, + right, + top, + bottom, + }); +} + /** * @param {LH.Artifacts.ClientRect[]} clientRects * @returns {LH.Artifacts.ClientRect[]} @@ -168,19 +185,9 @@ function mergeTouchingClientRects(clientRects) { if (canMerge) { - // create rect that contains both crA and crB - const left = Math.min(crA.left, crB.left); - const right = Math.max(crA.right, crB.right); - const top = Math.min(crA.top, crB.top); - const bottom = Math.max(crA.bottom, crB.bottom); - const replacementClientRect = addRectWidthAndHeight({ - left, - right, - top, - bottom, - }); - + const replacementClientRect = getBoundingRect(crA, crB); const mergedRectCenter = getRectCenterPoint(replacementClientRect); + if ( !( rectContainsPoint(crA, mergedRectCenter) || From 43c62593d5c87f42474f7ae7a58f4fdcfe0307c3 Mon Sep 17 00:00:00 2001 From: Matt Zeunert Date: Fri, 7 Dec 2018 14:16:41 +0000 Subject: [PATCH 07/11] Add test --- .../test/lib/client-rect-functions-test.js | 34 +++++++++++-------- 1 file changed, 20 insertions(+), 14 deletions(-) diff --git a/lighthouse-core/test/lib/client-rect-functions-test.js b/lighthouse-core/test/lib/client-rect-functions-test.js index b90b02a32ee8..44a8a3b80bc1 100644 --- a/lighthouse-core/test/lib/client-rect-functions-test.js +++ b/lighthouse-core/test/lib/client-rect-functions-test.js @@ -45,32 +45,32 @@ describe('simplifyClientRects', () => { containingRect, ]), [containingRect]); }); - it('Merges rects if a smaller rect is inside a larger one', () => { + it('Merges two horizontally adjacent client rects', () => { const res = simplifyClientRects([ makeClientRect({ x: 10, y: 10, - width: 50, + width: 100, height: 10, }), makeClientRect({ - x: 10, + x: 110, y: 10, width: 100, height: 10, }), - ]); assert.deepEqual(res, [ makeClientRect({ x: 10, y: 10, - width: 100, + width: 200, height: 10, }), ]); }); - it('Merges two horizontally adjacent client rects', () => { + + it('Merges three horizontally adjacent client rects', () => { const res = simplifyClientRects([ makeClientRect({ x: 10, @@ -84,35 +84,41 @@ describe('simplifyClientRects', () => { width: 100, height: 10, }), + makeClientRect({ + x: 210, + y: 10, + width: 100, + height: 10, + }), ]); assert.deepEqual(res, [ makeClientRect({ x: 10, y: 10, - width: 200, + width: 300, height: 10, }), ]); }); - it('Merges three horizontally adjacent client rects', () => { + it('Merges client rects correctly if one is duplicated', () => { const res = simplifyClientRects([ makeClientRect({ x: 10, y: 10, - width: 100, + width: 90, height: 10, }), makeClientRect({ - x: 110, + x: 10, y: 10, - width: 100, + width: 90, height: 10, }), makeClientRect({ - x: 210, + x: 100, y: 10, - width: 100, + width: 10, height: 10, }), ]); @@ -120,7 +126,7 @@ describe('simplifyClientRects', () => { makeClientRect({ x: 10, y: 10, - width: 300, + width: 100, height: 10, }), ]); From 3d1097a54d912ecb3160788cafb258f0fbc53fd3 Mon Sep 17 00:00:00 2001 From: Matt Zeunert Date: Fri, 7 Dec 2018 14:59:27 +0000 Subject: [PATCH 08/11] Split rect helpers and tap targets specific code --- lighthouse-core/lib/client-rect-functions.js | 325 ------------------ lighthouse-core/lib/rect-helpers.js | 257 ++++++++++++++ lighthouse-core/lib/tappable-rects.js | 100 ++++++ lighthouse-core/test/lib/rect-helpers-test.js | 87 +++++ ...est.js => tap-target-rect-helpers-test.js} | 109 +++--- types/artifacts.d.ts | 2 +- 6 files changed, 485 insertions(+), 395 deletions(-) delete mode 100644 lighthouse-core/lib/client-rect-functions.js create mode 100644 lighthouse-core/lib/rect-helpers.js create mode 100644 lighthouse-core/lib/tappable-rects.js create mode 100644 lighthouse-core/test/lib/rect-helpers-test.js rename lighthouse-core/test/lib/{client-rect-functions-test.js => tap-target-rect-helpers-test.js} (71%) diff --git a/lighthouse-core/lib/client-rect-functions.js b/lighthouse-core/lib/client-rect-functions.js deleted file mode 100644 index f058ecae0388..000000000000 --- a/lighthouse-core/lib/client-rect-functions.js +++ /dev/null @@ -1,325 +0,0 @@ -/** - * @license Copyright 2018 Google Inc. All Rights Reserved. - * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. You may obtain a copy of the License at http://www.apache.org/licenses/LICENSE-2.0 - * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License. - */ -'use strict'; - -/** - * @param {LH.Artifacts.ClientRect} cr - * @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(cr, {x, y}) { - return cr.left <= x && cr.right >= x && cr.top <= y && cr.bottom >= y; -} - -/** - * @param {LH.Artifacts.ClientRect} cr1 - * @param {LH.Artifacts.ClientRect} cr2 - */ -// 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(cr1, cr2) { - return ( - // top left corner - rectContainsPoint(cr1, { - x: cr2.left, - y: cr2.top, - }) && - // top right corner - rectContainsPoint(cr1, { - x: cr2.right, - y: cr2.top, - }) && - // bottom left corner - rectContainsPoint(cr1, { - x: cr2.left, - y: cr2.bottom, - }) && - // bottom right corner - rectContainsPoint(cr1, { - x: cr2.right, - y: cr2.bottom, - }) - ); -} - -/** - * Merge client rects together and remove small ones. This may result in a larger overall - * size than that of the individual client rects. - * @param {LH.Artifacts.ClientRect[]} clientRects - */ -function simplifyClientRects(clientRects) { - clientRects = filterOutTinyClientRects(clientRects); - clientRects = filterOutClientRectsContainedByOthers(clientRects); - clientRects = mergeTouchingClientRects(clientRects); - return clientRects; -} - -/** - * @param {LH.Artifacts.ClientRect[]} clientRects - * @returns {LH.Artifacts.ClientRect[]} - */ -function filterOutTinyClientRects(clientRects) { - // 1x1px rect shouldn't be reason to treat the rect as something the user should tap on. - // Often they're made invisble in some obscure way anyway, and only exist for e.g. accessibiliity. - const nonTinyClientRects = clientRects.filter( - rect => rect.width > 1 && rect.height > 1 - ); - if (nonTinyClientRects.length === 0) { - // If all client rects are tiny don't remove them, so later in the code we don't - // need to deal with elements that don't have client rects. - return clientRects; - } - return nonTinyClientRects; -} - -const rectContainsString = ` - ${rectContainsPoint.toString()} - ${rectContains.toString()}; -`; - -/** - * @param {LH.Artifacts.ClientRect[]} clientRects - * @returns {LH.Artifacts.ClientRect[]} - */ -function filterOutClientRectsContainedByOthers(clientRects) { - const rectsToKeep = new Set(clientRects); - - for (const cr of clientRects) { - for (const possiblyContainingRect of clientRects) { - if (cr === possiblyContainingRect) continue; - if (!rectsToKeep.has(possiblyContainingRect)) continue; - if (rectContains(possiblyContainingRect, cr)) { - rectsToKeep.delete(cr); - break; - } - } - } - - return Array.from(rectsToKeep); -} - -/** - * @param {number} a - * @param {number} b - */ -function almostEqual(a, b) { - // Sometimes a child will reach out of the parent by - // 1px or 2px, so be somewhat tolerant for merging - return Math.abs(a - b) <= 2; -} - -/** - * @param {LH.Artifacts.ClientRect} rect - */ -function getRectCenterPoint(rect) { - return { - x: rect.left + rect.width / 2, - y: rect.top + rect.height / 2, - }; -} - -/** - * @param {LH.Artifacts.ClientRect} crA - * @param {LH.Artifacts.ClientRect} crB - * @returns {boolean} - */ -function clientRectsTouchOrOverlap(crA, crB) { - // https://stackoverflow.com/questions/2752349/fast-rectangle-to-rectangle-intersection - return ( - crA.left <= crB.right && - crB.left <= crA.right && - crA.top <= crB.bottom && - crB.top <= crA.bottom - ); -} - -/** - * @param {LH.Artifacts.ClientRect} crA - * @param {LH.Artifacts.ClientRect} crB - */ -function getBoundingRect(crA, crB) { - const left = Math.min(crA.left, crB.left); - const right = Math.max(crA.right, crB.right); - const top = Math.min(crA.top, crB.top); - const bottom = Math.max(crA.bottom, crB.bottom); - return addRectWidthAndHeight({ - left, - right, - top, - bottom, - }); -} - -/** - * @param {LH.Artifacts.ClientRect[]} clientRects - * @returns {LH.Artifacts.ClientRect[]} - */ -function mergeTouchingClientRects(clientRects) { - for (let i = 0; i < clientRects.length; i++) { - for (let j = i + 1; j < clientRects.length; j++) { - const crA = clientRects[i]; - const crB = clientRects[j]; - - /** - * Examples of what we want to merge: - * - * AAABBB - * - * AAA - * AAA - * BBBBB - */ - const rectsLineUpHorizontally = - almostEqual(crA.top, crB.top) || almostEqual(crA.bottom, crB.bottom); - const rectsLineUpVertically = - almostEqual(crA.left, crB.left) || almostEqual(crA.right, crB.right); - const canMerge = - clientRectsTouchOrOverlap(crA, crB) && - (rectsLineUpHorizontally || rectsLineUpVertically); - - - if (canMerge) { - const replacementClientRect = getBoundingRect(crA, crB); - const mergedRectCenter = getRectCenterPoint(replacementClientRect); - - if ( - !( - rectContainsPoint(crA, mergedRectCenter) || - rectContainsPoint(crB, mergedRectCenter) - ) - ) { - // Don't merge because the new shape is too different from the - // merged rects, and tapping in the middle wouldn't actually hit - // either rect - continue; - } - - // Replace client rects with merged version - clientRects = clientRects.filter(cr => cr !== crA && cr !== crB); - clientRects.push(replacementClientRect); - - // Start over so we don't have to handle complexity introduced by array mutation. - // Client rect ararys rarely contain more than 5 rects, so starting again doesn't cause perf issues. - return mergeTouchingClientRects(clientRects); - } - } - } - - return clientRects; -} - -/** - * @param {{left:number, top:number, right:number, bottom: number}} rect - * @return {LH.Artifacts.ClientRect} - */ -function addRectWidthAndHeight({left, top, right, bottom}) { - return { - left, - top, - right, - bottom, - width: right - left, - height: bottom - top, - }; -} - -/** - * @param {LH.Artifacts.ClientRect} rect1 - * @param {LH.Artifacts.ClientRect} rect2 - */ -function getRectXOverlap(rect1, rect2) { - // https://stackoverflow.com/a/9325084/1290545 - return Math.max( - 0, - Math.min(rect1.right, rect2.right) - Math.max(rect1.left, rect2.left) - ); -} - -/** - * @param {LH.Artifacts.ClientRect} rect1 - * @param {LH.Artifacts.ClientRect} 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) - ); -} - -/** - * @param {LH.Artifacts.ClientRect} rect1 - * @param {LH.Artifacts.ClientRect} rect2 - */ -function getRectOverlapArea(rect1, rect2) { - return getRectXOverlap(rect1, rect2) * getRectYOverlap(rect1, rect2); -} - - -/** - * @param {LH.Artifacts.ClientRect} clientRect - * @param {number} fingerSize - */ -function getRectAtCenter(clientRect, fingerSize) { - return addRectWidthAndHeight({ - left: clientRect.left + clientRect.width / 2 - fingerSize / 2, - top: clientRect.top + clientRect.height / 2 - fingerSize / 2, - right: clientRect.right - clientRect.width / 2 + fingerSize / 2, - bottom: clientRect.bottom - clientRect.height / 2 + fingerSize / 2, - }); -} - -/** - * @param {LH.Artifacts.ClientRect} cr - */ -function getClientRectArea(cr) { - return cr.width * cr.height; -} - -/** - * @param {LH.Artifacts.ClientRect[]} clientRects - */ -function getLargestClientRect(clientRects) { - let largestCr = clientRects[0]; - for (const cr of clientRects) { - if (getClientRectArea(cr) > getClientRectArea(largestCr)) { - largestCr = cr; - } - } - return largestCr; -} - -/** - * - * @param {LH.Artifacts.ClientRect[]} crListA - * @param {LH.Artifacts.ClientRect[]} crListB - */ -function allClientRectsContainedWithinEachOther(crListA, crListB) { - for (const crA of crListA) { - for (const crB of crListB) { - if (!rectContains(crA, crB) && !rectContains(crB, crA)) { - return false; - } - } - } - return true; -} - -module.exports = { - rectContains, - rectContainsString, - simplifyClientRects, - addRectWidthAndHeight, - getRectXOverlap, - getRectYOverlap, - getRectOverlapArea, - getRectAtCenter, - getLargestClientRect, - allClientRectsContainedWithinEachOther, -}; diff --git a/lighthouse-core/lib/rect-helpers.js b/lighthouse-core/lib/rect-helpers.js new file mode 100644 index 000000000000..d82c7b995049 --- /dev/null +++ b/lighthouse-core/lib/rect-helpers.js @@ -0,0 +1,257 @@ +/** + * @license Copyright 2018 Google Inc. All Rights Reserved. + * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. You may obtain a copy of the License at http://www.apache.org/licenses/LICENSE-2.0 + * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License. + */ +'use strict'; + +/** + * @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; +} + +/** + * @param {LH.Artifacts.Rect} rect1 + * @param {LH.Artifacts.Rect} rect2 + */ +// 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, + }) + ); +} + +/** + * @param {LH.Artifacts.Rect[]} rects + * @returns {LH.Artifacts.Rect[]} + */ +function filterOutTinyRects(rects) { + return rects.filter( + rect => rect.width > 1 && rect.height > 1 + ); +} + +const rectContainsString = ` + ${rectContainsPoint.toString()} + ${rectContains.toString()}; +`; + +/** + * @param {LH.Artifacts.Rect[]} rects + * @returns {LH.Artifacts.Rect[]} + */ +function filterOutRectsContainedByOthers(rects) { + const rectsToKeep = new Set(rects); + + for (const rect of rects) { + for (const possiblyContainingRect of rects) { + if (rect === possiblyContainingRect) continue; + if (!rectsToKeep.has(possiblyContainingRect)) continue; + if (rectContains(possiblyContainingRect, rect)) { + rectsToKeep.delete(rect); + break; + } + } + } + + return Array.from(rectsToKeep); +} + +/** + * @param {LH.Artifacts.Rect} rect + */ +function getRectCenterPoint(rect) { + return { + x: rect.left + rect.width / 2, + y: rect.top + rect.height / 2, + }; +} + +/** + * @param {LH.Artifacts.Rect} rectA + * @param {LH.Artifacts.Rect} rectB + * @returns {boolean} + */ +function rectsTouchOrOverlap(rectA, rectB) { + // https://stackoverflow.com/questions/2752349/fast-rectangle-to-rectangle-intersection + return ( + rectA.left <= rectB.right && + rectB.left <= rectA.right && + rectA.top <= rectB.bottom && + rectB.top <= rectA.bottom + ); +} + +/** + * @param {LH.Artifacts.Rect} rectA + * @param {LH.Artifacts.Rect} rectB + */ +function getBoundingRect(rectA, rectB) { + const left = Math.min(rectA.left, rectB.left); + const right = Math.max(rectA.right, rectB.right); + const top = Math.min(rectA.top, rectB.top); + const bottom = Math.max(rectA.bottom, rectB.bottom); + return addRectWidthAndHeight({ + left, + right, + top, + bottom, + }); +} + +/** + * @param {{left:number, top:number, right:number, bottom: number}} rect + * @return {LH.Artifacts.Rect} + */ +function addRectWidthAndHeight({left, top, right, bottom}) { + return { + left, + top, + right, + bottom, + width: right - left, + height: bottom - top, + }; +} + +/** + * @param {{x:number, y:number, width:number, height: number}} rect + * @return {LH.Artifacts.Rect} + */ +function addRectTopAndBottom({x, y, width, height}) { + return { + left: x, + top: y, + right: x + width, + bottom: y + height, + width, + height, + }; +} + +/** + * @param {LH.Artifacts.Rect} rect1 + * @param {LH.Artifacts.Rect} rect2 + */ +function getRectXOverlap(rect1, rect2) { + // https://stackoverflow.com/a/9325084/1290545 + return Math.max( + 0, + Math.min(rect1.right, rect2.right) - Math.max(rect1.left, rect2.left) + ); +} + +/** + * @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) + ); +} + +/** + * @param {LH.Artifacts.Rect} rect1 + * @param {LH.Artifacts.Rect} rect2 + */ +function getRectOverlapArea(rect1, rect2) { + return getRectXOverlap(rect1, rect2) * getRectYOverlap(rect1, rect2); +} + +/** + * @param {LH.Artifacts.Rect} rect + * @param {number} centerRectSize + */ +function getRectAtCenter(rect, centerRectSize) { + return addRectWidthAndHeight({ + left: rect.left + rect.width / 2 - centerRectSize / 2, + top: rect.top + rect.height / 2 - centerRectSize / 2, + right: rect.right - rect.width / 2 + centerRectSize / 2, + bottom: rect.bottom - rect.height / 2 + centerRectSize / 2, + }); +} + +/** + * @param {LH.Artifacts.Rect} rect + */ +function getRectArea(rect) { + return rect.width * rect.height; +} + +/** + * @param {LH.Artifacts.Rect[]} rects + */ +function getLargestRect(rects) { + let largestRect = rects[0]; + for (const rect of rects) { + if (getRectArea(rect) > getRectArea(largestRect)) { + largestRect = rect; + } + } + return largestRect; +} + +/** + * + * @param {LH.Artifacts.Rect[]} rectListA + * @param {LH.Artifacts.Rect[]} rectListB + */ +function allRectsContainedWithinEachOther(rectListA, rectListB) { + for (const rectA of rectListA) { + for (const rectB of rectListB) { + if (!rectContains(rectA, rectB) && !rectContains(rectB, rectA)) { + return false; + } + } + } + return true; +} + +module.exports = { + rectContainsPoint, + rectContains, + rectContainsString, + addRectWidthAndHeight, + addRectTopAndBottom, + getRectXOverlap, + getRectYOverlap, + getRectOverlapArea, + getRectAtCenter, + getLargestRect, + getRectCenterPoint, + getBoundingRect, + rectsTouchOrOverlap, + allRectsContainedWithinEachOther, + filterOutRectsContainedByOthers, + filterOutTinyRects, +}; diff --git a/lighthouse-core/lib/tappable-rects.js b/lighthouse-core/lib/tappable-rects.js new file mode 100644 index 000000000000..2879e0f89038 --- /dev/null +++ b/lighthouse-core/lib/tappable-rects.js @@ -0,0 +1,100 @@ +/** + * @license Copyright 2018 Google Inc. All Rights Reserved. + * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. You may obtain a copy of the License at http://www.apache.org/licenses/LICENSE-2.0 + * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License. + */ +'use strict'; + +const { + filterOutRectsContainedByOthers, + filterOutTinyRects, + rectsTouchOrOverlap, + rectContainsPoint, + getBoundingRect, + getRectCenterPoint, +} = require('./rect-helpers'); + +/** + * Merge client rects together and remove small ones. This may result in a larger overall + * size than that of the individual client rects. + * @param {LH.Artifacts.Rect[]} clientRects + */ +function getTappableRectsFromClientRects(clientRects) { + // 1x1px rect shouldn't be reason to treat the rect as something the user should tap on. + // Often they're made invisble in some obscure way anyway, and only exist for e.g. accessibiliity. + clientRects = filterOutTinyRects(clientRects); + clientRects = filterOutRectsContainedByOthers(clientRects); + clientRects = mergeTouchingClientRects(clientRects); + return clientRects; +} + +/** + * @param {number} a + * @param {number} b + */ +function almostEqual(a, b) { + // Sometimes a child will reach out of the parent by + // 1px or 2px, so be somewhat tolerant for merging + return Math.abs(a - b) <= 2; +} + +/** + * @param {LH.Artifacts.Rect[]} clientRects + * @returns {LH.Artifacts.Rect[]} + */ +function mergeTouchingClientRects(clientRects) { + for (let i = 0; i < clientRects.length; i++) { + for (let j = i + 1; j < clientRects.length; j++) { + const crA = clientRects[i]; + const crB = clientRects[j]; + + /** + * Examples of what we want to merge: + * + * AAABBB + * + * AAA + * AAA + * BBBBB + */ + const rectsLineUpHorizontally = + almostEqual(crA.top, crB.top) || almostEqual(crA.bottom, crB.bottom); + const rectsLineUpVertically = + almostEqual(crA.left, crB.left) || almostEqual(crA.right, crB.right); + const canMerge = + rectsTouchOrOverlap(crA, crB) && + (rectsLineUpHorizontally || rectsLineUpVertically); + + if (canMerge) { + const replacementClientRect = getBoundingRect(crA, crB); + const mergedRectCenter = getRectCenterPoint(replacementClientRect); + + if ( + !( + rectContainsPoint(crA, mergedRectCenter) || + rectContainsPoint(crB, mergedRectCenter) + ) + ) { + // Don't merge because the new shape is too different from the + // merged rects, and tapping in the middle wouldn't actually hit + // either rect + continue; + } + + // Replace client rects with merged version + clientRects = clientRects.filter(cr => cr !== crA && cr !== crB); + clientRects.push(replacementClientRect); + + // Start over so we don't have to handle complexity introduced by array mutation. + // Client rect ararys rarely contain more than 5 rects, so starting again doesn't cause perf issues. + return mergeTouchingClientRects(clientRects); + } + } + } + + return clientRects; +} + +module.exports = { + getTappableRectsFromClientRects, +}; diff --git a/lighthouse-core/test/lib/rect-helpers-test.js b/lighthouse-core/test/lib/rect-helpers-test.js new file mode 100644 index 000000000000..22c0a19be04e --- /dev/null +++ b/lighthouse-core/test/lib/rect-helpers-test.js @@ -0,0 +1,87 @@ +/** + * @license Copyright 2018 Google Inc. All Rights Reserved. + * Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. You may obtain a copy of the License at http://www.apache.org/licenses/LICENSE-2.0 + * Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License. + */ +'use strict'; + +/* eslint-env jest */ + +const { + addRectTopAndBottom, + getRectOverlapArea, + getLargestRect, + getRectAtCenter, + allRectsContainedWithinEachOther, +} = require('../../lib/rect-helpers'); + +describe('Rect Helpers', () => { + it('getRectOverlapArea', () => { + const overlapArea = getRectOverlapArea( + addRectTopAndBottom({ + x: 0, + y: 0, + width: 10, + height: 10, + }), + addRectTopAndBottom({ + x: 8, + y: 6, + width: 10, + height: 10, + }) + ); + expect(overlapArea).toBe(8); + }); + + it('getLargestRect', () => { + const rect1 = addRectTopAndBottom({ + x: 0, + y: 0, + width: 5, + height: 5, + }); + const rect2 = addRectTopAndBottom({ + x: 0, + y: 0, + width: 10, + height: 10, + }); + const largestRect = getLargestRect([rect1, rect2]); + expect(largestRect).toBe(rect2); + }); + + it('getRectAtCenter', () => { + const rect = addRectTopAndBottom({ + x: 0, + y: 0, + width: 100, + height: 100, + }); + const largestRect = getRectAtCenter(rect, 10); + expect(largestRect).toEqual( + addRectTopAndBottom({ + x: 45, + y: 45, + width: 10, + height: 10, + }) + ); + }); + + describe('allRectsContainedWithinEachOther', () => { + it('Returns true if the rect lists are both empty', () => { + expect(allRectsContainedWithinEachOther([], [])).toBe(true); + }); + it('Returns true if rects are contained in each other', () => { + const rect1 = addRectTopAndBottom({x: 0, y: 0, width: 100, height: 100}); + const rect2 = addRectTopAndBottom({x: 40, y: 40, width: 20, height: 20}); + expect(allRectsContainedWithinEachOther([rect1], [rect2])).toBe(true); + }); + it('Returns true if rects aren\'t contained in each other', () => { + const rect1 = addRectTopAndBottom({x: 0, y: 0, width: 100, height: 100}); + const rect2 = addRectTopAndBottom({x: 200, y: 200, width: 20, height: 20}); + expect(allRectsContainedWithinEachOther([rect1], [rect2])).toBe(false); + }); + }); +}); diff --git a/lighthouse-core/test/lib/client-rect-functions-test.js b/lighthouse-core/test/lib/tap-target-rect-helpers-test.js similarity index 71% rename from lighthouse-core/test/lib/client-rect-functions-test.js rename to lighthouse-core/test/lib/tap-target-rect-helpers-test.js index 44a8a3b80bc1..030a4e39794d 100644 --- a/lighthouse-core/test/lib/client-rect-functions-test.js +++ b/lighthouse-core/test/lib/tap-target-rect-helpers-test.js @@ -7,53 +7,43 @@ /* eslint-env jest */ -const {simplifyClientRects, getRectOverlapArea} = require('../../lib/client-rect-functions'); +const {addRectTopAndBottom} = require('../../lib/rect-helpers'); +const {getTappableRectsFromClientRects} = require('../../lib/tappable-rects'); const assert = require('assert'); -function makeClientRect({x, y, width, height}) { - return { - left: x, - top: y, - right: x + width, - bottom: y + height, - width, - height, - }; -} - -describe('simplifyClientRects', () => { +describe('getTappableRectsFromClientRects', () => { it('Merges rects if a smaller rect is inside a larger one', () => { - const containingRect = makeClientRect({ + const containingRect = addRectTopAndBottom({ x: 10, y: 10, width: 100, height: 10, }); - const containedRect = makeClientRect({ + const containedRect = addRectTopAndBottom({ x: 10, y: 10, width: 50, height: 10, }); - assert.deepEqual(simplifyClientRects([ + assert.deepEqual(getTappableRectsFromClientRects([ containingRect, containedRect, ]), [containingRect]); - assert.deepEqual(simplifyClientRects([ + assert.deepEqual(getTappableRectsFromClientRects([ containedRect, containingRect, ]), [containingRect]); }); it('Merges two horizontally adjacent client rects', () => { - const res = simplifyClientRects([ - makeClientRect({ + const res = getTappableRectsFromClientRects([ + addRectTopAndBottom({ x: 10, y: 10, width: 100, height: 10, }), - makeClientRect({ + addRectTopAndBottom({ x: 110, y: 10, width: 100, @@ -61,7 +51,7 @@ describe('simplifyClientRects', () => { }), ]); assert.deepEqual(res, [ - makeClientRect({ + addRectTopAndBottom({ x: 10, y: 10, width: 200, @@ -71,20 +61,20 @@ describe('simplifyClientRects', () => { }); it('Merges three horizontally adjacent client rects', () => { - const res = simplifyClientRects([ - makeClientRect({ + const res = getTappableRectsFromClientRects([ + addRectTopAndBottom({ x: 10, y: 10, width: 100, height: 10, }), - makeClientRect({ + addRectTopAndBottom({ x: 110, y: 10, width: 100, height: 10, }), - makeClientRect({ + addRectTopAndBottom({ x: 210, y: 10, width: 100, @@ -92,7 +82,7 @@ describe('simplifyClientRects', () => { }), ]); assert.deepEqual(res, [ - makeClientRect({ + addRectTopAndBottom({ x: 10, y: 10, width: 300, @@ -102,20 +92,20 @@ describe('simplifyClientRects', () => { }); it('Merges client rects correctly if one is duplicated', () => { - const res = simplifyClientRects([ - makeClientRect({ + const res = getTappableRectsFromClientRects([ + addRectTopAndBottom({ x: 10, y: 10, width: 90, height: 10, }), - makeClientRect({ + addRectTopAndBottom({ x: 10, y: 10, width: 90, height: 10, }), - makeClientRect({ + addRectTopAndBottom({ x: 100, y: 10, width: 10, @@ -123,7 +113,7 @@ describe('simplifyClientRects', () => { }), ]); assert.deepEqual(res, [ - makeClientRect({ + addRectTopAndBottom({ x: 10, y: 10, width: 100, @@ -136,14 +126,14 @@ describe('simplifyClientRects', () => { // We do this because to fix issues with children (e.g. images) inside links. // The link itself might be small, so if we put a finger on it directly then it's // likely to overlap with something. - const res = simplifyClientRects([ - makeClientRect({ + const res = getTappableRectsFromClientRects([ + addRectTopAndBottom({ x: 10, y: 10, width: 100, height: 10, }), - makeClientRect({ + addRectTopAndBottom({ x: 10, y: 15, width: 200, @@ -151,7 +141,7 @@ describe('simplifyClientRects', () => { }), ]); assert.deepEqual(res, [ - makeClientRect({ + addRectTopAndBottom({ x: 10, y: 10, width: 200, @@ -161,14 +151,14 @@ describe('simplifyClientRects', () => { }); it('Does not merge if the center of the merged rect wouldn\'t be in the original rects', () => { - const res = simplifyClientRects([ - makeClientRect({ + const res = getTappableRectsFromClientRects([ + addRectTopAndBottom({ x: 10, y: 10, width: 10, height: 100, }), - makeClientRect({ + addRectTopAndBottom({ x: 10, y: 10, width: 200, @@ -180,14 +170,14 @@ describe('simplifyClientRects', () => { it('Merges two horizontally adjacent client rects that don\'t line up exactly', () => { // 2px difference is ok, often there are cases where an image is a 1px or 2px out of the main link client rect - const res = simplifyClientRects([ - makeClientRect({ + const res = getTappableRectsFromClientRects([ + addRectTopAndBottom({ x: 10, y: 10, width: 100, height: 10, }), - makeClientRect({ + addRectTopAndBottom({ x: 110, y: 12, width: 100, @@ -195,7 +185,7 @@ describe('simplifyClientRects', () => { }), ]); assert.deepEqual(res, [ - makeClientRect({ + addRectTopAndBottom({ x: 10, y: 10, width: 200, @@ -205,14 +195,14 @@ describe('simplifyClientRects', () => { }); it('Merges two identical client rects into one', () => { - const res = simplifyClientRects([ - makeClientRect({ + const res = getTappableRectsFromClientRects([ + addRectTopAndBottom({ x: 10, y: 10, width: 10, height: 10, }), - makeClientRect({ + addRectTopAndBottom({ x: 10, y: 10, width: 10, @@ -220,7 +210,7 @@ describe('simplifyClientRects', () => { }), ]); assert.deepEqual(res, [ - makeClientRect({ + addRectTopAndBottom({ x: 10, y: 10, width: 10, @@ -230,14 +220,14 @@ describe('simplifyClientRects', () => { }); it('Removes tiny 1x1px client rects', () => { - const res = simplifyClientRects([ - makeClientRect({ + const res = getTappableRectsFromClientRects([ + addRectTopAndBottom({ x: 10, y: 10, width: 100, height: 100, }), - makeClientRect({ + addRectTopAndBottom({ x: 5, y: 5, width: 1, @@ -245,7 +235,7 @@ describe('simplifyClientRects', () => { }), ]); assert.deepEqual(res, [ - makeClientRect({ + addRectTopAndBottom({ x: 10, y: 10, width: 100, @@ -254,22 +244,3 @@ describe('simplifyClientRects', () => { ]); }); }); - -describe('getRectOverlapArea', () => { - it('Works in a basic example', () => { - const overlapArea = getRectOverlapArea( - makeClientRect({ - x: 0, - y: 0, - width: 10, - height: 10, - }), makeClientRect({ - x: 8, - y: 6, - width: 10, - height: 10, - }) - ); - expect(overlapArea).toBe(8); - }); -}); diff --git a/types/artifacts.d.ts b/types/artifacts.d.ts index 1789d83a06b7..d37048300f07 100644 --- a/types/artifacts.d.ts +++ b/types/artifacts.d.ts @@ -278,7 +278,7 @@ declare global { }; } - export interface ClientRect { + export interface Rect { width: number; height: number; top: number; From 10c0e9f0b6242436fb711f4450105543ccd286e2 Mon Sep 17 00:00:00 2001 From: Matt Zeunert Date: Fri, 7 Dec 2018 15:10:04 +0000 Subject: [PATCH 09/11] Add comments --- lighthouse-core/lib/tappable-rects.js | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/lighthouse-core/lib/tappable-rects.js b/lighthouse-core/lib/tappable-rects.js index 2879e0f89038..118d87916d61 100644 --- a/lighthouse-core/lib/tappable-rects.js +++ b/lighthouse-core/lib/tappable-rects.js @@ -17,6 +17,7 @@ const { /** * Merge client rects together and remove small ones. This may result in a larger overall * size than that of the individual client rects. + * We use this to simulate a finger tap on those targets later on. * @param {LH.Artifacts.Rect[]} clientRects */ function getTappableRectsFromClientRects(clientRects) { @@ -39,6 +40,7 @@ function almostEqual(a, b) { } /** + * Merge touching rects based on what appears as one tappable area to the user. * @param {LH.Artifacts.Rect[]} clientRects * @returns {LH.Artifacts.Rect[]} */ @@ -49,6 +51,8 @@ function mergeTouchingClientRects(clientRects) { const crB = clientRects[j]; /** + * We try to determine whether the rects appear as a single tappable + * area to the user, so that they'd tap in the middle of the merged rect. * Examples of what we want to merge: * * AAABBB From 184550bdad712b7280b18238a3160bd8a0a143b1 Mon Sep 17 00:00:00 2001 From: Matt Zeunert Date: Fri, 7 Dec 2018 15:17:34 +0000 Subject: [PATCH 10/11] Reorder functions --- lighthouse-core/lib/rect-helpers.js | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/lighthouse-core/lib/rect-helpers.js b/lighthouse-core/lib/rect-helpers.js index d82c7b995049..9deb75bbfaa9 100644 --- a/lighthouse-core/lib/rect-helpers.js +++ b/lighthouse-core/lib/rect-helpers.js @@ -48,6 +48,12 @@ function rectContains(rect1, rect2) { ); } + +const rectContainsString = ` + ${rectContainsPoint.toString()} + ${rectContains.toString()}; +`; + /** * @param {LH.Artifacts.Rect[]} rects * @returns {LH.Artifacts.Rect[]} @@ -58,11 +64,6 @@ function filterOutTinyRects(rects) { ); } -const rectContainsString = ` - ${rectContainsPoint.toString()} - ${rectContains.toString()}; -`; - /** * @param {LH.Artifacts.Rect[]} rects * @returns {LH.Artifacts.Rect[]} From e0cb2dc987220eb7c4f369c57e9b8275b1dd731d Mon Sep 17 00:00:00 2001 From: Matt Zeunert Date: Fri, 7 Dec 2018 20:18:30 +0000 Subject: [PATCH 11/11] Change filename to tappable-rects-test --- .../{tap-target-rect-helpers-test.js => tappable-rects-test.js} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename lighthouse-core/test/lib/{tap-target-rect-helpers-test.js => tappable-rects-test.js} (100%) diff --git a/lighthouse-core/test/lib/tap-target-rect-helpers-test.js b/lighthouse-core/test/lib/tappable-rects-test.js similarity index 100% rename from lighthouse-core/test/lib/tap-target-rect-helpers-test.js rename to lighthouse-core/test/lib/tappable-rects-test.js