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

Components: Add isFocusable state to Button #19337

Merged
merged 8 commits into from
Jan 13, 2020
Merged

Conversation

diegohaz
Copy link
Member

@diegohaz diegohaz commented Dec 27, 2019

Description

This PR tries to address this comment made by @gziolo: #18931 (comment)

[...] I'd like to replicate focusable and disabled fair implemented in Reakit for Button [...]

This is specially useful when the presence of a button is important enough to be perceivable. For example: a disabled tab that will be activated when the user completes another task.

Even though we should take the low contrast into consideration, sighted users will generally perceive the presence of the button, but screen reader users navigating with Tab won't.

This also enables tooltips for disabled label="tooltip" buttons. That's the standard behavior when using <button disabled title="title">:

image

How has this been tested?

Checking unit tests and storybook. But I can't reliably test event handlers with Enzyme. #18855 or #17303 could help with that.

Types of changes

New feature

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR. .

@diegohaz diegohaz self-assigned this Dec 27, 2019
@diegohaz diegohaz added [a11y] Color Contrast [Feature] UI Components Impacts or related to the UI component system [Package] Components /packages/components [Status] In Progress Tracking issues with work in progress [Type] Enhancement A suggestion for improvement. [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). Needs Accessibility Feedback Need input from accessibility Needs Design Feedback Needs general design feedback. and removed [Feature] UI Components Impacts or related to the UI component system [a11y] Color Contrast Needs Design Feedback Needs general design feedback. [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). Needs Accessibility Feedback Need input from accessibility labels Dec 27, 2019
@@ -212,7 +212,7 @@
outline: none;
}

&:active:focus:enabled {
&:not([aria-disabled="true"]):active:focus {
box-shadow: none;
}
Copy link
Contributor

@youknowriad youknowriad Dec 27, 2019

Choose a reason for hiding this comment

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

The styles changes make me a bit nervous :P because we have some many button styles ovverrides in the UI that I know this can trigger specificity issues and should be tested properly.

@@ -30,6 +35,7 @@ export function Button( props, ref ) {
isSecondary,
isLink,
isDestructive,
isFocusable,
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this default to true instead? I feel this was needed in every disabled button we have in the UI. I'm even wondering if we need a prop or just do this automatically.

Copy link
Member

Choose a reason for hiding this comment

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

Now, it makes me wonder how often we want to have a button which is both disabled and not focusable at the same time. It feels like for accessibility reasons it should always be focusable.

https://ux.stackexchange.com/questions/103239/should-disabled-elements-be-focusable-for-accessibility-purposes - it only confirms this reasoning.

It might be a good idea to set isFocusable to true by default as suggested.

We might be some disabled and not focusable buttons but it might be simply wrong. We should found those cases and take further steps based on that.

Copy link
Member Author

Choose a reason for hiding this comment

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

Made isFocusable true by default

@gziolo
Copy link
Member

gziolo commented Dec 27, 2019

Is disabled isFocusable intuitive enough? Since isFocusable only makes sense when being used together with the disabled prop, maybe we should come up with a single prop that does the job: readOnly.

I did some quick research and it feels like readOnly doesn’t fit here. I can only see it work mostly in the context of form elements which take input.

@diegohaz
Copy link
Member Author

diegohaz commented Jan 6, 2020

I still don't know what's the best approach to keep the tooltip contrast high when the button is disabled. Right now, button has reduced opacity when it's disabled. Since the tooltip is inside the button in the DOM, it inherits the opacity.

Ideally, tooltips should always render outside the button using Portal or something similar. But maybe that's something for another PR.

@diegohaz diegohaz mentioned this pull request Jan 6, 2020
6 tasks
@youknowriad
Copy link
Contributor

I still don't know what's the best approach to keep the tooltip contrast high when the button is disabled.

Should we remove the tooltip when the button is disabled?

@gziolo
Copy link
Member

gziolo commented Jan 7, 2020

I still don't know what's the best approach to keep the tooltip contrast high when the button is disabled.

Should we remove the tooltip when the button is disabled?

This is what I found in https://inclusive-components.design/tooltips-toggletips/:

[...] icons can accelerate the understanding of an interface, and help to internationalize it. But icons provided in isolation are always in danger of completely confounding a user — because they don't spell anything out. To users who don't recognize or can't decipher the icon, information is missing.

Most of the time, you should simply be providing text alongside icons. Like the perma-visible field labels I just mentioned, textual labels are the most straightforward way of labeling things and they're automatically screen reader accessible if provided as real text (not images of text).

I think we should keep them. In addition, we should stop using aria-label in favor of aria-labelledby pointing to the text in the tooltip. That is something we should consider separately.

@gziolo
Copy link
Member

gziolo commented Jan 7, 2020

How can I reproduce the state where the button has the tooltip which has opacity applied? In the editor, it looks as before:
Screen Shot 2020-01-07 at 12 30 19

@diegohaz
Copy link
Member Author

diegohaz commented Jan 7, 2020

@gziolo You can reproduce it in the storybook (Button stories).

It seems that it works in the editor because it's using Popover.Slot, so the tooltip is placed in another place in the DOM.

@gziolo
Copy link
Member

gziolo commented Jan 8, 2020

You can reproduce it in the storybook (Button stories).

It seems that it works in the editor because it's using Popover.Slot, so the tooltip is placed in another place in the DOM.

It's a known issue then. I did some source code checking and I found this:

// In case there is no slot context in which to render,
// default to an in-place rendering.
if ( getSlot && getSlot( __unstableSlotName ) ) {
content = <Fill name={ __unstableSlotName }>{ content }</Fill>;
}
if ( anchorRef || anchorRect ) {
return content;
}
return (
<span ref={ anchorRefFallback }>
{ content }
</span>
);
} }

If there is no Popover.Slot found, then it renders inline. In the case of the Button component, it's placed as a child element that causes the issue.

Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

Let's include the note in the CHANGELOG file about this new prop:

https://github.com/WordPress/gutenberg/blob/master/packages/components/CHANGELOG.md

Otherwise, it looks good. @youknowriad, do you still have concerns? I would be in favor of landing this change.

@youknowriad
Copy link
Contributor

I don't have concerns, this is a great PR and I believe there's a lot of places where we can remove code as a follow-up. (We use this pattern in a lot of places).

I still wonder whether the prop is necessary at all, is there a place where we do want to effectively "disable" the button.

@diegohaz
Copy link
Member Author

diegohaz commented Jan 9, 2020

I can remove the prop for now and add it later if it’s needed or make it experimental?

@youknowriad
Copy link
Contributor

Maybe we can do a quick audit of our existing "disabled" buttons in our code base and remove it if we want this consistently?

@diegohaz
Copy link
Member Author

diegohaz commented Jan 9, 2020

Sounds good! I’ll have a look.

@gziolo
Copy link
Member

gziolo commented Jan 9, 2020

I still wonder whether the prop is necessary at all, is there a place where we do want to effectively "disable" the button.

Well, that's a great question. We need to find the answer first, I also shared it on WordPress Slack in the accessibility channel (link requires registration):

https://wordpress.slack.com/archives/C02RP4X03/p1578572361042000

@diegohaz
Copy link
Member Author

diegohaz commented Jan 9, 2020

The WAI-ARIA recommendations have a dedicated section to this topic: https://www.w3.org/TR/wai-aria-practices/#kbd_disabled_controls

After reading the article above and the conversation in the link @gziolo shared, since this will change how native buttons work in the browser, I'd make the prop false by default. To be even more safe, I'd expose it as __experimental until we make sure that it works well in Gutenberg use cases (this seems, to me, the best way to introduce any new feature here).

@youknowriad
Copy link
Contributor

ok, that works for me 👍

@gziolo gziolo self-requested a review January 13, 2020 07:11
Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

Still good, it makes sense to proceed this way 👍

@diegohaz diegohaz merged commit 8d1afc9 into master Jan 13, 2020
@diegohaz diegohaz deleted the update/button-is-focusable branch January 13, 2020 17:04
@ellatrix ellatrix added this to the Gutenberg 7.3 milestone Jan 20, 2020
@@ -39,6 +44,7 @@ export function Button( props, ref ) {
shortcut,
label,
children,
__experimentalIsFocusable: isFocusable,
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the story of this prop, should we make it stable @gziolo ? Recently I found a use-case where we have a button that switches from disabled to enabled and back and this causes focus loss unless this prop is true. So I'm going to use it but since it's now 2 years old, I was wondering if we should mark it stable? What's the reason for the experimental state.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, we can promote it to stable after checking how the same functionality has evolved in Reakit.

Copy link
Member Author

Choose a reason for hiding this comment

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

In Reakit it was named focusable. That caused a bit of confusion. It wasn't obvious that the prop would make the element focusable when it was disabled. Some people were setting this prop to false thinking it would disable focus on the element.

We're renaming it to accessibleWhenDisabled in v2 (this name may still change though).

@priethor priethor added [Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). and removed [a11y] Color Contrast labels Jul 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Focus] Accessibility (a11y) Changes that impact accessibility and need corresponding review (e.g. markup changes). [Package] Components /packages/components [Status] In Progress Tracking issues with work in progress [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants