Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

core(tap-targets): Helper functions for working with ClientRects #6703

Merged
merged 11 commits into from
Dec 7, 2018
311 changes: 311 additions & 0 deletions lighthouse-core/lib/client-rect-functions.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,311 @@
/**
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can all instances of client be removed? seems this could be generalized to just fun with rectangles.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, I've split it into rect helpers and tap target audit specific code now.

* @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';
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe add a quick note to the functions that are executed in the browser too?

the instanbul ignore next is a good clue but explicitly linking is even better :D


/**
* @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 = {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe inline all these? Seems like rectContains will be a hot function, and most calls would fail on the first part of the below condition. Could be worth it to minimize object allocations

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.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A tad unfortunate that we're letting the tap targets bit leak out into here :/

maybe we can clarify a bit in simplifyClientRects that it's removing small ones?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A tad unfortunate that we're letting the tap targets bit leak out into here :/

True... some of the logic isn't really useful in a more general way though, especially the simplifyClientRects logic is very specific to this audit.

Maybe we can split it into two files, rectHelpers and tapTargetClientRectHelpers? And in the rectHelpers we can address @hoten's point about renaming to just rects then.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've thought about this a bit more, and I'm actually somewhat convinced that this is something we'd probably want to do with anything that's looking at real objects on a page, so it's really just the "tap" language I think I'm pushing back a bit on.

I'm fine either way with A) just documenting in the jsdoc what exactly it does to the rects during simplification or B) splitting into two files as you suggested

// 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) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe add a note why we want to return the tiny ones anyway if it's empty?

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 => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: the filter but also keeping state with a bool return has me a bit confused, maybe just refactor this to a normal double for loop so we can just return the set at the end?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could keep track of the rects we want to keep instead of the ones we want to filter out, but then we'd need to infer what we decided to filter out based on that. I played around with some options, but I'm not quite sure what you have in mind.

Agreed that the current code doesn't read well.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking the flip to rects to keep too

wouldn't the check just then be !rectsToKeep.has(possiblyContainingRect)? or did I miss a step?

is the below the same?

for (const cr of clientRects) {
  for (const possiblyContainingRect of clientRects) {
    if (possiblyContainingRect === cr) continue
    if (!rectsToKeep.has(possiblyContainingRect)) continue
    if (rectContains()) { rectsToKeep.delete(cr); break; }
  }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, in my mind I was adding to rectsToKeep instead of deleting from it. This is much more sensible 👍

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) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: this sounds a little more like clientRectsTouchOrOverlap

// 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:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe a brief comment on why we wouldn't merge them if they overlap but don't align? perhaps this belongs on what simplify actually does, but it's not clear from this alone why it shouldn't merge them

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We want to merge two rects if the user would perceive them as a single tappable area to tap on. The user would intuitively tap on the center of the merged rect – we try to determine that rect and then place the simulated finger area in the middle.

Tried to make this a bit clearer in the code.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah gotcha, thanks!

*
* 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({
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move this stuff to getBoundingRect function.

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);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe just a quick comment that we're replacing the two client rects with the merged version? the recursive call is a bit unexpected too, I'm guessing it was messy to deal with concurrent modification but a bit of explanation of heads up in function description would help

Copy link
Contributor Author

@mattzeunert mattzeunert Dec 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the recursive call is a bit unexpected too

Yeah it felt simpler to just start over than modifying the indices and checking if the mutation introduces any edge cases. For client rects the arrays are also rarely more than 5 rects, so starting again doesn't cause perf issues.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For client rects the arrays are also rarely more than 5 rects, so starting again doesn't cause perf issues.

👍 throwing that in a comment would also be ace :)

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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: remove the space so this one's clickable :)

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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: remove the space so this one's clickable :)

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) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: maybe rename from finger? getRectAtCenter

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,
};
Loading