Open afercia opened 10 months ago
As a side note, this comment:
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.
Thank you for opening this issue! Switching to having our Button
s 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 benoop-ed
by default.
- The
disabled
prop should be renamed toisDisabled
and implement the above behavior.- A new prop
isFullyDisabled
(or a better name) should be used to render adisabled
attribute instead of the above.- A new custom eslint rule should be added to detect the usage of the
isFullyDisabled
prop or any otherdisabled
HTML attribute being rendered. The rule should prevent rendering adisabled
HTML attribute unldess aneslint-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.
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.
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">
.
Although I'm personally not a fan of mixing different types for a prop, the solution proposed by @andrewhayward has a few advantages:
Button
without the need to refactor them one by oneI think we can try this approach out.
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.
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:
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.
@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.
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.
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.
We took the following measures on this front:
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.
Description
Splitting this out from https://github.com/WordPress/gutenberg/issues/56396#issuecomment-1824180314 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
utilitiesfocusable
andtabbable
,useConstrainedTabbing
anduseFocusOutside
. 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 renderaria-disabled
instead of thedisabled
attribute to keep disabled buttons focusable. In https://github.com/WordPress/gutenberg/issues/56396#issuecomment-1824180314 a proposal was made to make this proptrue
bu 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 https://github.com/WordPress/gutenberg/issues/56396#issuecomment-1824180314 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:aria-disabled
to keep buttons focusablenoop
the disabled buttonThis is in place for two reasons:
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 benoop-ed
by default.disabled
prop should be renamed toisDisabled
and implement the above behavior.isFullyDisabled
(or a better name) should be used to render adisabled
attribute instead of the above.isFullyDisabled
prop or any otherdisabled
HTML attribute being rendered. The rule should prevent rendering adisabled
HTML attribute unless aneslint-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