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

Conversation

mattzeunert
Copy link
Contributor

Summary

Splitting this out from the main tap targets PR.

The tap targets gatherer collects a list of tappable targets and their client rects. The audit then checks if these client rects overlap.

The gatherer often collects multiple client rects for each tap target, but the client rects may overlap each other or can be ignored for some other reason. So simplifyClientRects prepares them to be consumed in the audit.

There are also a few general helper functions related to working with client rects.

Related Issues/PRs

Tap targets ticket: #4358

Copy link
Collaborator

@patrickhulce patrickhulce left a comment

Choose a reason for hiding this comment

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

Code all LGTM! Just a request for some explanations so future peeps won't think they know better

Thanks again for splitting this up!! MUCH easier to see what's going on :D

* 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

* @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

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?

*/
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 👍

* @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

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!

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 :)

* @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 :)

* @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 :)

* @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

width: 100,
height: 10,
}),
makeClientRect({
Copy link
Collaborator

Choose a reason for hiding this comment

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

would be really neat to replace all the x/y/w/h rect definitions with some sort of visual ascii

const input = makeRects(`
+--+--+--+
|..|..|..|
+--+--+--+
`);
const simple = simplifyClientRects(input)

assertEqual(simple, makeRect(`
+--------+
|........|
+--------+
`));

Obviously this is way more work. Maybe if we had dozens of these tests it'd be worth it :)

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

@@ -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.

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.

@paulirish
Copy link
Member

@patrickhulce if you're happy then this can land.

Copy link
Collaborator

@patrickhulce patrickhulce left a comment

Choose a reason for hiding this comment

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

sorrrrrrrrrrrry 😞 last thing I promise than I am so 🏅 on merging!

@@ -0,0 +1,246 @@
/**
Copy link
Collaborator

Choose a reason for hiding this comment

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

can we get this filename aligned with the source one? no preference and not sure if you already had some refactoring done that makes one name easier than the other, so up to you!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, changed it to tappable-rects-test.js.

Copy link
Collaborator

@patrickhulce patrickhulce left a comment

Choose a reason for hiding this comment

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

huzzah! awesome job @mattzeunert!! 🎉 thank you 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants