feat: add runtime localization support #1036

stephenmathieson · 2018-07-31T18:40:08Z

This patch adds support for runtime localization. A user may now provide a locale object (in the same shape as the locales in /locale/*.json) to axe.configure().

For example:

axe.configure({
  locale: {
    rules: {
      'some-rule': {
        help: 'the help message',
        description: 'the description'
      }
    },
    checks: {
      [...]
    }
  }
})

These locale strings are run thru doT, enabling template support. For example:

axe.configure({
  locale: {
    rules: {
      foo: {
        help: 'some help',
        description: 'foo: {{~it.data:value}} {{=value}}{{~}}.'
      }
    },
    checks: {
      bar: {
        pass: 'the pass message',
        fail: 'something useful: {{~it.data:value}} {{=value}}{{~}}.'
      }
    }
  }
})

Upon calling axe.reset(), any custom locale will be reset to the "default" axe-core shipped with.

Reviewer checks

Required fields, to be filled out by PR reviewer(s)

Follows the commit message policy, appropriate for next version
Has documentation updated, a DU ticket, or requires no documentation change
Includes new tests, or was unnecessary
Code is reviewed for security by: @isner, @JKODU

This patch adds support for runtime localization. A user may now provide a `locale` object (in the same shape as the locales in `/locale/*.json`) to `axe.configure()`. For example: ```js axe.configure({ locale: { rules: { 'some-rule': { help: 'the help message', description: 'the description' } }, checks: { [...] } } }) ``` These locale strings are run thru `doT`, enabling template support. For example: ```js axe.configure({ locale: { rules: { foo: { help: 'some help', description: 'foo: {{~it.data:value}} {{=value}}{{~}}.' } }, checks: { bar: { pass: 'the pass message', fail: 'something useful: {{~it.data:value}} {{=value}}{{~}}.' } } } }) ``` Upon calling `axe.reset()`, any custom locale will be reset to the "default" axe-core shipped with.

isner

Works like a charm!

isner · 2018-08-01T14:05:05Z

doc/API.md

@@ -183,6 +185,7 @@ axe.configure({
      * `tags` - array(optional, default `[]`). A list if the tags that "classify" the rule. In practice, you must supply some valid tags or the default evaluation will not invoke the rule. The convention is to include the standard (WCAG 2 and/or section 508), the WCAG 2 level, Section 508 paragraph, and the WCAG 2 success criteria. Tags are constructed by converting all letters to lower case, removing spaces and periods and concatinating the result. E.g. WCAG 2 A success criteria 1.1.1 would become ["wcag2a", "wcag111"]
      * `matches` - string(optional, default `*`). A filtering CSS selector that will exclude elements that do not match the CSS selector.
  * `disableOtherRules` - Disables all rules not included in the `rules` property.
+  * `locale` - A locale object to apply (at runtime) to all rules and checks, in the same shape as `/locales/*.json`.


Is this enough for users to understand how to utilize the templating feature?

Probably not. I'm hoping to get some feedback here that helps me create the missing portion(s) of documentation.

You could add a note to the README: https://github.com/dequelabs/axe-core#localization

@WilcoFiers and I have plans to work on documentation for this today.

marcysutton · 2018-08-02T01:20:31Z

doc/API.md

@@ -183,6 +185,7 @@ axe.configure({
      * `tags` - array(optional, default `[]`). A list if the tags that "classify" the rule. In practice, you must supply some valid tags or the default evaluation will not invoke the rule. The convention is to include the standard (WCAG 2 and/or section 508), the WCAG 2 level, Section 508 paragraph, and the WCAG 2 success criteria. Tags are constructed by converting all letters to lower case, removing spaces and periods and concatinating the result. E.g. WCAG 2 A success criteria 1.1.1 would become ["wcag2a", "wcag111"]
      * `matches` - string(optional, default `*`). A filtering CSS selector that will exclude elements that do not match the CSS selector.
  * `disableOtherRules` - Disables all rules not included in the `rules` property.
+  * `locale` - A locale object to apply (at runtime) to all rules and checks, in the same shape as `/locales/*.json`.


You could add a note to the README: https://github.com/dequelabs/axe-core#localization

marcysutton · 2018-08-02T01:24:01Z

lib/core/base/audit.js

+	}
+
+	const locale = {
+		checks: {},


Should we keep track of a lang key here for any reason? The existing locale files include that alongside rules and checks.

It's not used by anything, so saving it didn't seem super important. What do you think we need it for?

It's a good point. I think we should include a lang key on the results object. I think we should do that separately from this PR though.

marcysutton · 2018-08-02T01:33:54Z

lib/core/base/audit.js

+		fail = axe.imports.doT.compile(fail);
+	}
+	return {
+		impact: a.impact,


What if they want to override the impact?

This is not currently supported with the existing localization setup. Adding it now falls out of scope of the task I was given.

If it's something we know we want, we can add support after this PR lands.

+1 to Stephen's comment. You can change impact by doing this:

axe.configure({ checks: [{ id:'foo', metadata: { impact: 'minor' } }] });

I think that instead of copying over impact, we should just spread the rest of the props in (...a). That way we can't accidentally forget to copy stuff over if we add stuff to metadata in the future.

👍 I'll spread a here.

WilcoFiers · 2018-08-05T14:16:24Z

lib/core/base/audit.js

+ *        pass: string | function,
+ *        fail: string | function,
+ *        incomplete: string | {
+ *          [key: string]: string | function


I don't think the value can be a function.

It can, and will be. The result of calling dot.compile() on the string is a function.

WilcoFiers · 2018-08-05T14:20:52Z

lib/core/base/audit.js

+ * to the default ("first") configuration.
+ */
+
+Audit.prototype._setPreviousLocale = function() {


Not a huge deal, but I'd prefer we use named functions where we can in axe-core. It tends to make debugging a lot easier.

I think you're mistaking this method for a function expression. Because this is a method, we'll see its implicit name in stack traces.

Given the code:

function F () {} F.prototype.foo = function () { throw new Error('boom') } const f = new F f.foo()

We'll clearly see "foo" in the stack trace:

node foo.js /Users/stephen/dev/src/github.com/dequelabs/axe-core/foo.js:4 throw new Error('boom') ^ Error: boom at F.foo (/Users/stephen/dev/src/github.com/dequelabs/axe-core/foo.js:4:9) at Object.<anonymous> (/Users/stephen/dev/src/github.com/dequelabs/axe-core/foo.js:8:3) at Module._compile (internal/modules/cjs/loader.js:702:30) at Object.Module._extensions..js (internal/modules/cjs/loader.js:713:10) at Module.load (internal/modules/cjs/loader.js:612:32) at tryModuleLoad (internal/modules/cjs/loader.js:551:12) at Function.Module._load (internal/modules/cjs/loader.js:543:3) at Function.Module.runMain (internal/modules/cjs/loader.js:744:10) at startup (internal/bootstrap/node.js:238:19) at bootstrapNodeJSCore (internal/bootstrap/node.js:572:3)

WilcoFiers · 2018-08-05T14:37:40Z

lib/core/base/audit.js

+		fail = axe.imports.doT.compile(fail);
+	}
+	return {
+		impact: a.impact,


+1 to Stephen's comment. You can change impact by doing this:

axe.configure({ checks: [{ id:'foo', metadata: { impact: 'minor' } }] });

I think that instead of copying over impact, we should just spread the rest of the props in (...a). That way we can't accidentally forget to copy stuff over if we add stuff to metadata in the future.

WilcoFiers · 2018-08-05T14:38:28Z

lib/core/base/audit.js

+	}
+	return {
+		help: help || a.help,
+		description: description || a.description


This is missing the helpUrl property. I think we should spread a (...a) so we just copy over the remainder.

Sounds good, will update!

WilcoFiers · 2018-08-05T14:45:33Z

lib/core/base/audit.js

+ */
+
+Audit.prototype._setPreviousLocale = function() {
+	if (this._previousLocale) {


I think instead of calling this previousLocale, we should call this defaultLocale or originalLocale. Same for the function names. We're not going back to the previous one. This is going back to the original locale (which is what reset should do). You may want to put a test in place for that as well.

+1 or fallbackLocale?

It's not a fallback, but instead, it's the default. I agree with Wilco that it should be called defaultLocale.

WilcoFiers · 2018-08-05T14:51:50Z

lib/core/base/audit.js

+	}
+
+	const locale = {
+		checks: {},


It's a good point. I think we should include a lang key on the results object. I think we should do that separately from this PR though.

WilcoFiers · 2018-08-05T14:57:32Z

test/core/public/configure.js

@@ -239,4 +239,230 @@ describe('axe.configure', function() {
 		assert.equal(axe._audit.rules[3].id, 'black-panther');
 		assert.equal(axe._audit.rules[3].enabled, true);
 	});
+
+	describe('given a locale object', function() {


I think we need tests for:

An axe.configure call with both metadata and a locale, make sure that locale is always used over metadata (should it throw if we're trying to set both?)

Test that if you pass an object into the message function, the template ran correctly.

Tests have been added.

New commits :)

jeeyyy

Lgtm.

stephenmathieson requested review from WilcoFiers, marcysutton, isner and jeeyyy July 31, 2018 18:40

isner previously approved these changes Aug 1, 2018

View reviewed changes

isner reviewed Aug 1, 2018

View reviewed changes

marcysutton reviewed Aug 2, 2018

View reviewed changes

Merge branch 'develop' into add-runtime-locale

1067f4a

WilcoFiers previously requested changes Aug 5, 2018

View reviewed changes

Merge branch 'develop' into add-runtime-locale

b5b64d8

stephenmathieson requested a review from dylanb as a code owner August 8, 2018 13:28

chore: updates from code review

8b2c704

stephenmathieson dismissed isner’s stale review via 8b2c704 August 8, 2018 13:55

doc(README): add runtime locale info

a60a9ef

jeeyyy approved these changes Aug 8, 2018

View reviewed changes

Merge branch 'develop' into add-runtime-locale

e036e85

stephenmathieson merged commit 7d4b70f into develop Aug 8, 2018

stephenmathieson deleted the add-runtime-locale branch August 8, 2018 19:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add runtime localization support #1036

feat: add runtime localization support #1036

stephenmathieson commented Jul 31, 2018 •

edited by jeeyyy

Loading

isner left a comment

isner Aug 1, 2018

stephenmathieson Aug 1, 2018

marcysutton Aug 2, 2018

stephenmathieson Aug 2, 2018

marcysutton Aug 2, 2018

marcysutton Aug 2, 2018

stephenmathieson Aug 2, 2018

WilcoFiers Aug 5, 2018

marcysutton Aug 2, 2018

stephenmathieson Aug 2, 2018

WilcoFiers Aug 5, 2018

stephenmathieson Aug 8, 2018

WilcoFiers Aug 5, 2018

stephenmathieson Aug 8, 2018

WilcoFiers Aug 5, 2018

stephenmathieson Aug 8, 2018

WilcoFiers Aug 5, 2018

WilcoFiers Aug 5, 2018

stephenmathieson Aug 8, 2018

WilcoFiers Aug 5, 2018

jeeyyy Aug 8, 2018 •

edited

Loading

stephenmathieson Aug 8, 2018

WilcoFiers Aug 5, 2018

WilcoFiers Aug 5, 2018

stephenmathieson Aug 8, 2018

jeeyyy left a comment

feat: add runtime localization support #1036

feat: add runtime localization support #1036

Conversation

stephenmathieson commented Jul 31, 2018 • edited by jeeyyy Loading

Reviewer checks

isner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeeyyy Aug 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeeyyy left a comment

Choose a reason for hiding this comment

stephenmathieson commented Jul 31, 2018 •

edited by jeeyyy

Loading

jeeyyy Aug 8, 2018 •

edited

Loading