feat(rule): Label and Name from Content mismatch WCAG21 (Issue #1149) #1335

jeeyyy · 2019-01-28T11:16:04Z

This PR does the below:

Implements a new rule -> label and name from content mismatch.
This PR has been branched from PR fix: Solve several accessible-name issues #1163 , to utilise the a11y-name computation, so reviews required only on commit - ef1c638, also ideally this should be merged after the earlier said PR is merged.

Closes issue:

Rule: SC2-5-3-label-content-name-mismatch (PR #1159) #1149

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

lib/checks/label/label-content-name-mismatch.js

lib/rules/label-content-name-mismatch.json

lib/rules/label-content-name-mismatch-matches.js

lib/checks/label/label-content-name-mismatch.js

lib/checks/label/label-content-name-mismatch.json

lib/rules/label-content-name-mismatch-matches.js

lib/checks/label/label-content-name-mismatch.js

lib/commons/text/is-human-interpretable.js

test/commons/text/is-human-interpretable.js

Updated.

WilcoFiers · 2019-02-18T13:44:57Z

package.json

@@ -86,6 +86,7 @@
 		"clone": "~2.1.1",
 		"css-selector-parser": "^1.3.0",
 		"dot": "~1.1.2",
+		"emoji-regex": "^7.0.3",


I'm not comfortable pulling in package that is maintained by a single person, and that has very few weekly download. Either we lock down the version of this and do a security audit on it, or we ship our own.

Locked version to 7.0.3 and performed an audit.

WilcoFiers · 2019-02-18T13:47:11Z

test/commons/text/punctuation.js

@@ -0,0 +1,16 @@
+describe('text.replacePunctuation', function() {


Add tests for the two other sequences you've got in punctuation.js

WilcoFiers · 2019-02-18T13:48:09Z

lib/commons/text/punctuation.js

+ * @param {String} replaceWith (Optional) replacement string
+ * @returns {String}
+ */
+text.replacePunctuation = function replacePunctuation(str, replaceWith = '') {


Rename this file and its tests to replace-punctuation.js so that its consistent with the function.

WilcoFiers · 2019-02-18T13:49:48Z

lib/commons/text/unicode.js

+ * @param {String} str string to operate on
+ * @returns {String}
+ */
+text.replaceEmojiUnicode = function replaceEmojiUnicode(str, replaceWith = '') {


Is there a reason you put the punctuation stuff in a different file, but the emoji stuff in a shared file with nonBmp? Probably best to split those out too.

WilcoFiers · 2019-02-18T13:54:03Z

lib/commons/text/unicode.js

+ * @param {String} str string to verify
+ * @returns {Boolean}
+ */
+text.isNonBmpUnicode = function isNonBmpUnicode(str) {


I'd say this needs to be called hasNonBmpUnicode, seeing as you're checking if the string includes any such characters, rather than that it only contains those chars. Same for the other functions.

WilcoFiers · 2019-02-19T12:32:09Z

test/integration/rules/label-content-name-mismatch/label-content-name-mismatch.json

+{
+	"description": "label-content-name-mismatch tests",
+	"rule": "label-content-name-mismatch",
+	"violations": [["#fail1"], ["#fail2"], ["#fail3"], ["#fail4"], ["#fail5"]],


Please add "incomplete" integration tests as well.

WilcoFiers · 2019-02-19T12:33:28Z

test/rule-matches/label-content-name-mismatch-matches.js

+	});
+
+	it('returns false if element role is not supported with name from contents', function() {
+		var vNode = queryFixture('<textarea id="target"></textarea>');


This elm could just as easily be false because it doesn't have an accessible name and/or text content. This test doesn't prove what you say it does.

WilcoFiers · 2019-02-19T12:35:38Z

test/rule-matches/label-content-name-mismatch-matches.js

+		assert.isFalse(actual);
+	});
+
+	it('returns true for non-widget role that does support name from content', function() {


I don't think you resolved my comment here.

Tooltip is a widget role - https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/ARIA_Techniques

This is a typo in the test description. Fixed.

WilcoFiers · 2019-02-19T12:36:16Z

test/rule-matches/label-content-name-mismatch-matches.js

+
+	it('returns false for hidden (non visible) text content', function() {
+		var vNode = queryFixture(
+			'<button id="target" aria-label="close"><span style="display:none"></span></button>'


There is no hidden content in this test.

WilcoFiers · 2019-02-19T12:37:22Z

test/rule-matches/label-content-name-mismatch-matches.js

+		var actual = rule.matches(vNode.actualNode, vNode);
+		assert.isTrue(actual);
+	});
+});


You should add a test for an elm with an implicit name-from-content role that is replaced with an explicit role that does not have name from content. For example <button role="tooltip">

tooltip does support name from content, for example status does not, added a testcase.

Updated.

WilcoFiers · 2019-02-20T09:45:17Z

lib/checks/label/label-content-name-mismatch.js

-		text.replacePunctuation(
-			text.replaceNonBmpUnicode(text.replaceEmojiUnicode(str))
-		)
+	const noEmojiUnicodeStr = text.replaceUnicode(str, (type = 'emoji'));


Why not:

text.replaceUnicode(str, { emoji: true, nonBmp: true, punctuations: true })

Firstly, unicode only comprises emoji and nonBmp at the moment. Punctuations are handled separately. I wouldn't want to mix and match everything into the same options object as it can get verbose.

Hence went with the simpler type=* argument.

After discussion, updated as agreed.

WilcoFiers

Couple small problems with your tests still.

WilcoFiers · 2019-02-21T13:33:19Z

test/integration/rules/label-content-name-mismatch/label-content-name-mismatch.html

+<button id="incomplete1" aria-label="comet">☄️</button>
+<button id="incomplete2" aria-label="☄️">shooting star</button>
+<button id="incomplete3" aria-label="help">?</button>
+<button id="incomplete4" aria-label="&#x1F354">&#x1F354</button>


You didn't close this entity code with a ;.

WilcoFiers · 2019-02-21T13:33:37Z

test/integration/rules/label-content-name-mismatch/label-content-name-mismatch.html

+<button id='incomplete7' aria-label="close">X</button>
+<button id="incomplete8" aria-label="comet">☄️</button>
+<button id="incomplete9" aria-label="help">?</button>
+<button id="incomplete10" aria-label="&#x1F354">&#x1F354</button>


WilcoFiers · 2019-02-21T13:35:14Z

test/rule-matches/label-content-name-mismatch-matches.js

+	});
+
+	it('returns false if given element has no role', function() {
+		var vNode = queryFixture('<div id="target"></div>');


This test doesn't have content or aria-label either. Your test doesn't prove what it presupposes.

WilcoFiers · 2019-02-21T13:35:49Z

test/rule-matches/label-content-name-mismatch-matches.js

+
+	it('returns false if element role is not supported with name from contents', function() {
+		var vNode = queryFixture(
+			'<div id="target" role="progressbar" aria-valuenow="20" aria-valuemin="0" aria-valuemax="100">20 %</div>'


Same here, aria-label(ledby) is missing.

WilcoFiers · 2019-02-21T13:36:21Z

test/rule-matches/label-content-name-mismatch-matches.js

+		assert.isFalse(actual);
+	});
+
+	it('returns false if implicit element role is overridden to a role that dos not support name from contents', function() {


"dos" -> "does"

WilcoFiers · 2019-02-21T13:36:40Z

test/rule-matches/label-content-name-mismatch-matches.js

+
+	it('returns false if implicit element role is overridden to a role that dos not support name from contents', function() {
+		var vNode = queryFixture(
+			'<button id="target" role="status">Your changes were automatically saved.</button>'


Same here, aria-label(ledby) is missing.

Updated.

WilcoFiers and others added 26 commits September 20, 2018 10:45

chore(WIP): rewrite accessibleText

8c80f14

Merge branch 'develop' into a11y-name

82e5c74

chore: More refactoring for accname

9d2451e

chore(WIP): More improvements to accessibleName

305c864

feat: Reimplement accessible name computation

8501f34

chore: All accessible name tests passing

dec7220

chore(accName): All tests passing

abb0673

chore: Add tests

4de5489

chore: Test form-control-value

a98448b

chore: Merge develop

e264958

chore: Refactor and add docs to accessible-text

1aded4a

chore: Add tests for namedFromContents

6e67b52

chore: Refactor subtreeText method

2a5020e

chore: Refactor native accessible text methods

6ff002b

chore: Coverage for text.labelText

4c6c351

Merge branch 'develop' into a11y-name

6917ec9

Merge branch 'develop' into a11y-name

73ded40

fix: update to axe.commons.matches usage

2244ed2

Merge branch 'develop' into a11y-name

6c35b51

test: fix nativeTextboxValue tests

9bddd36

test: fix failing tests

3f9a969

chore: merge from develop

1227102

fix: compute includeHidden as a part of accessibleName fn

dfa1bd7

fix: do not mutate context in accessibleText

d0f45e7

Merge branch 'develop' into a11y-name

c3a1bdc

feat: rule for label and name from content mismatch

ef1c638

jeeyyy requested a review from a team as a code owner January 28, 2019 11:16

Merge branch 'develop' into new-rule-lcnm

14b09b6

jeeyyy changed the title ~~feat(rule): label and name from content mismatch - wcag21a~~ feat(rule): Label and Name from Content mismatch WCAG21 (Issue #1149) Jan 29, 2019

WilcoFiers previously requested changes Jan 29, 2019

View reviewed changes

jeeyyy requested a review from WilcoFiers February 6, 2019 21:42

WilcoFiers previously requested changes Feb 12, 2019

View reviewed changes

jeeyyy added 4 commits February 12, 2019 10:12

Merge branch 'develop' into new-rule-lcnm

ec2af57

Merge branch 'develop' into new-rule-lcnm

b21e631

refactor: update based on code review

3945006

test: update test

2b07290

jeeyyy requested review from WilcoFiers and a team February 12, 2019 19:23

jeeyyy added 3 commits February 18, 2019 05:41

Merge branch 'develop' into new-rule-lcnm

6696c5d

Merge branch 'develop' into new-rule-lcnm

9afed3f

chore: fix linting errors

b18226f

WilcoFiers previously requested changes Feb 19, 2019

View reviewed changes

jeeyyy added 3 commits February 19, 2019 08:37

Merge branch 'develop' into new-rule-lcnm

866fe88

refactor: updates based on code review

5fefd54

merge from develop

56e4680

jeeyyy requested a review from WilcoFiers February 19, 2019 16:04

WilcoFiers reviewed Feb 20, 2019

View reviewed changes

Merge branch 'develop' into new-rule-lcnm

fd003e7

jeeyyy requested a review from WilcoFiers February 20, 2019 12:05

refactor: unicode and punctuation methods

072711d

WilcoFiers previously requested changes Feb 21, 2019

View reviewed changes

jeeyyy added 2 commits February 21, 2019 10:03

Merge branch 'develop' into new-rule-lcnm

eabb7dd

test: update tests

92489be

jeeyyy requested a review from WilcoFiers February 21, 2019 15:12

WilcoFiers approved these changes Feb 21, 2019

View reviewed changes

WilcoFiers merged commit a4255da into develop Feb 21, 2019

WilcoFiers deleted the new-rule-lcnm branch February 21, 2019 18:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(rule): Label and Name from Content mismatch WCAG21 (Issue #1149) #1335

feat(rule): Label and Name from Content mismatch WCAG21 (Issue #1149) #1335

jeeyyy commented Jan 28, 2019 •

edited by WilcoFiers

Loading

WilcoFiers Feb 18, 2019

jeeyyy Feb 19, 2019

WilcoFiers Feb 18, 2019

WilcoFiers Feb 18, 2019

WilcoFiers Feb 18, 2019

WilcoFiers Feb 18, 2019

WilcoFiers Feb 19, 2019

WilcoFiers Feb 19, 2019

WilcoFiers Feb 19, 2019

jeeyyy Feb 19, 2019

WilcoFiers Feb 19, 2019

WilcoFiers Feb 19, 2019

jeeyyy Feb 19, 2019

WilcoFiers Feb 20, 2019

jeeyyy Feb 20, 2019

jeeyyy Feb 20, 2019

WilcoFiers left a comment

WilcoFiers Feb 21, 2019

WilcoFiers Feb 21, 2019

WilcoFiers Feb 21, 2019

WilcoFiers Feb 21, 2019

WilcoFiers Feb 21, 2019

WilcoFiers Feb 21, 2019

		@@ -0,0 +1,16 @@
		describe('text.replacePunctuation', function() {

feat(rule): Label and Name from Content mismatch WCAG21 (Issue #1149) #1335

feat(rule): Label and Name from Content mismatch WCAG21 (Issue #1149) #1335

Conversation

jeeyyy commented Jan 28, 2019 • edited by WilcoFiers Loading

Reviewer checks

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeeyyy commented Jan 28, 2019 •

edited by WilcoFiers

Loading