Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 24 commits into from
Nov 14, 2017

Conversation

sulsanaul
Copy link
Contributor

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.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use !landmarks.includes(role).

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We will make this change

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@WilcoFiers
We have updated the pull request with requested changes

@@ -0,0 +1,16 @@
{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove this rule from the PR.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

"metadata": {
"impact": "moderate",
"messages": {
"pass": "Main landmark is top level",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use a full sentence for both texts.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link

@jongund jongund Aug 18, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

@WilcoFiers WilcoFiers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

],
"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"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use .includes() instead.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@jongund
Copy link

jongund commented Aug 23, 2017 via email

@jongund
Copy link

jongund commented Aug 23, 2017 via email

@jongund
Copy link

jongund commented Aug 23, 2017

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
Copy link

jongund commented Aug 28, 2017

We updated the rule to use:

  1. axe.commons.getComposedParent
  2. 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
Copy link
Contributor

@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
Copy link

jongund commented Sep 11, 2017

Anymore questions or issues with this pull request?

What is the status of accepting this rule?

@WilcoFiers
Copy link
Contributor

@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.

Copy link
Contributor

@WilcoFiers WilcoFiers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please don't use spaces in attributes.


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a node count check too:

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ssanaul This issue isn't addressed yet.

Copy link
Contributor Author

@sulsanaul sulsanaul Oct 19, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

@sulsanaul
Copy link
Contributor Author

sulsanaul commented Sep 21, 2017

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
Copy link
Contributor

@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
Copy link
Contributor

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

@jongund
Copy link

jongund commented Oct 10, 2017 via email

@sulsanaul
Copy link
Contributor Author

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

@marcysutton
Copy link
Contributor

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
Copy link
Contributor Author

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);

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to work with role=form too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does line 5 not accomplish that?

@WilcoFiers WilcoFiers merged commit 63040bd into dequelabs:develop Nov 14, 2017
@dylanb
Copy link
Contributor

dylanb commented Nov 23, 2017

@ssanaul thank you for your contribution!!!

marcysutton pushed a commit that referenced this pull request Dec 7, 2017
* 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
mrtnvh pushed a commit to mrtnvh/axe-core that referenced this pull request Nov 24, 2023
…/react/examples/shadow-dom (dequelabs#462)

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants