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

Crash when nesting more than 3 group blocks #1486

Closed
1 of 3 tasks
pinarol opened this issue Oct 23, 2019 · 4 comments
Closed
1 of 3 tasks

Crash when nesting more than 3 group blocks #1486

pinarol opened this issue Oct 23, 2019 · 4 comments

Comments

@pinarol
Copy link
Contributor

pinarol commented Oct 23, 2019

We are having a crash when we nest more than 3 group blocks, details can be found in this comment.

66844791-33bddd00-ef6f-11e9-89e2-556ec5ccf6a3

Root cause analysis done by @dratwas:

We can read from our error that the app crashes while calculating the layout and it says “View which I get is not a descendant of ScrollView so I can not calculate the View layout relative to the ScrollView”

I checked it and it’s not true. The View with tag 7933 is a descendant of ScrollView with the tag 47. I spend a while on debugging and found this code

https://href.li/?https://github.com/wordpress-mobile/gutenberg-mobile/issues/1485

1 | NSInteger depth = 30; // max depth to search

I’m not sure why they use the 30 as a max depth but it is our cause of the crash. It seems like the shadow thread tries to calculate the layout of the group block which is nested in ScrollView with depth more than 30. We definitely should increase this number but I guess it could be good to set the maximum level of nesting in the editor or do something else to prevent that issue in the future.

Solution

We have multiple work to do about this.

  • Investigate the component hierarchy and see if we have some unnecessary containers that can be removed. Will be addressed by Try reducing the component hierarchy for Group block #1485
  • Open a PR to react-native repo to propose an increase to this limit
  • Even if we increase that limit there will still be a limit, so it looks like we still need to introduce a UI to handle this properly without any data loss or any odd behaviour. Maybe introducing a UI to communicate that this much nesting is not supported on mobile? @iamthomasbishop

cc @koke

@iamthomasbishop
Copy link
Contributor

Maybe introducing a UI to communicate that this much nesting is not supported on mobile?

I think while I'd like to avoid limiting nesting beyond a few levels, we could temporarily limit nesting beyond a certain level by hiding the appenders in those cases. However, we'll still need to support the display of infinitely nested blocks considering the web has no restriction. So I'd prefer to not limit if possible.

@koke
Copy link
Member

koke commented Oct 23, 2019

we'll still need to support the display of infinitely nested blocks considering the web has no restriction

0F3D0586-D913-4A67-A103-999ED33D65A3

It’s true that there are no restrictions set on the web, but “infinite” is a big number 😉 Id say it’s more that it hasn’t been addressed rather than its supported:

A5237730-8A71-4740-A69B-5BA62939D70C

I think it’s reasonable to expect way more than three levels of nesting, but I also think it’s reasonable to set a limit somewhere, because you just can’t render “infinite”. The only alternative I see is to consider some sort of modal editing where you would enter an edit mode for a block beyond the nesting limit, starting with that group as the top level item.

@iamthomasbishop
Copy link
Contributor

Id say it’s more that it hasn’t been addressed rather than its supported:

Oh, I fully agree – I'm not saying there shouldn't be a max, I'm just saying that's the reality at the moment.

It's worth noting that we pushed hard for a more flexible layout and interaction model for small screens on mobile that relies less on added padding on each level so in theory, we could more gracefully support deeper nesting.

With that said, I wouldn't be opposed to somehow setting a reasonable max. Maybe something like 5 levels is a good starting point.

@pinarol
Copy link
Contributor Author

pinarol commented Jan 29, 2020

We fixed this crash by patching react native.

@pinarol pinarol closed this as completed Jan 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants