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 5 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),
});
32 changes: 32 additions & 0 deletions docs/rules/no-disabled.md
@@ -0,0 +1,32 @@
# 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

Warns usage of `disabled` property.

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

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

## Accessibility guidelines
General best practice (reference resources)
courtyenn marked this conversation as resolved.
Show resolved Hide resolved

### 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)
3 changes: 3 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,7 @@ 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',
'jsx-a11y/no-distracting-elements': 'error',
'jsx-a11y/no-interactive-element-to-noninteractive-role': [
'error',
Expand Down Expand Up @@ -270,6 +272,7 @@ 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',
'jsx-a11y/no-distracting-elements': 'error',
'jsx-a11y/no-interactive-element-to-noninteractive-role': 'error',
'jsx-a11y/no-noninteractive-element-interactions': [
Expand Down
52 changes: 52 additions & 0 deletions src/rules/no-disabled.js
@@ -0,0 +1,52 @@
/**
* @fileoverview Rule to flag 'disabled' prop
* @author Courtney Nguyen <@courtyenn>
*/

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

import { propName, elementType } from 'jsx-ast-utils';

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',
];

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: [],
},

create: (context) => ({
JSXAttribute: (attribute) => {
// Only monitor eligible elements for "disabled".
const type = elementType(attribute.parent);
if (!DEFAULT_ELEMENTS.includes(type)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I recall that we have support issues with includes and older versions of Node. Could you rewrite this as a for loop?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@ljharb do you have an issue with includes?

Copy link
Member

Choose a reason for hiding this comment

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

I have a much larger issue with loops :-) we support down to eslint 3, which requires node 4+, which lacks .includes. However, we can and should use the array-includes package for this, which will prefer the native method when available.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Got it. I will endeavor to commit this detail to memory for the next time =D

return;
}

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