shadowDOM accessible text calculation #420

WilcoFiers · 2017-07-12T12:11:56Z

This PR aims to close #399.

dylanb · 2017-07-12T14:16:45Z

lib/commons/dom/find-elms-in-context.js

@@ -0,0 +1,26 @@
+/* global axe, dom */
+/**
+ * Find a elements reference from a given context


sp: referenced

dylanb · 2017-07-12T17:15:01Z

lib/commons/text/accessible-text.js

-function checkDescendant(element, nodeName) {
-	var candidate = element.querySelector(nodeName.toLowerCase());
+function checkDescendant({ actualNode }, nodeName) {
+	var candidate = actualNode.querySelector(nodeName.toLowerCase());


should we put a note in here to indicate that this function is intended to be used only on elements that cannot contain a shadow root?

dylanb · 2017-07-12T17:34:55Z

lib/commons/text/accessible-text.js

-	var encounteredNodes = [];
+	let accessibleNameComputation;
+	const encounteredNodes = [];
+	if (element instanceof Node) {


This works differently from a lot of the other commons - which do not try to maintain backwards compatibility. Whether we want to support backwards compatibility or not and if we do, we need to change those other utilities.

The reason I did this is because it would have taken a whole lot of additional changes to make sure every method that uses accessibleText has access to a virtual node. It wasn't even for backward compatibility, but that's a good additional reason. Further, I think these methods should be useful outside of a rule too, which they aren't currently, since they rely on setup that gets done in another place.

I think, instead of calling element = axe.utils.getNodeFromTree(axe._tree[0], element); we need a method that will take any DOM element and return it's virtual counterpart. Something like virtualNode = axe.utils.getVirtualNode(node). It can look at axe._tree[0] if available, and if it isn't, it should create a new tree and pull the virtual node from that. We could also make that method store virtual nodes in a set rather than a tree, so that lookup is faster.

Yeah, the biggest problem with not passing in the virtualNode is the potential performance impact. I am worried about memory leaks if we use these APIs (or make them usable) without understanding what they do. So I don't support building this cache if it doesn't exist.

I am going to accept this pull request and have opened a separate issue to deal with this #435

dylanb · 2017-07-12T17:36:25Z

lib/commons/text/accessible-text.js

 					returnText += ' ';
 				}
-				returnText += accessibleNameComputation(nodes[i], inLabelledByContext, inControlContext);
+				returnText += accessibleNameComputation(


This breaks our current style conventions

Ugh... It's really frustrating going between axe-core and axe-devtools. :/ sorry

dylanb · 2017-07-12T18:37:12Z

test/checks/forms/fieldset.js

 			var node = fixture.querySelector('#target');
 			assert.isFalse(checks.fieldset.evaluate.call(checkContext, node));
-


We need to add tests that deal with this sort of thing

<div> <form> <div class="fieldset"> <span slot="legend">Shadow DOM fieldset</span> <label>Option 1<input type="radio" value="one" name="group" /></label> <label>Option 2<input type="radio" value="two" name="group" /></label> </div> </form> </div> <script> function createContentSlotted() { var group = document.createElement('fieldset'); group.innerHTML = '<legend><slot name="legend"></slot></legend><slot></slot>'; return group; } function makeShadowTreeSlotted(node) { var root = node.attachShadow({mode: 'open'}); root.appendChild(createContentSlotted()); } document.addEventListener('DOMContentLoaded', function() { document.querySelectorAll('.fieldset').forEach(makeShadowTreeSlotted); }); </script>

Should be covered in #425

dylanb · 2017-07-12T18:40:00Z

test/checks/keyboard/focusable-no-name.js

 		var node = fixture.querySelector('span');
 		assert.isTrue(checks['focusable-no-name'].evaluate(node));
 	});

 	it('should pass if the element is tabbable but has an accessible name', function () {
-		fixture.innerHTML = '<a href="#" title="Hello"></a>';
+		fixtureSetup('<a href="#" title="Hello"></a>');
 		var node = fixture.querySelector('a');
 		assert.isFalse(checks['focusable-no-name'].evaluate(node));
 	});


Add a test where when an anchor has content inside the shadow root, it returns false. Test this for slotted content as well as fallback content inside the default slot

@WilcoFiers Can you add one more

var node = document.createElement('a'); var shadow = node.attachShadow({ mode: 'open' }); shadow.innerHTML = '<slot>Fallback content</slot>'; fixtureSetup(node); assert.isFalse(checks['focusable-no-name'].evaluate(node));

dylanb · 2017-07-12T18:45:05Z

test/commons/text/accessible-text.js

 			assert.equal(axe.commons.text.accessibleText(target), '');
 		});
+


I think we should add a couple more tests here;

fallback content for v1

correct use of an attribute (e.g. title) within the slotted shadow DOM content,

correct use of an attribute (e.g. title) within the shadow DOM content

dylanb · 2017-07-12T18:52:22Z

test/checks/label/implicit.js

 		var node = fixture.querySelector('#target');
 		assert.isTrue(checks['implicit-label'].evaluate(node));
 	});

 	it('should return false if a label is not present', function () {
 		var node = document.createElement('input');
 		node.type = 'text';
-		fixture.appendChild(node);
+		fixtureSetup(node);

 		assert.isFalse(checks['implicit-label'].evaluate(node));
 	});


This needs to support this example

<form> <label>Option 1 <span class="slotted"><input type="text" name="slottedLabel" /></span></label> \ </form> <script> function createContentSlotted() { var group = document.createElement('span'); group.innerHTML = '<slot></slot>'; return group; } function makeShadowTreeSlotted(node) { var root = node.attachShadow({mode: 'open'}); root.appendChild(createContentSlotted()); } document.addEventListener('DOMContentLoaded', function() { document.querySelectorAll('.slotted').forEach(makeShadowTreeSlotted); }); </script>

Which works properly in both Safari and Chrome with VO

dylanb · 2017-07-12T19:02:29Z

test/checks/forms/fieldset.js

@@ -43,8 +43,8 @@ describe('fieldset', function () {
 		});

 		it('should return false if the group has no legend element', function () {
-			fixture.innerHTML = '<fieldset><input type="' + type + '" id="target" name="uniqueyname">' +
-				'<input type="' + type + '" name="uniqueyname"></fieldset>';
+			fixtureSetup('<fieldset><input type="' + type + '" id="target" name="uniqueyname">' +


this test here should work when the fieldset has been found by going up the slotted content - see the example below

Should be covered in #425

dylanb · 2017-07-12T19:21:25Z

test/checks/tables/same-caption-summary.js

 		var node = fixture.querySelector('table');

 		assert.isFalse(checks['same-caption-summary'].evaluate(node));

 	});

 	it('should return true if summary and caption are the same', function () {
-		fixture.innerHTML = '<table summary="Hi"><caption>Hi</caption><tr><td></td></tr></table>';
+		fixtureSetup('<table summary="Hi"><caption>Hi</caption><tr><td></td></tr></table>');
 		var node = fixture.querySelector('table');

 		assert.isTrue(checks['same-caption-summary'].evaluate(node));


This should catch the following situation

<div class="table"> <span slot="caption">Caption</span> <span slot="one">Data element 1</span> <span slot="two">Data element 2</span> </div> <script> function createContentTable() { var group = document.createElement('table'); group.innerHTML = '<caption><slot name="caption"></slot></caption>' + '<tr><td><slot name="one"></slot></td><td><slot name="two"></slot></td></tr>'; group.setAttribute('summary', 'Caption'); return group; } function makeShadowTreeTable(node) { var root = node.attachShadow({mode: 'open'}); root.appendChild(createContentTable()); } document.addEventListener('DOMContentLoaded', function() { document.querySelectorAll('.table').forEach(makeShadowTreeTable); }); </script>

dylanb

please address the comments

dylanb · 2017-07-12T19:23:01Z

test/commons/text/accessible-text.js

-		assert.equal(axe.commons.text.accessibleText(rule2a), 'Beep');
+		// var rule2a = axe.utils.querySelectorAll(axe._tree, '#beep')[0];
+		var rule2b = axe.utils.querySelectorAll(axe._tree, '#flash')[0];
+		// assert.equal(axe.commons.text.accessibleText(rule2a), 'Beep');


why is this commented?

# Conflicts: # lib/commons/dom/find-elms-in-context.js # lib/commons/text/accessible-text.js # test/checks/keyboard/focusable-no-name.js # test/checks/tables/same-caption-summary.js # test/commons/text/accessible-text.js

WilcoFiers · 2017-07-14T10:44:50Z

@dylanb Thanks for all the excellent comments. I've updated the PR accordingly. Can you review again?

dylanb · 2017-07-14T13:15:04Z

test/checks/tables/same-caption-summary.js

+	(shadowSupport.v1 ? it : xit)('should match slotted caption elements', function () {
+		var node = document.createElement('div');
+		node.innerHTML = '<span slot="caption">Caption</span>' +
+      '<span slot="one">Data element 1</span>' +


@WilcoFiers indentation

it's an indentation feature ;)

dylanb · 2017-07-14T13:17:50Z

@WilcoFiers one small indentation problem and then I suggested another test - then we should be good to go

# Conflicts: # test/testutils.js

)

WilcoFiers changed the base branch from master to shadowDOM July 12, 2017 12:12

WilcoFiers requested review from dylanb and marcysutton July 12, 2017 12:12

dylanb reviewed Jul 12, 2017

View reviewed changes

dylanb requested changes Jul 12, 2017

View reviewed changes

WilcoFiers added 4 commits July 13, 2017 14:27

feat: Run accessibleText on virtual elements

d29d6cc

fix: Pass all tests that use accessibleText

48fca9d

feat: Add findElmsInContext method

0314c42

test: accessibleText with shadow DOM

19a9ff8

WilcoFiers force-pushed the sd/accessible-text branch from c37d993 to 55d6aa1 Compare July 14, 2017 10:44

test: More testing for accessibleText()

55d6aa1

# Conflicts: # lib/commons/dom/find-elms-in-context.js # lib/commons/text/accessible-text.js # test/checks/keyboard/focusable-no-name.js # test/checks/tables/same-caption-summary.js # test/commons/text/accessible-text.js

dylanb reviewed Jul 14, 2017

View reviewed changes

WilcoFiers added 2 commits July 17, 2017 12:02

Merge branch 'shadowDOM' into sd/accessible-text

e3ca631

# Conflicts: # test/testutils.js

test: Fix a few broken tests

9584884

WilcoFiers merged commit 414fcbe into shadowDOM Jul 17, 2017

dylanb deleted the sd/accessible-text branch July 17, 2017 13:57

mrtnvh pushed a commit to mrtnvh/axe-core that referenced this pull request Nov 24, 2023

docs(webdriverjs): add example using axe-core webdriverjs (dequelabs#420

0e566c2

)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

shadowDOM accessible text calculation #420

shadowDOM accessible text calculation #420

WilcoFiers commented Jul 12, 2017

dylanb Jul 12, 2017 •

edited

Loading

dylanb Jul 12, 2017

dylanb Jul 12, 2017

WilcoFiers Jul 13, 2017

dylanb Jul 13, 2017

dylanb Jul 13, 2017

dylanb Jul 12, 2017

WilcoFiers Jul 13, 2017

dylanb Jul 12, 2017

WilcoFiers Jul 13, 2017

dylanb Jul 12, 2017

dylanb Jul 14, 2017 •

edited

Loading

dylanb Jul 12, 2017

dylanb Jul 12, 2017 •

edited

Loading

dylanb Jul 12, 2017

WilcoFiers Jul 14, 2017

dylanb Jul 12, 2017

dylanb left a comment

dylanb Jul 12, 2017

WilcoFiers Jul 13, 2017

WilcoFiers commented Jul 14, 2017

dylanb Jul 14, 2017

WilcoFiers Jul 17, 2017

dylanb commented Jul 14, 2017

		var node = fixture.querySelector('#target');
		assert.isFalse(checks.fieldset.evaluate.call(checkContext, node));

		assert.equal(axe.commons.text.accessibleText(target), '');
		});

shadowDOM accessible text calculation #420

shadowDOM accessible text calculation #420

Conversation

WilcoFiers commented Jul 12, 2017

dylanb Jul 12, 2017 • 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

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

dylanb Jul 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dylanb Jul 12, 2017 • 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

dylanb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WilcoFiers commented Jul 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dylanb commented Jul 14, 2017

dylanb Jul 12, 2017 •

edited

Loading

dylanb Jul 14, 2017 •

edited

Loading

dylanb Jul 12, 2017 •

edited

Loading