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

[New] no-disabled rule #830

Draft
wants to merge 8 commits into
base: main
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
53 changes: 53 additions & 0 deletions __tests__/src/rules/no-disabled-test.js
@@ -0,0 +1,53 @@
/* eslint-env jest */
/**
* @fileoverview Enforce disabled prop is not used.
* @author Courtney Nguyen <@courtyenn>
*/

// -----------------------------------------------------------------------------
// Requirements
// -----------------------------------------------------------------------------

import { RuleTester } from 'eslint';
import rule from '../../../src/rules/no-disabled';
import parserOptionsMapper from '../../__util__/parserOptionsMapper';

// -----------------------------------------------------------------------------
// Tests
// -----------------------------------------------------------------------------

const ruleTester = new RuleTester();

const expectedWarning = {
message: 'The disabled prop removes the element from being detected by screen readers.',
type: 'JSXAttribute',
};

ruleTester.run('no-disabled', rule, {
valid: [
{ code: '<div />' },
{ code: '<div disabled />' },
{ code: '<input />' },
{ code: '<button />' },
{ code: '<select />' },
{ code: '<textarea />' },
{ code: '<option />' },
{ code: '<command />' },
{ code: '<fieldset />' },
{ code: '<keygen />' },
{ code: '<optgroup />' },
].map(parserOptionsMapper),
invalid: [
{ code: '<input disabled />', errors: [expectedWarning] },
{ code: '<input disabled="true" />', errors: [expectedWarning] },
{ code: '<input disabled="false" />', errors: [expectedWarning] },
{ code: '<button disabled />', errors: [expectedWarning] },
{ code: '<select disabled />', errors: [expectedWarning] },
{ code: '<textarea disabled />', errors: [expectedWarning] },
{ code: '<option disabled />', errors: [expectedWarning] },
{ code: '<command disabled />', errors: [expectedWarning] },
{ code: '<fieldset disabled />', errors: [expectedWarning] },
{ code: '<keygen disabled />', errors: [expectedWarning] },
{ code: '<optgroup disabled />', errors: [expectedWarning] },
].map(parserOptionsMapper),
});
28 changes: 28 additions & 0 deletions docs/rules/no-disabled.md
@@ -0,0 +1,28 @@
# no-disabled

Rule that `disabled` prop should be cautioned on elements. Disabling interactive elements removes the element from the accessibility tree. Consider using `aria-disabled`.

## Rule details
The general consensus is that `disabled` should be used with specific intent. It goes against intuition since `disabled` is a native HTML attribute, which disables. However, from a usability stand-point it is not a good UX for screen readers: It removes the element from the a11y tree, and may omit critical information. It is best to use `aria-disabled` and add the additional JS logic to "disable" the element.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
The general consensus is that `disabled` should be used with specific intent. It goes against intuition since `disabled` is a native HTML attribute, which disables. However, from a usability stand-point it is not a good UX for screen readers: It removes the element from the a11y tree, and may omit critical information. It is best to use `aria-disabled` and add the additional JS logic to "disable" the element.
The general consensus is that `disabled` should be used with specific intent. It goes against intuition since `disabled` is a native HTML attribute, which disables. However, from a usability standpoint it is not a good UX for screen readers: it removes the element from the a11y tree, and may omit critical information. It is best to use `aria-disabled` and add additional JS logic to "disable" the element.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm going to be a bit more prescriptive here with this text. Apologies for the heavy-handed editing:

"The disabled attribute renders an HTML form element inoperable -- it cannot be focused or pressed. Disabled elements are visually rendered with lighter backgrounds and borders, indicating through visual affordance that they are present but inoperable. Unfortunately, an HTML form element with the disabled attribute is not rendered to the Accessibility Tree, so if a user cannot perceive the interface visually, they will not perceive the disabled form elements.

For this reason, we recommend that authors not use the disabled HTML attribute, which admittedly goes against the common best practice to use semantic HTML. Instead, use aria-disabled to indicate to assistive technology agents that an element is disabled. Additionally, use CSS to alter the appearance of an element to give it the visual appearance of a disabled state and use JavaScript to prevent the default behaviors of the element."


### Succeed
```jsx
<div />
<div disabled />
<input />
<select />
<textarea />
<button />
```

### Fail
```jsx
<input disabled />
<input disabled="true" />
<input disabled="false" />
<input disabled={undefined} />
```

### Resources
- [MDN aria-disabled](https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Attributes/aria-disabled)
- [W3 KBD Disabled Controls](https://www.w3.org/TR/wai-aria-practices/#kbd_disabled_controls)
27 changes: 27 additions & 0 deletions src/index.js
Expand Up @@ -26,6 +26,7 @@ module.exports = {
'mouse-events-have-key-events': require('./rules/mouse-events-have-key-events'),
'no-access-key': require('./rules/no-access-key'),
'no-autofocus': require('./rules/no-autofocus'),
'no-disabled': require('./rules/no-disabled'),
'no-distracting-elements': require('./rules/no-distracting-elements'),
'no-interactive-element-to-noninteractive-role': require('./rules/no-interactive-element-to-noninteractive-role'),
'no-noninteractive-element-interactions': require('./rules/no-noninteractive-element-interactions'),
Expand Down Expand Up @@ -113,6 +114,19 @@ module.exports = {
'jsx-a11y/mouse-events-have-key-events': 'error',
'jsx-a11y/no-access-key': 'error',
'jsx-a11y/no-autofocus': 'error',
'jsx-a11y/no-disabled': ['warn', {
disabable: [
'button',
'command',
'fieldset',
'keygen',
'optgroup',
'option',
'select',
'textarea',
'input',
],
}],
'jsx-a11y/no-distracting-elements': 'error',
'jsx-a11y/no-interactive-element-to-noninteractive-role': [
'error',
Expand Down Expand Up @@ -270,6 +284,19 @@ module.exports = {
'jsx-a11y/mouse-events-have-key-events': 'error',
'jsx-a11y/no-access-key': 'error',
'jsx-a11y/no-autofocus': 'error',
'jsx-a11y/no-disabled': ['warn', {
disabable: [
'button',
'command',
'fieldset',
'keygen',
'optgroup',
'option',
'select',
'textarea',
'input',
],
}],
'jsx-a11y/no-distracting-elements': 'error',
'jsx-a11y/no-interactive-element-to-noninteractive-role': 'error',
'jsx-a11y/no-noninteractive-element-interactions': [
Expand Down
64 changes: 64 additions & 0 deletions src/rules/no-disabled.js
@@ -0,0 +1,64 @@
/**
* @fileoverview Rule to flag 'disabled' prop
* @author Courtney Nguyen <@courtyenn>
*/

// ----------------------------------------------------------------------------
// Rule Definition
// ----------------------------------------------------------------------------

import { propName, elementType } from 'jsx-ast-utils';
import {
enumArraySchema,
generateObjSchema,
} from '../util/schemas';

const warningMessage = 'The disabled prop removes the element from being detected by screen readers.';
Copy link
Collaborator

Choose a reason for hiding this comment

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

"The disabled attribute makes the element imperceivable to assistive technology agents, like screen readers."

Just a small nit that an attribute is a HTML feature, whereas a prop is a DOM concept. In this case, we are detecting the presence of an attribute, not a prop.

Also, we should avoid being screen-reader-centric. There are many assistive technology agents. Screen readers are common and probably what a developer is familiar with, so it helps to mention them. We just want to be sure we leave room for the full spectrum of agents that we need to support.


const DEFAULT_ELEMENTS = [
jessebeach marked this conversation as resolved.
Show resolved Hide resolved
'button',
'command',
'fieldset',
'keygen',
'optgroup',
'option',
'select',
'textarea',
'input',
];

const schema = generateObjSchema({
disabable: enumArraySchema(DEFAULT_ELEMENTS)
.filter((name) => DEFAULT_ELEMENTS.includes(name)),
});

export default {
meta: {
type: 'suggestion',
docs: {
recommended: false,
url: 'https://github.com/jsx-eslint/eslint-plugin-jsx-a11y/tree/HEAD/docs/rules/no-disabled.md',
},
schema: [schema],
},

create: (context) => ({
JSXAttribute: (attribute) => {
const disabable = (
context.options && context.options[0] && context.options[0].disabable
) || DEFAULT_ELEMENTS;
// Only monitor eligible elements for "disabled".
const type = elementType(attribute.parent);
if (!disabable.includes(type)) {
return;
}

if (propName(attribute) === 'disabled') {
context.report({
node: attribute,
message: warningMessage,
});
}
},
}),
};