Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fix accessibility event properties for TextInput #24641

Conversation

estevaolucas
Copy link

Summary

When a TextInput receives any accessibility event prop (onAccessibilityTap, onMagicTap, onAccessibilityEscape, onAccessibilityEscape), causes a crash.

Changelog

[iOS] [Fixed] - Fix accessibility event properties for TextInput

Test Plan

The following component shouldn't crash the app.

<TextInput
  accessible={true}
  onMagicTap={() => {
    console.log("--onMagicTap works");
  }}
  onAccessibilityTap={() => {
    console.log("--onAccessibilityTap works");
  }}
  onAccessibilityEscape={() => {
    console.log("--onAccessibilityEscape works");
  }}
/>

@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 Apr 28, 2019
@react-native-bot react-native-bot added Component: TextInput Related to the TextInput component. Includes Changelog labels Apr 28, 2019
Copy link
Contributor

@cpojer cpojer left a comment

Choose a reason for hiding this comment

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

Thanks!

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@cpojer is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@react-native-bot
Copy link
Collaborator

This pull request was successfully merged by @elucaswork in 61c5e35.

When will my fix make it into a release? | Upcoming Releases

@react-native-bot react-native-bot added the Merged This PR has been merged. label Apr 29, 2019
grabbou pushed a commit that referenced this pull request May 6, 2019
Summary:
When a `TextInput` receives any accessibility event prop (`onAccessibilityTap`, `onMagicTap`, `onAccessibilityEscape`, `onAccessibilityEscape`), causes a crash.

<p align=center><img src=https://user-images.githubusercontent.com/20709038/56871548-84f00980-69ed-11e9-8906-0206899e5435.jpg width=300></p>

[iOS] [Fixed] - Fix accessibility event properties for `TextInput`
Pull Request resolved: #24641

Differential Revision: D15120211

Pulled By: cpojer

fbshipit-source-id: 7996ab9f9b78588fab4986c3de6114817ec37296
grabbou added a commit that referenced this pull request May 8, 2019
@@ -505,13 +505,6 @@ - (CGSize)sizeThatFits:(CGSize)size
return fittingSize;
}

#pragma mark - Accessibility
Copy link
Contributor

Choose a reason for hiding this comment

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

I am afraid this particular change is simply wrong/unreasonable.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just to be clear: Removing this turns off all accessibility props of (by redirecting them to the wrapper view).

Copy link
Author

Choose a reason for hiding this comment

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

@shergin my idea of deleting this reactAccessibilityElement was to make RCTBaseTextInputView use reactAccessibilityElement inherited from RCTView (the wrapper).

When I opened this PR I tested TextField passing those events set from RCTViewManager, and it was working. But I might have missed something :-/.

Now I'll need to make more tests, especially because I this commit needed to be reverted (probably because of this detox failure https://circleci.com/gh/facebook/react-native/88586, right @grabbou?). I just don't know yet why this commit caused a regression into Switch component.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I think Detox couldn't find a TextInput on the top of the app (by its accessibility label), to type <switch /> and select that example.

So definitely related.

kelset pushed a commit that referenced this pull request Jun 5, 2019
Summary:
When a `TextInput` receives any accessibility event prop (`onAccessibilityTap`, `onMagicTap`, `onAccessibilityEscape`, `onAccessibilityEscape`), causes a crash.

<p align=center><img src=https://user-images.githubusercontent.com/20709038/56871548-84f00980-69ed-11e9-8906-0206899e5435.jpg width=300></p>

[iOS] [Fixed] - Fix accessibility event properties for `TextInput`
Pull Request resolved: #24641

Differential Revision: D15120211

Pulled By: cpojer

fbshipit-source-id: 7996ab9f9b78588fab4986c3de6114817ec37296
kelset added a commit that referenced this pull request Jun 5, 2019
douglowder pushed a commit to react-native-tvos/react-native-tvos that referenced this pull request Sep 11, 2019
Summary:
When a `TextInput` receives any accessibility event prop (`onAccessibilityTap`, `onMagicTap`, `onAccessibilityEscape`, `onAccessibilityEscape`), causes a crash.

<p align=center><img src=https://user-images.githubusercontent.com/20709038/56871548-84f00980-69ed-11e9-8906-0206899e5435.jpg width=300></p>

[iOS] [Fixed] - Fix accessibility event properties for `TextInput`
Pull Request resolved: facebook/react-native#24641

Differential Revision: D15120211

Pulled By: cpojer

fbshipit-source-id: 7996ab9f9b78588fab4986c3de6114817ec37296
@estevaolucas estevaolucas deleted the fix/ios-textfield-accessibility branch September 23, 2019 03:39
facebook-github-bot pushed a commit that referenced this pull request Jun 6, 2022
Summary:
This sync includes the following changes:
- **[dd4950c90](facebook/react@dd4950c90 )**: [Flight] Implement useId hook ([#24172](facebook/react#24172)) //<Josh Story>//
- **[26a5b3c7f](facebook/react@26a5b3c7f )**: Explicitly set `highWaterMark` to 0 for `ReadableStream` ([#24641](facebook/react#24641)) //<Josh Larson>//
- **[aec575914](facebook/react@aec575914 )**: [Fizz] Send errors down to client ([#24551](facebook/react#24551)) //<Josh Story>//
- **[a2766387e](facebook/react@a2766387e )**: [Fizz] Improve text separator byte efficiency ([#24630](facebook/react#24630)) //<Josh Story>//
- **[f7860538a](facebook/react@f7860538a )**: Fix typo in useSyncExternalStore main entry point error ([#24631](facebook/react#24631)) //<François Chalifour>//
- **[1bed20731](facebook/react@1bed20731 )**: Add a module map option to the Webpack Flight Client ([#24629](facebook/react#24629)) //<Sebastian Markbåge>//
- **[b2763d3ea](facebook/react@b2763d3ea )**: Move hydration code out of normal Suspense path ([#24532](facebook/react#24532)) //<Andrew Clark>//
- **[357a61324](facebook/react@357a61324 )**: [DevTools][Transition Tracing] Added support for Suspense Boundaries ([#23365](facebook/react#23365)) //<Luna Ruan>//
- **[2c8a1452b](facebook/react@2c8a1452b )**: Fix ignored setState in Safari when iframe is touched ([#24459](facebook/react#24459)) //<dan>//
- **[62662633d](facebook/react@62662633d )**: Remove enableFlipOffscreenUnhideOrder ([#24545](facebook/react#24545)) //<Ricky>//
- **[34da5aa69](facebook/react@34da5aa69 )**: Only treat updates to lazy as a new mount in legacy mode ([#24530](facebook/react#24530)) //<Ricky>//
- **[46a6d77e3](facebook/react@46a6d77e3 )**: Unify JSResourceReference Interfaces ([#24507](facebook/react#24507)) //<Timothy Yung>//
- **[6cbf0f7fa](facebook/react@6cbf0f7fa )**: Fork ReactSymbols ([#24484](facebook/react#24484)) //<Ricky>//
- **[a10a9a6b5](facebook/react@a10a9a6b5 )**: Add test for hiding children after layout destroy ([#24483](facebook/react#24483)) //<Ricky>//
- **[b4eb0ad71](facebook/react@b4eb0ad71 )**: Do not replay erroring beginWork with invokeGuardedCallback when suspended or previously errored ([#24480](facebook/react#24480)) //<Josh Story>//
- **[99eef9e2d](facebook/react@99eef9e2d )**: Hide children of Offscreen after destroy effects ([#24446](facebook/react#24446)) //<Ricky>//
- **[ce1386028](facebook/react@ce1386028 )**: Remove enablePersistentOffscreenHostContainer flag ([#24460](facebook/react#24460)) //<Andrew Clark>//
- **[72b7462fe](facebook/react@72b7462fe )**: Bump local package.json versions for 18.1 release ([#24447](facebook/react#24447)) //<Andrew Clark>//
- **[22edb9f77](facebook/react@22edb9f77 )**: React `version` field should match package.json ([#24445](facebook/react#24445)) //<Andrew Clark>//
- **[6bf3deef5](facebook/react@6bf3deef5 )**: Upgrade react-shallow-renderer to support react 18 ([#24442](facebook/react#24442)) //<Michael サイトー 中村 Bashurov>//

Changelog:
[General][Changed] - React Native sync for revisions bd4784c...d300ceb

jest_e2e[run_all_tests]

Reviewed By: cortinico, kacieb

Differential Revision: D36874368

fbshipit-source-id: c0ee015f4ef2fa56e57f7a1f6bc37dd05c949877
# for free to join this conversation on GitHub. Already have an account? # 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. Component: TextInput Related to the TextInput component. Merged This PR has been merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants