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.

Sign up for GitHub

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

Jump to bottom

core(tap-targets): disable font size and tap targets audit on desktop #7393

Merged
merged 2 commits into from
Mar 6, 2019
Merged

core(tap-targets): disable font size and tap targets audit on desktop #7393

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
82b1728
Make tap targets audit not applicable on desktop
mattzeunert Mar 6, 2019
13bc083
Make font size audit not applicable on desktop
mattzeunert Mar 6, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion lighthouse-core/audits/seo/font-size.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,7 +207,7 @@ class FontSize extends Audit {
title: str_(UIStrings.title),
failureTitle: str_(UIStrings.failureTitle),
description: str_(UIStrings.description),
requiredArtifacts: ['FontSize', 'URL', 'MetaElements'],
requiredArtifacts: ['FontSize', 'URL', 'MetaElements', 'TestedAsMobileDevice'],
};
}

Expand All @@ -217,6 +217,14 @@ class FontSize extends Audit {
* @return {Promise<LH.Audit.Product>}
*/
static async audit(artifacts, context) {
if (!artifacts.TestedAsMobileDevice) {
// Font size isn't important to desktop SEO
return {
rawValue: true,
notApplicable: true,
};
}

const viewportMeta = await ComputedViewportMeta.request(artifacts, context);
if (!viewportMeta.isMobileOptimized) {
return {
Expand Down
11 changes: 10 additions & 1 deletion lighthouse-core/audits/seo/tap-targets.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -264,7 +264,7 @@ class TapTargets extends Audit {
title: str_(UIStrings.title),
failureTitle: str_(UIStrings.failureTitle),
description: str_(UIStrings.description),
requiredArtifacts: ['MetaElements', 'TapTargets'],
requiredArtifacts: ['MetaElements', 'TapTargets', 'TestedAsMobileDevice'],
};
}

Expand All @@ -274,6 +274,15 @@ class TapTargets extends Audit {
* @return {Promise<LH.Audit.Product>}
*/
static async audit(artifacts, context) {
if (!artifacts.TestedAsMobileDevice) {
// Tap target sizes aren't important for desktop SEO, so disable the audit there.
// On desktop people also tend to have more precise pointing devices than fingers.
Copy link
Collaborator

Choose a reason for hiding this comment

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

It might be an interesting exercise to try to find tap targets completely occluded by other elements at some point in the future 🤔

return {
rawValue: true,
notApplicable: true,
};
}

const viewportMeta = await ComputedViewportMeta.request(artifacts, context);
if (!viewportMeta.isMobileOptimized) {
return {
Expand Down
26 changes: 24 additions & 2 deletions lighthouse-core/test/audits/seo/font-size-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ describe('SEO: Font size audit', () => {
URL,
MetaElements: [],
FontSize: [],
TestedAsMobileDevice: true,
};

const auditResult = await FontSizeAudit.audit(artifacts, getFakeContext());
Expand All @@ -45,6 +46,7 @@ describe('SEO: Font size audit', () => {
{textLength: 31, fontSize: 11, node: {nodeId: 2, localName: 'p', attributes: []}},
],
},
TestedAsMobileDevice: true,
};

const auditResult = await FontSizeAudit.audit(artifacts, getFakeContext());
Expand All @@ -66,6 +68,7 @@ describe('SEO: Font size audit', () => {
{textLength: 0, fontSize: 11, node: {nodeId: 1, localName: 'p', attributes: []}},
],
},
TestedAsMobileDevice: true,
};

const auditResult = await FontSizeAudit.audit(artifacts, getFakeContext());
Expand All @@ -86,6 +89,7 @@ describe('SEO: Font size audit', () => {
{textLength: 22, fontSize: 11, node: {nodeId: 2, localName: 'p', attributes: []}},
],
},
TestedAsMobileDevice: true,
};
const auditResult = await FontSizeAudit.audit(artifacts, getFakeContext());
assert.equal(auditResult.rawValue, true);
Expand Down Expand Up @@ -123,6 +127,7 @@ describe('SEO: Font size audit', () => {
{textLength: 2, fontSize: 10, node: {nodeId: 3}, cssRule: style2},
],
},
TestedAsMobileDevice: true,
};
const auditResult = await FontSizeAudit.audit(artifacts, getFakeContext());

Expand All @@ -145,6 +150,7 @@ describe('SEO: Font size audit', () => {
{textLength: 10, fontSize: 10, node: {nodeId: 1, localName: 'p', attributes: []}},
],
},
TestedAsMobileDevice: true,
};
const auditResult = await FontSizeAudit.audit(artifacts, getFakeContext());
assert.equal(auditResult.rawValue, false);
Expand All @@ -167,11 +173,13 @@ describe('SEO: Font size audit', () => {
{textLength: 50, fontSize: 10, node: {nodeId: 1, localName: 'p', attributes: []}},
],
},
TestedAsMobileDevice: true,
};
const auditResult = await FontSizeAudit.audit(artifacts, getFakeContext());
assert.equal(auditResult.rawValue, false);
expect(auditResult.explanation)
.toBeDisplayString('100% of text is too small (based on 50% sample).');
expect(auditResult.explanation).toBeDisplayString(
'100% of text is too small (based on 50% sample).'
);
expect(auditResult.displayValue).toBeDisplayString('0% legible text');
});

Expand All @@ -189,6 +197,7 @@ describe('SEO: Font size audit', () => {
{textLength: 22, fontSize: 11, node: {nodeId: 2, localName: 'p', attributes: []}},
],
},
TestedAsMobileDevice: true,
};
const auditResult = await FontSizeAudit.audit(artifacts, getFakeContext());
expect(auditResult.displayValue).toBeDisplayString('89.78% legible text');
Expand All @@ -208,8 +217,21 @@ describe('SEO: Font size audit', () => {
{textLength: 4, fontSize: 11, node: {nodeId: 2, localName: 'p', attributes: []}},
],
},
TestedAsMobileDevice: true,
};
const auditResult = await FontSizeAudit.audit(artifacts, getFakeContext());
expect(auditResult.displayValue).toBeDisplayString('2.48% legible text');
});

it('is not applicable on desktop', async () => {
const artifacts = {
URL,
MetaElements: [],
FontSize: {},
TestedAsMobileDevice: false,
};
const auditResult = await FontSizeAudit.audit(artifacts, getFakeContext());
expect(auditResult.rawValue).toBe(true);
expect(auditResult.notApplicable).toBe(true);
});
});
17 changes: 13 additions & 4 deletions lighthouse-core/test/audits/seo/tap-targets-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,13 +12,14 @@ const assert = require('assert');

const getFakeContext = () => ({computedCache: new Map()});

function auditTapTargets(tapTargets, metaElements = [{
function auditTapTargets(tapTargets, {MetaElements = [{
name: 'viewport',
content: 'width=device-width',
}]) {
}], TestedAsMobileDevice = true} = {}) {
const artifacts = {
TapTargets: tapTargets,
MetaElements: metaElements,
MetaElements,
TestedAsMobileDevice,
};

return TapTargetsAudit.audit(artifacts, getFakeContext());
Expand Down Expand Up @@ -203,11 +204,19 @@ describe('SEO: Tap targets audit', () => {
});

it('fails if no meta viewport tag is provided', async () => {
const auditResult = await auditTapTargets([], []);
const auditResult = await auditTapTargets([], {MetaElements: []});
assert.equal(auditResult.rawValue, false);

expect(auditResult.explanation).toBeDisplayString(
/* eslint-disable max-len */
'Tap targets are too small because there\'s no viewport meta tag optimized for mobile screens');
});

it('is not applicable on desktop', async () => {
const auditResult = await auditTapTargets(getBorderlineTapTargets({
overlapSecondClientRect: true,
}), {TestedAsMobileDevice: false});
assert.equal(auditResult.rawValue, true);
assert.equal(auditResult.notApplicable, true);
});
});