fix: avoid disabled accessibility state for active chips - #5011
Conversation
matkoson
left a comment
There was a problem hiding this comment.
src/components/Chip/Chip.tsx:217 @vivekjm wouldn't this make non-interactive chips ripple when pressed?
generated by an automated pr-review pipe, not written by a human. reply and tag the repo owner if it got something wrong.
|
Thanks for checking this. The intent is not to make non-interactive chips behave as actionable controls, only to prevent them from being exposed as disabled to accessibility APIs when the Chip itself is not disabled.\n\nThe fallback no-op handler is only used when the chip is not disabled and no touch handler was passed. It avoids TouchableRipple deriving a disabled accessibility state from the missing handler; explicitly disabled chips still pass through as disabled, and chips with real handlers still use their provided onPress/onPressIn/onPressOut behavior.\n\nIf you prefer avoiding a no-op onPress entirely, I can rework this to keep the accessibility state independent from TouchableRipple's disabled inference instead. |
75dd519 to
1e65c5f
Compare
satya164
left a comment
There was a problem hiding this comment.
thanks for the pr. please provide a video showing before/after
| @@ -12,6 +12,15 @@ import { getChipColors } from '../Chip/helpers'; | |||
|
|
|||
| const stateOpacity = tokens.md.sys.state.opacity; | |||
|
|
|||
| const getScaleValue = (testID: string) => { | |||
| const style = StyleSheet.flatten(screen.getByTestId(testID).props.style); | |||
| const scale = style.transform?.find( | |||
| (transform: { scale?: Animated.Value | number }) => 'scale' in transform | |||
| )?.scale; | |||
|
|
|||
| return typeof scale === 'number' ? scale : scale?.__getValue(); | |||
| }; | |||
|
|
|||
| it('renders chip with onPress', async () => { | |||
| const tree = ( | |||
| await render(<Chip onPress={() => {}}>Example Chip</Chip>) | |||
| @@ -70,10 +79,26 @@ it('renders selected chip', async () => { | |||
| expect(tree).toMatchSnapshot(); | |||
| }); | |||
|
|
|||
| it('renders disabled chip if there is no touch handler passed', async () => { | |||
| await render(<Chip testID="disabled-chip">Disabled chip</Chip>); | |||
| it('does not mark chip disabled if there is no touch handler passed', async () => { | |||
| await render(<Chip testID="informational-chip">Informational chip</Chip>); | |||
|
|
|||
| expect(screen.getByTestId('disabled-chip')).toBeDisabled(); | |||
| expect( | |||
| screen.getByTestId('informational-chip').props.accessibilityState | |||
| ).toMatchObject({ | |||
| disabled: false, | |||
| }); | |||
| }); | |||
|
|
|||
| it('marks chip disabled when disabled prop is passed', async () => { | |||
| await render( | |||
| <Chip disabled testID="disabled-chip"> | |||
| Disabled chip | |||
| </Chip> | |||
| ); | |||
|
|
|||
| expect(screen.getByTestId('disabled-chip').props.accessibilityState).toMatchObject({ | |||
| disabled: true, | |||
| }); | |||
| }); | |||
|
|
|||
| it('renders active chip if only onLongPress handler is passed', async () => { | |||
| @@ -83,7 +108,9 @@ it('renders active chip if only onLongPress handler is passed', async () => { | |||
| </Chip> | |||
| ); | |||
|
|
|||
| expect(screen.getByTestId('active-chip')).toBeEnabled(); | |||
| expect(screen.getByTestId('active-chip').props.accessibilityState).toMatchObject({ | |||
| disabled: false, | |||
| }); | |||
| }); | |||
|
|
|||
| it('renders chip with zero border radius', async () => { | |||
| @@ -93,9 +120,11 @@ it('renders chip with zero border radius', async () => { | |||
| </Chip> | |||
| ); | |||
|
|
|||
| expect(screen.getByTestId('active-chip')).toHaveStyle({ | |||
| borderRadius: 0, | |||
| }); | |||
| expect(StyleSheet.flatten(screen.getByTestId('active-chip').props.style)).toMatchObject( | |||
| { | |||
| borderRadius: 0, | |||
| } | |||
| ); | |||
| }); | |||
|
|
|||
| describe('getChipColors - text color', () => { | |||
| @@ -385,9 +414,7 @@ it('animated value changes correctly', async () => { | |||
| Example Chip | |||
| </Chip> | |||
| ); | |||
| expect(screen.getByTestId('chip-container-outer-layer')).toHaveStyle({ | |||
| transform: [{ scale: 1 }], | |||
| }); | |||
| expect(getScaleValue('chip-container-outer-layer')).toBe(1); | |||
|
|
|||
| Animated.timing(value, { | |||
| toValue: 1.5, | |||
| @@ -398,7 +425,5 @@ it('animated value changes correctly', async () => { | |||
| await act(() => { | |||
| jest.advanceTimersByTime(200); | |||
| }); | |||
| expect(screen.getByTestId('chip-container-outer-layer')).toHaveStyle({ | |||
| transform: [{ scale: 1.5 }], | |||
| }); | |||
| expect(getScaleValue('chip-container-outer-layer')).toBe(1.5); | |||
| }); | |||
There was a problem hiding this comment.
Please revert these changes. Tests shouldn't be reading internal implementation details and private properties.
The only test worth adding is verifying that the touchable doesn't have disabled.
| @@ -300,7 +302,7 @@ const Chip = ({ | |||
| borderless | |||
| background={background} | |||
| style={[{ borderRadius }, styles.touchable]} | |||
| onPress={onPress} | |||
| onPress={touchableOnPress} | |||
There was a problem hiding this comment.
this turns all chips pressables even when they aren't intended to be interactive. the proper fix would be not to render a TouchableRipple for informational/close-only chips
Motivation
Fixes an accessibility regression where Chips without an explicit disabled prop can still be exposed as disabled/dimmed to screen readers.
TouchableRipplemarks itself disabled when no touch handler is present, so informational or selected Chips could inherit a disabled accessibility state even though the Chip itself is not disabled.This keeps explicitly disabled Chips disabled, while allowing non-disabled Chips to remain active from an accessibility perspective.
Related issue
Fixes #4844
Test plan
yarn test src/components/__tests__/Chip.test.tsx --runInBand -uyarn eslint src/components/Chip/Chip.tsx src/components/__tests__/Chip.test.tsxyarn typescriptAlso attempted the default pre-commit hook, which runs full lint/types/tests. Lint and most tests ran, but the full Jest run failed in
src/babel/__tests__/index.jsbecause this checkout does not havelib/mappings.jsongenerated.