eslint-plugin-jsx-a11y icon indicating copy to clipboard operation
eslint-plugin-jsx-a11y copied to clipboard

[New] no-disabled rule

Open courtyenn opened this issue 2 years ago • 17 comments

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:

  • https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Attributes/aria-disabled
  • https://www.w3.org/TR/wai-aria-practices/#kbd_disabled_controls
  • https://medium.com/@izzmo/anti-pattern-using-disabled-or-read-only-for-form-input-e2c24c669f5b
  • https://css-tricks.com/making-disabled-buttons-more-inclusive/#aa-the-difference-between-disabled-and-aria-disabled
  • https://adrianroselli.com/2021/01/multi-function-button.html#Active

courtyenn avatar Jan 26 '22 21:01 courtyenn

Codecov Report

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

:exclamation: 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.

codecov[bot] avatar Jan 26 '22 21:01 codecov[bot]

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

ljharb avatar Jan 26 '22 21:01 ljharb

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

courtyenn avatar Jan 26 '22 21:01 courtyenn

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?

ljharb avatar Jan 26 '22 21:01 ljharb

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

courtyenn avatar Jan 26 '22 21:01 courtyenn

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.

ljharb avatar Jan 26 '22 22:01 ljharb

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

andreasmcdermott avatar Jan 26 '22 22:01 andreasmcdermott

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?

ljharb avatar Jan 26 '22 22:01 ljharb

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.

andreasmcdermott avatar Jan 26 '22 22:01 andreasmcdermott

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)

courtyenn avatar Jan 26 '22 23:01 courtyenn

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.

ljharb avatar Jan 27 '22 04:01 ljharb

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

courtyenn avatar Jan 27 '22 04:01 courtyenn

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.

ljharb avatar Jan 27 '22 05:01 ljharb

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.

jessebeach avatar Jan 31 '22 03:01 jessebeach

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

ljharb avatar Jan 31 '22 05:01 ljharb

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

jessebeach avatar Jan 31 '22 20:01 jessebeach

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

jessebeach avatar Feb 16 '22 19:02 jessebeach