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

Add RN patch for RCTShadowView.m to prevent crash in nested group block #13310

Merged
merged 2 commits into from
Jan 29, 2020

Conversation

dratwas
Copy link
Contributor

@dratwas dratwas commented Jan 29, 2020

Fixes wordpress-mobile/gutenberg-mobile#1747

There is an issue on iOS with nested views.

Searching for the ancestor view while measuring a shadow view layout relative to the ancestor is limited to 30. I couldn't find the reason nor commit where it was introduced (because I have no access to internal facebook commits).
I think we should keep looking for an ancestor view until the shadow view has the superview property.
facebook/react-native#26986

This is fixed here facebook/react-native#26986
It's merged to master and will land in 0.62 version of react-native, however it is a blocker for us since we can not nest more than 3 group blocks because of that. We could cherry-pick this commit to our fork but as I can see we do not use our fork anymore.

To fix that on our side we used a patch mechanism. The mechanism is very simple. I created a patch that is in the patches directory and contains my changes. The patch is applied in post_install hook of cocoapods.

To test:

  • install dependencies

  • run app and open gutenberg editor

  • add at least 4 nested group blocks

  • the app shouldn't crash

  • I have considered adding unit tests where possible.

  • I have considered adding accessibility improvements for my changes.

  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Jan 29, 2020

You can trigger an installable build for these changes by visiting CircleCI here.

@lukewalczak
Copy link
Contributor

lukewalczak commented Jan 29, 2020

Tested manually in WordPress-iOS app - nested 5 groups:

nesting_groups

No crash, works as expected 🎉

@pinarol pinarol added this to the 14.2 milestone Jan 29, 2020
Copy link
Contributor

@pinarol pinarol left a comment

Choose a reason for hiding this comment

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

Great job @dratwas ! 🎉

Thank you very much for testing @lukewalczak ! 🙇

Let's :shipit:

@pinarol pinarol merged commit e8d5df6 into develop Jan 29, 2020
pull bot pushed a commit to scope-demo/WordPress-iOS that referenced this pull request Jan 29, 2020
@pinarol pinarol deleted the fix/rn-patch branch January 29, 2020 14:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cherry pick "max depth" fix on our react-native fork
3 participants