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

Switch Component doesn't disable click functionality when disabled #31199

Closed

Conversation

fabOnReact
Copy link
Contributor

@fabOnReact fabOnReact commented Mar 19, 2021

Summary

🚧 Still work in progress 🚧
This issue fixes #30944

Changelog

[Android] [Fixed] - Fix

Test Plan

CLICK TO OPEN TESTS RESULTS - SET ACCESSIBILITY TO TRUE IF DISABLED

TEST SCENARIO

  • first switch has disabled={true} and accessibilityState={{disabled: false}}
  • second switch is not disabled

RESULT

  • the screen reader announces disabled on the first switch, but not on the second switch
      <Switch
        accessibilityState={{disabled: false}}
        disabled={true}
        trackColor={{
          true: 'yellow',
          false: 'purple',
        }}
      />
      <Switch
        trackColor={{
          true: 'yellow',
          false: 'purple',
        }}
      />
2021-03-19.16-45-03.mp4

CLICK TO OPEN TESTS RESULTS - FAILURE - DISABLE SWITCH

TEST SCENARIO

  • The first Switch is disabled with disabled={true}

RESULT - FAILURE

  • The Switch is not disabled. Adding accessibiltyState props will re-enable disabled switches
         <Switch
          disabled={true}
          testID="on-off-initial-off"
          onValueChange={value => this.setState({falseSwitchIsOn: value})}
          trackColor={{
            true: 'yellow',
            false: 'purple',
          }}
          value={this.state.falseSwitchIsOn}
        />
        <Switch
          testID="on-off-initial-on"
          onValueChange={value => this.setState({trueSwitchIsOn: value})}
          value={this.state.trueSwitchIsOn}
        />

export default (codegenNativeComponent<NativeProps>('AndroidSwitch', {
interfaceOnly: true,
}): NativeType);

2021-03-19.17-31-27.mp4

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Mar 19, 2021
@analysis-bot
Copy link

Platform Engine Arch Size (bytes) Diff
ios - universal n/a --

Base commit: ac7ba3e

@analysis-bot
Copy link

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 8,932,947 -18,820
android hermes armeabi-v7a 8,424,011 -16,581
android hermes x86 9,423,134 -22,159
android hermes x86_64 9,367,917 -19,930
android jsc arm64-v8a 10,666,007 -17,987
android jsc armeabi-v7a 10,140,166 -15,744
android jsc x86 10,717,686 -21,343
android jsc x86_64 11,302,739 -19,111

Base commit: ac7ba3e

@fabOnReact fabOnReact closed this Mar 19, 2021
@lunaleaps
Copy link
Contributor

@fabriziobertoglio1987 Sorry missing context, why was this closed?

@fabOnReact
Copy link
Contributor Author

fabOnReact commented May 2, 2021

Thanks a lot @lunaleaps The pull request adds this functionality, as you can see in the below test case the screen reader announces disabled as required

CLICK TO OPEN TESTS RESULTS - SET ACCESSIBILITY TO TRUE IF DISABLED

TEST SCENARIO

  • first switch has disabled={true} and accessibilityState={{disabled: false}}
  • second switch is not disabled

RESULT

  • the screen reader announces disabled on the first switch, but not on the second switch
      <Switch
        accessibilityState={{disabled: false}}
        disabled={true}
        trackColor={{
          true: 'yellow',
          false: 'purple',
        }}
      />
      <Switch
        trackColor={{
          true: 'yellow',
          false: 'purple',
        }}
      />
2021-03-19.16-45-03.mp4

adding prop accessibilityState to AndroidSwitchNativeComponent creates regressions with disabled/enable Switch functionality.

For example in the below example the <Switch disabled={true} /> is disabled, but the Switch can be selected/unselected

CLICK TO OPEN TESTS RESULTS - FAILURE - DISABLE SWITCH

TEST SCENARIO

  • The first Switch is disabled with disabled={true}

RESULT - FAILURE

  • The Switch is not disabled. Adding accessibiltyState props will re-enable disabled switches
         <Switch
          disabled={true}
          testID="on-off-initial-off"
          onValueChange={value => this.setState({falseSwitchIsOn: value})}
          trackColor={{
            true: 'yellow',
            false: 'purple',
          }}
          value={this.state.falseSwitchIsOn}
        />
        <Switch
          testID="on-off-initial-on"
          onValueChange={value => this.setState({trueSwitchIsOn: value})}
          value={this.state.trueSwitchIsOn}
        />

export default (codegenNativeComponent<NativeProps>('AndroidSwitch', {
interfaceOnly: true,
}): NativeType);

2021-03-19.17-31-27.mp4

I did not further investigate it, possibly the mistake is my code implementation. I ended up working on other prs and though of closing this pr as I did not have a solution at the moment for the problem. Sorry If I did not completely answer your question. 🙏 ☮️

@lunaleaps
Copy link
Contributor

Thanks @fabriziobertoglio1987 for the clarification!

fabOnReact added a commit to fabOnReact/react-native that referenced this pull request Feb 8, 2022
- [There is no changes][1]
- I have a test case ready to push that verify no regression on iOS.
  I compared the snapshot generated on main and on my branch. They are identical, but I decided to not commit the test case from [pull#31199][2]
- I may consider later adding some test cases, but I think they should target the Android Platform

[1]: 135c562) on the iOS functionality to test (previously tested with facebook#31199
[2]: facebook#31199
fabOnReact added a commit to fabOnReact/react-native that referenced this pull request Feb 8, 2022
- [There is no changes](135c562) on the iOS functionality to test (previously tested with facebook#31199)
- I have a test case ready to push that verify no regression on iOS.
  I compared the snapshot generated on main and on my branch. They are identical, but I decided to not commit the test case from facebook#31199.
- I may consider later adding some test cases, but I think they should target the Android Platform
facebook-github-bot pushed a commit that referenced this pull request Feb 16, 2022
Summary:
This issue fixes #30944 fixes #30840 ([Test Case 7.1][7.1], [Test Case 7.3][7.3], [Test Case 7.5][7.5]) which affects Platform Android. Previous PR #31199.
The issue is caused by the missing prop `accessibilityState` in the Switch component.

The solution consists of passing the accessibilityState to the `AndroidSwitchNativeComponent` component as previously implemented in other components (for example, [Button][8]).

Relevant discussions #30840 (comment) and https://github.com/facebook/react-native/pull/31001/files#r578827409.

[8]: https://github.com/facebook/react-native/pull/31001/files#diff-4f225d043edf4cf5b8288285b6a957e2187fc0242f240bde396e41c4c25e4124R281-R289

The solution proposed in this pull request consists of:
1. Passing `accessibilityState` to the `AndroidSwitchNativeComponent`
2. If the value of prop `accessibilityState.disabled` is different from the prop `disabled`, the prop `disabled` over-rides the `accessibilityState.disabled` value.

For example:
```jsx
<Switch disabled={true} accessibilityState={{disabled: false}} />
````
becomes:
````jsx
<Switch disabled={true} accessibilityState={{disabled: true}} />
````

## Changelog

<!-- Help reviewers and the release process by writing your own changelog entry. For an example, see:
https://github.com/facebook/react-native/wiki/Changelog
-->

[General] [Fixed] - Switch Component doesn't disable click functionality when disabled

Pull Request resolved: #33070

Test Plan:
[1]. Switch has `disabled` and `accessibilityState={{disabled: false}}`
[2]. Switch has `disabled`
[3]. Switch has `accessibilityState={{disabled: true}}`
[4]. Switch has `accessibilityState={{disabled:false}}`
[5]. Switch has `disabled={false}`  and `accessibilityState={{disabled:true}}`
7. Test Cases on the main branch
[7.1]. Switch has `disabled` and `accessibilityState={{disabled: false}}`
[7.3] Switch has `accessibilityState={{disabled: true}}`
[7.5] Switch has `disabled={false}`  and `accessibilityState={{disabled:true}}`

[1]: fabOnReact/react-native-notes#5 (comment)
[2]: fabOnReact/react-native-notes#5 (comment)
[3]: fabOnReact/react-native-notes#5 (comment)
[4]: fabOnReact/react-native-notes#5 (comment)
[5]: fabOnReact/react-native-notes#5 (comment)
[7.1]: fabOnReact/react-native-notes#5 (comment)
[7.3]: fabOnReact/react-native-notes#5 (comment)
[7.5]: fabOnReact/react-native-notes#5 (comment)

Reviewed By: kacieb

Differential Revision: D34189484

Pulled By: blavalla

fbshipit-source-id: 8ea9221a5641d05c20d0309abdb3f0d02c569f2f
Saadnajmi pushed a commit to Saadnajmi/react-native-macos that referenced this pull request Jan 15, 2023
Summary:
This issue fixes facebook#30944 fixes facebook#30840 ([Test Case 7.1][7.1], [Test Case 7.3][7.3], [Test Case 7.5][7.5]) which affects Platform Android. Previous PR facebook#31199.
The issue is caused by the missing prop `accessibilityState` in the Switch component.

The solution consists of passing the accessibilityState to the `AndroidSwitchNativeComponent` component as previously implemented in other components (for example, [Button][8]).

Relevant discussions facebook#30840 (comment) and https://github.com/facebook/react-native/pull/31001/files#r578827409.

[8]: https://github.com/facebook/react-native/pull/31001/files#diff-4f225d043edf4cf5b8288285b6a957e2187fc0242f240bde396e41c4c25e4124R281-R289

The solution proposed in this pull request consists of:
1. Passing `accessibilityState` to the `AndroidSwitchNativeComponent`
2. If the value of prop `accessibilityState.disabled` is different from the prop `disabled`, the prop `disabled` over-rides the `accessibilityState.disabled` value.

For example:
```jsx
<Switch disabled={true} accessibilityState={{disabled: false}} />
````
becomes:
````jsx
<Switch disabled={true} accessibilityState={{disabled: true}} />
````

## Changelog

<!-- Help reviewers and the release process by writing your own changelog entry. For an example, see:
https://github.com/facebook/react-native/wiki/Changelog
-->

[General] [Fixed] - Switch Component doesn't disable click functionality when disabled

Pull Request resolved: facebook#33070

Test Plan:
[1]. Switch has `disabled` and `accessibilityState={{disabled: false}}`
[2]. Switch has `disabled`
[3]. Switch has `accessibilityState={{disabled: true}}`
[4]. Switch has `accessibilityState={{disabled:false}}`
[5]. Switch has `disabled={false}`  and `accessibilityState={{disabled:true}}`
7. Test Cases on the main branch
[7.1]. Switch has `disabled` and `accessibilityState={{disabled: false}}`
[7.3] Switch has `accessibilityState={{disabled: true}}`
[7.5] Switch has `disabled={false}`  and `accessibilityState={{disabled:true}}`

[1]: fabOnReact/react-native-notes#5 (comment)
[2]: fabOnReact/react-native-notes#5 (comment)
[3]: fabOnReact/react-native-notes#5 (comment)
[4]: fabOnReact/react-native-notes#5 (comment)
[5]: fabOnReact/react-native-notes#5 (comment)
[7.1]: fabOnReact/react-native-notes#5 (comment)
[7.3]: fabOnReact/react-native-notes#5 (comment)
[7.5]: fabOnReact/react-native-notes#5 (comment)

Reviewed By: kacieb

Differential Revision: D34189484

Pulled By: blavalla

fbshipit-source-id: 8ea9221a5641d05c20d0309abdb3f0d02c569f2f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Switch Component doesn't disable click functionality when disabled
4 participants