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

i18n: tap targets #7111

Merged
merged 6 commits into from
Feb 4, 2019
Merged

i18n: tap targets #7111

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
63906f6
Make UISTRINGS_REGEX not global
mattzeunert Jan 28, 2019
da0339a
i18n for tap targets audit
mattzeunert Jan 27, 2019
a543af6
Limit ESLint disable
mattzeunert Jan 30, 2019
f90606e
Update sample json
mattzeunert Jan 30, 2019
287a105
Fix tests
mattzeunert Jan 30, 2019
840eaf5
Test case for display value with percentage
mattzeunert Feb 4, 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
42 changes: 32 additions & 10 deletions lighthouse-core/audits/seo/tap-targets.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,29 @@ const {
getLargestRect,
} = require('../../lib/rect-helpers');
const {getTappableRectsFromClientRects} = require('../../lib/tappable-rects');
const i18n = require('../../lib/i18n/i18n.js');

const UIStrings = {
/** Title of a Lighthouse audit that provides detail on whether tap targets (like buttons and links) on a page are big enough so they can easily be tapped on a mobile device. This descriptive title is shown when tap targets are easy to tap on. */
title: 'Tap targets are sized appropriately',
/** Title of a Lighthouse audit that provides detail on whether tap targets (like buttons and links) on a page are big enough so they can easily be tapped on a mobile device. This descriptive title is shown when tap targets are not easy to tap on. */
failureTitle: 'Tap targets are not sized appropriately',
/** Description of a Lighthouse audit that tells the user why buttons and links need to be big enough and what 'big enough' means. This is displayed after a user expands the section to see more. No character length limits. 'Learn More' becomes link text to additional documentation. */
description: 'Interactive elements like buttons and links should be large enough (48x48px), 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).',
/** Label of a table column that identifies tap targets (like buttons and links) that have failed the audit and aren't easy to tap on. */
tapTargetHeader: 'Tap Target',
/** Label of a table column that specifies the size of tap targets like buttons and links. */
sizeHeader: 'Size',
/** Label of a table column that identifies a tap target (like a link or button) that overlaps with another tap target. */
overlappingTargetHeader: 'Overlapping Target',
/** Explanatory message stating that there was a failure in an audit caused by the viewport meta tag not being optimized for mobile screens, which caused tap targets like buttons and links to be too small to tap on. */
/* eslint-disable max-len */
explanationViewportMetaNotOptimized: 'Tap targets are too small because there\'s no viewport meta tag optimized for mobile screens',
/** Explanatory message stating that a certain percentage of the tap targets (like buttons and links) on the page are of an appropriately large size. */
displayValue: '{decimalProportion, number, percent} appropriately sized tap targets',
Copy link
Collaborator

Choose a reason for hiding this comment

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

is percent it's own thing already or do we need to make a custom one? I definitely see extendedPercent

Copy link
Member

Choose a reason for hiding this comment

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

percent is a default iirc, it just prunes decimals which is why we needed extendedPercent. As long as we are okay with no decimals then we should be good with percent here. Might need to add this to the LR tc bundler though, since I don't think we've ever used just percent so far.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Might need to add this to the LR tc bundler though, since I don't think we've ever used just percent so far.

What does this mean, and is it something I have to do?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Anything left for me to do here? Or is this PR good to go?

Copy link
Collaborator

Choose a reason for hiding this comment

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

AFAICT, we never actually flex this in the tests, can we add an assertion for it looking like we expect? then I'm good :)

off to @exterkamp on the "LR tc" bit since I'm not 100% where that change would be made either

};

const str_ = i18n.createMessageInstanceIdFn(__filename, UIStrings);

const FINGER_SIZE_PX = 48;
// Ratio of the finger area tapping on an unintended element
Expand Down Expand Up @@ -244,10 +267,9 @@ class TapTargets extends Audit {
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 (48x48px), 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).',
title: str_(UIStrings.title),
failureTitle: str_(UIStrings.failureTitle),
description: str_(UIStrings.description),
requiredArtifacts: ['MetaElements', 'TapTargets'],
};
}
Expand All @@ -261,8 +283,7 @@ class TapTargets extends Audit {
if (!hasViewportSet) {
return {
rawValue: false,
// eslint-disable-next-line
explanation: 'Tap targets are too small because there\'s no viewport meta tag optimized for mobile screens',
explanation: str_(UIStrings.explanationViewportMetaNotOptimized),
};
}

Expand All @@ -272,9 +293,9 @@ class TapTargets extends Audit {
const tableItems = getTableItems(overlapFailuresForDisplay);

const headings = [
{key: 'tapTarget', itemType: 'node', text: 'Tap Target'},
{key: 'size', itemType: 'text', text: 'Size'},
{key: 'overlappingTarget', itemType: 'node', text: 'Overlapping Target'},
{key: 'tapTarget', itemType: 'node', text: str_(UIStrings.tapTargetHeader)},
{key: 'size', itemType: 'text', text: str_(UIStrings.sizeHeader)},
{key: 'overlappingTarget', itemType: 'node', text: str_(UIStrings.overlappingTargetHeader)},
];

const details = Audit.makeTableDetails(headings, tableItems);
Expand All @@ -284,7 +305,7 @@ class TapTargets extends Audit {
const passingTapTargetCount = tapTargetCount - failingTapTargetCount;

const score = tapTargetCount > 0 ? passingTapTargetCount / tapTargetCount : 1;
const displayValue = Math.round(score * 100) + '% appropriately sized tap targets';
const displayValue = str_(UIStrings.displayValue, {decimalProportion: score});

return {
rawValue: tableItems.length === 0,
Expand All @@ -298,6 +319,7 @@ class TapTargets extends Audit {
TapTargets.FINGER_SIZE_PX = FINGER_SIZE_PX;

module.exports = TapTargets;
module.exports.UIStrings = UIStrings;


/** @typedef {{
Expand Down
32 changes: 32 additions & 0 deletions lighthouse-core/lib/i18n/en-US.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -739,6 +739,38 @@
"message": "Document uses legible font sizes",
"description": "Imperative title of a Lighthouse audit that tells the user that they should use font sizes that are easily read by the user. This is displayed in a list of audit titles that Lighthouse generates."
},
"lighthouse-core/audits/seo/tap-targets.js | description": {
"message": "Interactive elements like buttons and links should be large enough (48x48px), 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).",
"description": "Description of a Lighthouse audit that tells the user why buttons and links need to be big enough and what 'big enough' means. This is displayed after a user expands the section to see more. No character length limits. 'Learn More' becomes link text to additional documentation."
},
"lighthouse-core/audits/seo/tap-targets.js | displayValue": {
"message": "{decimalProportion, number, percent} appropriately sized tap targets",
"description": "Explanatory message stating that a certain percentage of the tap targets (like buttons and links) on the page are of an appropriately large size."
},
"lighthouse-core/audits/seo/tap-targets.js | explanationViewportMetaNotOptimized": {
"message": "Tap targets are too small because there's no viewport meta tag optimized for mobile screens",
"description": "Explanatory message stating that there was a failure in an audit caused by the viewport meta tag not being optimized for mobile screens, which caused tap targets like buttons and links to be too small to tap on."
},
"lighthouse-core/audits/seo/tap-targets.js | failureTitle": {
"message": "Tap targets are not sized appropriately",
"description": "Title of a Lighthouse audit that provides detail on whether tap targets (like buttons and links) on a page are big enough so they can easily be tapped on a mobile device. This descriptive title is shown when tap targets are not easy to tap on."
},
"lighthouse-core/audits/seo/tap-targets.js | overlappingTargetHeader": {
"message": "Overlapping Target",
"description": "Label of a table column that identifies a tap target (like a link or button) that overlaps with another tap target."
},
"lighthouse-core/audits/seo/tap-targets.js | sizeHeader": {
"message": "Size",
"description": "Label of a table column that specifies the size of tap targets like buttons and links."
},
"lighthouse-core/audits/seo/tap-targets.js | tapTargetHeader": {
"message": "Tap Target",
"description": "Label of a table column that identifies tap targets (like buttons and links) that have failed the audit and aren't easy to tap on."
},
"lighthouse-core/audits/seo/tap-targets.js | title": {
"message": "Tap targets are sized appropriately",
"description": "Title of a Lighthouse audit that provides detail on whether tap targets (like buttons and links) on a page are big enough so they can easily be tapped on a mobile device. This descriptive title is shown when tap targets are easy to tap on."
},
"lighthouse-core/audits/time-to-first-byte.js | description": {
"message": "Time To First Byte identifies the time at which your server sends a response. [Learn more](https://developers.google.com/web/tools/lighthouse/audits/ttfb).",
"description": "Description of a Lighthouse audit that tells the user *why* they should reduce the amount of time it takes their server to start responding to requests. This is displayed after a user expands the section to see more. No character length limits. 'Learn More' becomes link text to additional documentation."
Expand Down
2 changes: 1 addition & 1 deletion lighthouse-core/scripts/i18n/collect-strings.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ const path = require('path');
const esprima = require('esprima');

const LH_ROOT = path.join(__dirname, '../../../');
const UISTRINGS_REGEX = /UIStrings = (.|\s)*?\};\n/gim;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

If this regex is global it tries to match any content following module.exports.UIStrings = UIStrings; and then backtracks. If there's some content after that line the collect-strings script hangs.

Copy link
Collaborator

Choose a reason for hiding this comment

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

btw this no longer collects all matches, but that's fine cuz we only ever use the first instance.

const UISTRINGS_REGEX = /UIStrings = (.|\s)*?\};\n/im;

/**
* @typedef ICUMessageDefn
Expand Down
6 changes: 5 additions & 1 deletion 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 @@ -116,6 +116,7 @@ describe('SEO: Tap targets audit', () => {
it('passes when there are no tap targets', () => {
const auditResult = auditTapTargets([]);
assert.equal(auditResult.rawValue, true);
expect(auditResult.displayValue).toBeDisplayString('100% appropriately sized tap targets');
assert.equal(auditResult.score, 1);
});

Expand Down Expand Up @@ -202,6 +203,9 @@ describe('SEO: Tap targets audit', () => {
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'));

expect(auditResult.explanation).toBeDisplayString(
/* eslint-disable max-len */
'Tap targets are too small because there\'s no viewport meta tag optimized for mobile screens');
});
});
6 changes: 6 additions & 0 deletions lighthouse-core/test/results/sample_v2.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4999,6 +4999,12 @@
"path": "audits[font-size].displayValue"
}
],
"lighthouse-core/audits/seo/tap-targets.js | failureTitle": [
"audits[tap-targets].title"
],
"lighthouse-core/audits/seo/tap-targets.js | description": [
"audits[tap-targets].description"
],
"lighthouse-core/config/default-config.js | performanceCategoryTitle": [
"categories.performance.title"
],
Expand Down