mattermost / react-native-paste-input

React Native TextInput replacement to allow pasting files
MIT License
89 stars 15 forks source link

fix: iOS crash on blur #20

Closed loganrosen closed 11 months ago

loganrosen commented 11 months ago

Summary

On iOS, there is currently a crash when the component unmounts and the blur method is called automatically. This is the error that appears:

Screenshot 2023-07-09 at 10 03 57 PM

Consumers like Bluesky have had to manually work around this by explicitly blurring the input themselves before unmounting it (see bluesky-social/social-app#56 for example).

I've put in a fix for this by creating an explicit blur command that issues viewCommands.blur() on the input reference, based on the other examples in that file. I don't know if this is the correct fix, but it seems to work in the simulator.

To better demonstrate this issue, I've modified the example app to have a button to show/hide the input box. Without this change, you'll see the crash mentioned above.

The other changes were automatically generated during the build/editing process.

mattermost-build commented 11 months ago

Hello @loganrosen,

Thanks for your pull request! A Core Committer will review your pull request soon. For code contributions, you can learn more about the review process here.

loganrosen commented 11 months ago

I've investigated this further, and I found a couple things:

  1. The crash doesn't happen on Android for the same test case. However:
  2. After updating the test case to have an onBlur handler set for the input, I found that it doesn't get triggered on unmount on either platform (even with the "fix" on iOS). It does get triggered if you manually call .blur() on the input ref.

This makes me wonder if this logic to blur on unmount makes sense/would ever work based on how React works. It seems that the component unmount happens before onBlur would be triggered, even if the blur goes through successfully. Do we know why this was added to begin with and what the intentions were?

loganrosen commented 11 months ago

I've adjusted the example app to demonstrate the issue. Note how the focus state only gets changed when you click the other input I added (and not when clicking the "Hide Input" button, which calls blur() on the paste input). If the intention behind calling blur() on unmount is to allow onBlur hooks to be invoked, that doesn't seem to be working.

enahum commented 11 months ago

This makes me wonder if this logic to blur on unmount makes sense/would ever work based on how React works. It seems that the component unmount happens before onBlur would be triggered, even if the blur goes through successfully.

so is this PR needed ?

loganrosen commented 11 months ago

We definitely need to make some change, as the crash is happening on iOS when it tries to blur on unmount. But I was wondering if we should just remove the logic completely to blur on unmount on iOS and Android, as it doesn't seem to actually trigger the onBlur handler and has no apparent effect. Would you be open to that?

enahum commented 11 months ago

So allow me to inderstand, you are calling textInputRef.blur() on a component that you are unmounting? Is that what is causing the crash?

loganrosen commented 11 months ago

That's what the current code is doing here. That leads to a crash without my fix. But then even with the fix, blurring the input on unmount doesn't seem to have a clear purpose, so I wonder if we can remove that block completely.

enahum commented 11 months ago

I see, it makes sense.. ok let's add the fix but remove the changes to the example app as they are not really needed.

loganrosen commented 11 months ago

Okay, I took a deeper look into this, and it turns out there's a simpler fix that doesn't risk breaking consumers of this blur behavior. This React Native commit is reflected in the Android code but not the iOS code, and it looks like we're hitting the exact condition mentioned there. So I made the iOS code mirror the Android code (by using useLayoutEffect), and now toggling the input visibility in the example app works fine.

I've reverted the other changes, such as displaying the onBlur behavior. I can also revert the change to add the visibility toggle in the example app, but I find it to be a useful test case to make sure that behavior is working. Let me know.