ES Lint: Add a rule to prevent use of the disabled attribute #59518
Conversation
scruffian
added
[Focus] Accessibility (a11y)
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
Size Change: 0 B Total Size: 1.71 MB ℹ️ View Unchanged
|
It will only check changed code. I'd like more detail on why this is a good idea though. Are we sure there is never a case when |
|
@scruffian It could be under some very specific situations and I think one of the ESLint exclude rules could be used in these cases. All in all, I do not think we should be encouraging inaccessible code, there is no way to make the This way, the button is disabled for clicks but it still remains in the tab order for keyboard users. It is quite tricky to see buttons visually but tab key never lands on them. Happy to hear feedback from @afercia and @joedolson. I think this is still a good step to take. |
|
I think having linting rules that check for this is a good idea. Creating controls or interface elements that are unfindable is highly problematic. We can be willing to address exceptions, but I think one of the benefits to linting tools is that we have to make a conscious decision to override our tooling, which is a good double check on our intentions, to assess whether this is a legitimate case for disabling something. I'll also note that any disabled input or control should come with some indication of why it's disabled and what conditions are required to make it enabled; otherwise disabled controls can be very confusing. |
|
@WordPress/gutenberg-components - Could the |
|
@jeryj I think that is a logical next step but preventing this in the first place is good. This allows us to avoid inaccessible defaults in the future dev work. |
|
While I can understand the motivation behind this PR, and agree generally with the comments so far, I'm not sure whether this is the best way to proceed as a first step. Have we looked properly at where and how Perhaps the lint rule could/should initially only look at native elements? // ✅ These would be fine (for now, at least):
...( <Button disabled /> );
...( <Checkbox disabled /> );
// ⛔️ These would not be:
...( <button disabled /> );
...( <input type="checkbox" disabled /> ); |
|
@andrewhayward The way I understood it, this rule would only catch new errors, not existing. |
Sorry @alexstine, I wasn't super clear. My concern is that if someone makes a change that involves setting <MyComponent
{ ...props }
+ disabled
/>Equally, I would find it very confusing to have eslint overrides dotted around to bypass 'restricted syntax' errors, where <MyComponent
{ ...props }
+ {/* eslint-disable-next-line no-restricted-syntax */}
+ disabled
/>As an aside, it actually looks like the lint rule is catching all current instances of |
|
@andrewhayward Understand your concern totally. How do you think we should better educate developers on |
|
Related: #56547 This topic (not necessarily this PR specifically) affects many of our components and will potentially result in changes to their default behavior and API design. In this PR thread, it looks like there is an implicit consensus that focusable disabled should not only be the default behavior, but should be enforced 100% of the time. In my research, I could not find evidence that this is a widely held consensus or a de facto standard outside of this project. Given that focusable disabled is not the default behavior with native form elements in the browser, and the APG says to use your best judgement in each instance, could some of you elaborate on the following:
I don't have a strong opinion either way, but I just want to make sure we have good, documented arguments to justify the decision before making broad changes to our component APIs. |
|
@mirka Answers.
Because, it is really confusing when you can visually see a button and not be able to access it with the keyboard. For blind users like myself, I might want to tab to the save button and hear that it is disabled so I can go try to figure out why vs. just never being able to find it in the first place. The third big reason and one that has hurt all of us a time or two in the project is disabled buttons cannot take focus so this is a really easy way to introduce focus loss especially in React where
I feel like there might be some use-cases for this but having a hard time coming up with any at the moment. Others I am sure will chime in. Could we instead convert this to a warning? It wouldn't be a failure but something that would make developers ask "am I sure I should use |
|
To be clear, I'm not questioning the pros of focusable disabled, rather I want to know why in our particular project it clearly outweighs the cons. The cons being:
If most of us here think that yes, the pros overwhelmingly outweigh the cons, that's great. Let's get that discussion written down so we can point to it to justify our decision. Because contrary to what I assumed before I looked it up, there are not a lot of external sources we can point to that says "focusable disabled is always the desirable behavior and we should petition browser vendors to change their defaults", for example. |
|
@mirka Understood, your concerns are well noted. :) Want to make sure things are done right.
My personal opinion about this, if it is available to the sighted, it should be available to all. That kind of goes back to the concerns I raised over the use of inert but that was a much more severe issue. I also do not think we have to change This should return something like:
Does ESLint have such an ability? I am perfectly fine with just trying out the warning as a starting proof of concept. So, the idea isn't to change how HTML attributes are used or how CSS selectors can target, simply warn developers that this might be a bad idea. Going forward, I think we could help devs out by introducing Also worth noting, I agree. This should be restricted to new code only and Gutenberg development. This should not be published in the ESLint package for people simply creating custom blocks. Thanks. |
|
I can't find a way to make only this rule a warning |
|
All, how can we move this forward? We really need to start putting some accessibility safety checks in place because Gutenberg has simply become way too unmanageable to review all new PRs for a single day in the time I have available. The solution, let's try to add some guard rails and hope people follow them. @mirka I was actually thinking a bit about your comments from earlier and the other thing I'll add, there are a lot of standards out there on the web today. Some good, some okay, some bad. The one common theme, most of the choices that are being made at the W3C level are not by the users that are impacted by such choices. I'm still brainstorming on how to fix that bigger issue but I think as an open-source community, if we divert from standards we can all agree cause more harm than good, let's go for it. It's really ironic that people making standards for screen readers are not majority visually impaired/blind, at a certain point, I just take too much of an issue with that to read every standard as pure truth. It would be like me trying to set standards for users of wheelchairs. While I have some idea about wide doors, ramps, elevators, automatic doors, I don't have that type of disability that will truly allow me to ever fully understand it. Thanks. |
The problem is not the Many developers that decide to dynamically disable a focusable control while it may have keyboard focus aren't educated or trained to understand how much that is impactful. For seven years, contributors that are more aware that this is a huge problem tried to educate other contributors to avoid this pattern. Alas, it appears that education efforts aren't enough. There several cases where focus losses triggered by a At this point, I think the only viable solution is to enforce education via code by adding an ESLint rule. However, as I mentioned on another pull request, there are legitimate cases where an interactive control needs to be 'truly' disabled. Perhaps, a viable option could be:
I'd tend to think something like the above would also have an important educational value:
|
|
I'm ok with moving forward with a lint rule if need be. Before that though, if this is mostly about focus loss in cases when a button is dynamically disabled, can we also consider the alternative solution, which is to automatically apply focusable disabled when a button becomes disabled while it's the focused element? I'd prefer trying that first, since it will benefit all consumers of Button and not just projects with the lint rule. |
I'm not sure that would cover all the cases of a focus loss triggered by disabling a button (or other interactive, focusable control). For example, a control may not have the current focus but it could be the candidate to receive focus after some action. E.g.
If I remember correctly, there have been a couple cases like the one described above. The point is that may be other scenarios besides the 'button becomes disabled while it's the focused element' one. |
|
Happy for someone else to pick this up and merge it if they want to |
|
@mirka Please see my PR here as a good example of the point made by @afercia . Because the |
|
I proposed an alternative #62080 way to approach this lint rule that I think would be a bit more measured to start with, but can still scale out as necessary. |
What?
Adds a rule to prevent the use of the disabled attribute.
Why?
How?
Adds a new ES Lint rule to catch this.
Testing Instructions
disabledto any React component