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

Fix crash if Shared Element ID is invalid #6511

Merged
merged 2 commits into from
Sep 1, 2020
Merged

Fix crash if Shared Element ID is invalid #6511

merged 2 commits into from
Sep 1, 2020

Conversation

mrousavy
Copy link
Collaborator

Fixes a hard-crash on iOS if the specified Shared Element ID could not be found/invalid nativeID has been set.

Instead, the Shared Element Transition, which could not locate the specified View by it's nativeID, will just be ignored.

Fixes a hard-crash on iOS if the specified Shared Element ID could not be found/invalid nativeID has been set
@mrousavy
Copy link
Collaborator Author

mrousavy commented Aug 25, 2020

I just noticed that when one nativeID (either source or target) is invalid/cannot be found, the Android app gets stuck. So the similar issue can be reproduced on Android.

I'm guessing this has something to do with OptimisticViewFinder:: find, since that cannot find the view via the sync approach (ReactFindViewUtil.findView(root.view, nativeId)), and therefore falls back to adding a view found listener - which will never be called, because the view doesn't exist.

So either we only try the sync approach, and if it can't be found, we skip this shared element transition (I'm guessing this would require every shared element transition to have a waitForRender: true, which makes sense anyways doesn't it?), or we somehow add some complex timeout logic.

With the timeout logic I still don't think this would make sense, as the navigation and therefore the whole app is completely stalled during this time.

@yogevbd
Copy link
Collaborator

yogevbd commented Sep 1, 2020

@mrousavy When defining shared element transition we already wait for render. I think we should also throw an informative exception in such case in addition to the check you've added.

@mrousavy
Copy link
Collaborator Author

mrousavy commented Sep 1, 2020

@yogevbd I see. We could use the LogBox to show a warning

@yogevbd yogevbd merged commit fcf23d2 into wix:master Sep 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants