new_audit(tap-targets): check that tap targets are big enough and don't overlap #5846

kdzwinel · 2018-08-16T15:44:45Z

Summary
This audit checks if all potential tap targets (see 1) are sized appropriately (see 2) and are far enough from each other to be unambiguous (see 3).

Limitations

we take potential tap targets into account (a, button, input, …) - this list may not include some tap targets that are made "tappable" via JS (e.g. some div used as a button) and may include some tap targets that don't cause any action (e.g. dummy button that does nothing)
We define finger size to be 48x48px
Implemented algorithm is simpler from what Chrome does (e.g. Chrome is often able to figure out which link was clicked even if tap area covers two links). The algorithm boils down to checking if there is some other tap target located < 32px from the center of the currently audited one.

Demo (note that visual output is not part of this audit):

TODO

descriptions
linting
tests

Related Issues/PRs
Closes #4358

googlebot · 2018-10-05T10:57:20Z

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

patrickhulce

I'll do a more thorough lookthrough of the approach soon, just flushing some initial impressions!

lighthouse-core/gather/gatherers/seo/tap-targets-test.js

patrickhulce · 2018-11-06T20:52:29Z

lighthouse-core/gather/gatherers/seo/tap-targets.js

+/**
+ * @param {Element} node
+ */
+function isVisible({node, bcrs = getBCRs(node, false)}) {


this is a nice little generic helper! might be nice in page-functions.js

we've also been trying to keep all our in-page functions there, but we're going to have so many for this function it's probably worth creating a dedicated file for them.

do you think it's easiest for it to be basically one giant in-page evaluation?

I ask because it's generally hard to debug and figure out what's going on in the in-page functions than the node side, but it looks like you've worked out a decent method of development here with the fs call. There might also be some opportunity to share some BCR logic between audit and gatherer?

We've mostly been using a manually synced gist for development. It also gives a visual output, which we're hoping to add to the DevTools Audit panel one day.

Will move the BCR logic to a shared file.

lighthouse-core/gather/gatherers/seo/tap-targets.js

lighthouse-core/lib/page-functions.js

patrickhulce

just trying to get a handle on a few things that might change impl quite a bit

lighthouse-core/audits/seo/tap-targets.js

lighthouse-core/test/lib/page-functions-test.js

lighthouse-core/gather/gatherers/seo/tap-targets.js

lighthouse-core/gather/gatherers/dobetterweb/anchors-with-no-rel-noopener.js

lighthouse-core/audits/seo/tap-targets.js

mattzeunert · 2018-11-08T16:24:11Z

@patrickhulce For reference, here's a drawing that Rick made for how we detect a failure-causing overlap between two tap targets. The approach is taken from the Mobile-Friendly Test implementation.

The blue dotted box is the finger tap area centered around the top anchor. The bottom anchor overlaps the tap area and that shaded area is more than half the shaded area of the top anchor. So this is a violation.

Also, I think you noticed already, but the code is still very much work-in-progress, so don't spend too much time on the details 😂.

… in one

mattzeunert · 2019-01-19T12:43:06Z

Thanks for the initial feedback @brendankenny!

Looks good but biiiiiiiig :)

Let me know if you see a good way I could have split the PR into smaller bits. Might make future PRs easier 🙂.

@patrickhulce already told me to split off the two smaller PRs for rect helpers and page functions.

brendankenny · 2019-01-23T17:08:20Z

lighthouse-core/gather/gatherers/seo/tap-targets.js

+  /**
+   * @param {Element} element
+   */
+  function hasTextNodeSiblingsFormingTextBlock(element) {


hasTextNodeSiblingsFormingTextBlock doesn't rely on being closed over by elementIsInTextBlock, so move outside to a top-level function?

Makes sense, I was trying to avoid having another function that needs to be imported in the evalAsync string, but this is cleaner 👍

brendankenny · 2019-01-23T17:26:08Z

lighthouse-core/gather/gatherers/seo/tap-targets.js

+        continue;
+      }
+      const siblingTextContent = (sibling.textContent || '').trim();
+      if (sibling.nodeType === Node.TEXT_NODE && siblingTextContent.length > 0) {


Will add a comment to the code.

great, that definitely helps

brendankenny · 2019-01-23T17:37:58Z

lighthouse-core/gather/gatherers/seo/tap-targets.js

+      overflowX,
+      overflowY,
+    } = getComputedStyle(element);
+    if ((overflowX === 'hidden' && overflowY === 'hidden') || element.children.length === 0) {


if all the client rects are size 0, does it matter whether or not the children would be visible since their client rects are also size 0 (since they're included in the allClientRectsEmpty(clientRects) check)?

No it shouldn't matter, and from trying it out on a few sites it doesn't matter. Will remove the if.

brendankenny · 2019-01-23T17:39:33Z

types/artifacts.d.ts

+      /** The value of the <meta name="theme=color">'s content attribute, or null. */
+      ThemeColor: string|null;
+      /** The value of the <meta name="viewport">'s content attribute, or null. */
+      Viewport: string|null;


ThemeColor and Viewport accidentally got merged back in here

brendankenny

feeling good :)

Some of the n^2 stuff in the audit could be an issue for nastier pages, but let's get good correctness coverage in the audit test and it'll be easy to follow up later to speed it up in those cases

brendankenny · 2019-01-23T19:12:51Z

lighthouse-core/audits/seo/tap-targets.js

+   * @return {LH.Audit.Product}
+   */
+  static audit(artifacts) {
+    const hasViewportSet = ViewportAudit.audit(artifacts).rawValue;


can you file an issue to move the guts of the viewport audit to a computed artifact? We should avoid audits calling each other if at all possible, but not worth adding to this PR

brendankenny · 2019-01-23T19:17:26Z

lighthouse-core/audits/seo/tap-targets.js

+      return {
+        rawValue: false,
+        explanation:
+          'Tap targets are too small because of a missing viewport config',


should this only happen with the missing meta tag? The audit rawValue will also be false if the width/initial-scale check fails.

(if not, may need to tweak the explanation)

Yeah it should fail if the viewport tag doesn't make it work for mobile. What do you think of this explanation:

Tap targets are too small because there's no viewport meta tag optimized for mobile screens

Changed it here and in the font size audit.

What do you think of this explanation

sounds great!

brendankenny · 2019-01-23T19:28:53Z

lighthouse-core/audits/seo/tap-targets.js

+ * @returns {LH.Artifacts.TapTarget[]}
+ */
+function getTooSmallTargets(targets) {
+  return targets.filter(targetIsTooSmall);


it seems wasteful to split this up so much. Maybe

/** * @param {LH.Artifacts.Rect} cr */ function clientRectBelowMinimumSize(cr) { return cr.width < FINGER_SIZE_PX || cr.height < FINGER_SIZE_PX; } /** * 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[]} */ function getTooSmallTargets(targets) { return targets.filter(target => { return target.clientRects.every(clientRectBelowMinimumSize); }); }

brendankenny · 2019-01-23T19:38:25Z

lighthouse-core/audits/seo/tap-targets.js

+ */
+function getOverlapFailureForTargetPair(tapTarget, maybeOverlappingTarget) {
+  // Convert client rects to unique tappable areas from a user's perspective
+  const tappableRects = getTappableRectsFromClientRects(tapTarget.clientRects);


move this below the href checks to where it's used (and maybe save a little getTappableRects work :)

brendankenny · 2019-01-23T19:58:13Z

lighthouse-core/audits/seo/tap-targets.js

+      allTargets
+    );
+
+    if (overlappingTargets.length > 0) {


nit: since overlappingTargets is always an array, no need to check the length before concating

brendankenny · 2019-01-23T20:34:58Z

lighthouse-core/audits/seo/tap-targets.js

+      tapTargetScore,
+      overlappingTargetScore,
+      overlapScoreRatio,
+    }) => {


style nit: I don't personally find this very readable, not sure how others feel about it. You've lost track of the map() by the time you get to the bottom of the destructured props :) I'd rather just have something like

const tableItems = overlapFailures.map(failure => { const largestCR = getLargestRect(failure.tapTarget.clientRects); // ... });

brendankenny · 2019-01-23T20:40:21Z

types/audit.d.ts

@@ -70,17 +70,32 @@ declare global {
      wastedPercent?: number;
    }

+    export interface TapTargetTableItem extends DetailsObject {


can this be moved to tap-targets.js as well? Since it's a DetailsObject and the table renderer can handle it, seems like it's ok to be internal to the audit until/unless we need it externally

brendankenny · 2019-01-23T20:41:40Z

lighthouse-core/test/audits/seo/tap-targets-test.js

+    TapTargets: tapTargets,
+    MetaElements: [{
+      name: 'viewport',
+      content: 'width=device-width',


There are a few cases that still need covering, e.g. need viewport failure test(s). See the red lines in https://coveralls.io/builds/21238984/source?filename=lighthouse-core%2Faudits%2Fseo%2Ftap-targets.js#L42

…ific

mattzeunert · 2019-01-25T08:30:17Z

Thanks for the comments @brendankenny, can you take another look?

brendankenny

🖐 🎯 📐

LGTM!

kdzwinel requested review from brendankenny, patrickhulce and paulirish as code owners August 16, 2018 15:44

kdzwinel requested a review from ebidel as a code owner October 5, 2018 10:57

patrickhulce reviewed Nov 6, 2018

View reviewed changes

patrickhulce reviewed Nov 7, 2018

View reviewed changes

mattzeunert force-pushed the tap-targets branch 3 times, most recently from 57704d7 to 6afbf13 Compare November 8, 2018 14:54

mattzeunert force-pushed the tap-targets branch from df8a312 to 7be42d9 Compare November 15, 2018 09:55

kdzwinel requested a review from exterkamp as a code owner November 15, 2018 09:55

mattzeunert force-pushed the tap-targets branch from 7be42d9 to b3e068e Compare November 15, 2018 09:56

mattzeunert added 2 commits November 15, 2018 10:03

Support multi line outerHTML snippets

b0969df

WIP

30669ce

mattzeunert force-pushed the tap-targets branch from b3e068e to 30669ce Compare November 15, 2018 10:05

mattzeunert added 5 commits November 15, 2018 10:55

Check each target for overlap failure, even if it is already involved…

db12c55

… in one

Fix table

f44c2fa

makeClientRects helper function

cd6ec76

Clean up gatherer

9fc3125

Filter out tiny client rects

366f033

mattzeunert force-pushed the tap-targets branch from fd2f9f0 to 366f033 Compare November 15, 2018 11:44

mattzeunert added 7 commits November 15, 2018 12:50

Clean up client rect functions

4823e53

Improve data available for debugging/tweaking finger size

b943ffd

Tweak table titles

c0ad3b1

Rename BCR to client rects

34e0257

Move getNodePath to page functions

c70ec6e

Add path to tap target node info

f80ef46

Add node selector to tap target node info

440c86e

mattzeunert added 2 commits January 19, 2019 12:10

Add full audit error to smoke test expectations

1fc2c8f

Fix line length

900dd3f

brendankenny reviewed Jan 23, 2019

View reviewed changes

mattzeunert added 6 commits January 23, 2019 19:16

Use useIsolation to handle matches being overwritten

9f58755

Move hasTextNodeSiblingsFormingTextBlock outside function block

5bfd792

Remove unnecessary if statement

d320c4f

Remove ThemeColor/Viewport that was restored during merge

a001852

Removed unused window var

be53ec6

Add test case for overwritten matches function

ba795c7

brendankenny requested changes Jan 23, 2019

View reviewed changes

mattzeunert mentioned this pull request Jan 24, 2019

Move viewport audit logic to computed artifact #7084

Closed

mattzeunert added 9 commits January 24, 2019 13:34

Make failed viewport audit message in tap targets/font size more spec…

8fd667c

…ific

Fix tests following previous change

efec03b

Fix line length for previous changes

f58fe21

Tweaks

4ae7575

Shorten getTableItems code

de60ebb

Move TapTargetTableItem type

7d497ae

Add test case for no viewport meta tag

1597590

Test case for only one tap target in a pair failing

2a33179

Rename test options to make more sense

729eaaf

brendankenny removed the waiting4committer label Jan 28, 2019

brendankenny changed the title ~~[WIP] new-audit(tap-targets): check if all tap targets big enough and don't overlap~~ new_audit(tap-targets): check that tap targets are big enough and don't overlap Jan 28, 2019

brendankenny approved these changes Jan 28, 2019

View reviewed changes

brendankenny merged commit 487acd4 into GoogleChrome:master Jan 28, 2019

mattzeunert mentioned this pull request Jan 29, 2019

i18n: tap targets #7111

Merged

brendankenny mentioned this pull request Feb 1, 2019

core: optimize tap-targets audit #7130

Merged

mattzeunert mentioned this pull request Mar 3, 2019

Include position absolute elements in tap targets audit #7365

Closed

brendankenny mentioned this pull request Jan 11, 2022

Replace tap-targets with target-size rule from Axe and put under Accessibility, not SEO #12345

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

new_audit(tap-targets): check that tap targets are big enough and don't overlap #5846

new_audit(tap-targets): check that tap targets are big enough and don't overlap #5846

kdzwinel commented Aug 16, 2018 •

edited

Loading

googlebot commented Oct 5, 2018

patrickhulce left a comment

patrickhulce Nov 6, 2018

patrickhulce Nov 6, 2018

mattzeunert Nov 8, 2018

patrickhulce left a comment

mattzeunert commented Nov 8, 2018 •

edited

Loading

mattzeunert commented Jan 19, 2019

brendankenny Jan 23, 2019

mattzeunert Jan 23, 2019

brendankenny Jan 23, 2019

brendankenny Jan 23, 2019

mattzeunert Jan 23, 2019

brendankenny Jan 23, 2019

brendankenny left a comment

brendankenny Jan 23, 2019

mattzeunert Jan 24, 2019

brendankenny Jan 23, 2019

mattzeunert Jan 24, 2019

brendankenny Jan 24, 2019

brendankenny Jan 23, 2019

brendankenny Jan 23, 2019

brendankenny Jan 23, 2019

brendankenny Jan 23, 2019

brendankenny Jan 23, 2019

brendankenny Jan 23, 2019

mattzeunert commented Jan 25, 2019

brendankenny left a comment

new_audit(tap-targets): check that tap targets are big enough and don't overlap #5846

new_audit(tap-targets): check that tap targets are big enough and don't overlap #5846

Conversation

kdzwinel commented Aug 16, 2018 • edited Loading

googlebot commented Oct 5, 2018

patrickhulce left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickhulce left a comment

Choose a reason for hiding this comment

mattzeunert commented Nov 8, 2018 • edited Loading

mattzeunert commented Jan 19, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mattzeunert commented Jan 25, 2019

brendankenny left a comment

Choose a reason for hiding this comment

kdzwinel commented Aug 16, 2018 •

edited

Loading

mattzeunert commented Nov 8, 2018 •

edited

Loading