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

WIP: core(axe): new audits empty-table-header, frame-focusable-content #12536

Closed
wants to merge 19 commits into from
Closed
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 32 additions & 0 deletions lighthouse-cli/test/cli/__snapshots__/index-test.js.snap
Original file line number Diff line number Diff line change
Expand Up @@ -222,6 +222,9 @@ Object {
Object {
"path": "accessibility/aria-roles",
},
Object {
"path": "accessibility/aria-text",
},
Object {
"path": "accessibility/aria-toggle-field-name",
},
Expand Down Expand Up @@ -261,9 +264,15 @@ Object {
Object {
"path": "accessibility/duplicate-id-aria",
},
Object {
"path": "accessibility/empty-table-header",
},
Object {
"path": "accessibility/form-field-multiple-labels",
},
Object {
"path": "accessibility/frame-focusable-content",
},
Object {
"path": "accessibility/frame-title",
},
Expand Down Expand Up @@ -300,6 +309,9 @@ Object {
Object {
"path": "accessibility/meta-viewport",
},
Object {
"path": "accessibility/nested-interactive",
},
Object {
"path": "accessibility/object-alt",
},
Expand Down Expand Up @@ -535,6 +547,11 @@ Object {
"id": "aria-roles",
"weight": 10,
},
Object {
"group": "a11y-navigation",
"id": "aria-text",
"weight": 0,
},
Object {
"group": "a11y-aria",
"id": "aria-toggle-field-name",
Expand Down Expand Up @@ -600,11 +617,21 @@ Object {
"id": "duplicate-id-aria",
"weight": 10,
},
Object {
"group": "a11y-tables-lists",
"id": "empty-table-header",
"weight": 0,
},
Object {
"group": "a11y-names-labels",
"id": "form-field-multiple-labels",
"weight": 2,
},
Object {
"group": "a11y-best-practices",
"id": "frame-focusable-content",
"weight": 0,
},
Object {
"group": "a11y-names-labels",
"id": "frame-title",
Expand Down Expand Up @@ -665,6 +692,11 @@ Object {
"id": "meta-viewport",
"weight": 10,
},
Object {
"group": "a11y-navigation",
"id": "nested-interactive",
"weight": 0,
},
Object {
"group": "a11y-names-labels",
"id": "object-alt",
Expand Down
15 changes: 15 additions & 0 deletions lighthouse-cli/test/fixtures/dobetterweb/dbw_tester.html
Original file line number Diff line number Diff line change
Expand Up @@ -225,6 +225,21 @@ <h2>Do better web tester page</h2>
<button class="small-button">Do something</button>
<button class="small-button">Do something else</button>

<!-- FAIL - frame-focusable-content -->
<iframe tabindex="-1" src="focusable-iframe.html"></iframe>
Copy link
Collaborator

Choose a reason for hiding this comment

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

use srcdoc instead to avoid the 1-line extra file? :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That was my first try, didn't work! Not supported I suppose.

Copy link
Member

Choose a reason for hiding this comment

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

That was my first try, didn't work! Not supported I suppose.

interesting, are srcdoc iframes a unique origin and so not available in the parent via js (so axe wouldn't be able to see inside)?

<!-- FAIL - nested-interactive -->
<button><span tabindex="0">fail</span></button>
<!-- FAIL - empty-table-header -->
<table>
<tr>
<th></th>
</tr>
</table>
<!-- FAIL - aria-text -->
<div role="text">
Still an <a href="#">interactive link</a> because of the author error.
</div>
Copy link
Member

Choose a reason for hiding this comment

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

move to a11y_tester with the other accessibility smoke tests?

Copy link
Collaborator Author

@connorjclark connorjclark May 24, 2021

Choose a reason for hiding this comment

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

These are here to populate the sample LHR. Do we care about coverage there?

Copy link
Member

Choose a reason for hiding this comment

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

These are here to populate the sample LHR. Do we care about coverage there?

ah, got it. Ideally yes, so maybe just duplicate in there? It's very helpful to have yarn smoke a11y cover (almost?) everything in the accessibility category


<!-- Some websites overwrite the original Error object. The captureJSCallUsage function
relies on the native Error object and prepareStackTrace from V8. When overwriting the stack
property the e.stack inside the gatherer won't be in the correct format
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
<a href='/'>Home</a>
44 changes: 44 additions & 0 deletions lighthouse-core/audits/accessibility/aria-text.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
/**
* @license Copyright 2021 The Lighthouse Authors. All Rights Reserved.
* Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. You may obtain a copy of the License at http://www.apache.org/licenses/LICENSE-2.0
* Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License.
*/
'use strict';

/**
* @fileoverview Ensures `role=text` is used on elements with no focusable descendants.
connorjclark marked this conversation as resolved.
Show resolved Hide resolved
* See base class in axe-audit.js for audit() implementation.
*/

const AxeAudit = require('./axe-audit.js');
const i18n = require('../../lib/i18n/i18n.js');

const UIStrings = {
/** Title of an accesibility audit that checks if there are any elements with `role=text` that have focusable descendants. This title is descriptive of the successful state and is shown to users when no user action is required. */
title: 'Ensures `role=text` is only used on elements with no focusable descendants',
connorjclark marked this conversation as resolved.
Show resolved Hide resolved
/** Title of an accesibility audit that checks if there are any elements with `role=text` that have focusable descendants. This title is descriptive of the failing state and is shown to users when there is a failure that needs to be addressed. */
failureTitle: '`role=text` elements should not have focusable descendants',
connorjclark marked this conversation as resolved.
Show resolved Hide resolved
// TODO: need web.dev article.
connorjclark marked this conversation as resolved.
Show resolved Hide resolved
/** Description of a Lighthouse audit that tells the user *why* they should try to pass. This is displayed after a user expands the section to see more. No character length limits. 'Learn More' becomes link text to additional documentation. */
description: 'Browsers may ignore `role=text` if it contains focusable descendants.',
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
description: 'Browsers may ignore `role=text` if it contains focusable descendants.',
description: 'Browsers may ignore elements with `role=text` if they contain focusable descendants.',

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

hmm not sure on this one. browsers ignore the element _as far as role=text is concerned, which is why I put the focus on that. The element still exists, is visible, etc. so it isn't entirely ignored :)

};

const str_ = i18n.createMessageInstanceIdFn(__filename, UIStrings);

class AriaText extends AxeAudit {
/**
* @return {LH.Audit.Meta}
*/
static get meta() {
return {
id: 'aria-text',
title: str_(UIStrings.title),
failureTitle: str_(UIStrings.failureTitle),
description: str_(UIStrings.description),
requiredArtifacts: ['Accessibility'],
};
}
}

module.exports = AriaText;
module.exports.UIStrings = UIStrings;
44 changes: 44 additions & 0 deletions lighthouse-core/audits/accessibility/empty-table-header.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
/**
* @license Copyright 2021 The Lighthouse Authors. All Rights Reserved.
* Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. You may obtain a copy of the License at http://www.apache.org/licenses/LICENSE-2.0
* Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License.
*/
'use strict';

/**
* @fileoverview Ensures table headers have discernible text.
* See base class in axe-audit.js for audit() implementation.
*/

const AxeAudit = require('./axe-audit.js');
const i18n = require('../../lib/i18n/i18n.js');

const UIStrings = {
/** Title of an accesibility audit that checks if there are any duplicate ARIA IDs on the page. This title is descriptive of the successful state and is shown to users when no user action is required. */
connorjclark marked this conversation as resolved.
Show resolved Hide resolved
title: 'Ensures table headers have discernible text',
connorjclark marked this conversation as resolved.
Show resolved Hide resolved
/** Title of an accesibility audit that checks if there are any duplicate ARIA IDs on the page. This title is descriptive of the failing state and is shown to users when there is a failure that needs to be addressed. */
failureTitle: 'Table header text must not be empty',
connorjclark marked this conversation as resolved.
Show resolved Hide resolved
// TODO: need web.dev article.
/** Description of a Lighthouse audit that tells the user *why* they should try to pass. This is displayed after a user expands the section to see more. No character length limits. 'Learn More' becomes link text to additional documentation. */
description: 'Table headers should have discernible text.',
connorjclark marked this conversation as resolved.
Show resolved Hide resolved
};

const str_ = i18n.createMessageInstanceIdFn(__filename, UIStrings);

class EmptyTableHeader extends AxeAudit {
/**
* @return {LH.Audit.Meta}
*/
static get meta() {
return {
id: 'empty-table-header',
title: str_(UIStrings.title),
failureTitle: str_(UIStrings.failureTitle),
description: str_(UIStrings.description),
requiredArtifacts: ['Accessibility'],
};
}
}

module.exports = EmptyTableHeader;
module.exports.UIStrings = UIStrings;
44 changes: 44 additions & 0 deletions lighthouse-core/audits/accessibility/frame-focusable-content.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
/**
* @license Copyright 2021 The Lighthouse Authors. All Rights Reserved.
* Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. You may obtain a copy of the License at http://www.apache.org/licenses/LICENSE-2.0
* Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License.
*/
'use strict';

/**
* @fileoverview No `<iframe tabindex=-1>` has focusable content.
* See base class in axe-audit.js for audit() implementation.
*/

const AxeAudit = require('./axe-audit.js');
const i18n = require('../../lib/i18n/i18n.js');

const UIStrings = {
/** Title of an accesibility audit that checks if there are any iframes with focusable content that mistakently set tabindex=1. This title is descriptive of the successful state and is shown to users when no user action is required. */
connorjclark marked this conversation as resolved.
Show resolved Hide resolved
title: 'No `<iframe tabindex=-1>` has focusable content',
connorjclark marked this conversation as resolved.
Show resolved Hide resolved
/** Title of an accesibility audit that checks if there are any iframes with focusable content that mistakently set tabindex=1. This title is descriptive of the failing state and is shown to users when there is a failure that needs to be addressed. */
connorjclark marked this conversation as resolved.
Show resolved Hide resolved
failureTitle: '`<iframe tabindex=-1>` has focusable content',
connorjclark marked this conversation as resolved.
Show resolved Hide resolved
// TODO: need web.dev article.
/** Description of a Lighthouse audit that tells the user *why* they should try to pass. This is displayed after a user expands the section to see more. No character length limits. 'Learn More' becomes link text to additional documentation. */
description: '`<iframe>` should only have `tabindex=1` if there it contains no focusable elements.',
connorjclark marked this conversation as resolved.
Show resolved Hide resolved
};

const str_ = i18n.createMessageInstanceIdFn(__filename, UIStrings);

class FrameFocusableContent extends AxeAudit {
/**
* @return {LH.Audit.Meta}
*/
static get meta() {
return {
id: 'frame-focusable-content',
title: str_(UIStrings.title),
failureTitle: str_(UIStrings.failureTitle),
description: str_(UIStrings.description),
requiredArtifacts: ['Accessibility'],
};
}
}

module.exports = FrameFocusableContent;
module.exports.UIStrings = UIStrings;
44 changes: 44 additions & 0 deletions lighthouse-core/audits/accessibility/nested-interactive.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
/**
* @license Copyright 2021 The Lighthouse Authors. All Rights Reserved.
* Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. You may obtain a copy of the License at http://www.apache.org/licenses/LICENSE-2.0
* Unless required by applicable law or agreed to in writing, software distributed under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License.
*/
'use strict';

/**
* @fileoverview Ensure interactive controls are not nested.
* See base class in axe-audit.js for audit() implementation.
*/

const AxeAudit = require('./axe-audit.js');
const i18n = require('../../lib/i18n/i18n.js');

const UIStrings = {
/** Title of an accesibility audit that checks if there are any duplicate ARIA IDs on the page. This title is descriptive of the successful state and is shown to users when no user action is required. */
connorjclark marked this conversation as resolved.
Show resolved Hide resolved
title: 'Ensure interactive controls are not nested',
connorjclark marked this conversation as resolved.
Show resolved Hide resolved
/** Title of an accesibility audit that checks if there are any duplicate ARIA IDs on the page. This title is descriptive of the failing state and is shown to users when there is a failure that needs to be addressed. */
failureTitle: 'Interactive controls should not be nested',
connorjclark marked this conversation as resolved.
Show resolved Hide resolved
// TODO: need web.dev article.
/** Description of a Lighthouse audit that tells the user *why* they should try to pass. This is displayed after a user expands the section to see more. No character length limits. 'Learn More' becomes link text to additional documentation. */
description: 'Nested interactive controls should not be nested, because that would result in confusing screen reader experiences.',
connorjclark marked this conversation as resolved.
Show resolved Hide resolved
};

const str_ = i18n.createMessageInstanceIdFn(__filename, UIStrings);

class NestedInteractive extends AxeAudit {
Copy link
Member

Choose a reason for hiding this comment

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

if nested-interactive is problematic, I say we just put 'nested-interactive': {enabled: false}, in accessibility.js and put a TODO linked to your dequelabs/axe-core#2958. Looking at the history of nested-interactive, the discussion seems to be all around <button> and e.g. links inside elements that are role="checkbox", but then it became all presentational elements. We thought we were doing well in the report with our <details>, so I think in this case we at least need to figure out how we fix ourselves before we demand other people fix their own sites :)

/**
* @return {LH.Audit.Meta}
*/
static get meta() {
return {
id: 'nested-interactive',
title: str_(UIStrings.title),
failureTitle: str_(UIStrings.failureTitle),
description: str_(UIStrings.description),
requiredArtifacts: ['Accessibility'],
};
}
}

module.exports = NestedInteractive;
module.exports.UIStrings = UIStrings;
12 changes: 12 additions & 0 deletions lighthouse-core/config/default-config.js
Original file line number Diff line number Diff line change
Expand Up @@ -255,6 +255,7 @@ const defaultConfig = {
'accessibility/aria-required-children',
'accessibility/aria-required-parent',
'accessibility/aria-roles',
'accessibility/aria-text',
'accessibility/aria-toggle-field-name',
'accessibility/aria-tooltip-name',
'accessibility/aria-treeitem-name',
Expand All @@ -268,7 +269,9 @@ const defaultConfig = {
'accessibility/document-title',
'accessibility/duplicate-id-active',
'accessibility/duplicate-id-aria',
'accessibility/empty-table-header',
'accessibility/form-field-multiple-labels',
'accessibility/frame-focusable-content',
'accessibility/frame-title',
'accessibility/heading-order',
'accessibility/html-has-lang',
Expand All @@ -281,6 +284,7 @@ const defaultConfig = {
'accessibility/listitem',
'accessibility/meta-refresh',
'accessibility/meta-viewport',
'accessibility/nested-interactive',
'accessibility/object-alt',
'accessibility/tabindex',
'accessibility/td-headers-attr',
Expand Down Expand Up @@ -508,6 +512,8 @@ const defaultConfig = {
{id: 'aria-required-children', weight: 10, group: 'a11y-aria'},
{id: 'aria-required-parent', weight: 10, group: 'a11y-aria'},
{id: 'aria-roles', weight: 10, group: 'a11y-aria'},
/** TODO: add weight for v8 */
{id: 'aria-text', weight: 0, group: 'a11y-navigation'},
{id: 'aria-toggle-field-name', weight: 3, group: 'a11y-aria'},
{id: 'aria-tooltip-name', weight: 3, group: 'a11y-aria'},
{id: 'aria-treeitem-name', weight: 3, group: 'a11y-aria'},
Expand All @@ -521,7 +527,11 @@ const defaultConfig = {
{id: 'document-title', weight: 3, group: 'a11y-names-labels'},
{id: 'duplicate-id-active', weight: 3, group: 'a11y-navigation'},
{id: 'duplicate-id-aria', weight: 10, group: 'a11y-aria'},
/** TODO: add weight for v8 */
{id: 'empty-table-header', weight: 0, group: 'a11y-tables-lists'},
connorjclark marked this conversation as resolved.
Show resolved Hide resolved
{id: 'form-field-multiple-labels', weight: 2, group: 'a11y-names-labels'},
/** TODO: add weight for v8 */
{id: 'frame-focusable-content', weight: 0, group: 'a11y-best-practices'},
connorjclark marked this conversation as resolved.
Show resolved Hide resolved
{id: 'frame-title', weight: 3, group: 'a11y-names-labels'},
{id: 'heading-order', weight: 2, group: 'a11y-navigation'},
{id: 'html-has-lang', weight: 3, group: 'a11y-language'},
Expand All @@ -534,6 +544,8 @@ const defaultConfig = {
{id: 'listitem', weight: 3, group: 'a11y-tables-lists'},
{id: 'meta-refresh', weight: 10, group: 'a11y-best-practices'},
{id: 'meta-viewport', weight: 10, group: 'a11y-best-practices'},
/** TODO: add weight for v8 */
{id: 'nested-interactive', weight: 0, group: 'a11y-navigation'},
{id: 'object-alt', weight: 3, group: 'a11y-names-labels'},
{id: 'tabindex', weight: 3, group: 'a11y-navigation'},
{id: 'td-headers-attr', weight: 3, group: 'a11y-tables-lists'},
Expand Down
2 changes: 2 additions & 0 deletions lighthouse-core/gather/gatherers/accessibility.js
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,8 @@ async function runA11yChecks() {
'svg-img-alt': {enabled: false},
'audio-caption': {enabled: false},
'aria-treeitem-name': {enabled: true},
'empty-table-header': {enabled: true},
'aria-text': {enabled: true},
brendankenny marked this conversation as resolved.
Show resolved Hide resolved
},
});

Expand Down
Loading