feat(landmark-one-main): add rule ensuring one main landmark in document #498

sulsanaul · 2017-08-23T17:50:06Z

This is the rule that incorporates the checks from the previous PR's as requested.

jongund · 2017-08-23T18:40:34Z

This pull request combined the at least one main landmark rule and more than one main landmark rule and is ready for your review.

WilcoFiers

This is really starting to shape up! Excellent! I've added a few more comments. Also, make sure you run the tests next time before creating the PR. Unfortunately those don't run with CircleCI for PRs from forks. You would've caught a few issues.

WilcoFiers · 2017-08-25T11:32:49Z

lib/checks/keyboard/has-at-least-one-main.js

@@ -0,0 +1,3 @@
+var main = document.querySelector('main,[role=main]');
+this.data(!!main);


What is the purpose of this line?

This tells the after function whether or not the main exists; if there's a main in any one document, all of the documents will pass. This will probably make more sense when I add the file for the after function.

WilcoFiers · 2017-08-25T11:35:43Z

lib/checks/keyboard/has-at-least-one-main.js

@@ -0,0 +1,3 @@
+var main = document.querySelector('main,[role=main]');


Please do the following instead. That way you'll query shadow DOM trees too.

const main = axe.utils.querySelectorAll(virtualNode, 'main,[role=main')

What purpose does adding 'virtualnode' serve? I am getting errors concerning an undefined object.

The change to use "const main = axe.utils.querySelectorAll(virtualNode, 'main,[role=main')" seems to break all our test cases and we get the following error:

"TypeError: Cannot read property 'length' of undefined"

Is there something we need to change in the test cases?

@jongund When you call the check in a test, and that test uses virtualNode, you need to pass a virtualNode object in. We've got a test utility to help set that up on axe.testUtils.checkSetup. You can see an example here:

https://github.com/dequelabs/axe-core/blob/develop/test/checks/aria/required-children.js#L22

The test utilities are also documented here: https://github.com/dequelabs/axe-core/blob/master/doc/developer-guide.md#test-utilities

It doesn't appear to be possible to test the rule on an 'html' node using checkSetup. Do you have any suggestions on how to go about doing this?

Forgive me for trying to catch up on this PR...but why do you need to test the HTML node for a main rule? Tests are scoped to the #fixture element, which is already present in the test environment, to avoid auditing the test harness UI itself.

The selector of the rule itself is 'html' so that the rule runs even if there are no main landmarks. My thought was that it wouldn't really be a proper test if it didn't begin from the specified node.

WilcoFiers · 2017-08-25T11:36:22Z

lib/checks/keyboard/has-no-more-than-one-main.js

@@ -0,0 +1,2 @@
+var mains = document.querySelectorAll('main,[role=main]');


As above, use axe.utils.querySelectorAll instead, so you can query shadow DOM trees.

WilcoFiers · 2017-08-25T11:37:38Z

lib/rules/landmark-one-main.json

+	},
+	"all": [],
+	"any": [
+		"has-at-least-one-main",


These should be on all. Putting them on any just requires one of them to pass, which it always will.

WilcoFiers · 2017-08-25T11:40:32Z

test/checks/keyboard/has-at-least-one-main.js

+	'use strict';
+
+	var fixture = document.getElementById('fixture');
+	var checkContext = {


I recommend using var checkContext = new axe.testUtils.MockCheckContext();

WilcoFiers · 2017-08-25T11:42:16Z

test/checks/keyboard/has-at-least-one-main.js

+		var results = [{data: false, result: false}, {data: false, result: false}];
+		assert.isFalse(checks['has-at-least-one-main'].after(results)[0].result && checks['has-at-least-one-main'].after(results)[1].result);
+	});
+


Please add a shadow DOM test too. Have a look at the develop docs https://github.com/dequelabs/axe-core/blob/develop/doc/developer-guide.md#test-utilities about testUtils.shadowSupport

WilcoFiers · 2017-08-25T11:44:13Z

test/checks/keyboard/has-at-least-one-main.js

+		var node = document.querySelector('html');
+		var mainLandmark = document.createElement('div');
+		mainLandmark.setAttribute('role','main');
+		node.appendChild(mainLandmark);


It's better to append these to the fixture instead. Saves you a few lines of code + it's guaranteed to be cleaned up with afterEach.

WilcoFiers · 2017-08-25T11:46:53Z

test/integration/full/landmark-at-least-one-main/frames/level2.html

@@ -0,0 +1,10 @@
+<!doctype html>


This dir, and some of the files in it, should be renamed from landmark-at-least-one-main to landmark-one-main so that it matches the rule name.

WilcoFiers · 2017-08-25T12:36:20Z

test/integration/full/landmark-at-least-one-main/landmark-at-least-one-main-fail.html

@@ -0,0 +1,24 @@
+<!doctype html>


We need integration tests for when there are no main landmarks too.

jongund · 2017-09-11T20:07:19Z

Any additional questions on this pull request?
Do you need anymore changes?

WilcoFiers · 2017-09-16T11:30:55Z

@ssanaul @jongund Several of my comments haven't been addressed yet, including the one that's causing the tests not to run. There isn't much I can review if the tests aren't passing, let alone not running at all. Please resolve these issues.

jongund · 2017-09-18T20:01:01Z

@ssanaul @WilcoFiers

Wilco we have a question about how the "target" attribute is used in a unit test, since we are looking at page level rules.

Also we just wanted to verify that all of the files we need to run test are included.

marcysutton · 2017-09-19T20:48:21Z

Upon preparing remarks for whatwg/html#100, it occurred to me that we might need to account for inert subtrees. What about this scenario:

A view of a web application opens in a new modal layer, with the background rendered inert either using the inert attribute (and polyfill) or using aria-hidden="true" and tabindex="-1" on every focusable control. Deque best practices state that h1 elements can start over in this new layer, since the heading hierarchy is isolated. Shouldn't that also be the case with <main>? That would allow an AT user to easily jump to the "new" main content. As long as the two elements had different labels and visibility was managed so only one was available at a time, I'd think that would still be navigable.

If that scenario was acceptable, we'd want to check to make sure a <main> element in the inert background layer wasn't counted against the total number of elements.

Just wanted to throw that out there for your input before I respond more fully to WHATWG. A real-life example of this scenario can be found in the Blackboard Learn Application: https://www.youtube.com/watch?v=6tmBd9USe6g&t=1s

WilcoFiers · 2017-10-10T11:07:13Z

@ssanaul @jongund What's the status of this PR? Tests still aren't passing so I'm assuming you're still working on it?

sulsanaul · 2017-10-18T21:07:41Z

Even though the unit tests are passing, the integration tests for the rule are only returning results for has-at-least-one-main, which itself is only checking the main DOM, not any iframes.
Any thoughts on why this is the case?

WilcoFiers · 2017-11-14T13:15:45Z

test/integration/full/landmark-one-main/landmark-one-main-fail.js

+
+	describe('violations', function () {
+		it('should find 1', function () {
+			assert.lengthOf(results.violations, 1);


Check the number of nodes. I think that should be 2. You're not testing the second node here either.

@WilcoFiers @jongund
It appears as if results.violations only has one node even though it should be observing two violations. We couldn't find any implementation of a test for more than one violation.
However, the testing results.passes in landmark-one-main-pass.js does return 4 nodes.
Any thoughts on why violations only seems to contain one node, for our test as well as the other tests in the aXe library?

@ssanaul Looks like another timing issue. You're calling axe.run before the frames are loaded. Have a look at how that was solved in landmark-main-is-top-level in the before.

@WilcoFiers @jongund
Both nodes are tested now.

Excellent. I think that will do it!! Thanks @ssanaul!

WilcoFiers · 2017-11-14T13:19:22Z

test/testutils.js

+ * @param String  			Target for the check, CSS selector (default: '#target')
+ * @return Array
+ */
+testUtils.shadowCheckSetup = function (content, shadowContent, options, target) {


Why is this here?

I created it for them in a separate branch, there was a lot of boilerplate for shadow DOM tests that I wanted to smooth out.

WilcoFiers · 2017-11-14T13:22:03Z

lib/checks/keyboard/has-at-least-one-main-after.js

@@ -0,0 +1,8 @@
+var hasMain = false;
+for (var i = 0; i < results.length && !hasMain; i++) {
+	hasMain = results[i].data;


This doesn't look like it would work. Basically, you're taking the last item on results and assigning that as the result for everything. That doesn't seem right. What was this supposed to do? Please comment the code, as these double loops are confusing.

@WilcoFiers @jongund

var hasMain = false; //iterate through results from each document //stops if any document contains a main landmark for (var i = 0; i < results.length && !hasMain; i++) { hasMain = results[i].data; } //if any document contains a main landmark, set all documents to pass the check //otherwise, fail all documents //since this is a page level rule, all documents either pass or fail the requirement for (var i = 0; i < results.length; i++) { results[i].result = hasMain; } return results;

Do these comments clear up the behavior of the function?

Since this is a page level rule all documents either fail or pass this rule.

We only need one main in any of the documents (e.g. main document or an iframe) to pass this rule,

If some documents fail this rule (e.g. iframe), people will think they need to have a main landmark in every iframe.

Does this make sense?

Are we getting the logic right here, do we need to update the individual document results?

WilcoFiers · 2017-11-17T14:52:49Z

lib/checks/keyboard/has-at-least-one-main-after.js

+
+//iterate through results from each document
+//stops if any document contains a main landmark
+for (var i = 0; i < results.length && !hasMain; i++) {


Tip for future work. For-loops generally make code harder to read. JS has a bunch of very excellent array methods which let you write more expressive code. If you had written your code like this, you wouldn't have needed the comment.

const hasMain = results.some(result => result.data === true);

Great, I'll keep that in mind. Thanks for the tip!

…ent (#498) * feat(landmark-one-main): add rule ensuring one main landmark in document * fix: rename integration tests * fix: move checks from 'any' to 'all' * fix: add missing 'after' check * fix: update to pass virtualnode in to check * fix: change incorrect rule name in integration tests * fix: remove line for debugging * fix: correct faulty check tests * test: add shadowCheckSetup util * test: fix main tests for shadowdom * fix: resolve timeout issues * fix: add event listener * fix: change where to check for passes * style: comment code for comprehension * test: add test for second violation node

…s#498)

feat(landmark-one-main): add rule ensuring one main landmark in document

59e1844

WilcoFiers requested changes Aug 25, 2017

View reviewed changes

sulsanaul added 2 commits August 30, 2017 13:30

fix: rename integration tests

4737cbd

fix: move checks from 'any' to 'all'

ac0c517

Merge https://github.com/dequelabs/axe-core into main/only-one

d80efbd

sulsanaul added 3 commits September 26, 2017 11:40

fix: add missing 'after' check

456415c

Merge https://github.com/dequelabs/axe-core into main/only-one

337d129

fix: update to pass virtualnode in to check

c0f2efb

sulsanaul and others added 8 commits October 10, 2017 15:54

fix: change incorrect rule name in integration tests

ff6417b

fix: remove line for debugging

4319175

fix: correct faulty check tests

f1d1b2f

test: add shadowCheckSetup util

93e8f11

test: fix main tests for shadowdom

0319db8

Merge https://github.com/dequelabs/axe-core into main/only-one

5562767

feat: add shadow support

8640681

fix: resolve timeout issues

59e6d16

sulsanaul added 2 commits October 20, 2017 11:46

fix: add event listener

26ee38b

fix: change where to check for passes

47c7e66

WilcoFiers reviewed Nov 14, 2017

View reviewed changes

WilcoFiers added the waiting on feedback/tests label Nov 15, 2017

style: comment code for comprehension

a3beee1

WilcoFiers reviewed Nov 17, 2017

View reviewed changes

test: add test for second violation node

6359af1

WilcoFiers approved these changes Nov 19, 2017

View reviewed changes

WilcoFiers added 2 commits November 19, 2017 14:01

Merge branch 'develop' into main/only-one

823ef51

Merge branch 'develop' into main/only-one

5ed8ccf

WilcoFiers merged commit dfc6069 into dequelabs:develop Nov 19, 2017

dependencies bot mentioned this pull request Dec 19, 2017

Update 10 dependencies from npm storybookjs/storybook#2520

Merged

toolness mentioned this pull request Dec 20, 2017

aXe tests fail on CircleCI with error "Document has no main landmarks" uswds/uswds#2284

Closed

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

chore: fix changelog with what actually changed since 4.4.0 (dequelab…

742bbd0

…s#498)

		@@ -0,0 +1,3 @@
		var main = document.querySelector('main,[role=main]');
		this.data(!!main);

		@@ -0,0 +1,2 @@
		var mains = document.querySelectorAll('main,[role=main]');

feat(landmark-one-main): add rule ensuring one main landmark in document #498

feat(landmark-one-main): add rule ensuring one main landmark in document #498

Conversation

sulsanaul commented Aug 23, 2017

jongund commented Aug 23, 2017

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

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

jongund commented Sep 11, 2017

WilcoFiers commented Sep 16, 2017

jongund commented Sep 18, 2017

marcysutton commented Sep 19, 2017 • edited Loading

WilcoFiers commented Oct 10, 2017

sulsanaul commented Oct 18, 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

marcysutton commented Sep 19, 2017 •

edited

Loading