Add rule, landmark-main-is-top-level #462

sulsanaul · 2017-07-26T16:14:12Z

This rule ensures that each main landmark in a document is not nested within another landmark element. Once again, please let me know if there is anything you would like to see differently.

…k in a document is not within another landmark element

WilcoFiers · 2017-07-31T09:19:42Z

lib/checks/keyboard/main-is-top-level.js

+	if (!role){
+		role = axe.commons.aria.implicitRole(parent);
+	}
+	if (role && landmarks.indexOf(role) >= 0){


Please use !landmarks.includes(role).

We will make this change

@WilcoFiers
We have updated the pull request with requested changes

WilcoFiers · 2017-07-31T09:28:09Z

lib/rules/landmark-no-more-than-one-main.json

@@ -0,0 +1,16 @@
+{


Please remove this rule from the PR.

WilcoFiers · 2017-07-31T09:28:26Z

lib/rules/landmark-main-is-top-level.json

+  ],
+  "metadata": {
+    "description": "Each main landmark in a document must not be nested in another landmark",
+    "help": ""


Make sure to fill this out too. Description says what the rule does, help gives a hint on how to fix the issue.

The help text also goes in the aXe extension sidebar where we list out the violations.

WilcoFiers · 2017-07-31T09:28:35Z

lib/checks/keyboard/main-is-top-level.json

+  "metadata": {
+    "impact": "moderate",
+    "messages": {
+      "pass": "Main landmark is top level",


Please use a full sentence for both texts.

WilcoFiers · 2017-07-31T09:28:47Z

lib/checks/keyboard/main-is-top-level.js

+while (parent){
+	var role = parent.getAttribute('role');
+	if (!role){
+		role = axe.commons.aria.implicitRole(parent);


I think this is too strict. There are roles that can certainly be permitted, off the top of my head: form, application, presentation. Probably a bunch more. Can you go through the list and make sure those kind of things get ignored?

WIlco,

In ARIA 1.1 application and presentation roles are not considered landmarks.

In the case of a form landmark:
2.a Form landmarks should be fairly rare, there is discussion in the ARIA and HTML5 working groups that form landmarks should/must have an accessible name to be considered a landmark or at least useful, since without knowing what the form is about the navigation feature is somewhat limited since getting to form controls is easy from screen readers and makes the landmark more of a distraction than a useful feature.
2.b Forms are typically just one part of a page so should be in another landmark, the main landmark is what people will be looking for, not a form landmark. In general Form landmarks should only be used if there are multiple forms on a page.

@jongund Both of those suggestions make sense to me. Seeing as it's still an open discussion if forms should be a landmark, make an exception for this rule. That one is particularly relevant for the region rule. You can adjust the type of application and presentation in lib/commons/aria/index.

Wilco,

We will modify the file lib/commons/aria/index for role application:
type: 'landmark' to type: 'structure'

role presentation is already has type: 'structure' so no change need for it.

… update aria index file so application type is structure

WilcoFiers

@jongund @ssanaul I've left a few more comments, listing some of the changes that didn't get done correctly yet. These PRs have been open for a while now. I'd like to make sure we can merge them in soon. What are your plans for this? If you want, we can set up a call and get through these last few issues.

WilcoFiers · 2017-08-23T09:30:17Z

lib/checks/keyboard/main-is-top-level.json

+  "metadata": {
+    "impact": "moderate",
+    "messages": {
+      "pass": "The main landmark is top level.",


This isn't a correct sentence, please use "is as the top level."

WilcoFiers · 2017-08-23T09:31:34Z

lib/rules/landmark-main-is-top-level.json

+  ],
+  "metadata": {
+    "description": "Each main landmark in a document must not be nested in another landmark",
+    "help": "The main landmark in this document should not be contained within another landmark"


This is too wordy and won't fit well into the extension UI. I'd suggestion: "The main landmark should not be in another landmark"

WilcoFiers · 2017-08-23T09:32:18Z

test/integration/full/landmark-main-is-top-level/frames/level2-a.html

+</head>
+<body>
+	<p>This iframe is also a violation</p>
+	<div role = "form">


This shouldn't be a failure. Neither should role="application". Make sure there is a test for that too.

WilcoFiers · 2017-08-23T09:33:14Z

lib/checks/keyboard/main-is-top-level.js

+	if (!role){
+		role = axe.commons.aria.implicitRole(parent);
+	}
+	if (role && landmarks.indexOf(role) >= 0){


Use .includes() instead.

WilcoFiers · 2017-08-23T09:36:15Z

lib/checks/keyboard/main-is-top-level.js

@@ -0,0 +1,13 @@
+var landmarks = axe.commons.aria.getRolesByType('landmark');
+var parent = node.parentElement;


This PR has been open for a while now. Since than, we've merged shadow DOM in. It has a different method of getting at parent nodes. Please use axe.commons.dom.getComposedParent(). That way this check will look passed shadow DOM boundaries. You'll have to rebase to get the latest on develop into this branch.

@jongund This line still uses node.parentElement, please fix this too.

jongund · 2017-08-23T15:16:40Z

Wilco, I am meeting with the student later today and we will: 1. Make the make the changes outlined in this e-mail 2. Update the pull requests to remove the obsolete proposed rules If there are still additional issues after the changes maybe we can schedule a call to work on the remaining issues. Jon From: Wilco Fiers <notifications@github.com> Reply-To: dequelabs/axe-core <reply@reply.github.com> Date: Wednesday, August 23, 2017 at 4:38 AM To: dequelabs/axe-core <axe-core@noreply.github.com> Cc: Jon Gunderson <jongund@illinois.edu>, Mention <mention@noreply.github.com> Subject: Re: [dequelabs/axe-core] Add rule, landmark-main-is-top-level (#462) @WilcoFiers commented on this pull request. @jongund<https://github.com/jongund> @ssanaul<https://github.com/ssanaul> I've left a few more comments, listing some of the changes that didn't get done correctly yet. These PRs have been open for a while now. I'd like to make sure we can merge them in soon. What are your plans for this? If you want, we can set up a call and get through these last few issues.

________________________________ In lib/checks/keyboard/main-is-top-level.json<#462 (comment)>:

@@ -0,0 +1,11 @@

+{ + "id": "main-is-top-level", + "evaluate": "main-is-top-level.js", + "metadata": { + "impact": "moderate", + "messages": { + "pass": "The main landmark is top level.", This isn't a correct sentence, please use "is as the top level."

________________________________ In lib/rules/landmark-main-is-top-level.json<#462 (comment)>:

@@ -0,0 +1,16 @@

+{ + "id": "landmark-main-is-top-level", + "selector": "main,[role=main]", + "tags": [ + "best-practice" + ], + "metadata": { + "description": "Each main landmark in a document must not be nested in another landmark", + "help": "The main landmark in this document should not be contained within another landmark" This is too wordy and won't fit well into the extension UI. I'd suggestion: "The main landmark should not be in another landmark"

________________________________ In test/integration/full/landmark-main-is-top-level/frames/level2-a.html<#462 (comment)>:

@@ -0,0 +1,15 @@

+<!doctype html> +<html id="violation4"> +<head> + <meta charset="utf8"> + <script src="/axe.js"></script> +</head> +<body> + <p>This iframe is also a violation</p> + <div role = "form"> This shouldn't be a failure. Neither should role="application". Make sure there is a test for that too.

________________________________ In lib/checks/keyboard/main-is-top-level.js<#462 (comment)>:

@@ -0,0 +1,13 @@

+var landmarks = axe.commons.aria.getRolesByType('landmark'); +var parent = node.parentElement; +while (parent){ + var role = parent.getAttribute('role'); + if (!role){ + role = axe.commons.aria.implicitRole(parent); + } + if (role && landmarks.indexOf(role) >= 0){ Use .includes() instead.

________________________________ In lib/checks/keyboard/main-is-top-level.js<#462 (comment)>:

@@ -0,0 +1,13 @@

+var landmarks = axe.commons.aria.getRolesByType('landmark'); +var parent = node.parentElement; This PR has been open for a while now. Since than, we've merged shadow DOM in. It has a different method of getting at parent nodes. Please use axe.commons.dom.getComposedParent(). That way this check will look passed shadow DOM boundaries. You'll have to rebase to get the latest on develop into this branch. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<#462 (review)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/ABcE7N_akj2vx5TBgadPCjJavPSNyGihks5sa_MngaJpZM4OkJzP>.

jongund · 2017-08-23T18:22:42Z

Wilco, In our discussion: jongund<https://github.com/jongund> 6 days ago <#462 (comment)> WIlco, 1. In ARIA 1.1 application and presentation roles are not considered landmarks. 2. In the case of a form landmark: 2.a Form landmarks should be fairly rare, there is discussion in the ARIA and HTML5 working groups that form landmarks should/must have an accessible name to be considered a landmark or at least useful, since without knowing what the form is about the navigation feature is somewhat limited since getting to form controls is easy from screen readers and makes the landmark more of a distraction than a useful feature. 2.b Forms are typically just one part of a page so should be in another landmark, the main landmark is what people will be looking for, not a form landmark. In general Form landmarks should only be used if there are multiple forms on a page. Top of Form Bottom of Form [WilcoFiers]<https://github.com/WilcoFiers> WilcoFiers<https://github.com/WilcoFiers> 5 days ago <#462 (comment)> Contributor @jongund<https://github.com/jongund> Both of those suggestions make sense to me. Seeing as it's still an open discussion if forms should be a landmark, make an exception for this rule. That one is particularly relevant for the region rule. You can adjust the type of application and presentation in lib/commons/aria/index. It sounded like you agreed to change “application” in /lib/commons/aria/index.js to not be a landmark and that is part of our pull request and that you seemed to agree that FORM landmark restriction was also OK, since FORM landmarks should be rare in practice and a MAIN landmark would probably be better for identifying a page that is mostly a form. So the test for bening contained in a FORM landmark should fail and APPLICATION does need to be tested because it is not a landmark in ARIA 1.1. Jon

________________________________ In test/integration/full/landmark-main-is-top-level/frames/level2-a.html<#462 (comment)>:

@@ -0,0 +1,15 @@

+<!doctype html> +<html id="violation4"> +<head> + <meta charset="utf8"> + <script src="/axe.js"></script> +</head> +<body> + <p>This iframe is also a violation</p> + <div role = "form"> This shouldn't be a failure. Neither should role="application". Make sure there is a test for that too.

… into main/is-top-level

jongund · 2017-08-23T18:37:07Z

We have updated the pull request, including changing the messaging and using axe.commons.getComposedParent.

The only outstanding issue seems to be whether a MAIN landmark can be contained by a FORM landmark. Based on FORM landmarks being rare and if a FORM is the primary content of a page, the MAIN landmark would probably be as good if not better , since users ae looking for MAIN landmarks. So the tests still view FORM as a failure. Do we want to discuss it in a teleconference?

jongund · 2017-08-28T19:01:03Z

We updated the rule to use:

axe.commons.getComposedParent
Updated the rule now ignores the element for the purpose of looking at parent nodes for FORM landmarks. The only way a element would be tested as a FORM landmark is when the element has an explicit role=form attribute.

WilcoFiers · 2017-08-29T07:47:59Z

@jongund The tests aren't passing yet. Be sure to run grunt test-fast before you commit code for review. The other PR had this problem too.

jongund · 2017-09-11T20:08:24Z

Anymore questions or issues with this pull request?

What is the status of accepting this rule?

WilcoFiers · 2017-09-12T09:42:56Z

@jongund We're a little busy with the 2.4 releases at the moment. We certainly haven't forgotten about these PRs and will get to reviewing them as soon as possible.

WilcoFiers

Almost there! Just a few more small changes. One of the tests is failing still. Please make sure all tests are passing before you request a review next time.

WilcoFiers · 2017-09-16T10:59:00Z

test/checks/keyboard/main-is-top-level.js

+		var div = document.createElement('div');
+		var shadow = div.attachShadow({ mode: 'open' });
+		shadow.innerHTML = '<main>Main content</main>';
+		var checkArgs = checkSetup(div);


This will select the div, not the <main> element. This should be checkSetup(shadow.querySelector('main'));

WilcoFiers · 2017-09-16T10:59:27Z

test/integration/full/landmark-main-is-top-level/frames/level1-fail.html

+</head>
+<body>
+	<p>This iframe should fail, too</p>
+	<div role = "complementary">


Please don't use spaces in attributes.

WilcoFiers · 2017-09-16T11:15:02Z

test/integration/full/landmark-main-is-top-level/landmark-main-is-top-level-fail.js

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


Add a node count check too:

assert.lengthOf(results.violations[0].nodes, 2);

@ssanaul This issue isn't addressed yet.

I thought 3e5c6ad resolved it. It checks for 4 nodes because I believe there are 4 violations. @WilcoFiers

WilcoFiers · 2017-09-16T11:18:17Z

lib/commons/aria/index.js

@@ -189,7 +189,7 @@ lookupTables.role = {
 		context: null
 	},
 	'application': {
-		type: 'landmark',


This is causing the following test to fail, be sure to fix that:

1) table.isDataTable should be true if the table has a landmark role application:

@WilcoFiers
In ARIA 1.1, 'application' is not a landmark role.
https://www.w3.org/TR/wai-aria-1.1/#application
The new superclass role is 'structure'.
Can you update the 'develop' branch to meet the ARIA 1.1 specification?

… into main/is-top-level

sulsanaul · 2017-09-21T19:52:12Z

table-fake-caption
    table-fake-caption test
      passes
        √ should find ["#pass1"]
        √ should find ["#pass2"]
        √ should not return other results
      violations
        √ should find ["#fail1"]
        √ should find ["#fail2"]
        √ should not return other results

  td-has-header
    td-has-header test

Warning: PhantomJS timed out, possibly due to a missing Mocha run() call. Use --force to continue.

Aborted due to warnings.

This is the warning message I'm receiving currently. Is this what you're seeing as well @WilcoFiers ?

marcysutton · 2017-09-26T00:36:13Z

@ssanaul you can always fire up the tests in a browser using grunt dev to see if there are any errors in the JavaScript console. That's usually how I debug those failures

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

@ssanaul @jongund What's the status of this PR?

jongund · 2017-10-10T17:47:20Z

Wilco, Sulaiman is trying to setup a meeting with Marcy Sutton to try to debug why the integration tests are failing. It seems we need a little more help with configuring the test cases correctly. Do you have any time to meet with us this week to review our code with us? Jon From: Wilco Fiers <notifications@github.com> Reply-To: dequelabs/axe-core <reply@reply.github.com> Date: Tuesday, October 10, 2017 at 7:07 AM To: dequelabs/axe-core <axe-core@noreply.github.com> Cc: Jon Gunderson <jongund@illinois.edu>, Mention <mention@noreply.github.com> Subject: Re: [dequelabs/axe-core] Add rule, landmark-main-is-top-level (#462) @ssanaul<https://github.com/ssanaul> @jongund<https://github.com/jongund> What's the status of this PR? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<#462 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/ABcE7Lh9ffehEvYql-NzIrxbhU9Xes6pks5sq1ACgaJpZM4OkJzP>.

sulsanaul · 2017-10-18T21:23:20Z

Is there anything I've excluded from this PR? I thought I had addressed all review comments.

marcysutton · 2017-10-18T23:29:47Z

I'm still seeing two pretty simple comments from Wilco that weren't addressed, otherwise looks good to me.

test/integration/full/landmark-main-is-top-level/frames/level2-a.html
test/integration/full/landmark-main-is-top-level/landmark-main-is-top-level-fail.js

sulsanaul · 2017-10-19T00:31:42Z

Ok, I think that last commit should have resolved everything. Although, I think the suggested node count check assert.lengthOf(results.violations[0].nodes, 2); should be assert.lengthOf(results.violations[0].nodes, 4);

WilcoFiers · 2017-10-19T09:24:32Z

lib/checks/keyboard/main-is-top-level.js

+var parent = axe.commons.dom.getComposedParent(node);
+while (parent){
+	var role = parent.getAttribute('role');
+	if (!role && (parent.tagName.toLowerCase() !== 'form')){


This needs to work with role=form too.

Does line 5 not accomplish that?

dylanb · 2017-11-23T22:25:07Z

@ssanaul thank you for your contribution!!!

* init * feat: add new rule, landmark-no-more-than-one-main * test(landmark-at-least-one-main): updated integration tests for check * feat(landmark-main-is-top-level): add rule ensuring each main landmark in a document is not within another landmark element * refactor(landmark-main-is-top-level): change help messages * feat(landmark-main-is-top-level): change a function used in check and update aria index file so application type is structure * refactor(main-is-top-level): change pass/fail messages * refactor(landmark-main-is-top-level): change description/help messages * feat(main-is-top-level): update check for shadow dom features * fix(main-is-top-level): update check to ignore form as a landmark * fix: edit incorrect usage of getComposedParent * test: add unit test to check if main landmark in shadow dom is top level * style: remove spaces in attributes * fix: update test so that checkSetup passes in correct argument * test: add test to ensure correct number of violations nodes * fix: revert 'application' type to landmark * style: remove spaces in attributes * fix: allow main landmark to be in form

…/react/examples/shadow-dom (dequelabs#462) Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

sulsanaul added 5 commits July 19, 2017 00:45

init

edeb860

feat: add new rule, landmark-no-more-than-one-main

3d37aab

test(landmark-at-least-one-main): updated integration tests for check

4e1b18b

feat(landmark-main-is-top-level): add rule ensuring each main landmar…

2840b02

…k in a document is not within another landmark element

Merge https://github.com/dequelabs/axe-core into main/is-top-level

fa6069d

WilcoFiers requested changes Jul 31, 2017

View reviewed changes

refactor(landmark-main-is-top-level): change help messages

6eb553b

marcysutton mentioned this pull request Aug 18, 2017

New rule, main landmark at least one #418

Closed

feat(landmark-main-is-top-level): change a function used in check and…

094e251

… update aria index file so application type is structure

WilcoFiers reviewed Aug 23, 2017

View reviewed changes

sulsanaul added 2 commits August 23, 2017 13:10

refactor(main-is-top-level): change pass/fail messages

2c55cbb

refactor(landmark-main-is-top-level): change description/help messages

b9b1bf5

sulsanaul added 3 commits August 23, 2017 13:27

Merge https://github.com/dequelabs/axe-core into main/is-top-level

bf85e0d

Merge branch 'main/is-top-level' of https://github.com/ssanaul/axe-core…

747990d

… into main/is-top-level

feat(main-is-top-level): update check for shadow dom features

9c0e75c

fix(main-is-top-level): update check to ignore form as a landmark

824ee2d

sulsanaul added 2 commits September 5, 2017 12:10

fix: edit incorrect usage of getComposedParent

05ee0a7

test: add unit test to check if main landmark in shadow dom is top level

73ed9cf

WilcoFiers requested changes Sep 16, 2017

View reviewed changes

sulsanaul and others added 4 commits September 21, 2017 12:40

style: remove spaces in attributes

f82c667

fix: update test so that checkSetup passes in correct argument

34407e9

test: add test to ensure correct number of violations nodes

3e5c6ad

Merge branch 'main/is-top-level' of https://github.com/ssanaul/axe-core…

ff463ef

… into main/is-top-level

sulsanaul and others added 2 commits September 21, 2017 14:42

fix: revert 'application' type to landmark

03e5434

Merge https://github.com/dequelabs/axe-core into main/is-top-level

56ad598

Merge https://github.com/dequelabs/axe-core into main/is-top-level

00b10c1

style: remove spaces in attributes

f936751

fix: allow main landmark to be in form

12b1903

WilcoFiers reviewed Oct 19, 2017

View reviewed changes

WilcoFiers approved these changes Nov 14, 2017

View reviewed changes

WilcoFiers merged commit 63040bd into dequelabs:develop Nov 14, 2017

dependencies bot mentioned this pull request Dec 19, 2017

Update 10 dependencies from npm storybookjs/storybook#2520

Merged

		@@ -0,0 +1,13 @@
		var landmarks = axe.commons.aria.getRolesByType('landmark');
		var parent = node.parentElement;

Add rule, landmark-main-is-top-level #462

Add rule, landmark-main-is-top-level #462

Conversation

sulsanaul commented Jul 26, 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

jongund Aug 18, 2017 • edited Loading

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

jongund commented Aug 23, 2017 via email

jongund commented Aug 23, 2017 via email

jongund commented Aug 23, 2017

jongund commented Aug 28, 2017

WilcoFiers commented Aug 29, 2017

jongund commented Sep 11, 2017

WilcoFiers commented Sep 12, 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

sulsanaul Oct 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sulsanaul commented Sep 21, 2017 • edited Loading

marcysutton commented Sep 26, 2017

WilcoFiers commented Oct 10, 2017

jongund commented Oct 10, 2017 via email

sulsanaul commented Oct 18, 2017

marcysutton commented Oct 18, 2017

sulsanaul commented Oct 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dylanb commented Nov 23, 2017

jongund Aug 18, 2017 •

edited

Loading

sulsanaul Oct 19, 2017 •

edited

Loading

sulsanaul commented Sep 21, 2017 •

edited

Loading