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

make disabled buttons interactable #1461

Merged
merged 23 commits into from
Aug 2, 2023
Merged

make disabled buttons interactable #1461

merged 23 commits into from
Aug 2, 2023

Conversation

mayank99
Copy link
Contributor

@mayank99 mayank99 commented Jul 31, 2023

Changes

Created new <ButtonBase> component used to replace all instances of <button>. This component intercepts the disabled prop, converts it to aria-disabled and manually disables clicks. This keeps the tooltip focusable and allows tooltips to appear. See https://css-tricks.com/making-disabled-buttons-more-inclusive/

Other notes:

  • Because disabling clicks requires javascript, the disabled attribute is still used on first render to make it progressively enhanced.
  • aria-disabled is incorrect in the case of non-button elements (e.g. links), so an additional data-iui-disabled attribute has been added. While "disabled links" don't make sense, this serves as a safeguard in case users decide to do such weird things.
  • Added iui-button-base class with a small reset for normalizing button styles. This caused some very minor image changes.

Testing

  • ✅ focusable
  • ✅ hoverable (tooltip appears)
  • ✅ not clickable
  • ✅ styled like disabled (dark grey)
  • ✅ conveyed as disabled by AT ("dimmed"/"unavailable")
Screen.Recording.2023-07-31.at.2.58.37.PM.mov
Screen.Recording.2023-08-01.at.2.15.19.PM.mov

Safari + VO:

Screen.Recording.2023-07-31.at.3.04.45.PM.mov

Firefox + NVDA:

Untitled.mov

^Note: These screen reader tests required focusing the button to read out the tooltip. Simply traversing to the button using the virtual cursor didn't announce the tooltip text; this might be a bug in Tooltip.

Docs

Added changesets.

@mayank99 mayank99 added this to the React 3.0 milestone Jul 31, 2023
@mayank99 mayank99 self-assigned this Jul 31, 2023
@mayank99 mayank99 linked an issue Jul 31, 2023 that may be closed by this pull request
@mayank99 mayank99 linked an issue Jul 31, 2023 that may be closed by this pull request
@mayank99 mayank99 force-pushed the mayank/disabled-buttons branch from 00f33af to dbdb4a2 Compare July 31, 2023 21:13
@mayank99 mayank99 marked this pull request as ready for review August 1, 2023 18:20
@mayank99 mayank99 requested review from a team as code owners August 1, 2023 18:20
@mayank99 mayank99 requested review from gretanausedaite and adamhock and removed request for a team August 1, 2023 18:20
Copy link
Contributor

@gretanausedaite gretanausedaite left a comment

Choose a reason for hiding this comment

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

Amazing work!

Copy link
Contributor

@adamhock adamhock left a comment

Choose a reason for hiding this comment

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

Just a quick question, but other than that LGTM

@@ -25,3 +26,5 @@ afterEach(() => {
.querySelectorAll('[data-tippy-root]')
.forEach((tippy) => tippy.remove());
});

global.TextEncoder = TextEncoder;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this change related to this PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, it's needed for the SSR test where I'm calling renderToString

@mayank99 mayank99 enabled auto-merge August 2, 2023 15:17
@mayank99 mayank99 added this pull request to the merge queue Aug 2, 2023
Merged via the queue into dev with commit cd9280d Aug 2, 2023
@mayank99 mayank99 deleted the mayank/disabled-buttons branch August 2, 2023 15:55
This was referenced Aug 22, 2023
@imodeljs-admin imodeljs-admin mentioned this pull request Oct 23, 2023
@mayank99 mayank99 mentioned this pull request May 9, 2024
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

IconButton: Label not displayed on disabled button Disabled elements should be interactable for a11y
3 participants