Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: deprecate the use doT.js for messages #1938

Merged
merged 11 commits into from
Dec 18, 2019
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
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
29 changes: 27 additions & 2 deletions build/configure.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,17 @@
'use strict';

var clone = require('clone');
var dot = require('@deque/dot');
var templates = require('./templates');
var buildManual = require('./build-manual');
var entities = new (require('html-entities')).AllHtmlEntities();
var dotRegex = /\{\{.+?\}\}/g;
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't think I'm familiar with a .+? in regex, what does that do?

Copy link
Contributor Author

@straker straker Dec 17, 2019

Choose a reason for hiding this comment

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

The ? makes the .+ not greedy. So instead of {{foo bar}} {{foo}} matching the entire string, it will stop at the first set of }} so you get 2 matches. Not that it really matters in this case but I'm use to making things not greedy.

Copy link
Contributor

Choose a reason for hiding this comment

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

ahh, didn't know that. Cool


var descriptionHeaders =
'| Rule ID | Description | Impact | Tags | Enabled by default | Failures | Needs Review |\n| :------- | :------- | :------- | :------- | :------- | :------- | :------- |\n';

dot.templateSettings.strip = false;

function getLocale(grunt, options) {
var localeFile;
if (options.locale) {
Expand Down Expand Up @@ -52,6 +56,24 @@ function buildRules(grunt, options, commons, callback) {
}
var result = clone(data) || {};

if (result.messages) {
Object.keys(result.messages).forEach(function(key) {
// only convert to templated function for strings
// objects handled later in publish-metadata.js
if (
typeof result.messages[key] !== 'object' &&
dotRegex.test(result.messages[key])
) {
result.messages[key] = dot
.template(result.messages[key])
.toString();
}
});
}
//TODO this is actually failureSummaries, property name should better reflect that
if (result.failureMessage && dotRegex.test(result.failureMessage)) {
result.failureMessage = dot.template(result.failureMessage).toString();
}
return result;
}

Expand All @@ -68,8 +90,11 @@ function buildRules(grunt, options, commons, callback) {
function getIncompleteMsg(summaries) {
var result = {};
summaries.forEach(function(summary) {
if (summary.incompleteFallbackMessage) {
result = summary.incompleteFallbackMessage;
if (
summary.incompleteFallbackMessage &&
dotRegex.test(summary.incompleteFallbackMessage)
) {
result = dot.template(summary.incompleteFallbackMessage).toString();
}
});
return result;
Expand Down
2 changes: 1 addition & 1 deletion lib/checks/color/color-contrast.js
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ const data = {
contrastRatio: cr ? truncatedResult : undefined,
fontSize: `${((fontSize * 72) / 96).toFixed(1)}pt (${fontSize}px)`,
fontWeight: bold ? 'bold' : 'normal',
missingData: missing,
messageKey: missing,
expectedContrastRatio: cr.expectedContrastRatio + ':1'
};

Expand Down
22 changes: 10 additions & 12 deletions lib/checks/color/color-contrast.json
Original file line number Diff line number Diff line change
Expand Up @@ -8,18 +8,16 @@
"fail": "Element has insufficient color contrast of ${data.contrastRatio} (foreground color: ${data.fgColor}, background color: ${data.bgColor}, font size: ${data.fontSize}, font weight: ${data.fontWeight}). Expected contrast ratio of ${data.expectedContrastRatio}",
"incomplete": {
"default": "Unable to determine contrast ratio",
"missingData": {
"bgImage": "Element's background color could not be determined due to a background image",
"bgGradient": "Element's background color could not be determined due to a background gradient",
"imgNode": "Element's background color could not be determined because element contains an image node",
"bgOverlap": "Element's background color could not be determined because it is overlapped by another element",
"fgAlpha": "Element's foreground color could not be determined because of alpha transparency",
"elmPartiallyObscured": "Element's background color could not be determined because it's partially obscured by another element",
"elmPartiallyObscuring": "Element's background color could not be determined because it partially overlaps other elements",
"outsideViewport": "Element's background color could not be determined because it's outside the viewport",
"equalRatio": "Element has a 1:1 contrast ratio with the background",
"shortTextContent": "Element content is too short to determine if it is actual text content"
}
"bgImage": "Element's background color could not be determined due to a background image",
"bgGradient": "Element's background color could not be determined due to a background gradient",
"imgNode": "Element's background color could not be determined because element contains an image node",
"bgOverlap": "Element's background color could not be determined because it is overlapped by another element",
"fgAlpha": "Element's foreground color could not be determined because of alpha transparency",
"elmPartiallyObscured": "Element's background color could not be determined because it's partially obscured by another element",
"elmPartiallyObscuring": "Element's background color could not be determined because it partially overlaps other elements",
"outsideViewport": "Element's background color could not be determined because it's outside the viewport",
"equalRatio": "Element has a 1:1 contrast ratio with the background",
"shortTextContent": "Element content is too short to determine if it is actual text content"
}
}
}
Expand Down
4 changes: 2 additions & 2 deletions lib/checks/color/link-in-text-block.js
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ if (color.elementIsDistinct(node, parentBlock)) {
} else if (contrast >= 3.0) {
axe.commons.color.incompleteData.set('fgColor', 'bgContrast');
this.data({
missingData: axe.commons.color.incompleteData.get('fgColor')
messageKey: axe.commons.color.incompleteData.get('fgColor')
});
axe.commons.color.incompleteData.clear();
// User needs to check whether there is a hover and a focus style
Expand All @@ -74,7 +74,7 @@ if (color.elementIsDistinct(node, parentBlock)) {
}
axe.commons.color.incompleteData.set('fgColor', reason);
this.data({
missingData: axe.commons.color.incompleteData.get('fgColor')
messageKey: axe.commons.color.incompleteData.get('fgColor')
});
axe.commons.color.incompleteData.clear();
return undefined;
Expand Down
12 changes: 5 additions & 7 deletions lib/checks/color/link-in-text-block.json
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,11 @@
"fail": "Links need to be distinguished from surrounding text in some way other than by color",
"incomplete": {
"default": "Unable to determine contrast ratio",
"missingData": {
"bgContrast": "Element's contrast ratio could not be determined. Check for a distinct hover/focus style",
"bgImage": "Element's contrast ratio could not be determined due to a background image",
"bgGradient": "Element's contrast ratio could not be determined due to a background gradient",
"imgNode": "Element's contrast ratio could not be determined because element contains an image node",
"bgOverlap": "Element's contrast ratio could not be determined because of element overlap"
}
"bgContrast": "Element's contrast ratio could not be determined. Check for a distinct hover/focus style",
"bgImage": "Element's contrast ratio could not be determined due to a background image",
"bgGradient": "Element's contrast ratio could not be determined due to a background gradient",
"imgNode": "Element's contrast ratio could not be determined because element contains an image node",
"bgOverlap": "Element's contrast ratio could not be determined because of element overlap"
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion lib/checks/forms/fieldset.js
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ var data = {

var result = runCheck(virtualNode);
if (!result) {
data.failureCode = failureCode;
data.messageKey = failureCode;
}
this.data(data);

Expand Down
12 changes: 5 additions & 7 deletions lib/checks/forms/fieldset.json
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,11 @@
"pass": "Element is contained in a fieldset",
"fail": {
"default": "Element does not have a containing fieldset or ARIA group",
"failureCode": {
"no-legend": "Fieldset does not have a legend as its first child",
"empty-legend": "Legend does not have text that is visible to screen readers",
"mixed-inputs": "Fieldset contains unrelated inputs",
"no-group-label": "ARIA group does not have aria-label or aria-labelledby",
"group-mixed-inputs": "ARIA group contains unrelated inputs"
}
"no-legend": "Fieldset does not have a legend as its first child",
"empty-legend": "Legend does not have text that is visible to screen readers",
"mixed-inputs": "Fieldset contains unrelated inputs",
"no-group-label": "ARIA group does not have aria-label or aria-labelledby",
"group-mixed-inputs": "ARIA group contains unrelated inputs"
}
}
}
Expand Down
4 changes: 2 additions & 2 deletions lib/checks/forms/group-labelledby.js
Original file line number Diff line number Diff line change
Expand Up @@ -57,9 +57,9 @@ if (uniqueLabels.length > 0 && sharedLabels.length > 0) {
}

if (uniqueLabels.length > 0 && sharedLabels.length === 0) {
data.failureCode = 'no-shared-label';
data.messageKey = 'no-shared-label';
} else if (uniqueLabels.length === 0 && sharedLabels.length > 0) {
data.failureCode = 'no-unique-label';
data.messageKey = 'no-unique-label';
}

this.data(data);
Expand Down
6 changes: 2 additions & 4 deletions lib/checks/forms/group-labelledby.json
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,8 @@
"pass": "Elements with the name \"${data.name}\" have both a shared label, and a unique label, referenced through aria-labelledby",
"fail": {
"default": "Elements with the name \"${data.name}\" do not all have both a shared label, and a unique label referenced through aria-labelledby",
"failureCode": {
"no-shared-label": "Elements with the name \"${data.name}\" do not all have a shared label referenced through aria-labelledby",
"no-unique-label": "Elements with the name \"${data.name}\" do not all have a unique label referenced through aria-labelledby"
}
"no-shared-label": "Elements with the name \"${data.name}\" do not all have a shared label referenced through aria-labelledby",
"no-unique-label": "Elements with the name \"${data.name}\" do not all have a unique label referenced through aria-labelledby"
}
}
}
Expand Down
4 changes: 3 additions & 1 deletion lib/checks/lists/listitem.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,9 @@ if (parentRole === 'list') {
}

if (parentRole && axe.commons.aria.isValidRole(parentRole)) {
this.data('roleNotValid');
this.data({
messageKey: 'roleNotValid'
});
return false;
}

Expand Down
2 changes: 1 addition & 1 deletion lib/checks/lists/listitem.json
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
"pass": "List item has a <ul>, <ol> or role=\"list\" parent element",
"fail": {
"default": "List item does not have a <ul>, <ol> parent element",
"roleNotValid": "List item does not have a <ul>, <ol> without a role, or a role=\"list\" parent element"
"roleNotValid": "List item does not have a <ul>, <ol> parent element without a role, or a role=\"list\""
}
}
}
Expand Down
4 changes: 3 additions & 1 deletion lib/checks/shared/non-empty-if-present.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,9 @@ let type = (node.getAttribute('type') || '').toLowerCase();
let label = node.getAttribute('value');

if (label) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The message for this check used the existence of a label to determine the output, which doesn't work with the current schema. So I updated it since the data only needed to know a label was present and not what it was.

this.data('has-label');
this.data({
messageKey: 'has-label'
});
}

if (nodeName === 'INPUT' && ['submit', 'reset'].includes(type)) {
Expand Down
14 changes: 8 additions & 6 deletions lib/core/base/audit.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
/*global Rule, Check, RuleResult, commons: true */
var dotRegex = /\{\{.+?\}\}/g;
straker marked this conversation as resolved.
Show resolved Hide resolved

/*eslint no-unused-vars: 0*/
function getDefaultConfiguration(audit) {
'use strict';
Expand Down Expand Up @@ -146,10 +148,10 @@ const mergeCheckLocale = (a, b) => {
let { pass, fail } = b;
// If the message(s) are Strings, they have not yet been run
// thru doT (which will return a Function).
if (typeof pass === 'string') {
if (typeof pass === 'string' && dotRegex.test(pass)) {
pass = axe.imports.doT.compile(pass);
}
if (typeof fail === 'string') {
if (typeof fail === 'string' && dotRegex.test(fail)) {
fail = axe.imports.doT.compile(fail);
}
return {
Expand Down Expand Up @@ -177,10 +179,10 @@ const mergeRuleLocale = (a, b) => {
let { help, description } = b;
// If the message(s) are Strings, they have not yet been run
// thru doT (which will return a Function).
if (typeof help === 'string') {
if (typeof help === 'string' && dotRegex.test(help)) {
help = axe.imports.doT.compile(help);
}
if (typeof description === 'string') {
if (typeof description === 'string' && dotRegex.test(description)) {
description = axe.imports.doT.compile(description);
}
return {
Expand All @@ -198,7 +200,7 @@ const mergeFailureMessage = (a, b) => {
let { failureMessage } = b;
// If the message(s) are Strings, they have not yet been run
// thru doT (which will return a Function).
if (typeof failureMessage === 'string') {
if (typeof failureMessage === 'string' && dotRegex.test(failureMessage)) {
failureMessage = axe.imports.doT.compile(failureMessage);
}
return {
Expand All @@ -212,7 +214,7 @@ const mergeFailureMessage = (a, b) => {
*/

const mergeFallbackMessage = (a, b) => {
if (typeof b === 'string') {
if (typeof b === 'string' && dotRegex.test(b)) {
b = axe.imports.doT.compile(b);
}
return b || a;
Expand Down
4 changes: 3 additions & 1 deletion lib/core/reporters/helpers/incomplete-fallback-msg.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,5 +7,7 @@
*/
helpers.incompleteFallbackMessage = function incompleteFallbackMessage() {
'use strict';
return axe._audit.data.incompleteFallbackMessage;
return typeof axe._audit.data.incompleteFallbackMessage === 'function'
? axe._audit.data.incompleteFallbackMessage()
: axe._audit.data.incompleteFallbackMessage;
};
23 changes: 6 additions & 17 deletions lib/core/utils/process-message.js
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,10 @@ axe.utils.processMessage = function processMessage(message, data) {
if (Array.isArray(data)) {
data.values = data.join(', ');

if (message.singular || message.plural) {
if (
typeof message.singular === 'string' &&
typeof message.plural === 'string'
) {
const str = data.length === 1 ? message.singular : message.plural;
return substitute(str, data);
}
Expand All @@ -60,26 +63,12 @@ axe.utils.processMessage = function processMessage(message, data) {

// message uses value of data property to determine message
if (typeof message === 'object') {
straker marked this conversation as resolved.
Show resolved Hide resolved
// find the first message that matches the data or use the default
const keys = Object.keys(message).filter(key => key !== 'default');
let str = message.default || helpers.incompleteFallbackMessage();
for (let i = 0; data && i < keys.length; i++) {
const key = keys[i];
let value = data[key];

// incomplete reason
if (Array.isArray(value)) {
value = value[0].reason;
}

if (message[key][value]) {
str = message[key][value];
break;
}
if (data && data.messageKey && message[data.messageKey]) {
str = message[data.messageKey];
}

return processMessage(str, data);
}

return helpers.incompleteFallbackMessage();
};
59 changes: 57 additions & 2 deletions lib/core/utils/publish-metadata.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,42 @@
/* global helpers */
/**
* Construct incomplete message from check.data
* @param {Object} checkData Check result with reason specified
* @param {Object} messages Source data object with message options
* @return {String}
* @private
*/
function getIncompleteReason(checkData, messages) {
function getDefaultMsg(messages) {
if (messages.incomplete && messages.incomplete.default) {
// fall back to the default message if no reason specified
return messages.incomplete.default;
} else {
return helpers.incompleteFallbackMessage();
}
}
if (checkData && checkData.missingData) {
try {
var msg = messages.incomplete[checkData.missingData[0].reason];
if (!msg) {
throw new Error();
}
return msg;
} catch (e) {
if (typeof checkData.missingData === 'string') {
// return a string with the appropriate reason
return messages.incomplete[checkData.missingData];
} else {
return getDefaultMsg(messages);
}
}
} else if (checkData && checkData.messageKey) {
return messages.incomplete[checkData.messageKey];
} else {
return getDefaultMsg(messages);
}
}

/**
* Extend checksData with the correct result message
* @param {Object} checksData The check result data
Expand All @@ -13,12 +52,28 @@ function extender(checksData, shouldBeTrue) {
var data = Object.assign({}, sourceData);
delete data.messages;
if (check.result === undefined) {
data.message = messages.incomplete;
// handle old doT template
if (
typeof messages.incomplete === 'object' &&
!Array.isArray(check.data)
) {
data.message = getIncompleteReason(check.data, messages);
}

// fallback to new process message style
if (!data.message) {
data.message = messages.incomplete;
}
} else {
data.message =
check.result === shouldBeTrue ? messages.pass : messages.fail;
}
data.message = axe.utils.processMessage(data.message, check.data);

// don't process doT template functions
if (typeof data.message !== 'function') {
data.message = axe.utils.processMessage(data.message, check.data);
}

axe.utils.extendMetaData(check, data);
};
}
Expand Down
Loading