Fix: Use virtualNode in hidden-content rule #404

WilcoFiers · 2017-07-03T12:51:57Z

No description provided.

WilcoFiers · 2017-07-03T12:52:48Z

Should resolve #392 and #395.

dylanb · 2017-07-03T15:48:55Z

test/checks/visibility/hidden-content.js

@@ -10,37 +10,44 @@ describe('hidden content', function () {
 		}
 	};

+	function checkSetup (html, options, target) {
+		fixture.innerHTML = html;
+    axe._tree = axe.utils.getFlattenedTree(fixture);


indentation

Annoying... this file had space/tabs mixed up when I started, and the editor didn't help :/

dylanb · 2017-07-03T15:49:16Z

test/checks/visibility/hidden-content.js

-    fixture.innerHTML = '<div id="target" style="display: none;"><p>Some paragraph text.</p></div>';
-		var node = fixture.querySelector('#target');
-		assert.isUndefined(checks['hidden-content'].evaluate.call(checkContext, node));
+    var params = checkSetup('<div id="target" style="display: none;"><p>Some paragraph text.</p></div>');


indentation

There are a bunch more of these below, not going to comment all of them

dylanb · 2017-07-03T15:54:57Z

This check needs some actual shadow DOM support built in. It checks the parentNode. This means it needs to do:

	parent = (element.assignedSlot) ? element.assignedSlot : element.parentNode;
	if (parent.nodeType === 11) {
		parent = parent.host;
	}

Which we may want to generalize into a commons function so we can use it elsewhere

You will also want to add shadow DOM tests for this check.

WilcoFiers · 2017-07-06T10:50:04Z

@marcysutton @dylanb I've made a bunch of changes. Please review once more.

dylanb · 2017-07-06T14:28:19Z

lib/checks/visibility/hidden-content.js

@@ -1,13 +1,16 @@
-let styles = window.getComputedStyle(node);
+const whitelist = ['SCRIPT', 'HEAD', 'TITLE', 'NOSCRIPT', 'STYLE', 'TEMPLATE'];
+if (


Why this weird style? No need to introduce new styles for if statements.

This is a weird style? I've been writing multi-line if's like this since forever. The line was getting a bit long so I wrapped it.

I notice that this style is used elsewhere in the code - but not consistently. I would prefer we stick to the original coding style for if statements and only break lines if they go above 80 characters - and also keep braces and parentheses on the same lines

There are currently only 5 files that use this coding style - lets change them all back

I agree that one line is more readable, as long as it doesn't run over 80-100 chars.

I would like us to stick with an 80 char line length. My setup enlarged screen doesn't fit more than that on it. I also don't much like reading longer lines. They hare hard to scan. Anyway, I fixed the line, adding double indent.

We could consider something like Prettier, if we want to get a cleaner, more consistent format.

marcysutton · 2017-07-06T21:15:29Z

lib/commons/dom/get-composed-parent.js

+      return parentNode.host; // Shadow root
+    }
+  }
+  return null; // Root node


Thanks for all the comments! They're helpful. 👍

marcysutton · 2017-07-06T21:18:53Z

test/checks/visibility/hidden-content.js

 	var checkContext = {
 		_data: null,
 		data: function (d) {
 			this._data = d;
 		}
 	};

+	function checkSetup (html, options, target) {


Should we abstract this into a reusable utility so it doesn't have to get repeated in every test file needing Shadow DOM support?

Not sure. I like having some test utilities to work with, but I don't much like just dropping a bunch of extra functions on the global object. I suppose we could create a /test/utils dir, and have a global testUtils object that has everything on it. Something like that?

Yes. Something we could import would be helpful–then if it needed additional code, we could change it in one place.

marcysutton · 2017-07-06T21:19:38Z

test/checks/visibility/hidden-content.js

+		assert.isTrue(checks['hidden-content'].evaluate(node, undefined, virtualNode));
+	});
+
+	(shadowSupport ? it : xit)('works on elements in a shadow DOM', function () {


Crafty 👍

marcysutton · 2017-07-06T21:28:28Z

test/commons/dom/get-composed-parent.js

+  'use strict';
+  var getComposedParent = axe.commons.dom.getComposedParent;
+  var fixture = document.getElementById('fixture');
+  var shadowSupport = document.body && typeof document.body.attachShadow === 'function';


I know it's short, but this line seems like something we could abstract into a common test utility.

This line is only shadow DOM v1, so it should only be used when we are sure that we don't need tests for shadow DOM v0

@dylanb I'm not sure what you're suggesting. With Shadow DOM v0 I take it you mean the <content> element and createShadowRoot() method? Those are deprecated now, right? I hadn't considered it. I can see that we might need to write specific tests for our utilities to cover those cases, but I'm not sure our checks need them. Are you suggesting we do?

The fact that there are different APIs for v0 and v1 make SD pretty difficult to work with. I'd be curious to hear how much test coverage we need for both.

I think the general approach should be that if we are relying on utils and commons that have been tested with both versions, then the tests for checks and rules themselves do not need both

…elabs#404)

Fix: Use virtualNode in hidden-content rule

2beede6

dylanb reviewed Jul 3, 2017

View reviewed changes

WilcoFiers added 3 commits July 4, 2017 13:51

fix: whitespace in hidden-content test

99e8b73

feat: Add dom.getComposedParent function

aac57c0

Merge branch 'shadowDOM' into sd/hidden-content

a76db4c

WilcoFiers force-pushed the sd/hidden-content branch from 24e8361 to 789d62e Compare July 5, 2017 16:26

WilcoFiers added 2 commits July 5, 2017 18:26

feat: Allow hidden-content to work through shadow DOM bounds

789d62e

fix: Remove log statements

6a6bd73

dylanb reviewed Jul 6, 2017

View reviewed changes

marcysutton reviewed Jul 6, 2017

View reviewed changes

fix: Adjust if formatting

2211d78

marcysutton mentioned this pull request Jul 7, 2017

Shadow DOM support for color contrast matches #413

Merged

dylanb merged commit f8f7344 into shadowDOM Jul 10, 2017

marcysutton deleted the sd/hidden-content branch July 21, 2017 16:55

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

fix(webdriverJS): include/exclude chaining and iframe selectors (dequ…

c7c50fb

…elabs#404)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: Use virtualNode in hidden-content rule #404

Fix: Use virtualNode in hidden-content rule #404

WilcoFiers commented Jul 3, 2017

WilcoFiers commented Jul 3, 2017

dylanb Jul 3, 2017 •

edited

Loading

WilcoFiers Jul 4, 2017

dylanb Jul 3, 2017

dylanb Jul 3, 2017

dylanb commented Jul 3, 2017

WilcoFiers commented Jul 6, 2017

dylanb Jul 6, 2017

WilcoFiers Jul 6, 2017

dylanb Jul 6, 2017

dylanb Jul 6, 2017

marcysutton Jul 6, 2017

WilcoFiers Jul 7, 2017

marcysutton Jul 6, 2017

marcysutton Jul 6, 2017

WilcoFiers Jul 7, 2017

marcysutton Jul 7, 2017

marcysutton Jul 6, 2017

marcysutton Jul 6, 2017

dylanb Jul 6, 2017

WilcoFiers Jul 7, 2017

marcysutton Jul 7, 2017

dylanb Jul 7, 2017

Fix: Use virtualNode in hidden-content rule #404

Fix: Use virtualNode in hidden-content rule #404

Conversation

WilcoFiers commented Jul 3, 2017

WilcoFiers commented Jul 3, 2017

dylanb Jul 3, 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 commented Jul 3, 2017

WilcoFiers commented Jul 6, 2017

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

dylanb Jul 3, 2017 •

edited

Loading