feat(rule): xml-lang-mismatch #999

jeeyyy · 2018-07-10T09:22:52Z

The rule checks that for the html element, there is no mismatch between the primary language in non-empty lang and xml:lang attributes, if both are used.

See also - https://auto-wcag.github.io/auto-wcag/rules/SC3-1-1-html-xml-lang-match.html for further details of the rule.

Closes issue:

New rule: xml-lang-mismatch #558

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: @WilcoFiers

jeeyyy · 2018-08-06T07:46:39Z

Tidying up. Closing PR, will re-open when work on this rule is started again.

jeeyyy · 2018-08-13T16:32:07Z

Updated PR. Reviews welcome.

WilcoFiers · 2018-08-14T11:48:51Z

lib/core/utils/get-base-lang.js

+ * @param {String} value value specified as lang or xml:lang attribute
+ * @return {String}
+ */
+axe.utils.getBaseLang = function getBaseLang(lang) {


This should be on axe.commons, not on axe.utils. axe.utils is reserved for things necessary in core.

WilcoFiers · 2018-08-14T11:58:48Z

lib/rules/xml-lang-mismatch.json

@@ -0,0 +1,18 @@
+{
+	"id": "xml-lang-mismatch",
+	"matches": "xml-lang-mismatch-matches.js",


We could simplify the matches by offloading some of the work to the selector: "selector": "html[lang][xml:lang]"

WilcoFiers · 2018-08-14T11:59:27Z

lib/rules/xml-lang-mismatch-matches.js

+const langAndXmlLangAttrs = Array.from(node.attributes).filter(attr => {
+	const attrName = attr.nodeName.toLowerCase();
+	const attrValue = attr.nodeValue;
+	if (!attrValue) {


Check that the subtag is valid here (I.e. its defined in the subtag registry). If not, it's inapplicable.

WilcoFiers · 2018-08-14T12:11:43Z

lib/checks/language/xml-lang-mismatch.json

+    "impact": "moderate",
+    "messages": {
+      "pass": "Value of lang or xml:lang attribute match if both are specified and included in the list of valid languages",
+      "fail": "Value of lang or xml:lang attribute either does not match or not included in the list of valid languages"


We should simplify, something like "Lang and xml:lang attributes do not have the same base language"

WilcoFiers · 2018-08-14T12:15:04Z

lib/checks/language/xml-lang-mismatch.js

+const isValid = axe.utils.validLangs().includes(langSubTagsToValidate[0]);
+
+// return
+return isValid;


This shouldn't look at valid langs. That's covered by another rule (see my comment on matches). Instead, all this should do is

const { getBaseLang } = axe.commons.text; return getBaseLang(node.getAttribute('lang')) === getBaseLang(node.getAttribute('xml:lang'));

WilcoFiers · 2018-08-14T12:27:19Z

lib/checks/language/xml-lang-mismatch.js

+
+// unique the produced array, if identical primary subtag values were supplied, this would result in an array of length 1.
+// eg: if lang='en-GB' and xml:lang='en-US', the below will produce langSubTagsToValidate = ['en']
+langSubTagsToValidate = axe.utils.uniqueArray(langSubTagsToValidate, []);


Deduplicating an array is a very round-about way to check that two values are the same. Please just use two variables and do an equality comparison.

WilcoFiers · 2018-08-14T12:35:42Z

test/core/utils/get-base-lang.js

+
+	it('returns false when expecting fr instead of en', function() {
+		var actual = axe.utils.getBaseLang('en');
+		assert.notEqual(actual, 'fr');


This seems unnecessary. What you should check is case sensitivity though.

WilcoFiers · 2018-08-14T12:37:15Z

test/core/utils/get-base-lang.js

@@ -0,0 +1,18 @@
+describe('axe.utils.getBaseLang', function() {


Also check things with multiple dashes. What should this do 'foo-bar-baz'? It's clearly invalid, so maybe this needs to return empty or null?

WilcoFiers · 2018-08-14T12:40:21Z

test/checks/language/xml-lang-mismatch.js

+	var checkContext = axe.testUtils.MockCheckContext();
+
+	beforeEach(function() {
+		node = document.createElement('html');


You could use a div instead, so you don't get into browser support trouble. The check is agnostic to what node its testing.

jeeyyy · 2018-08-14T16:15:50Z

lib/checks/language/xml-lang-mismatch.json

@@ -0,0 +1,11 @@
+{
+  "id": "xml-lang-mismatch",


Updated. Review again.

WilcoFiers · 2018-08-15T15:05:17Z

lib/checks/language/xml-lang-mismatch.js

+const primaryLangValue = getBaseLang(node.getAttribute('lang') || '');
+const primaryXmlLangValue = getBaseLang(node.getAttribute('xml:lang') || '');
+
+return primaryLangValue === primaryXmlLangValue;


This could be the cleanest looking check we've got. Great job Jey.

WilcoFiers

Couple of very minor points.

WilcoFiers · 2018-08-15T15:10:05Z

lib/commons/utils/get-base-lang.js

+ * @param {String} value value specified as lang or xml:lang attribute
+ * @return {String}
+ */
+axe.utils.getBaseLang = function getBaseLang(lang) {


Very minor: I'd suggest letting this function handle the null case, so that you don't have to do it separately a bunch of times.

WilcoFiers · 2018-08-15T15:13:02Z

lib/rules/html-xml-lang-mismatch-matches.js

@@ -0,0 +1,16 @@
+// Rule: xml-lang-mismatch


This comment is wrong. I'd also rename this to xml-lang-mismatch-matches.js. It's a fair bet we're going to reuse this for the non-HTML version.

WilcoFiers · 2018-08-15T15:16:09Z

lib/rules/html-xml-lang-mismatch.json

+	],
+	"metadata": {
+		"description": "Ensure that HTML elements with both valid lang and xml:lang attributes agree on the base language of the page",
+	  "help": "HTML elements with lang and xml:lang must have the same base language"


Inconsistent spacing.

jeeyyy · 2018-08-15T18:34:51Z

@WilcoFiers
All minor points, attended to. Cheers.

jeeyyy added 2 commits July 10, 2018 09:58

feat: rule xml-lang-mismatch on body and child nodes

04022cb

test: add integration tests

4120f3c

jeeyyy requested a review from WilcoFiers as a code owner July 10, 2018 09:22

Merge branch 'develop' into new-rule-wcag20-xml-lang-mismatch-issue-558

f0ea8ff

jeeyyy changed the title ~~feat(rule): xml-lang-mismatch~~ [WIP] feat(rule): xml-lang-mismatch Jul 11, 2018

jeeyyy closed this Aug 6, 2018

fix: update rule for checking lang and xml:lang on html element

3541c90

jeeyyy reopened this Aug 13, 2018

jeeyyy requested a review from dylanb as a code owner August 13, 2018 16:23

jeeyyy changed the title ~~[WIP] feat(rule): xml-lang-mismatch~~ feat(rule): xml-lang-mismatch Aug 13, 2018

Merge branch 'develop' into new-rule-wcag20-xml-lang-mismatch-issue-558

05e3544

jeeyyy requested review from marcysutton, stephenmathieson and isner August 13, 2018 16:31

Merge branch 'develop' into new-rule-wcag20-xml-lang-mismatch-issue-558

c909c50

WilcoFiers previously requested changes Aug 14, 2018

View reviewed changes

jeeyyy added 4 commits August 14, 2018 16:49

fix: refactor based on review

30fcefc

doc: update rule description

0d471c8

test: refactor and add more integration tests

1c50f83

refactor: rename rule id

76b9515

jeeyyy requested a review from a team as a code owner August 14, 2018 16:14

Merge branch 'develop' into new-rule-wcag20-xml-lang-mismatch-issue-558

8cb1aab

jeeyyy mentioned this pull request Aug 15, 2018

New rule: xml-lang-mismatch #558

Closed

WilcoFiers reviewed Aug 15, 2018

View reviewed changes

jeeyyy added 2 commits August 15, 2018 19:33

fix: refactor based on review

fbf6a54

Merge branch 'develop' into new-rule-wcag20-xml-lang-mismatch-issue-558

57c66b7

WilcoFiers approved these changes Aug 16, 2018

View reviewed changes

Merge branch 'develop' into new-rule-wcag20-xml-lang-mismatch-issue-558

31e37e9

WilcoFiers merged commit 7452a51 into develop Aug 16, 2018

WilcoFiers deleted the new-rule-wcag20-xml-lang-mismatch-issue-558 branch August 16, 2018 10:14

jeeyyy mentioned this pull request Sep 5, 2018

New rule: xml-lang #531

Closed

jeeyyy mentioned this pull request Nov 2, 2018

Refactor: html lang rules #1066

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(rule): xml-lang-mismatch #999

feat(rule): xml-lang-mismatch #999

jeeyyy commented Jul 10, 2018 •

edited by WilcoFiers

Loading

jeeyyy commented Aug 6, 2018

jeeyyy commented Aug 13, 2018

WilcoFiers Aug 14, 2018

WilcoFiers Aug 14, 2018

WilcoFiers Aug 14, 2018

WilcoFiers Aug 14, 2018

WilcoFiers Aug 14, 2018

WilcoFiers Aug 14, 2018

WilcoFiers Aug 14, 2018

WilcoFiers Aug 14, 2018

WilcoFiers Aug 14, 2018

This comment was marked as outdated.

This comment was marked as resolved.

jeeyyy Aug 14, 2018

WilcoFiers Aug 15, 2018

WilcoFiers left a comment

WilcoFiers Aug 15, 2018

jeeyyy Aug 15, 2018

WilcoFiers Aug 15, 2018

WilcoFiers Aug 15, 2018

jeeyyy commented Aug 15, 2018

		@@ -0,0 +1,18 @@
		describe('axe.utils.getBaseLang', function() {

feat(rule): xml-lang-mismatch #999

feat(rule): xml-lang-mismatch #999

Conversation

jeeyyy commented Jul 10, 2018 • edited by WilcoFiers Loading

Reviewer checks

jeeyyy commented Aug 6, 2018

jeeyyy commented Aug 13, 2018

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

This comment was marked as outdated.

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WilcoFiers 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

jeeyyy commented Aug 15, 2018

jeeyyy commented Jul 10, 2018 •

edited by WilcoFiers

Loading