[a11y] When aria-disabled is set on SegmentedControl, don't allow action #7047
Conversation
…t_segmented_control_tooltip`" This reverts commit 602b29a.
…stories.tsx Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Tyler Jones <tylerjdev@github.com>
🦋 Changeset detectedLatest commit: fe20f4f The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
github-actions
bot
added
the
integration-tests: recommended
|
👋 Hi, this pull request contains changes to the source code that github/github depends on. If you are GitHub staff, we recommend testing these changes with github/github using the integration workflow. Thanks! |
github-actions
bot
added
integration-tests: failing
github-actions
bot
removed
the
integration-tests: failing
github-actions
bot
added
the
integration-tests: passing
|
🟢 ci completed with status |
|
Changes needed for release: https://github.com/github/github-ui/pull/5186/commits/3b1557f08ad7c1e6d8533bb0c80a9899d92c7021 |
There was a problem hiding this comment.
Pull Request Overview
This PR enhances the SegmentedControl component to properly handle disabled states for accessibility purposes. It removes the feature flag primer_react_segmented_control_tooltip (making tooltip behavior the default) and ensures that when aria-disabled or disabled is set on a button, click actions are prevented while maintaining keyboard-accessible tooltips.
Key changes:
- Added support for
disabledandaria-disabledprops toSegmentedControl.ButtonandSegmentedControl.IconButton - Updated click handlers to check disabled state before executing actions
- Applied appropriate styling for disabled buttons via CSS
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| SegmentedControlIconButton.tsx | Added disabled and aria-disabled props; removed feature flag logic |
| SegmentedControlIconButton.stories.tsx | Added disabled and aria-disabled args to Storybook controls |
| SegmentedControlButton.tsx | Added disabled and aria-disabled props support |
| SegmentedControl.tsx | Modified onClick handlers to prevent actions when buttons are disabled |
| SegmentedControl.test.tsx | Removed feature flag wrapper from tests |
| SegmentedControl.module.css | Added styles for aria-disabled state |
| SegmentedControl.examples.stories.tsx | Added example story demonstrating disabled buttons |
| SegmentedControl.dev.stories.tsx | Added dev stories for testing disabled and aria-disabled states |
| DefaultFeatureFlags.ts | Removed primer_react_segmented_control_tooltip feature flag |
| strong-mangos-rest.md | Added changeset documenting the changes |
| /** Applies `aria-disabled` to the button. This will disable certain functionality, such as `onClick` events. */ | ||
| disabled?: boolean | ||
| /** Applies `aria-disabled` to the button. This will disable certain functionality, such as `onClick` events. */ |
There was a problem hiding this comment.
The documentation for both disabled and aria-disabled is identical. The disabled prop should be documented to clarify that it applies the aria-disabled attribute and disables click functionality, distinguishing it from the aria-disabled prop.
| /** Applies `aria-disabled` to the button. This will disable certain functionality, such as `onClick` events. */ | |
| disabled?: boolean | |
| /** Applies `aria-disabled` to the button. This will disable certain functionality, such as `onClick` events. */ | |
| /** | |
| * Disables the button, preventing click functionality and applying the `aria-disabled` attribute. | |
| * Use this prop to make the button non-interactive. | |
| */ | |
| disabled?: boolean | |
| /** | |
| * Applies the `aria-disabled` attribute to the button for accessibility purposes, but does not disable click functionality. | |
| * Use this prop if you want the button to appear disabled to assistive technologies but remain interactive. | |
| */ |
| onChange(index) | ||
| isUncontrolled && setSelectedIndexInternalState(index) | ||
| child.props.onClick && child.props.onClick(event) | ||
| const isDisabled = child.props.disabled === true || child.props['aria-disabled'] === true |
There was a problem hiding this comment.
The disabled check is duplicated on lines 170 and 178. Consider extracting this logic into a reusable function or variable before the conditional branches to avoid repetition.
| disabled, | ||
| 'aria-disabled': ariaDisabled, |
There was a problem hiding this comment.
im confused on what the difference between these two props is.
Also, does the *.docs.json need to be updated?
There was a problem hiding this comment.
This is to be backwards compatible from what I recall. Ideally we'll remove one or the other once we reduce usage in dotcom.
There was a problem hiding this comment.
should we deprecate one now then?
| component: SegmentedControl, | ||
| } as Meta<typeof SegmentedControl> | ||
|
|
||
| export const WithDisabledButtons = () => ( |
Closes: https://github.com/github/primer/issues/4723
Reverts the reverted PR: #6986
Changelog
SegmentedControl.IconButtonwill show a tooltip by default, and we want this to be accessible for keyboard users even when the control is disabled.primer_react_segmented_control_tooltip" #6412)primer_react_segmented_control_tooltip, 1 integration test failed in dotcom. This is because a tooltip was now being applied on adisabledcontrol, causing the<Tooltip>component to thrown an area. This will be resolved by the aforementioned change.Rollout strategy
Merge checklist