-
Notifications
You must be signed in to change notification settings - Fork 24.4k
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
Fix accessibilityState overwriting view's disabled state on Android #34287
Conversation
Hi @okwasniewski! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at [email protected]. Thanks! |
Base commit: 3f50440 |
Base commit: 3f50440 |
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
@dmitryrykun has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
ReactAndroid/src/main/java/com/facebook/react/uimanager/BaseViewManager.java
Outdated
Show resolved
Hide resolved
ReactAndroid/src/main/java/com/facebook/react/uimanager/BaseViewManager.java
Outdated
Show resolved
Hide resolved
@dmitryrykun has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
@NickGerleman I've added your suggested fix, and all checks have passed. Can you take a look again? |
This pull request was successfully merged by @okwasniewski in f35d18c. When will my fix make it into a release? | Upcoming Releases |
…34287) Summary: While I was working on rewriting `react-native-slider` to Fabric I found a weird bug that prevented the slider to be set as disabled (to be exact: call the method `slider.setEnabled(false)`. As it turned out the `accessibilityState` (with value: `accessibilityState={{disabled: true}}` prop occurred after the `enabled={false}` prop that I was passing to the slider, which lead to both of this props overwrite each other. Handling of `accessibilityState` props inside view leads to always overwriting the enabled prop to true (even if we explicitly set it to `{disabled: false}`. Workaround for this was to reorder the props, so that the `accesibilityState` occur before `disabled`, but I think it's better to not set `view.setEnabled(true)` if we are passing a disabled property. ## Changelog [Android] [Fixed] - Fix accessibilityState overwriting view's disabled state on Android Pull Request resolved: #34287 Test Plan: Change order of props inside native component implementation (that `disabled` occurs before `accesibilityState`). For example: `Libraries/Components/Slider/Slider.js` <details> <summary>Video showing the bug in RNTester (using Switch component)</summary> https://user-images.githubusercontent.com/52801365/181287547-964f50e2-55dc-450f-b413-0d1c14d4bb83.mp4 </details> Reviewed By: NickGerleman Differential Revision: D38209232 Pulled By: dmitryrykun fbshipit-source-id: 93d423716f89b45251be9d5aefcf01f7bd776f2c
…acebook#34287) Summary: While I was working on rewriting `react-native-slider` to Fabric I found a weird bug that prevented the slider to be set as disabled (to be exact: call the method `slider.setEnabled(false)`. As it turned out the `accessibilityState` (with value: `accessibilityState={{disabled: true}}` prop occurred after the `enabled={false}` prop that I was passing to the slider, which lead to both of this props overwrite each other. Handling of `accessibilityState` props inside view leads to always overwriting the enabled prop to true (even if we explicitly set it to `{disabled: false}`. Workaround for this was to reorder the props, so that the `accesibilityState` occur before `disabled`, but I think it's better to not set `view.setEnabled(true)` if we are passing a disabled property. ## Changelog [Android] [Fixed] - Fix accessibilityState overwriting view's disabled state on Android Pull Request resolved: facebook#34287 Test Plan: Change order of props inside native component implementation (that `disabled` occurs before `accesibilityState`). For example: `Libraries/Components/Slider/Slider.js` <details> <summary>Video showing the bug in RNTester (using Switch component)</summary> https://user-images.githubusercontent.com/52801365/181287547-964f50e2-55dc-450f-b413-0d1c14d4bb83.mp4 </details> Reviewed By: NickGerleman Differential Revision: D38209232 Pulled By: dmitryrykun fbshipit-source-id: 93d423716f89b45251be9d5aefcf01f7bd776f2c
…acebook#34287) Summary: While I was working on rewriting `react-native-slider` to Fabric I found a weird bug that prevented the slider to be set as disabled (to be exact: call the method `slider.setEnabled(false)`. As it turned out the `accessibilityState` (with value: `accessibilityState={{disabled: true}}` prop occurred after the `enabled={false}` prop that I was passing to the slider, which lead to both of this props overwrite each other. Handling of `accessibilityState` props inside view leads to always overwriting the enabled prop to true (even if we explicitly set it to `{disabled: false}`. Workaround for this was to reorder the props, so that the `accesibilityState` occur before `disabled`, but I think it's better to not set `view.setEnabled(true)` if we are passing a disabled property. ## Changelog [Android] [Fixed] - Fix accessibilityState overwriting view's disabled state on Android Pull Request resolved: facebook#34287 Test Plan: Change order of props inside native component implementation (that `disabled` occurs before `accesibilityState`). For example: `Libraries/Components/Slider/Slider.js` <details> <summary>Video showing the bug in RNTester (using Switch component)</summary> https://user-images.githubusercontent.com/52801365/181287547-964f50e2-55dc-450f-b413-0d1c14d4bb83.mp4 </details> Reviewed By: NickGerleman Differential Revision: D38209232 Pulled By: dmitryrykun fbshipit-source-id: 93d423716f89b45251be9d5aefcf01f7bd776f2c
Summary
While I was working on rewriting
react-native-slider
to Fabric I found a weird bug that prevented the slider to be set as disabled (to be exact: call the methodslider.setEnabled(false)
. As it turned out theaccessibilityState
(with value:accessibilityState={{disabled: true}}
prop occurred after theenabled={false}
prop that I was passing to the slider, which lead to both of this props overwrite each other.Handling of
accessibilityState
props inside view leads to always overwriting the enabled prop to true (even if we explicitly set it to{disabled: false}
.Workaround for this was to reorder the props, so that the
accesibilityState
occur beforedisabled
, but I think it's better to not setview.setEnabled(true)
if we are passing a disabled property.Changelog
[Android] [Fixed] - Fix accessibilityState overwriting view's disabled state on Android
Test Plan
Change order of props inside native component implementation (that
disabled
occurs beforeaccesibilityState
). For example:Libraries/Components/Slider/Slider.js
Video showing the bug in RNTester (using Switch component)
SwitchExample.mp4