[RNMobile] Fix crash related to accessing undefined value in TextColorEdit
#55664
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
fluiddot
added
[Type] Bug
fluiddot
commented
Oct 27, 2023
|
Size Change: 0 B Total Size: 1.7 MB ℹ️ View Unchanged
|
fluiddot
force-pushed
the
rnmobile/fix/undefined-length-property-crash
branch
from
0a0bc48
to
5a6d1d3
Compare
|
Flaky tests detected in 5a6d1d3. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6800639409
|
derekblank
approved these changes
Nov 9, 2023
There was a problem hiding this comment.
Thanks for addressing this, @fluiddot. I was able to successfully follow the testing instructions, and did not experience a crash. I did some further testing with other blocks related to colors with retired themes in an attempt to break them, but did not experience any other exceptions either.
As far as I investigated, the only reason that would explain why this setting is undefined is that the color setting of the block-editor Redux store is removed. It's totally unclear to me why this might happen, at least I haven't found a potential culprit.
I did not note a clear culprit either. Given the undefined value and the nature of the error message in wordpress-mobile/gutenberg-mobile#6208, I agree that your explanation is very plausible, and seems likely to be correct. I also agree that updating the default palette setting to remove the color values is still a better user experience than a crash. Nice work! 🚀
4 tasks
cbravobernal
pushed a commit
that referenced
this pull request
Nov 14, 2023
…orEdit` (#55664) * Guard `hasColorsToChoose` against potential `colors` undefined value * Avoid returning undefined in `useMobileGlobalStylesColors` * Update `react-native-editor` changelog
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Fixes a crash produced in an edge case related to potentially using retired themes.
Unfortunately, I couldn't manage to reproduce the issue. However, after inspecting several sessions, the pattern I identified is that all sites have retired themes that don't support FSE and have Global Styles limited.
Why?
Fixes wordpress-mobile/gutenberg-mobile#6208.
How?
The only setting related to theme colors that I narrowed down to produce the exceptions (due to accessing an undefined value) is
editorDefaultPalettereturned inuseMobileGlobalStylesColorsfunction. As far as I investigated, the only reason that would explain why this setting isundefinedis that thecolorsetting of theblock-editorRedux store is removed. It's totally unclear to me why this might happen, at least I haven't found a potential culprit.gutenberg/packages/components/src/mobile/global-styles-context/utils.native.js
Lines 360 to 364 in dded3a7
gutenberg/packages/block-editor/src/store/defaults.js
Lines 44 to 101 in 5a6d1d3
For the rest of the theme color settings, the ones under
__experimentalFeaturessetting, seem we ensure that always have a value and never lead to an exception. In all tests I've performed forcing undefined values there, the editor ended up using the default colors provided in default settings.gutenberg/packages/components/src/mobile/global-styles-context/utils.native.js
Lines 426 to 434 in 5a6d1d3
gutenberg/packages/components/src/mobile/global-styles-context/utils.native.js
Lines 381 to 396 in 5a6d1d3
Testing Instructions
colorssetting to be removed.NOTE: You might notice that no colors are displayed when opening the block settings of the Paragraph block. The same will be experienced in other blocks like Cover and Buttons. This is expected (yet not ideal) due to the side effect of removing the default editor colors. The purpose of this PR is to avoid the crash, so in the future, it would be great to investigate this edge case further and potentially provide a better solution to include the default colors.
Testing Instructions for Keyboard
N/A
Screenshots or screencast
N/A