Make sure disabled buttons are always focusable by default

[afercia]

Description

Splitting this out from #56396 (comment) for a broader discussion. Cc @ciampo @andrewhayward @alexsanford @joedolson

As noted in this comment, focusable elements that are dynamically added or removed / hidden / disabled potentially trigger a focus loss and impact other utilities, components and hooks like the @wordpress/dom utilities focusable and tabbable, useConstrainedTabbing and useFocusOutside. There have been several cases so far where focus losses were triggered because of this. When using a keyboard, focus losses are a terrible user experience that needs to be avoided.

RIght now, the Button component has an __experimentalIsFocusable prop that is meant to render aria-disabled instead of the disabled attribute to keep disabled buttons focusable. In #56396 (comment) a proposal was made to make this prop true bu default:

any potential drawbacks if the Button component had the __experimentalIsFocusable prop true by default?

I'm not sure this would be ideal. If I understand correctly, the current situation is something like this, in pseudo-code:

<Button disabled __experimentalIsFocusable />

Conceptually, I find this confusing. disabled is a HTML attribute, it should not be used as a prop name. I think this is source of confusion for many new-generation contributors that are used to just use components props without a deep understanding of what happens in terms of the DOM and the rendered HTML.

If I understand correctly, what has been proposed in #56396 (comment) would be, in pseudo-code, something like this:

<Button disabled __experimentalIsFocusable={ true } />

And when both props are true, under the hood the button would use aria-disabled. I'd think this would be even more confusing because it dosn't explain what it does and it doesn't educate contributors.

What we need for accessibility

In 90% of the cases, the disabled attribute should not be used. In fact, in the editor we established a pattern for 'disabled' buttons to:

• use aria-disabled to keep buttons focusable
• noop the disabled button

This is in place for two reasons:

• avoid focus losses
• discoverablity of disabled controls

See W3C: ARIA Authoring Practices Guide (APG) > Developing a Keyboard Interface > Focusability of disabled controls
https://www.w3.org/WAI/ARIA/apg/practices/keyboard-interface/#focusabilityofdisabledcontrols

Considering that this is what we'd want in 90% of the caees, I'd like to propose to consider to reverse the logic.

Instead of making the exception the default behavior, disabled buttons should render aria-disabled and be noop-ed by default.

• The disabled prop should be renamed to isDisabled and implement the above behavior.
• A new prop isFullyDisabled (or a better name) should be used to render a disabled attribute instead of the above.
• A new custom eslint rule should be added to detect the usage of the isFullyDisabled prop or any other disabled HTML attribute being rendered. The rule should prevent rendering a disabled HTML attribute unless an eslint-disable* comment is added that requires to specify the disable reason.

I may entirely be missing something but I think the editor needs a solid way to prevent focus losses that are often triggered by disabling buttons while they are focused. Also, contributors education is key and all this should be very well documented.

Any thoughts and feedback more than welcome.

Step-by-step reproduction instructions

Not applicable.

Screenshots, screen recording, code snippet

No response

Environment info

No response

Please confirm that you have searched existing issues in the repo.

Yes

Please confirm that you have tested with all plugins deactivated except Gutenberg.

Yes

[afercia]

As a side note, this comment:

gutenberg/packages/components/src/button/index.tsx

Lines 179 to 180 in 0adb73f

	// In this case, the button will be disabled, but still focusable and
	// perceivable by screen reader users.

doesn't greatly help educating contributors. Accessibility is not only about screen readers. Focusability matters for all keyboard users and all assistive technologies that require or emulate keyboard navigation.

[ciampo]

Thank you for opening this issue! Switching to having our Buttons to use aria-disabled by default seems like a sensible change.

Considering that this is what we'd want in 90% of the caees, I'd like to propose to consider to reverse the logic.

Instead of making the exception the default behavior, disabled buttons should render aria-disabled and be noop-ed by default.

• The disabled prop should be renamed to isDisabled and implement the above behavior.
• A new prop isFullyDisabled (or a better name) should be used to render a disabled attribute instead of the above.
• A new custom eslint rule should be added to detect the usage of the isFullyDisabled prop or any other disabled HTML attribute being rendered. The rule should prevent rendering a disabled HTML attribute unldess an eslint-disable* comment is added that requires to specify the disable reason.

I may entirely be missing something but I think the editor needs a solid way to prevent focus losses that are often triggered by disabling buttons while they are focused. Also, contributors education is key and all this should be very well documented.

The proposed approach of simply changing the default value of the __experimentalIsFocusable prop from false to true is that all instances of Button would automatically adopt the new disabled behavior. With the approach that you're suggesting, instead, we'd need to do a lot of manual refactors in Gutenberg, with no guarantees that third-party consumers outside of Gutenberg would make those changes in their code bases (especially because they may not use the same eslint rule set).

Also, I'm not sure if we can stop using the __experimentalIsFocusable and disabled prop for backward-compat reasons.

[afercia]

I will leave the architectural decisions to contributors more used to that kind of considerations than me. I;d just like to note one thing:

I'm not sure if we can stop using the __experimentalIsFocusable and disabled prop for backward-compat reasons.

Gutenberg continuously changes but it also provides deprecations strategies and ways to remediate. I'd say that whe a change is worth it, we should just implement it and provide a proper deprecation policu and upgrade paths to handle backward-compat.

[andrewhayward]

disabled is a HTML attribute, it should not be used as a prop name.

The problem with this is that it's not just an HTML attribute, it's also a JavaScript property in the HTMLButtonElement API, in the same way that id and className are in the root Element API.

So while it might feel confusing to some, to others it's a perfectly appropriate name. (I'm not picking sides on that one!)

I'm not sure if we can stop using the __experimentalIsFocusable and disabled prop for backward-compat reasons.

Just to throw out a thought, we could break spec and make the disabled prop boolean | 'full' (or similar), rather than strictly boolean.

With this, <Button disabled> would be equivalent to what is currently <Button disabled __experimentalIsFocusable>, and <Button disabled="full"> would be equivalent to the current <Button disabled>.

We could check for the presence and value of __experimentalIsFocusable as a transition/holdover, so <Button disabled __experimentalIsFocusable={ false }> could carry on as it does now, essentially mapping to <Button disabled="full">.

[ciampo]

Although I'm personally not a fan of mixing different types for a prop, the solution proposed by @andrewhayward has a few advantages:

• it allows us to change the default "disabled" behaviour for all instances of Button without the need to refactor them one by one
• it doesn't introduce new props, and it deprecates one

I think we can try this approach out.

[andrewhayward]

Although I'm personally not a fan of mixing different types for a prop...

Yes, ideally we'd avoid it, but it seemed worth a go in this instance.

[mirka]

When I looked deeper into this subject (focusability of disabled controls), the impression I got was that there is really no strong consensus around what the behavior ought to be.

The only "facts" I found that are readily available to us are:

• The APG guidance, which doesn't say that disabled buttons should always be focusable, but rather recommends that we should weigh the pros/cons of either behavior in any given case, and lists some conventions to guide these decisions. The tone leans heavily into the "use your best judgement" territory, even compared to other guidances in the APG.
• The default browser behavior for disabled controls are that they are removed from the tab sequence.

Given these two points, I find it hard to easily conclude that all our disabled Buttons should be focusable by default. In fact, before I read into the topic I was sure that yes, obviously we want our disabled buttons to be focusable by default! But now I'm not sure.

If we're going against standard browser behavior, and change existing Button behavior, and take on the potential downsides of adding a lot more disabled controls into the tab sequence, we need strong arguments to support our decision.

in the editor we established a pattern for 'disabled' buttons

I'm personally not aware of this, but if we did collectively establish this pattern in the editor, I think that's fine. We can even add context-based systems to make this default in specific parts of the editor, if that's easier. However, that doesn't immediately justify changing the default for the Button component everywhere else.

[afercia]

@mirka thanks for your feedback.

It is true the Aria Authoring Practices Guidelines only suggest to keep disabled controls focusable and that's not a strict requirement. That's not the point. No one here stated it's a requirement. It's a pattern we established long time ago in this project, mainly to avoid focus losses.

I'm personally not aware of this, but if we did collectively establish this pattern in the editor, I think that's fine.

I would suggest you to search through the history of this project and find the relevant discussions where this pattern was established, which now dates to many years ago.

The main point here is to provide the best possible user experience. Personally, I think user experience must always prevail over any other argumentation. Giving priority to developers convenience or 'pureness' of architectural approach or any other 'high-level' engineering argumentation is pointless when such approach doesn't guarantee the best possible user experience.

In the 7 years of life of this project, there have been several cases where developers set the disabled attribute dynamically on focusable controls while they are focused. That triggers an obvious focus loss. Focus losses are a terrible experience for keyboard users and must be avoided.

For 7 years we tried to educate developers and reviewers in this project to not trigger such focus losses. Unfortunately, they keep doing it. That proves that education isn't a solution.

• Few contributors to this project have the knowledge to understand why this is important.
• No one, or at least very very few, tests their pull requests with the keyboard, which is very disappointing. To me, keyboard testing should be a requirement.
• Reviewers keep missing this kind of checks in their reviews.
• Finally, non-accessible code gets merged and released thus providing a broken keyboard experience.

How can we avoid this? How can we make sure there's no focus losses? Personally, I think we should enforce this via code. If you all have a better idea to make sure that in the future this will not happen ever again, I'm all for it.

What I would expect here is a pragmatical, effective, solution to the problem. I would greatly appreciate to not hear dismissive feedback and argumentations that don't really help solve the problem.

[mirka]

How can we make sure there's no focus losses?

Just so I understand better, is this the main concern here? (And less about universal discoverability?) If so, one path I can see is to add a hook so the focus can be maintained when the button is disabled while actively focused. I think that would be non-controversial since there are really no downsides, and no backward compatibility concerns.

[mirka]

We took the following measures on this front:

• Button: Stabilize __experimentalIsFocusable prop #62282
• Add lint rule for inaccessible disabled Button #62080
• ToolbarButton: Always keep focusable when disabled #63102

[afercia]

Just so I understand better, is this the main concern here? (And less about universal discoverability?)

It's both :) But avoiding focus losses is a primary concern as that's a terrible experience for all keyboard users. There have been several cases in the history of this project where contributors merged new UIs with buttons that got disabled on the fly, thus triggering focus losses. To me, the primary concern is to avoid this to happen again.

Discoverability of disabled controls is important as well but it needs to be evaluated on a case by cases basis. In some UIs, it may make sense, in other UIs it may not.