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

Conversation

courtyenn
Copy link

@courtyenn courtyenn commented Jan 26, 2022

I didn't see a contribution guideline. I still need to test this out, but just thought I'd submit a PR with the proposed changes.

According to MDN:

The disabled Boolean attribute provides CSS styles and semantics and removes the ability to click or focus while not disabling hover. By removing the ability to focus and removing it from the accessibility tree, it makes it invisible to assistive technology users. For good user experience, you want to make sure everyone can access all the visible content, no matter how they access the web. It is important to be aware that using the disabled attribute can harm usability.

Sources:

@codecov
Copy link

codecov bot commented Jan 26, 2022

Codecov Report

Merging #830 (b95ebd2) into main (566011b) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head b95ebd2 differs from pull request most recent head 23d6ef9. Consider uploading reports for the commit 23d6ef9 to get more accurate results

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #830   +/-   ##
=======================================
  Coverage   99.22%   99.22%           
=======================================
  Files          98       99    +1     
  Lines        1419     1428    +9     
  Branches      479      481    +2     
=======================================
+ Hits         1408     1417    +9     
  Misses         11       11           
Impacted Files Coverage Δ
src/index.js 100.00% <ø> (ø)
src/rules/no-disabled.js 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 566011b...23d6ef9. Read the comment docs.

@ljharb
Copy link
Member

ljharb commented Jan 26, 2022

Can you elaborate on this? There's many perfectly reasonable cases where it's better to have an input be disabled.

@courtyenn
Copy link
Author

@ljharb

Can you elaborate on this? There's many perfectly reasonable cases where it's better to have an input be disabled.

I am simply going off MDN on this:

The disabled Boolean attribute provides CSS styles and semantics and removes the ability to click or focus while not disabling hover. By removing the ability to focus and removing it from the accessibility tree, it makes it invisible to assistive technology users. For good user experience, you want to make sure everyone can access all the visible content, no matter how they access the web. It is important to be aware that using the disabled attribute can harm usability.

@ljharb
Copy link
Member

ljharb commented Jan 26, 2022

I did read that, and that is the first time in my entire career I've heard that advice - and it seems a VERY strong position.

Are there other references, a11y guidelines, ecosystem adoption, that confirm the opinion on MDN?

@courtyenn
Copy link
Author

@ljharb I can certainly do additional digging, but do you think there is a place for this plugin where maybe we warn others of this? I just learned of it today and I would prefer to give this concern exposure.

@ljharb
Copy link
Member

ljharb commented Jan 26, 2022

I absolutely think that this PR is the correct place/way to handle this, assuming the concern is as urgent and valid as MDN implies.

@andreasmcdermott
Copy link

W3's best practices lists a few specific cases where disabled should be avoided.

Removing focusability from disabled elements can offer users both advantages and disadvantages. Allowing keyboard users to skip disabled elements usually reduces the number of key presses required to complete a task. However, preventing focus from moving to disabled elements can hide their presence from screen reader users who "see" by moving the focus.

Authors are encouraged to adopt a consistent set of conventions for the focusability of disabled elements. The examples in this guide adopt the following conventions, which both reflect common practice and attempt to balance competing concerns.

For elements that are in the tab sequence when enabled, remove them from the tab sequence when disabled.
For the following composite widget elements, keep them focusable when disabled:

  • Options in a Listbox
  • Menu items in a Menu or menu bar
  • Tab elements in a set of Tabs
  • Tree items in a Tree View

@ljharb
Copy link
Member

ljharb commented Jan 26, 2022

That suggests that it'd be better to forbid "disabled" by default only on those four kinds of elements (altho an option to forbid it "everywhere" is fine). Thoughts?

@andreasmcdermott
Copy link

To me, two key take-aways from WAI-Aria's section:

  • Authors are encouraged to adopt a consistent set of conventions for the focusability of disabled elements.

  • [...] make them focusable if discoverability is a concern.

The second bullet refers to elements in a toolbar, but it seems like it could apply to any element. Eg I've seen it recommended that submit-buttons that are "disabled" until all required fields are completed should be focusable to ensure users can find it even before they complete the fields.

I'm not sure what the eslint rule should be though.. Maybe it is reasonable to only default to the four types of composite widgets mentioned in the WAI-Aria section. But I'm not sure that it's intended to be a complete list.

@courtyenn
Copy link
Author

courtyenn commented Jan 26, 2022

W3 states,

Authors are encouraged to adopt a consistent set of conventions for the focusability of disabled elements.

I think that means disabled is up for interpretation, but whatever it is, it should be adhered to. I believe assistive technology users should know about form elements, whether they are disabled or not. (Assuming this content is visible) Same goes for buttons that may be disabled due to permission. (Tabs, buttons, etc.) They ought to know. No?

To me a question I'd like answered is if the input isn’t hidden for visual users (when disabled); why hide it for non-visual users?

**Edit: (when disabled)

@ljharb
Copy link
Member

ljharb commented Jan 27, 2022

There’s a lot of things visually on a page that are hidden for non-visual users - borders, aspects of how a button looks, background colors and images - it doesn’t automatically make sense for something to be in the a11y tree just because it’s visible.

That said, i can imagine a good chunk of the places I’ve used the disabled attribute in the past are things whose presence would be useful for a non-visual user to know about. To be honest, this really feels like a bug/flaw in the way the a11y tree is built - ie, that it shouldn’t be omitting these elements. To be forced to use an aria attribute seems pretty absurd; their purpose is to provide additional info, or to hack around non-semantic html.

@courtyenn
Copy link
Author

The attribute was brought up in a code review session today.

I have a list of actions I'd like the user to take to onboard. However, they are not able to manually click on these checkboxes, but are there to serve as a visual cue if the user completed the actions. In this case, I chose to use disabled but that would hide the completion status from the screen-reader, so I chose to use aria-disabled instead.

image

@ljharb
Copy link
Member

ljharb commented Jan 27, 2022

The choice is meaningful and important; but it really does seem like the default should be to include them, and the aria override should be to hide them.

@jessebeach
Copy link
Collaborator

That suggests that it'd be better to forbid "disabled" by default only on those four kinds of elements (altho an option to forbid it "everywhere" is fine). Thoughts?

To be honest, this really feels like a bug/flaw in the way the a11y tree is built - ie, that it shouldn’t be omitting these elements.

Great conversation here. This came up at work the other day and Matt King also recommended not using the disabled attribute because, as folks have noted here, it knocks the elements out of the AX Tree. Instead, authors should use aria-disabled, which conveys the disabled-ness of the element, but leaves the element in the AX Tree so it can be perceived. @courtyenn notes this approach above, as well.

I'm in favor of this rule. Let me review the code. I think we should get it in.

Copy link
Collaborator

@jessebeach jessebeach left a comment

Choose a reason for hiding this comment

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

@courtyenn this is really fantastic! It's close, just a few nits to clean up. The basic logic is there. This is a great idea. Thank you for proposing it!

docs/rules/no-disabled.md Outdated Show resolved Hide resolved
src/rules/no-disabled.js Show resolved Hide resolved
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

@ljharb
Copy link
Member

ljharb commented Jan 31, 2022

@jessebeach is there any effort to fix this in browsers? semantic non-aria attributes are supposed to be the best approach, and this seems like the odd one out.

@jessebeach
Copy link
Collaborator

is there any effort to fix this in browsers? semantic non-aria attributes are supposed to be the best approach, and this seems like the odd one out.

Agreed. I haven't heard any rumblings of changing this behavior and my sense here is that this horse is long out of the barn, never to return. I think they just got the default behavior wrong here, probably because this attribute predates or is contemporaneous with assistive tech cursor traversal.

@courtyenn I think what @ljharb is insinuating, is that this rule is encouraging counter-intuitive behavior. It runs contrary to the general heuristic "Use semantic HTML first; ARIA second or not at all". So the explanation for this rule should acknowledge that this behavior runs counter to the general advice: https://github.com/jsx-eslint/eslint-plugin-jsx-a11y/pull/830/files#diff-1cf5c60cfe95228a04407905406f8fc98a66b9b0d0bc27f474e23a8976f8b206R3

@@ -3,8 +3,7 @@
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.
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

@jessebeach jessebeach left a comment

Choose a reason for hiding this comment

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

@courtyenn this is really close! Just a few copy changes to tighten up the documentation. This is really good stuff!

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

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.

@jessebeach
Copy link
Collaborator

@courtyenn it looks like the JS error in the tests is caused by the changes you introduced. Could you have a look?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants