[RNMobile] Fix issue related to receiving undefined ref in text color format #56686
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
In rare cases, `TextColorEdit` might receive the `RichText` ref as undefined. This ref is used to get the background color of the text and use it in the toolbar button.
fluiddot
changed the title
|
Size Change: 0 B Total Size: 1.72 MB ℹ️ View Unchanged
|
|
Flaky tests detected in 91e9e27. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7047345689
|
fluiddot
added
[Type] Bug
fluiddot
commented
Nov 30, 2023
| const { | ||
| props: { style = {} }, | ||
| } = element; | ||
| const style = element?.props?.style ?? {}; |
There was a problem hiding this comment.
This logic relies on the fact that the ref object (contentRef), passed from the RichText component, contains the props passed to the AztecView component.
gutenberg/packages/format-library/src/text-color/index.native.js
Lines 51 to 58 in a2b4784
gutenberg/packages/format-library/src/text-color/index.native.js
Lines 65 to 86 in a2b4784
gutenberg/packages/block-editor/src/components/rich-text/native/index.native.js
Lines 1336 to 1337 in a2b4784
gutenberg/packages/block-editor/src/components/rich-text/native/index.native.js
Lines 1269 to 1272 in a2b4784
In theory, the props are not exposed in ref objects, so I investigated further where are they coming from. I couldn't pinpoint the code from React/React Native but I confirmed that ref objects coming from native components (like Aztec or Video (react-native-video) contain extra properties like props or forceUpdate. On the other hand, I couldn't find any documentation related to this, so I assume this approach is not recommended and might lead to issues in future React Native versions.
SiobhyB
approved these changes
Dec 1, 2023
There was a problem hiding this comment.
@fluiddot, thank you for the detailed explanation of the code and the workarounds for reproducing! I was able to confirm that these changes prevent the app crashing in the testing steps you outlined, and I agree they'll likely prevent the "real world" crashes too.
fluiddot
added a commit
that referenced
this pull request
Dec 5, 2023
… format (#56686) * Fix issue related to receiving undefined ref in text color format In rare cases, `TextColorEdit` might receive the `RichText` ref as undefined. This ref is used to get the background color of the text and use it in the toolbar button. * Update `react-native-editor` changelog * Add test to cover undefined `contentRef` case * Correct typo in `changelog`
4 tasks
fluiddot
added a commit
that referenced
this pull request
Dec 5, 2023
* Release script: Update react-native-editor version to 1.109.0 * Release script: Update CHANGELOG for version 1.109.0 * Release script: Update podfile * Update `react-native-editor` changelog * Update `react-native-editor` changelog * Mobile - Fix issue when backspacing in an empty Paragraph block (#56496) * Bring changes from #55134 to the mobile code * Mobile - RichText - Force focus when the block is selected but the textinput is not, for cases when merging blocks. * Update Buttons integration test due to a change in the logic of the app where deleting the only button available does not remove the block * Mobile - Heading block - Adds integration test for merging a Heading block with an empty Paragraph block * Mobile - Paragraph block - Adds integration test to check that backspacing in an empty Paragraph block merges succesfully with the previous block and keeps the focus on the TextInput * Mobile - RichText - Set selection values to be the last character position when merging and adds some comments to explain what is doing * Mobile - Paragraph block test - Use focusTextInput to check the TextInput is in focused instead of checking for the fomatting toolbar button * Rename shouldFocusTextInputAfterUpdate to shouldFocusTextInputAfterMerge * Update CHANGELOG * Release script: Update react-native-editor version to 1.109.1 * Release script: Update CHANGELOG for version 1.109.1 * Release script: Update podfile * [RNMobile] Fixes a crash on pasting MS Word list markup (#56653) * Add polyfill for Element.prototype.remove * Enable unit tests of `raw-handling` API filter `ms-list-converter` * Update `react-native-editor` changelog * [RNMobile] Fix issue related to receiving undefined ref in text color format (#56686) * Fix issue related to receiving undefined ref in text color format In rare cases, `TextColorEdit` might receive the `RichText` ref as undefined. This ref is used to get the background color of the text and use it in the toolbar button. * Update `react-native-editor` changelog * Add test to cover undefined `contentRef` case * Correct typo in `changelog` * [RNMobile] Fix HTML to blocks conversion when no transformations are available (#56723) * Add native workaround for HTML block in `htmlToBlocks` * Add raw handling tests This file is a clone of the same `blocks-raw-handling.js` file located in `gutenberg/test/integration`. The reason for the separation is that several of the test cases fail in the native version. For now, we are going to skip them, but we'd need to work on them in the future. Once all issues in tests are addressed, we'll remove this file in favor of the original one. * Update blocks raw handling test snapshot with original values * Disable more pasteHandler test cases due to not matching test snapshot * Comment obsolete snapshots of blocks raw handling tests The reason for commenting them instead of removing is that, in the future, we'll restore them once we address the failing test cases. * RichText (native): remove HTML check in getFormatColors (#56684) * Mobile - Global Styles - Fix issue with custom color variables not being parsed (#56752) * [RNMobile] Address `NullPointerException` crash in `AztecHeadingSpan` (#56757) Address rare cases where a null value is passed to a heading block, causing a crash. * Release script: Update react-native-editor version to 1.109.2 * Release script: Update CHANGELOG for version 1.109.2 * Release script: Update podfile --------- Co-authored-by: Gerardo Pacheco <[email protected]> Co-authored-by: Ella <[email protected]> Co-authored-by: Siobhan Bamber <[email protected]>
SiobhyB
pushed a commit
that referenced
this pull request
Dec 14, 2023
* Release script: Update react-native-editor version to 1.109.0 * Release script: Update CHANGELOG for version 1.109.0 * Release script: Update podfile * Update `react-native-editor` changelog * Update `react-native-editor` changelog * Mobile - Fix issue when backspacing in an empty Paragraph block (#56496) * Bring changes from #55134 to the mobile code * Mobile - RichText - Force focus when the block is selected but the textinput is not, for cases when merging blocks. * Update Buttons integration test due to a change in the logic of the app where deleting the only button available does not remove the block * Mobile - Heading block - Adds integration test for merging a Heading block with an empty Paragraph block * Mobile - Paragraph block - Adds integration test to check that backspacing in an empty Paragraph block merges succesfully with the previous block and keeps the focus on the TextInput * Mobile - RichText - Set selection values to be the last character position when merging and adds some comments to explain what is doing * Mobile - Paragraph block test - Use focusTextInput to check the TextInput is in focused instead of checking for the fomatting toolbar button * Rename shouldFocusTextInputAfterUpdate to shouldFocusTextInputAfterMerge * Update CHANGELOG * Release script: Update react-native-editor version to 1.109.1 * Release script: Update CHANGELOG for version 1.109.1 * Release script: Update podfile * [RNMobile] Fixes a crash on pasting MS Word list markup (#56653) * Add polyfill for Element.prototype.remove * Enable unit tests of `raw-handling` API filter `ms-list-converter` * Update `react-native-editor` changelog * [RNMobile] Fix issue related to receiving undefined ref in text color format (#56686) * Fix issue related to receiving undefined ref in text color format In rare cases, `TextColorEdit` might receive the `RichText` ref as undefined. This ref is used to get the background color of the text and use it in the toolbar button. * Update `react-native-editor` changelog * Add test to cover undefined `contentRef` case * Correct typo in `changelog` * [RNMobile] Fix HTML to blocks conversion when no transformations are available (#56723) * Add native workaround for HTML block in `htmlToBlocks` * Add raw handling tests This file is a clone of the same `blocks-raw-handling.js` file located in `gutenberg/test/integration`. The reason for the separation is that several of the test cases fail in the native version. For now, we are going to skip them, but we'd need to work on them in the future. Once all issues in tests are addressed, we'll remove this file in favor of the original one. * Update blocks raw handling test snapshot with original values * Disable more pasteHandler test cases due to not matching test snapshot * Comment obsolete snapshots of blocks raw handling tests The reason for commenting them instead of removing is that, in the future, we'll restore them once we address the failing test cases. * RichText (native): remove HTML check in getFormatColors (#56684) * Mobile - Global Styles - Fix issue with custom color variables not being parsed (#56752) * [RNMobile] Address `NullPointerException` crash in `AztecHeadingSpan` (#56757) Address rare cases where a null value is passed to a heading block, causing a crash. * Release script: Update react-native-editor version to 1.109.2 * Release script: Update CHANGELOG for version 1.109.2 * Release script: Update podfile * Release script: Update react-native-editor version to 1.109.3 * Release script: Update CHANGELOG for version 1.109.3 * Release script: Update podfile * Mobile - Fix having the default font sizes when there are theme font sizes available * Update CHANGELOG entry --------- Co-authored-by: Carlos Garcia <[email protected]> Co-authored-by: Gerardo Pacheco <[email protected]> Co-authored-by: Ella <[email protected]>
artemiomorales
pushed a commit
that referenced
this pull request
Jan 4, 2024
* Release script: Update react-native-editor version to 1.109.0 * Release script: Update CHANGELOG for version 1.109.0 * Release script: Update podfile * Update `react-native-editor` changelog * Update `react-native-editor` changelog * Mobile - Fix issue when backspacing in an empty Paragraph block (#56496) * Bring changes from #55134 to the mobile code * Mobile - RichText - Force focus when the block is selected but the textinput is not, for cases when merging blocks. * Update Buttons integration test due to a change in the logic of the app where deleting the only button available does not remove the block * Mobile - Heading block - Adds integration test for merging a Heading block with an empty Paragraph block * Mobile - Paragraph block - Adds integration test to check that backspacing in an empty Paragraph block merges succesfully with the previous block and keeps the focus on the TextInput * Mobile - RichText - Set selection values to be the last character position when merging and adds some comments to explain what is doing * Mobile - Paragraph block test - Use focusTextInput to check the TextInput is in focused instead of checking for the fomatting toolbar button * Rename shouldFocusTextInputAfterUpdate to shouldFocusTextInputAfterMerge * Update CHANGELOG * Release script: Update react-native-editor version to 1.109.1 * Release script: Update CHANGELOG for version 1.109.1 * Release script: Update podfile * [RNMobile] Fixes a crash on pasting MS Word list markup (#56653) * Add polyfill for Element.prototype.remove * Enable unit tests of `raw-handling` API filter `ms-list-converter` * Update `react-native-editor` changelog * [RNMobile] Fix issue related to receiving undefined ref in text color format (#56686) * Fix issue related to receiving undefined ref in text color format In rare cases, `TextColorEdit` might receive the `RichText` ref as undefined. This ref is used to get the background color of the text and use it in the toolbar button. * Update `react-native-editor` changelog * Add test to cover undefined `contentRef` case * Correct typo in `changelog` * [RNMobile] Fix HTML to blocks conversion when no transformations are available (#56723) * Add native workaround for HTML block in `htmlToBlocks` * Add raw handling tests This file is a clone of the same `blocks-raw-handling.js` file located in `gutenberg/test/integration`. The reason for the separation is that several of the test cases fail in the native version. For now, we are going to skip them, but we'd need to work on them in the future. Once all issues in tests are addressed, we'll remove this file in favor of the original one. * Update blocks raw handling test snapshot with original values * Disable more pasteHandler test cases due to not matching test snapshot * Comment obsolete snapshots of blocks raw handling tests The reason for commenting them instead of removing is that, in the future, we'll restore them once we address the failing test cases. * RichText (native): remove HTML check in getFormatColors (#56684) * Mobile - Global Styles - Fix issue with custom color variables not being parsed (#56752) * [RNMobile] Address `NullPointerException` crash in `AztecHeadingSpan` (#56757) Address rare cases where a null value is passed to a heading block, causing a crash. * Release script: Update react-native-editor version to 1.109.2 * Release script: Update CHANGELOG for version 1.109.2 * Release script: Update podfile * Release script: Update react-native-editor version to 1.109.3 * Release script: Update CHANGELOG for version 1.109.3 * Release script: Update podfile * Mobile - Fix having the default font sizes when there are theme font sizes available * Update CHANGELOG entry --------- Co-authored-by: Carlos Garcia <[email protected]> Co-authored-by: Gerardo Pacheco <[email protected]> Co-authored-by: Ella <[email protected]>
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?
In rare cases, probably related to slow performance in low-end devices, the ref object
contentRefpassed from theRichTextcomponent to theTextColorEditcomponent can be undefined upon mounting. This ref object is used to get the background color of the text and use it in the toolbar button. However, the logic of the text color format assumed that this ref is always defined, leading to a crash.Why?
Fixes wordpress-mobile/gutenberg-mobile#6189.
Fixes wordpress-mobile/gutenberg-mobile#6126.
How?
This PR addresses this issue by checking the existence of the ref object
contentRef.Testing Instructions
NOTE: I couldn't find a reliable way to reproduce the issue except modifying the code.
(UPDATE) Reload React Native
I noticed while testing another way to reproduce it, although it only works when connecting the app with a local Metro server. I don't think this behavior can be reproduced in a production version, however, it confirms the fact that
contentRefcan be undefined in some cases.Testing Instructions for Keyboard
N/A
Screenshots or screencast
NOTE: The theme has the background color set to
#ABB8C3.contentRefis definedcontentRefis undefined