-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
[RNMobile] Limit inner blocks nesting depth to avoid call stack size exceeded crash #54382
[RNMobile] Limit inner blocks nesting depth to avoid call stack size exceeded crash #54382
Conversation
Size Change: 0 B Total Size: 1.62 MB ℹ️ View Unchanged
|
f9891cf
to
e028e9a
Compare
@osullivanchris I've shared some screenshots and video captures in the PR. Let me know if you have any feedback regarding design and copy, thanks 🙇 ! |
@fluiddot looks good. Nice job! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well done! Thank you for identifying a useful resolution to this crash.
I left several comments for consideration. Additionally, I noted that one can change the background color, text color, and alignment of these blocks. Only the first of those three results in a visual change. Is this expected? Should we disable those controls? Is it confusing? Does it matter? I'm not sure. 🤔
packages/block-editor/src/components/inner-blocks/warning-max-depth-exceeded.native.js
Outdated
Show resolved
Hide resolved
packages/block-editor/src/components/inner-blocks/warning-max-depth-exceeded.native.js
Outdated
Show resolved
Hide resolved
packages/block-editor/src/components/inner-blocks/warning-max-depth-exceeded.native.js
Outdated
Show resolved
Hide resolved
packages/block-editor/src/components/inner-blocks/warning-max-depth-exceeded.native.js
Show resolved
Hide resolved
packages/block-editor/src/components/block-fallback-web-version/style.native.scss
Outdated
Show resolved
Hide resolved
packages/block-editor/src/components/block-fallback-web-version/index.native.js
Outdated
Show resolved
Hide resolved
packages/block-editor/src/components/block-fallback-web-version/index.native.js
Outdated
Show resolved
Hide resolved
packages/block-editor/src/components/block-fallback-web-version/index.native.js
Outdated
Show resolved
Hide resolved
Co-authored-by: David Calhoun <github@davidcalhoun.me>
Given this is somewhat of an edge case within an edge case, I feel comfortable leaving the implementation as-is. It could cause some confusion, but it is not destructive in any way. |
`MAX_NESTING_DEPTH` has been moved to the constants file.
@dcalhoun I addressed the feedback from the review and generated new installable builds. Let me know if you could take another look. I also added two new WP hooks to allow GBM edit the messaging based on the UBE's availability (6e64270, 167418f).
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀 I verified the test plan succeeds. Thank you for improving this experience.
We should likely add a release note. |
Good point. I'll add an entry to the changelog and release notes before merging the PR. |
Flaky tests detected in 3b5c09a. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6302497204
|
Related PRs:
What?
Limits
InnerBlock
component to render blocks that are deeply nested, in order to prevent a crash related to exceeding the maximum call stack trace set by the Hermes engine.Why?
Fixes wordpress-mobile/WordPress-Android#18750.
Fixes wordpress-mobile/gutenberg-mobile#6123
How?
In the
InnerBlocks
component we check the number of parent blocks associated with the block, this determines the nesting depth level. If it exceeds a threshold defined here, we render a warning message box instead of rendering the inner blocks.gutenberg/packages/block-editor/src/components/inner-blocks/index.native.js
Lines 130 to 139 in e028e9a
Additionally, I extracted the UI elements used in the "missing" block (i.e. the block used to render unsupported blocks) into a separate component (
BlockFallbackWebVersion
). In a separate PR, I'll update that block to use this new component.Testing Instructions
The warning IS NOT displayed when NOT exceeding the threshold
Deeply nested block structure (8 levels)
The warning IS displayed when exceeding the limit
Deeply nested block structure (20 levels)
Option for editing using the web editor is not present in self-hosted sites without Jetpack
Deeply nested block structure (20 levels)
Self-hosted site with Jetpack
Deeply nested block structure (20 levels)
WordPress.com
login to edit using UBE.WordPress.com
login".Unsupported Block Editing
Since we are using UBE in the warning message box, it would be great to cover some of the test cases of that feature.
Testing Instructions for Keyboard
N/A
Screenshots or screencast
ios-max-depth-warning-wpcom.mp4
ios-max-depth-warning-self-hosted.mp4