From b0969dfef856e249dddd2eab37bd709cc8da0919 Mon Sep 17 00:00:00 2001 From: Matt Zeunert Date: Thu, 15 Nov 2018 10:03:31 +0000 Subject: [PATCH 001/102] Support multi line outerHTML snippets --- lighthouse-core/lib/page-functions.js | 2 +- lighthouse-core/test/lib/page-functions-test.js | 5 +++++ 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/lighthouse-core/lib/page-functions.js b/lighthouse-core/lib/page-functions.js index 960476f15ca3..0a949888bfcc 100644 --- a/lighthouse-core/lib/page-functions.js +++ b/lighthouse-core/lib/page-functions.js @@ -119,7 +119,7 @@ function getOuterHTMLSnippet(element, ignoreAttrs = []) { clone.removeAttribute(attribute); }); - const reOpeningTag = /^.*?>/; + const reOpeningTag = /^[\s\S]*?>/; const match = clone.outerHTML.match(reOpeningTag); return (match && match[0]) || ''; diff --git a/lighthouse-core/test/lib/page-functions-test.js b/lighthouse-core/test/lib/page-functions-test.js index 8db4648fb44c..920f82098644 100644 --- a/lighthouse-core/test/lib/page-functions-test.js +++ b/lighthouse-core/test/lib/page-functions-test.js @@ -44,5 +44,10 @@ describe('DetailsRenderer', () => { ['style-missing', 'aria-label-missing'] ), '
'); }); + + it('works if attribute values contain line breaks', () => { + assert.equal(pageFunctions.getOuterHTMLSnippet( + dom.createElement('div', '', {style: 'style1\nstyle2'})), '
'); + }); }); }); From 30669ce5d8aca2ee60bd2d1960f704c114e7165b Mon Sep 17 00:00:00 2001 From: Matt Zeunert Date: Thu, 15 Nov 2018 10:05:14 +0000 Subject: [PATCH 002/102] WIP --- .../test/cli/__snapshots__/index-test.js.snap | 11 + lighthouse-core/audits/seo/tap-targets.js | 407 ++++++++++++++++++ lighthouse-core/config/default-config.js | 3 + .../gather/gatherers/seo/tap-targets.js | 326 ++++++++++++++ lighthouse-core/lib/client-rect-functions.js | 168 ++++++++ .../test/audits/seo/tap-targets-test.js | 144 +++++++ .../test/lib/client-rect-functions-test.js | 182 ++++++++ lighthouse-core/test/results/sample_v2.json | 20 + proto/sample_v2_round_trip.json | 19 + typings/artifacts.d.ts | 21 + typings/audit.d.ts | 8 + 11 files changed, 1309 insertions(+) create mode 100644 lighthouse-core/audits/seo/tap-targets.js create mode 100644 lighthouse-core/gather/gatherers/seo/tap-targets.js create mode 100644 lighthouse-core/lib/client-rect-functions.js create mode 100644 lighthouse-core/test/audits/seo/tap-targets-test.js create mode 100644 lighthouse-core/test/lib/client-rect-functions-test.js diff --git a/lighthouse-cli/test/cli/__snapshots__/index-test.js.snap b/lighthouse-cli/test/cli/__snapshots__/index-test.js.snap index d2f3e5cca5b6..7f442b9f72d9 100644 --- a/lighthouse-cli/test/cli/__snapshots__/index-test.js.snap +++ b/lighthouse-cli/test/cli/__snapshots__/index-test.js.snap @@ -342,6 +342,9 @@ Object { Object { "path": "seo/robots-txt", }, + Object { + "path": "seo/tap-targets", + }, Object { "path": "seo/hreflang", }, @@ -947,6 +950,11 @@ Object { "id": "plugins", "weight": 1, }, + Object { + "group": "seo-mobile", + "id": "tap-targets", + "weight": 1, + }, Object { "id": "mobile-friendly", "weight": 0, @@ -1114,6 +1122,9 @@ Object { Object { "path": "seo/robots-txt", }, + Object { + "path": "seo/tap-targets", + }, ], "networkQuietThresholdMs": 1000, "passName": "defaultPass", diff --git a/lighthouse-core/audits/seo/tap-targets.js b/lighthouse-core/audits/seo/tap-targets.js new file mode 100644 index 000000000000..01a11ac3060c --- /dev/null +++ b/lighthouse-core/audits/seo/tap-targets.js @@ -0,0 +1,407 @@ +/** + * @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'; + +/** + * @fileoverview Checks that links, buttons, etc. are sufficiently large and don't overlap. + */ +const Audit = require('../audit'); +const ViewportAudit = require('../viewport'); +const { + rectContains, + simplifyClientRects, + addRectWidthAndHeight, +} = require('../../lib/client-rect-functions'); +const FINGER_SIZE_PX = 48; + +/** + * @param {LH.Artifacts.ClientRect} clientRect + */ +function getFingerAtCenter(clientRect) { + return addRectWidthAndHeight({ + left: clientRect.left + clientRect.width / 2 - FINGER_SIZE_PX / 2, + top: clientRect.top + clientRect.height / 2 - FINGER_SIZE_PX / 2, + right: clientRect.right - clientRect.width / 2 + FINGER_SIZE_PX / 2, + bottom: clientRect.bottom - clientRect.height / 2 + FINGER_SIZE_PX / 2, + }); +} + +/** + * @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 getRectOverlap(rect1, rect2) { + return getRectXOverlap(rect1, rect2) * getRectYOverlap(rect1, rect2); +} + +/** + * @param {LH.Artifacts.ClientRect} rect + * @return {LH.Artifacts.ClientRect[]} + */ +function getFingerQuadrants(rect) { + return [ + addRectWidthAndHeight({ + left: rect.left + rect.width / 2 - FINGER_SIZE_PX / 2, + top: rect.top + rect.height / 2 - FINGER_SIZE_PX / 2, + right: rect.right - rect.width / 2, + bottom: rect.bottom - rect.height / 2, + }), + addRectWidthAndHeight({ + left: rect.left + rect.width / 2, + top: rect.top + rect.height / 2 - FINGER_SIZE_PX / 2, + right: rect.right - rect.width / 2 + FINGER_SIZE_PX / 2, + bottom: rect.bottom - rect.height / 2, + }), + addRectWidthAndHeight({ + left: rect.left + rect.width / 2 - FINGER_SIZE_PX / 2, + top: rect.top + rect.height / 2, + right: rect.right - rect.width / 2, + bottom: rect.bottom - rect.height / 2 + FINGER_SIZE_PX / 2, + }), + addRectWidthAndHeight({ + left: rect.left + rect.width / 2, + top: rect.top + rect.height / 2, + right: rect.right - rect.width / 2 + FINGER_SIZE_PX / 2, + bottom: rect.bottom - rect.height / 2 + FINGER_SIZE_PX / 2, + }), + ]; +} + +/** + * @param {LH.Artifacts.ClientRect} rectWithFinger + * @param {LH.Artifacts.ClientRect} scoredRect + */ +function getFingerScore(rectWithFinger, scoredRect) { + if (getRectOverlap(getFingerAtCenter(rectWithFinger), scoredRect) === 0) { + // No overlap at all, don't need to get per-quadrant score + return 0; + } + + const q = getFingerQuadrants(rectWithFinger); + + let maxScore = 0; + q.forEach(finger => { + const score = getRectOverlap(finger, scoredRect); + if (score > maxScore) { + maxScore = score; + } + }); + + return maxScore; +} + +/** + * + * @param {LH.Artifacts.TapTarget[]} targets + */ +function getTooCloseTargets(targets) { + const count = targets.length; + + /** @typedef {{targetA: LH.Artifacts.TapTarget, targetB: LH.Artifacts.TapTarget, overlap: number}} TapTargetFailure */ + + /** @type TapTargetFailure[] */ + const failures = []; + + /** + * @param {LH.Artifacts.TapTarget} targetA + * @param {LH.Artifacts.TapTarget} targetB + */ + function targetPairAlreadyHasFailure(targetA, targetB) { + return failures.some(failure => { + return failure.targetA === targetA && failure.targetB === targetB; + }); + } + + for (let i = 0; i < count; i++) { + for (let j = 0; j < count; j++) { + if (i === j) { + continue; + } + + const targetA = targets[i]; + const targetB = targets[j]; + if (/https?:\/\//.test(targetA.href) && targetA.href === targetB.href) { + // no overlap because same target action + continue; + } + + if (targetPairAlreadyHasFailure(targetB, targetA)) { + continue; + } + + let maxExtraDistanceNeeded = 0; + // todo: wouldn't a better name for simplifyclientrects be getrectsthatneedfinger? + simplifyClientRects(targetA.clientRects).forEach(crA => { + const fingerAtCenter = getFingerAtCenter(crA); + const aOverlapScore = getFingerScore(crA, crA); + + for (const crB of targetB.clientRects) { + if (rectContains(crB, crA)) { + return; + } + } + + targetB.clientRects.forEach(crB => { + for (const crA of targetA.clientRects) { + if (rectContains(crA, crB)) { + return; + } + } + + const bOverlapScore = getFingerScore(crA, crB); + + if (bOverlapScore > aOverlapScore / 2) { + const overlapAreaExcess = Math.ceil( + bOverlapScore - aOverlapScore / 2 + ); + const xMovementNeededToFix = + overlapAreaExcess / getRectXOverlap(fingerAtCenter, crB); + const yMovementNeededToFix = + overlapAreaExcess / getRectYOverlap(fingerAtCenter, crB); + const extraDistanceNeeded = Math.min( + xMovementNeededToFix, + yMovementNeededToFix + ); + if (extraDistanceNeeded > maxExtraDistanceNeeded) { + maxExtraDistanceNeeded = extraDistanceNeeded; + } + } + }); + }); + + if (maxExtraDistanceNeeded > 0) { + failures.push({ + targetA, + targetB, + overlap: Math.ceil(maxExtraDistanceNeeded), + }); + } + } + } + + return failures; +} + +/** + * @param {LH.Artifacts.ClientRect} cr + */ +function clientRectMeetsMinimumSize(cr) { + return cr.width >= FINGER_SIZE_PX && cr.height >= FINGER_SIZE_PX; +} + +/** + * @param {LH.Artifacts.TapTarget} target + */ +function targetIsTooSmall(target) { + for (const cr of target.clientRects) { + if (clientRectMeetsMinimumSize(cr)) { + return false; + } + } + return true; +} + +/** + * @param {LH.Artifacts.ClientRect} cr + */ +function getClientRectArea(cr) { + return cr.width * cr.height; +} + +/** + * @param {LH.Artifacts.TapTarget} target + */ +function getLargestClientRect(target) { + let largestBcr = target.clientRects[0]; + for (const bcr of target.clientRects) { + if (getClientRectArea(bcr) > getClientRectArea(largestBcr)) { + largestBcr = bcr; + } + } + return largestBcr; +} + +/** + * + * @param {LH.Artifacts.TapTarget[]} targets + */ +function getTooSmallTargets(targets) { + return targets.filter(targetIsTooSmall); +} + +class TapTargets extends Audit { + /** + * @return {LH.Audit.Meta} + */ + static get meta() { + return { + id: 'tap-targets', + title: 'Tap targets are sized appropriately', + failureTitle: 'Tap targets are not sized appropriately', + description: + 'Interactive elements like buttons and links should be large enough, and have enough space around them, to be easy enough to tap without overlapping onto other elements. [Learn more](https://developers.google.com/web/fundamentals/accessibility/accessible-styles#multi-device_responsive_design).', + requiredArtifacts: ['Viewport', 'TapTargets'], + }; + } + + /** + * @param {LH.Artifacts} artifacts + * @return {LH.Audit.Product} + */ + static audit(artifacts) { + const hasViewportSet = ViewportAudit.audit(artifacts).rawValue; + if (!hasViewportSet) { + return { + rawValue: false, + explanation: + 'Tap targets are too small because of a missing viewport config', + }; + } + + const tooSmallTargets = getTooSmallTargets(artifacts.TapTargets); + + const tooClose = getTooCloseTargets(artifacts.TapTargets); + + /** @type {Array} */ + const tableItems = []; + + tooSmallTargets.forEach(target => { + const largestBcr = getLargestClientRect(target); + const largestClientRectArea = getClientRectArea(largestBcr); + const size = + Math.floor(largestBcr.width) + 'x' + Math.floor(largestBcr.height); + // todo: better name for this var + const overlappingTargets = tooClose.filter( + tooClose => tooClose.targetA === target + ); + + /** + * @param {{targetB: LH.Audit.DetailsRendererNodeDetailsJSON, extraDistanceNeeded: number}} args + * @returns {LH.Audit.TooSmallTapTargetItem} + */ + function makeItem({targetB, extraDistanceNeeded}) { + return { + // todo: include path etc + targetA: { + type: 'node', + snippet: target.snippet, + }, + // todo: should not be type node if it's not a node... + targetB, + size, + extraDistanceNeeded, + largestClientRectArea, + }; + } + if (overlappingTargets.length > 0) { + overlappingTargets.forEach(({targetB, overlap}) => { + tableItems.push( + makeItem({ + targetB: { + type: 'node', + snippet: targetB.snippet, + }, + extraDistanceNeeded: overlap, + }) + ); + }); + } else { + tableItems.push( + makeItem({ + targetB: {type: 'node', snippet: 'n/a'}, + extraDistanceNeeded: 0, + }) + ); + } + }); + + tableItems.sort((a, b) => { + /** + * @param {LH.Audit.TooSmallTapTargetItem} failure + */ + function getFailureSeriousness(failure) { + let magnitude = failure.largestClientRectArea; + if (failure.extraDistanceNeeded) { + magnitude -= failure.extraDistanceNeeded * 10000; + } + return magnitude; + } + return getFailureSeriousness(a) - getFailureSeriousness(b); + }); + + const headings = [ + {key: 'targetA', itemType: 'node', text: 'Element 1'}, + {key: 'issue', itemType: 'text', text: 'Size (px)'}, + {key: 'targetB', itemType: 'node', text: 'Element that\'s too close'}, + ]; + + // todo: figure out how to fix ts failure here + // "[x: string]: any" on TooSmallTapTarget fixes it, but seems bad + const details = Audit.makeTableDetails(headings, tableItems); + + let displayValue; + if (tableItems.length) { + displayValue = + tableItems.length > 1 + ? `${tableItems.length} issues found` + : '1 issue found'; + } + + const scorePerElement = new Map(); + artifacts.TapTargets.forEach(target => { + scorePerElement.set(target, 1); + }); + tooClose.forEach(({targetA}) => { + scorePerElement.set(targetA, 0); + }); + + let score = 1; + if (artifacts.TapTargets.length > 0) { + score = 0; + artifacts.TapTargets.forEach(target => { + const elementScore = scorePerElement.get(target); + score += elementScore / artifacts.TapTargets.length; + }); + } + + // handle floating point number issue where score is greater than 1, e.g. 1.00...0002) + score = Math.round(score * 1000) / 1000; + + return { + rawValue: tooClose.length === 0, + score, + details, + displayValue, + }; + } +} + +TapTargets.FINGER_SIZE_PX = FINGER_SIZE_PX; + +module.exports = TapTargets; diff --git a/lighthouse-core/config/default-config.js b/lighthouse-core/config/default-config.js index 39ad7100f57c..62ed9f59398c 100644 --- a/lighthouse-core/config/default-config.js +++ b/lighthouse-core/config/default-config.js @@ -111,6 +111,7 @@ const defaultConfig = { 'seo/embedded-content', 'seo/canonical', 'seo/robots-txt', + 'seo/tap-targets', ], }, { @@ -244,6 +245,7 @@ const defaultConfig = { 'seo/link-text', 'seo/is-crawlable', 'seo/robots-txt', + 'seo/tap-targets', 'seo/hreflang', 'seo/plugins', 'seo/canonical', @@ -477,6 +479,7 @@ const defaultConfig = { {id: 'canonical', weight: 1, group: 'seo-content'}, {id: 'font-size', weight: 1, group: 'seo-mobile'}, {id: 'plugins', weight: 1, group: 'seo-content'}, + {id: 'tap-targets', weight: 1, group: 'seo-mobile'}, // Manual audits {id: 'mobile-friendly', weight: 0}, {id: 'structured-data', weight: 0}, diff --git a/lighthouse-core/gather/gatherers/seo/tap-targets.js b/lighthouse-core/gather/gatherers/seo/tap-targets.js new file mode 100644 index 000000000000..db099545a041 --- /dev/null +++ b/lighthouse-core/gather/gatherers/seo/tap-targets.js @@ -0,0 +1,326 @@ +/** + * @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'; + +/* global document, getComputedStyle, Node */ + +const Gatherer = require('../gatherer'); +const pageFunctions = require('../../../lib/page-functions.js'); +const {rectContains} = require('../../../lib/client-rect-functions'); + +const TARGET_SELECTORS = [ + 'button', + 'a', + 'input', + 'textarea', + 'select', + 'option', + '[role=button]', + '[role=checkbox]', + '[role=link]', + '[role=menuitem]', + '[role=menuitemcheckbox]', + '[role=menuitemradio]', + '[role=option]', + '[role=scrollbar]', + '[role=slider]', + '[role=spinbutton]', +]; + +/** + * @param {LH.Artifacts.ClientRect[]} clientRects + */ +function allClientRectsEmpty(clientRects) { + return ( + clientRects.length === 0 || + clientRects.every(cr => cr.width === 0 && cr.height === 0) + ); +} + +/** + * @param {{node: Element, clientRects?: LH.Artifacts.ClientRect[], checkClientRectsInsideParents?: boolean}} options + */ +function isVisible({ + node, + clientRects = getClientRects(node, false), + checkClientRectsInsideParents = true, +}) { + const { + overflowX, + overflowY, + display, + opacity, + visibility, + } = getComputedStyle(node); + + if ( + ((overflowX === 'hidden' && overflowY === 'hidden') || + node.children.length === 0) && + allClientRectsEmpty(clientRects) + ) { + return false; + } + + if ( + display === 'none' || + visibility === 'hidden' || + visibility === 'collapse' || + (opacity && parseFloat(opacity) < 0.1) + ) { + return false; + } + + if (display === 'block' || display === 'inline-block') { + if (node.clientWidth === 0 && overflowX === 'hidden') { + return false; + } + if (node.clientHeight === 0 && overflowY === 'hidden') { + return false; + } + } + + if (checkClientRectsInsideParents) { + if (!isWithinAncestorsVisibleScrollArea(node, clientRects)) { + return false; + } + } + + const parent = node.parentElement; + if ( + parent && + parent.tagName !== 'HTML' && + !isVisible({node: parent, checkClientRectsInsideParents: false}) + ) { + return false; + } + + return true; +} + +/** + * @param {Element} node + * @param {LH.Artifacts.ClientRect[]} clientRects + * @returns {boolean} + */ +function isWithinAncestorsVisibleScrollArea(node, clientRects) { + const parent = node.parentElement; + if (!parent) { + return true; + } + if (getComputedStyle(parent).overflowY !== 'visible') { + for (let i = 0; i < clientRects.length; i++) { + const clientRect = clientRects[i]; + if (!rectContains(parent.getBoundingClientRect(), clientRect)) { + return false; + } + } + } + if (parent.parentElement && parent.parentElement.tagName !== 'HTML') { + return isWithinAncestorsVisibleScrollArea( + parent.parentElement, + clientRects + ); + } + return true; +} + +/** + * @param {string} str + * @param {number} maxLength + * @returns {string} + */ +function truncate(str, maxLength) { + if (str.length <= maxLength) { + return str; + } + return str.slice(0, maxLength - 1) + '…'; +} + +/** + * @param {Element} node + * @param {boolean} includeChildren + * @returns {LH.Artifacts.ClientRect[]} + */ +function getClientRects(node, includeChildren = true) { + const rawClientRects = /** @type {LH.Artifacts.ClientRect[]} */ Array.from( + node.getClientRects() + ); + + const clientRects = Array.from(rawClientRects).map(clientRect => { + const {width, height, left, top, right, bottom} = clientRect; + return {width, height, left, top, right, bottom}; + }); + if (includeChildren) { + for (const child of node.children) { + getClientRects(child).forEach(childClientRect => + clientRects.push(childClientRect) + ); + } + } + + return clientRects; +} + +/** + * Check if node is in a block of text, such as paragraph with a bunch of links in it. + * @param {Node} node + * @returns {boolean} + */ +function nodeIsInTextBlock(node) { + /** + * @param {Node} node + * @returns {boolean} + */ + function isInline(node) { + // if (!node) { + // return false; + // } + if (node.nodeType === Node.TEXT_NODE) { + return true; + } + if (node.nodeType !== Node.ELEMENT_NODE) { + return false; + } + const element = /** @type {Element} */ (node); + return ( + getComputedStyle(element).display === 'inline' || + getComputedStyle(element).display === 'inline-block' + ); + } + + /** + * @param {Node} node + */ + function hasTextNodeSiblingsFormingTextBlock(node) { + if (!node.parentElement) { + return false; + } + + const parentElement = node.parentElement; + + const nodeText = node.textContent || ''; + const parentText = parentElement.textContent || ''; + if (parentText.length - nodeText.length < 5) { + // Parent text mostly consists of this node, so the parent + // is not a text block container + return false; + } + + const potentialSiblings = node.parentElement.childNodes; + for (let i = 0; i < potentialSiblings.length; i++) { + const sibling = potentialSiblings[i]; + if (sibling === node) { + continue; + } + // check for at least 3 chars so e.g. " || " doesn't count as content + if ( + sibling.nodeType === Node.TEXT_NODE && + sibling.textContent && + sibling.textContent.trim().length > 0 + ) { + return true; + } + } + + return false; + } + + if (!isInline(node)) { + return false; + } + + if (hasTextNodeSiblingsFormingTextBlock(node)) { + return true; + } else { + if (node.parentElement) { + return nodeIsInTextBlock(node.parentElement); + } else { + return false; + } + } +} + +function gatherTapTargets() { + // todo: move this code out of there and run toString on the function + // migth need stuff like this: // @ts-ignore - getOuterHTMLSnippet put into scope via stringification + + const selector = TARGET_SELECTORS.join(','); + // const elements = getElementsInDocument(selector); + + /** @type Array */ + let targets = Array.from(document.querySelectorAll(selector)).map(node => ({ + node, + clientRects: getClientRects(node), + snippet: truncate(node.outerHTML, 700), + href: node.getAttribute('href') || '', + })); + + targets = targets.filter(target => !nodeIsInTextBlock(target.node)); + + targets = targets.filter(isVisible); + + return targets.map(t => { + return { + ...t, + node: undefined, + }; + }); +} + +/** + * @param {function} fn + * @param {(args: any[]) => any} getCacheKey + */ +function cacheResults(fn, getCacheKey) { + const cache = new Map(); + /** + * @this {any} + * @param {...any} args + */ + function fnWithCaching(...args) { + const cacheKey = getCacheKey(args); + if (cache.get(cacheKey)) { + return cache.get(cacheKey); + } + + const result = fn.apply(this, args); + cache.set(cacheKey, result); + return result; + } + return fnWithCaching; +} + +class TapTargets extends Gatherer { + /** + * @param {LH.Gatherer.PassContext} passContext + * @return {Promise} All visible tap targets with their positions and sizes + */ + afterPass(passContext) { + const expression = `(function() { + ${pageFunctions.getElementsInDocumentString}; + ${isWithinAncestorsVisibleScrollArea.toString()}; + ${isVisible.toString()}; + ${truncate.toString()}; + ${getClientRects.toString()}; + ${cacheResults.toString()}; + ${nodeIsInTextBlock.toString()}; + ${allClientRectsEmpty.toString()}; + ${rectContains.toString()}; + ${pageFunctions.getOuterHTMLSnippetString}; + ${gatherTapTargets.toString()}; + + const TARGET_SELECTORS = ${JSON.stringify(TARGET_SELECTORS)}; + isVisible = cacheResults(isVisible, args => args[0].node) + + return gatherTapTargets(); + + })()`; + + return passContext.driver.evaluateAsync(expression); + } +} + +module.exports = TapTargets; diff --git a/lighthouse-core/lib/client-rect-functions.js b/lighthouse-core/lib/client-rect-functions.js new file mode 100644 index 000000000000..56b506825b0e --- /dev/null +++ b/lighthouse-core/lib/client-rect-functions.js @@ -0,0 +1,168 @@ +/** + * @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'; + +// todo: check this works, mayb write test case +/** + * @param {LH.Artifacts.ClientRect} cr1 + * @param {LH.Artifacts.ClientRect} cr2 + */ +function rectContains(cr1, cr2) { + /** + * @param {LH.Artifacts.ClientRect} cr + * @param {{x:number, y:number}} point + */ + function rectContainsPoint(cr, {x, y}) { + return cr.left <= x && cr.right >= x && cr.top <= y && cr.bottom >= y; + } + + 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 = filterClientRectsContainedByOthers(clientRects); + clientRects = mergeTouchingClientRects(clientRects); + return clientRects; +} + +/** + * @param {LH.Artifacts.ClientRect[]} clientRects + * @returns {LH.Artifacts.ClientRect[]} + */ +function filterClientRectsContainedByOthers(clientRects) { + return clientRects.filter(cr => { + for (const possiblyContainingRect of clientRects) { + if (possiblyContainingRect === cr) { + continue; + } + if (rectContains(possiblyContainingRect, cr)) { + return false; + } + } + return true; + }); +} + +/** + * @param {LH.Artifacts.ClientRect[]} clientRects + * @returns {LH.Artifacts.ClientRect[]} + */ +function mergeTouchingClientRects(clientRects) { + /** + * @param {number} a + * @param {number} b + */ + function almostEqual(a, b) { + return Math.abs(a - b) <= 2; + } + + // better description/name: merge parallel bcrs with overlap + // console.log('merge adjacent', JSON.stringify(bcrs, null, 4)); + // todo: merge these two loops! + // console.log('----'); + for (let i = 0; i < clientRects.length; i++) { + for (let j = 0; j < clientRects.length; j++) { + if (i === j) { + continue; + } + const crA = clientRects[i]; + const crB = clientRects[j]; + + let canMerge = false; + // todo: can simplify these if statements and get rid of canMerge... would that be more readable though + if ( + almostEqual(crA.top, crB.top) && + almostEqual(crA.bottom, crB.bottom) + ) { + // they line up horizontally + if (crA.right <= crB.left && crB.right >= crA.right) { + canMerge = true; + } + } else if ( + almostEqual(crA.left, crB.left) || + almostEqual(crA.right, crB.right) + ) { + // they line up vertically + if (crB.bottom >= crB.bottom && crB.top >= crA.top) { + canMerge = true; + } + } + + if (canMerge) { + // todo: sometimes you can merge 1,0 but not 0,1... why is that?? + // console.log('canmerge', i, j); + 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, + }); + clientRects.push(replacementClientRect); + + clientRects.splice(i, 1); + if (i < j) { + j--; // update index after delete + } + clientRects.splice(j, 1); + + 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, + }; +} + +module.exports = { + rectContains, + simplifyClientRects, + addRectWidthAndHeight, +}; diff --git a/lighthouse-core/test/audits/seo/tap-targets-test.js b/lighthouse-core/test/audits/seo/tap-targets-test.js new file mode 100644 index 000000000000..6c1b7d442907 --- /dev/null +++ b/lighthouse-core/test/audits/seo/tap-targets-test.js @@ -0,0 +1,144 @@ +/** + * @license Copyright 2017 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 TapTargetsAudit = require('../../../audits/seo/tap-targets.js'); +const assert = require('assert'); + +function auditTapTargets(tapTargets) { + const artifacts = { + TapTargets: tapTargets, + Viewport: 'width=device-width', + }; + + return TapTargetsAudit.audit(artifacts); +} + +function getBorderlineTapTargets(options = {}) { + const tapTargetSize = 10; + const mainTapTarget = { + snippet: '
', + clientRects: [ + { + top: 0, + bottom: tapTargetSize, + left: 0, + right: tapTargetSize, + width: tapTargetSize, + height: tapTargetSize, + }, + ], + }; + const tapTargetBelow = { + snippet: '', + clientRects: [ + { + top: mainTapTarget.clientRects[0].top + TapTargetsAudit.FINGER_SIZE_PX, + bottom: mainTapTarget.clientRects[0].bottom + TapTargetsAudit.FINGER_SIZE_PX, + left: 0, + right: tapTargetSize, + width: tapTargetSize, + height: tapTargetSize, + }, + ], + }; + const tapTargetToTheRight = { + snippet: '', + clientRects: [ + { + top: 0, + bottom: tapTargetSize, + left: mainTapTarget.clientRects[0].left + TapTargetsAudit.FINGER_SIZE_PX, + right: mainTapTarget.clientRects[0].right + TapTargetsAudit.FINGER_SIZE_PX, + width: tapTargetSize, + height: tapTargetSize, + }, + ], + }; + + const minimalOverlapCausingDistance = (TapTargetsAudit.FINGER_SIZE_PX - tapTargetSize) / 2; + const minimalFailingOverlapDistance = + minimalOverlapCausingDistance + Math.ceil(tapTargetSize / 2 / 2); + const overlapAmount = options.overlapAmount || minimalFailingOverlapDistance; + if (options.failRight) { + tapTargetToTheRight.clientRects[0].left -= overlapAmount; + tapTargetToTheRight.clientRects[0].right -= overlapAmount; + } + if (options.failBelow) { + tapTargetBelow.clientRects[0].top -= overlapAmount; + tapTargetBelow.clientRects[0].bottom -= overlapAmount; + } + if (options.failSecondClientRect) { + mainTapTarget.clientRects.push({ + top: overlapAmount, + bottom: tapTargetSize + overlapAmount, + left: 0, + right: tapTargetSize, + width: tapTargetSize, + height: tapTargetSize, + }); + } + + return [mainTapTarget, tapTargetBelow, tapTargetToTheRight]; +} + +describe('SEO: Tap targets audit', () => { + it('passes when there are no tap targets', () => { + const auditResult = auditTapTargets([]); + assert.equal(auditResult.rawValue, true); + assert.equal(auditResult.score, 1); + }); + + it('passes when tap targets don\'t overlap', () => { + const auditResult = auditTapTargets(getBorderlineTapTargets()); + assert.equal(auditResult.rawValue, true); + }); + + it('fails if a tap target overlaps horizontally', () => { + const auditResult = auditTapTargets( + getBorderlineTapTargets({ + failRight: true, + }) + ); + assert.equal(auditResult.rawValue, false); + assert.equal(Math.round(auditResult.score * 100), 67); + const failure = auditResult.details.items[0]; + assert.equal(failure.targetA.snippet, '
'); + assert.equal(failure.targetB.snippet, ''); + assert.equal(failure.size, '10x10'); + }); + + it('fails if a tap target overlaps vertically', () => { + const auditResult = auditTapTargets( + getBorderlineTapTargets({ + failBelow: true, + }) + ); + assert.equal(auditResult.rawValue, false); + }); + + it('fails when one of the client rects overlaps', () => { + const auditResult = auditTapTargets( + getBorderlineTapTargets({ + failSecondClientRect: true, + }) + ); + assert.equal(auditResult.rawValue, false); + }); + + it('reports 2 failures if the main target is overlapped both vertically and horizontally', () => { + const auditResult = auditTapTargets( + getBorderlineTapTargets({ + failRight: true, + failBelow: true, + }) + ); + const failures = auditResult.details.items.filter(item => item.extraDistanceNeeded > 0); + assert.equal(failures.length, 2); + }); +}); 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..21c09e9dd4e2 --- /dev/null +++ b/lighthouse-core/test/lib/client-rect-functions-test.js @@ -0,0 +1,182 @@ +/** + * @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} = require('../../lib/client-rect-functions'); +const assert = require('assert'); + +describe('simplifyClientRects', () => { + it('Merges rects if a smaller rect is inside a larger one', () => { + const res = simplifyClientRects([ + { + left: 10, + right: 110, + width: 100, + height: 10, + top: 10, + bottom: 20, + }, + { + left: 10, + right: 60, + width: 50, + height: 10, + top: 10, + bottom: 20, + }, + ]); + assert.deepEqual(res, [ + { + left: 10, + right: 110, + width: 100, + height: 10, + top: 10, + bottom: 20, + }, + ]); + }); + it('Merges two horizontally adjacent client rects', () => { + const res = simplifyClientRects([ + { + left: 10, + right: 110, + width: 100, + height: 10, + top: 10, + bottom: 20, + }, + { + left: 110, + right: 210, + width: 100, + height: 10, + top: 10, + bottom: 20, + }, + ]); + assert.deepEqual(res, [ + { + left: 10, + right: 210, + width: 200, + height: 10, + top: 10, + bottom: 20, + }, + ]); + }); + + it('Merges three horizontally adjacent client rects', () => { + const res = simplifyClientRects([ + { + left: 10, + right: 110, + width: 100, + height: 10, + top: 10, + bottom: 20, + }, + { + left: 110, + right: 210, + width: 100, + height: 10, + top: 10, + bottom: 20, + }, + { + left: 210, + right: 310, + width: 100, + height: 10, + top: 10, + bottom: 20, + }, + ]); + assert.deepEqual(res, [ + { + left: 10, + right: 310, + width: 300, + height: 10, + top: 10, + bottom: 20, + }, + ]); + }); + + it('Merges two vertically adjacent client rects even if one is wider than the other', () => { + // todo: rephrase + // We do this because to fix issues with images inside links. + // If we don't merge we'll put a finger on the image and link separately, with the link + // being small and on one side and overlapping with something. + const res = simplifyClientRects([ + { + left: 10, + right: 110, + width: 100, + height: 10, + top: 10, + bottom: 20, + }, + { + left: 10, + right: 210, + width: 200, + height: 10, + top: 15, + bottom: 30, + }, + ]); + assert.deepEqual(res, [ + { + left: 10, + right: 210, + width: 200, + height: 20, + top: 10, + bottom: 30, + }, + ]); + }); + + // todO: rename this one!!!! + it('Merges two horizontally adjacent client recttha tare only adjcentishs', () => { + // 2px diff is ok, often there are cases where an image is a px or two out of the main link bcr + // should not be called simplofybcrs... + const res = simplifyClientRects([ + { + left: 10, + right: 110, + width: 100, + height: 10, + top: 10, + bottom: 20, + }, + { + left: 110, + right: 210, + width: 100, + height: 10, + top: 12, + bottom: 22, + }, + ]); + assert.deepEqual(res, [ + { + left: 10, + right: 210, + width: 200, + height: 12, + top: 10, + bottom: 22, + }, + ]); + }); +}); diff --git a/lighthouse-core/test/results/sample_v2.json b/lighthouse-core/test/results/sample_v2.json index 053f381a53b9..f74a4388374c 100644 --- a/lighthouse-core/test/results/sample_v2.json +++ b/lighthouse-core/test/results/sample_v2.json @@ -2680,6 +2680,15 @@ "rawValue": null, "errorMessage": "Audit error: Required RobotsTxt gatherer did not run." }, + "tap-targets": { + "id": "tap-targets", + "title": "Tap targets are not sized appropriately", + "description": "Interactive elements like buttons and links should be large enough, and have enough space around them, to be easy enough to tap without overlapping onto other elements. [Learn more](https://developers.google.com/web/fundamentals/accessibility/accessible-styles#multi-device_responsive_design).", + "score": null, + "scoreDisplayMode": "error", + "rawValue": null, + "errorMessage": "Audit error: Required TapTargets gatherer did not run." + }, "hreflang": { "id": "hreflang", "title": "Document has a valid `hreflang`", @@ -3357,6 +3366,11 @@ "weight": 1, "group": "seo-content" }, + { + "id": "tap-targets", + "weight": 1, + "group": "seo-mobile" + }, { "id": "mobile-friendly", "weight": 0 @@ -4146,6 +4160,12 @@ "duration": 100, "entryType": "measure" }, + { + "startTime": 0, + "name": "lh:audit:tap-targets", + "duration": 100, + "entryType": "measure" + }, { "startTime": 0, "name": "lh:audit:hreflang", diff --git a/proto/sample_v2_round_trip.json b/proto/sample_v2_round_trip.json index 551bb7fb9f00..c8236a23f054 100644 --- a/proto/sample_v2_round_trip.json +++ b/proto/sample_v2_round_trip.json @@ -1919,6 +1919,14 @@ "scoreDisplayMode": "not_applicable", "title": "No element has a `[tabindex]` value greater than 0" }, + "tap-targets": { + "description": "Interactive elements like buttons and links should be large enough, and have enough space around them, to be easy enough to tap without overlapping onto other elements. [Learn more](https://developers.google.com/web/fundamentals/accessibility/accessible-styles#multi-device_responsive_design).", + "errorMessage": "Audit error: Required TapTargets gatherer did not run.", + "id": "tap-targets", + "score": null, + "scoreDisplayMode": "error", + "title": "Tap targets are not sized appropriately" + }, "td-headers-attr": { "description": "Screen readers have features to make navigating tables easier. Ensuring `` cells using the `[headers]` attribute only refer to other cells in the same table may improve the experience for screen reader users. [Learn more](https://dequeuniversity.com/rules/axe/2.2/td-headers-attr?application=lighthouse).", "id": "td-headers-attr", @@ -3175,6 +3183,11 @@ "id": "plugins", "weight": 1.0 }, + { + "group": "seo-mobile", + "id": "tap-targets", + "weight": 1.0 + }, { "id": "mobile-friendly", "weight": 0.0 @@ -4002,6 +4015,12 @@ "name": "lh:audit:robots-txt", "startTime": 0.0 }, + { + "duration": 100.0, + "entryType": "measure", + "name": "lh:audit:tap-targets", + "startTime": 0.0 + }, { "duration": 100.0, "entryType": "measure", diff --git a/typings/artifacts.d.ts b/typings/artifacts.d.ts index b681ae429662..fdd2624962f9 100644 --- a/typings/artifacts.d.ts +++ b/typings/artifacts.d.ts @@ -113,6 +113,8 @@ declare global { StartUrl: {statusCode: number, explanation?: string}; /** Information on From 8fd667c91084c9cbaa4b6dd8f2aa6939ef6688b3 Mon Sep 17 00:00:00 2001 From: Matt Zeunert Date: Thu, 24 Jan 2019 13:34:24 +0000 Subject: [PATCH 094/102] Make failed viewport audit message in tap targets/font size more specific --- lighthouse-cli/test/smokehouse/seo/expectations.js | 2 +- lighthouse-core/audits/seo/font-size.js | 2 +- lighthouse-core/audits/seo/tap-targets.js | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/lighthouse-cli/test/smokehouse/seo/expectations.js b/lighthouse-cli/test/smokehouse/seo/expectations.js index 7952d860049f..0e5edc95b943 100644 --- a/lighthouse-cli/test/smokehouse/seo/expectations.js +++ b/lighthouse-cli/test/smokehouse/seo/expectations.js @@ -99,7 +99,7 @@ module.exports = [ }, 'font-size': { rawValue: false, - explanation: 'Text is illegible because of a missing viewport config', + explanation: 'Text is illegible because there\'s no viewport meta tag optimized for mobile screens', }, 'link-text': { score: 0, diff --git a/lighthouse-core/audits/seo/font-size.js b/lighthouse-core/audits/seo/font-size.js index 440ea50ecc2b..4bd0a9bd06a5 100644 --- a/lighthouse-core/audits/seo/font-size.js +++ b/lighthouse-core/audits/seo/font-size.js @@ -212,7 +212,7 @@ class FontSize extends Audit { if (!hasViewportSet) { return { rawValue: false, - explanation: 'Text is illegible because of a missing viewport config', + explanation: 'Text is illegible because there\'s no viewport meta tag optimized for mobile screens', }; } diff --git a/lighthouse-core/audits/seo/tap-targets.js b/lighthouse-core/audits/seo/tap-targets.js index ebcdde08be15..5a7c208c851c 100644 --- a/lighthouse-core/audits/seo/tap-targets.js +++ b/lighthouse-core/audits/seo/tap-targets.js @@ -281,7 +281,7 @@ class TapTargets extends Audit { return { rawValue: false, explanation: - 'Tap targets are too small because of a missing viewport config', + 'Tap targets are too small because there\'s no viewport meta tag optimized for mobile screens', }; } From efec03bce82d4cb1bed963e180c0e3184be8048c Mon Sep 17 00:00:00 2001 From: Matt Zeunert Date: Thu, 24 Jan 2019 13:45:20 +0000 Subject: [PATCH 095/102] Fix tests following previous change --- lighthouse-cli/test/smokehouse/seo/expectations.js | 3 ++- lighthouse-core/audits/seo/font-size.js | 3 ++- lighthouse-core/audits/seo/tap-targets.js | 4 ++-- lighthouse-core/test/audits/seo/font-size-test.js | 2 +- 4 files changed, 7 insertions(+), 5 deletions(-) diff --git a/lighthouse-cli/test/smokehouse/seo/expectations.js b/lighthouse-cli/test/smokehouse/seo/expectations.js index 0e5edc95b943..06197e2537b1 100644 --- a/lighthouse-cli/test/smokehouse/seo/expectations.js +++ b/lighthouse-cli/test/smokehouse/seo/expectations.js @@ -99,7 +99,8 @@ module.exports = [ }, 'font-size': { rawValue: false, - explanation: 'Text is illegible because there\'s no viewport meta tag optimized for mobile screens', + explanation: + 'Text is illegible because there\'s no viewport meta tag optimized for mobile screens', }, 'link-text': { score: 0, diff --git a/lighthouse-core/audits/seo/font-size.js b/lighthouse-core/audits/seo/font-size.js index 4bd0a9bd06a5..591288ba9a52 100644 --- a/lighthouse-core/audits/seo/font-size.js +++ b/lighthouse-core/audits/seo/font-size.js @@ -212,7 +212,8 @@ class FontSize extends Audit { if (!hasViewportSet) { return { rawValue: false, - explanation: 'Text is illegible because there\'s no viewport meta tag optimized for mobile screens', + explanation: + 'Text is illegible because there\'s no viewport meta tag optimized for mobile screens', }; } diff --git a/lighthouse-core/audits/seo/tap-targets.js b/lighthouse-core/audits/seo/tap-targets.js index 5a7c208c851c..6d04cd1d0f34 100644 --- a/lighthouse-core/audits/seo/tap-targets.js +++ b/lighthouse-core/audits/seo/tap-targets.js @@ -280,8 +280,8 @@ class TapTargets extends Audit { if (!hasViewportSet) { return { rawValue: false, - explanation: - 'Tap targets are too small because there\'s no viewport meta tag optimized for mobile screens', + // eslint-disable-next-line + explanation: 'Tap targets are too small because there\'s no viewport meta tag optimized for mobile screens', }; } diff --git a/lighthouse-core/test/audits/seo/font-size-test.js b/lighthouse-core/test/audits/seo/font-size-test.js index bd33d18434cf..f6513a045050 100644 --- a/lighthouse-core/test/audits/seo/font-size-test.js +++ b/lighthouse-core/test/audits/seo/font-size-test.js @@ -25,7 +25,7 @@ describe('SEO: Font size audit', () => { const auditResult = FontSizeAudit.audit(artifacts); assert.equal(auditResult.rawValue, false); - assert.ok(auditResult.explanation.includes('missing viewport')); + assert.ok(auditResult.explanation.includes('no viewport meta tag')); }); it('fails when less than 60% of text is legible', () => { From f58fe2170101e7f205436a1cc5095e5c4b81ed44 Mon Sep 17 00:00:00 2001 From: Matt Zeunert Date: Thu, 24 Jan 2019 13:45:31 +0000 Subject: [PATCH 096/102] Fix line length for previous changes --- lighthouse-core/audits/seo/tap-targets.js | 22 ++++++---------------- 1 file changed, 6 insertions(+), 16 deletions(-) diff --git a/lighthouse-core/audits/seo/tap-targets.js b/lighthouse-core/audits/seo/tap-targets.js index 6d04cd1d0f34..2ec20974daec 100644 --- a/lighthouse-core/audits/seo/tap-targets.js +++ b/lighthouse-core/audits/seo/tap-targets.js @@ -28,29 +28,19 @@ const MAX_ACCEPTABLE_OVERLAP_SCORE_RATIO = 0.25; /** * @param {LH.Artifacts.Rect} cr */ -function clientRectMeetsMinimumSize(cr) { - return cr.width >= FINGER_SIZE_PX && cr.height >= FINGER_SIZE_PX; -} - -/** - * A target is "too small" if none of it's clientRects are at least the size of a finger. - * @param {LH.Artifacts.TapTarget} target - */ -function targetIsTooSmall(target) { - for (const cr of target.clientRects) { - if (clientRectMeetsMinimumSize(cr)) { - return false; - } - } - return true; +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(targetIsTooSmall); + return targets.filter(target => { + return target.clientRects.every(clientRectBelowMinimumSize); + }); } /** From 4ae7575b47bbdcab0c2ef9536ffb2ad79fcf05db Mon Sep 17 00:00:00 2001 From: Matt Zeunert Date: Thu, 24 Jan 2019 13:48:03 +0000 Subject: [PATCH 097/102] Tweaks --- lighthouse-core/audits/seo/tap-targets.js | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/lighthouse-core/audits/seo/tap-targets.js b/lighthouse-core/audits/seo/tap-targets.js index 2ec20974daec..b29c85dfafa1 100644 --- a/lighthouse-core/audits/seo/tap-targets.js +++ b/lighthouse-core/audits/seo/tap-targets.js @@ -58,9 +58,7 @@ function getAllOverlapFailures(tooSmallTargets, allTargets) { allTargets ); - if (overlappingTargets.length > 0) { - failures = failures.concat(overlappingTargets); - } + failures = failures.concat(overlappingTargets); }); return failures; @@ -97,14 +95,14 @@ function getAllOverlapFailuresForTarget(tapTarget, allTapTargets) { * @returns {TapTargetOverlapFailure | null} */ function getOverlapFailureForTargetPair(tapTarget, maybeOverlappingTarget) { - // Convert client rects to unique tappable areas from a user's perspective - const tappableRects = getTappableRectsFromClientRects(tapTarget.clientRects); const isHttpOrHttpsLink = /https?:\/\//.test(tapTarget.href); if (isHttpOrHttpsLink && tapTarget.href === maybeOverlappingTarget.href) { // no overlap because same target action return null; } + // Convert client rects to unique tappable areas from a user's perspective + const tappableRects = getTappableRectsFromClientRects(tapTarget.clientRects); if (allRectsContainedWithinEachOther(tappableRects, maybeOverlappingTarget.clientRects)) { // If one tap target is fully contained within the other that's // probably intentional (e.g. an item with a delete button inside). From de60ebb569bfd31092515e4a99d9882ae4ba3408 Mon Sep 17 00:00:00 2001 From: Matt Zeunert Date: Thu, 24 Jan 2019 13:57:27 +0000 Subject: [PATCH 098/102] Shorten getTableItems code --- lighthouse-core/audits/seo/tap-targets.js | 39 ++++++++++------------- 1 file changed, 16 insertions(+), 23 deletions(-) diff --git a/lighthouse-core/audits/seo/tap-targets.js b/lighthouse-core/audits/seo/tap-targets.js index b29c85dfafa1..d75a485c6d15 100644 --- a/lighthouse-core/audits/seo/tap-targets.js +++ b/lighthouse-core/audits/seo/tap-targets.js @@ -200,29 +200,22 @@ function mergeSymmetricFailures(overlapFailures) { * @returns {LH.Audit.TapTargetTableItem[]} */ function getTableItems(overlapFailures) { - const tableItems = overlapFailures.map( - ({ - tapTarget, - overlappingTarget, - tapTargetScore, - overlappingTargetScore, - overlapScoreRatio, - }) => { - const largestCR = getLargestRect(tapTarget.clientRects); - const width = Math.floor(largestCR.width); - const height = Math.floor(largestCR.height); - const size = width + 'x' + height; - return { - tapTarget: targetToTableNode(tapTarget), - overlappingTarget: targetToTableNode(overlappingTarget), - size, - width, - height, - tapTargetScore, - overlappingTargetScore, - overlapScoreRatio, - }; - }); + const tableItems = overlapFailures.map(failure => { + const largestCR = getLargestRect(failure.tapTarget.clientRects); + const width = Math.floor(largestCR.width); + const height = Math.floor(largestCR.height); + const size = width + 'x' + height; + return { + tapTarget: targetToTableNode(failure.tapTarget), + overlappingTarget: targetToTableNode(failure.overlappingTarget), + tapTargetScore: failure.tapTargetScore, + overlappingTargetScore: failure.overlappingTargetScore, + overlapScoreRatio: failure.overlapScoreRatio, + size, + width, + height, + }; + }); tableItems.sort((a, b) => { return b.overlapScoreRatio - a.overlapScoreRatio; From 7d497ae7996b468900c16e706025e8763820a3e8 Mon Sep 17 00:00:00 2001 From: Matt Zeunert Date: Thu, 24 Jan 2019 14:01:17 +0000 Subject: [PATCH 099/102] Move TapTargetTableItem type --- lighthouse-core/audits/seo/tap-targets.js | 13 ++++++++++++- types/audit.d.ts | 11 ----------- 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/lighthouse-core/audits/seo/tap-targets.js b/lighthouse-core/audits/seo/tap-targets.js index d75a485c6d15..d71dbe38581a 100644 --- a/lighthouse-core/audits/seo/tap-targets.js +++ b/lighthouse-core/audits/seo/tap-targets.js @@ -197,7 +197,7 @@ function mergeSymmetricFailures(overlapFailures) { /** * @param {TapTargetOverlapFailure[]} overlapFailures - * @returns {LH.Audit.TapTargetTableItem[]} + * @returns {TapTargetTableItem[]} */ function getTableItems(overlapFailures) { const tableItems = overlapFailures.map(failure => { @@ -313,3 +313,14 @@ module.exports = TapTargets; tapTarget: LH.Artifacts.TapTarget; overlappingTarget: LH.Artifacts.TapTarget; }} TapTargetOverlapFailure */ + +/** @typedef {{ + tapTarget: LH.Audit.DetailsRendererNodeDetailsJSON; + overlappingTarget: LH.Audit.DetailsRendererNodeDetailsJSON; + size: string; + overlapScoreRatio: number; + height: number; + width: number; + tapTargetScore: number; + overlappingTargetScore: number; +}} TapTargetTableItem */ diff --git a/types/audit.d.ts b/types/audit.d.ts index 4e71000fd34f..1fb2b36ff18e 100644 --- a/types/audit.d.ts +++ b/types/audit.d.ts @@ -70,17 +70,6 @@ declare global { wastedPercent?: number; } - export interface TapTargetTableItem extends DetailsObject { - tapTarget: DetailsRendererNodeDetailsJSON; - overlappingTarget: DetailsRendererNodeDetailsJSON; - size: string; - overlapScoreRatio: number; - height: number; - width: number; - tapTargetScore: number; - overlappingTargetScore: number; - } - // TODO: placeholder typedefs until Details are typed export interface DetailsRendererDetailsSummary { wastedMs?: number; From 15975905c9e42a6802b200a7385b691460e25e4e Mon Sep 17 00:00:00 2001 From: Matt Zeunert Date: Thu, 24 Jan 2019 14:15:55 +0000 Subject: [PATCH 100/102] Add test case for no viewport meta tag --- .../test/audits/seo/tap-targets-test.js | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/lighthouse-core/test/audits/seo/tap-targets-test.js b/lighthouse-core/test/audits/seo/tap-targets-test.js index 794bd850a1e5..9a014e570861 100644 --- a/lighthouse-core/test/audits/seo/tap-targets-test.js +++ b/lighthouse-core/test/audits/seo/tap-targets-test.js @@ -10,13 +10,13 @@ const TapTargetsAudit = require('../../../audits/seo/tap-targets.js'); const assert = require('assert'); -function auditTapTargets(tapTargets) { +function auditTapTargets(tapTargets, metaElements = [{ + name: 'viewport', + content: 'width=device-width', +}]) { const artifacts = { TapTargets: tapTargets, - MetaElements: [{ - name: 'viewport', - content: 'width=device-width', - }], + MetaElements: metaElements, }; return TapTargetsAudit.audit(artifacts); @@ -181,4 +181,10 @@ describe('SEO: Tap targets audit', () => { // so it's the failure that appears in the report assert.equal(failures[0].tapTarget.snippet, ''); }); + + it('fails if no meta viewport tag is provided', () => { + const auditResult = auditTapTargets([], []); + assert.equal(auditResult.rawValue, false); + assert.ok(auditResult.explanation.includes('no viewport meta tag')); + }); }); From 2a3317993835a8f5755a3859671689f8663a310f Mon Sep 17 00:00:00 2001 From: Matt Zeunert Date: Thu, 24 Jan 2019 14:22:38 +0000 Subject: [PATCH 101/102] Test case for only one tap target in a pair failing --- .../test/audits/seo/tap-targets-test.js | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/lighthouse-core/test/audits/seo/tap-targets-test.js b/lighthouse-core/test/audits/seo/tap-targets-test.js index 9a014e570861..7c6676bf4fff 100644 --- a/lighthouse-core/test/audits/seo/tap-targets-test.js +++ b/lighthouse-core/test/audits/seo/tap-targets-test.js @@ -73,6 +73,10 @@ function getBorderlineTapTargets(options = {}) { tapTargetToTheRight.clientRects[0].width -= 1; tapTargetToTheRight.clientRects[0].right -= 1; } + if (options.increaseRightWidth) { + tapTargetToTheRight.clientRects[0].width += 10; + tapTargetToTheRight.clientRects[0].right += 10; + } const targets = [mainTapTarget, tapTargetBelow, tapTargetToTheRight]; @@ -182,6 +186,19 @@ describe('SEO: Tap targets audit', () => { assert.equal(failures[0].tapTarget.snippet, ''); }); + it('reports 1 failure if only one tap target involved in an overlap fails', () => { + const auditResult = auditTapTargets( + getBorderlineTapTargets({ + failRight: true, + increaseRightWidth: true, + }) + ); + assert.equal(Math.round(auditResult.score * 100), 67); + const failures = auditResult.details.items; + //
fails, but doesn't + assert.equal(failures[0].tapTarget.snippet, '
'); + }); + it('fails if no meta viewport tag is provided', () => { const auditResult = auditTapTargets([], []); assert.equal(auditResult.rawValue, false); From 729eaaf3e38729d0a0fa3bf8a70dd65b70ee35bb Mon Sep 17 00:00:00 2001 From: Matt Zeunert Date: Thu, 24 Jan 2019 14:25:14 +0000 Subject: [PATCH 102/102] Rename test options to make more sense --- .../test/audits/seo/tap-targets-test.js | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/lighthouse-core/test/audits/seo/tap-targets-test.js b/lighthouse-core/test/audits/seo/tap-targets-test.js index 7c6676bf4fff..22a1b9a65afc 100644 --- a/lighthouse-core/test/audits/seo/tap-targets-test.js +++ b/lighthouse-core/test/audits/seo/tap-targets-test.js @@ -81,11 +81,11 @@ function getBorderlineTapTargets(options = {}) { const targets = [mainTapTarget, tapTargetBelow, tapTargetToTheRight]; const overlapAmount = minimalFailingOverlapCausingDistance; - if (options.failRight) { + if (options.overlapRight) { tapTargetToTheRight.clientRects[0].left -= overlapAmount; tapTargetToTheRight.clientRects[0].right -= overlapAmount; } - if (options.failBelow) { + if (options.overlapBelow) { tapTargetBelow.clientRects[0].top -= overlapAmount; tapTargetBelow.clientRects[0].bottom -= overlapAmount; } @@ -100,7 +100,7 @@ function getBorderlineTapTargets(options = {}) { ], }); } - if (options.failSecondClientRect) { + if (options.overlapSecondClientRect) { mainTapTarget.clientRects.push( makeClientRects({ x: 0, @@ -134,7 +134,7 @@ describe('SEO: Tap targets audit', () => { it('fails if two tap targets overlaps each other horizontally', () => { const auditResult = auditTapTargets( getBorderlineTapTargets({ - failRight: true, + overlapRight: true, }) ); assert.equal(auditResult.rawValue, false); @@ -154,7 +154,7 @@ describe('SEO: Tap targets audit', () => { it('fails if a tap target overlaps vertically', () => { const auditResult = auditTapTargets( getBorderlineTapTargets({ - failBelow: true, + overlapBelow: true, }) ); assert.equal(auditResult.rawValue, false); @@ -163,7 +163,7 @@ describe('SEO: Tap targets audit', () => { it('fails when one of the client rects overlaps', () => { const auditResult = auditTapTargets( getBorderlineTapTargets({ - failSecondClientRect: true, + overlapSecondClientRect: true, }) ); assert.equal(auditResult.rawValue, false); @@ -173,9 +173,9 @@ describe('SEO: Tap targets audit', () => { // Main is overlapped by right + below, right and below are each overlapped by main const auditResult = auditTapTargets( getBorderlineTapTargets({ - failRight: true, + overlapRight: true, reduceRightWidth: true, - failBelow: true, + overlapBelow: true, }) ); assert.equal(Math.round(auditResult.score * 100), 0); // all tap targets are overlapped by something @@ -189,7 +189,7 @@ describe('SEO: Tap targets audit', () => { it('reports 1 failure if only one tap target involved in an overlap fails', () => { const auditResult = auditTapTargets( getBorderlineTapTargets({ - failRight: true, + overlapRight: true, increaseRightWidth: true, }) );