[RNMobile] Fix HTML to blocks conversion when no transformations are available #56723
Conversation
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.
| // NOTE: 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. | ||
| describe( 'Blocks raw handling', () => { |
There was a problem hiding this comment.
As explained in the inline comment, this file is a copy with an extra test case.
In a follow-up GitHub issue, we'll track the different test cases to address and analyze why they fail then in the native version.
There was a problem hiding this comment.
My rationale for running the same tests as the web version is that the raw block handling should works the same way on both platforms. Especially, the processing shouldn’t produce exception as would lead to crashes. For the cases where the issue is related to produce a different output, we’d need determine if that produce conflicts between versions.
There was a problem hiding this comment.
As a side note, adding these tests and addressing them for the native version is also related to #55652.
There was a problem hiding this comment.
I agree that raw block handling should work the same way on both platforms, and that we should utilize the web tests as a foundation for the native tests. I took a close look at these tests to understand why some of the skipped ones were failing on native. Unless I'm misunderstanding why the snapshots are obsolete in the first place, I found that several of the skipped tests only needed the snapshots updated in the native context to pass. I've added those in ca8cf35 as an experiment. If they look promising, feel free to incorporate in this PR, or we could use it as the starting point for a followup PR.
For some of the other failures, particularly when the assertion fails due to a console.log expectation (#), I noted that web sometimes aliases the window object when logging, like in the paste-handler here:
As noted in the approval, I don't feel this is a blocker for merging.
There was a problem hiding this comment.
Unless I'm misunderstanding why the snapshots are obsolete in the first place, I found that several of the skipped tests only needed the snapshots updated in the native context to pass. I've added those in ca8cf35 as an experiment. If they look promising, feel free to incorporate in this PR, or we could use it as the starting point for a followup PR.
I originally updated the snapshots in 78dab04 for the native version, however, I was concerned that the output might not be the expected one compared to the web snapshots. That's why I updated it in bce611a. This resulted in disabling other test cases referenced in 5e44132. It might be possible that the output of the processing differs in the native version, but we'd need to check first that the differences are (1) expected and (2) won't lead to conflicts in the content between platforms.
That said, I'd postpone updating the snapshots until we investigate further what's causing the test failures.
| // This is an extra test added to cover the case fixed in: | ||
| // `rnmobile/fix/div-tag-convert-to-blocks`. | ||
| it( 'should convert to unsupported HTML block when no transformation is available', () => { | ||
| const HTML = '<div><p>Hello world!</p></div>'; | ||
| expect( serialize( rawHandler( { HTML } ) ) ).toMatchSnapshot(); | ||
| } ); |
There was a problem hiding this comment.
This is the extra test cases added to cover the issue that the PR fixes.
| return parse( | ||
| `<!-- wp:html -->${ node.outerHTML }<!-- /wp:html -->` | ||
| ); |
There was a problem hiding this comment.
Since the HTML block is not registered in the native version, the block will be generated by simply parsing the raw HTML content of an HTML block with the HTML code provided.
|
Size Change: +38 B (0%) Total Size: 1.72 MB
ℹ️ View Unchanged
|
The reason for commenting them instead of removing is that, in the future, we'll restore them once we address the failing test cases.
There was a problem hiding this comment.
This test snapshot is a copy of the original snapshot with an extra entry for the test case rawHandler should convert to unsupported HTML block when no transformation is available.
Note that some snapshots are commented due to:
- Avoid a test failure due to having obsolete snapshots.
- In the future, they will be restored once we address the test cases that fail in the native version.
|
@derekblank I've assigned you for a preliminary review, as you were also assigned to the issues. Let me know if you can take a quick look. Thanks 🙇 ! |
fluiddot
added
[Type] Bug
There was a problem hiding this comment.
I was able to test iOS successfully: I did not experience the crash when applying this fix, and the steps to produce the correct HTML succeeded.
On Android, I did not experience the crash after applying this fix, but I did experience slightly different results when producing the correct HTML. I noted that if the sample raw HTML snippet is pasted as the first piece of content at the beginning of the post body (i.e., no other blocks before it), an extra <p></p> tag is added to the HTML block when converted from a Classic block. If these steps are repeated, an extra </p> is added to the subsequent converted HTML blocks, with no opening <p> tag.
However, when a properly registered block is added (like a normal paragraph block with text), and then the raw HTML is pasted in code mode, the HTML is correct, and the Android behavior matches the correct behavior of web and iOS. So perhaps there is something to do with the <!-- wp:x --> element of the parser when interpreting the rawTransform when no other <!-- wp:x --> elements are present. This feels like an edge case, however, but perhaps it's a clue to start unblocking some of the skipped tests.
In the video examples below, the left-hand video shows the extra <p> tag being added. Also noting that the existing <p> tag from the pasted HTML is stripped out as soon as the [⋮] button is pressed. The right-hand side video demonstrates that the correct HTML is added when a registered block precedes the raw HTML.
| Before | After |
|---|---|
Extra.HTML.mov |
Correct.HTML.mov |
As this fix eliminates the fatal exception for both platforms (which is most important), I'm approving, and would be in support of moving this change out of draft status and merging it as-is, and then addressing the tests and the non-fatal Android HTML block behavior in follow-up PRs.
That's great, thank you so much @derekblank for the review 🙇 !
After checking the videos you shared, I noticed that the HTML is automatically modified after pasting it (see attached video capture). This makes me wonder if any reformatting is being applied on Android in the HTML mode and if this is the cause of adding the extra Kapture.2023-12-04.at.11.16.18.mp4
The reason for leaving it in draft was mainly to finish testing and document the changes I applied. It would be interesting to also test that no regressions are introduced in the web version. Hence, I might add another reviewer so we can incorporate it in the ongoing betafix ( |
I tested the web version and works as expected. So, I'll proceed with merging the PR 🎊 . |
I did note that the HTML is automatically modified as soon as the [⋮] button is pressed as well. Previously I was testing with a physical Samsung Galaxy S20. Upon testing again with an emulated Pixel 3 Pro, I did note that the HTML was automatically modified when pressing [⋮], but the transformed HTML in the block was actually correct (see example video). My initial gut reaction when testing was that the extra HTML may be a quirk of the Samsung keyboard. Pixel3.mov |
Although this issue has been merged, just to confirm, the web matched iOS behavior in my testing. 👍 |
…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.
* 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]>
* 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]>
* 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]>
What?
Fixes an issue related to converting HTML to blocks when using the unsupported HTML block in the native version.
This issue was introduced in #55461 after enabling the Convert to blocks option in the Classic block.
Why?
Fixes wordpress-mobile/gutenberg-mobile#6420.
Fixes wordpress-mobile/gutenberg-mobile#6084.
Related to wordpress-mobile/gutenberg-mobile#6416.
How?
htmlToBlocksso instead of using the HTML block, it will render the HTML block as an unsupported block.Testing Instructions
Testing Instructions for Keyboard
N/A
Screenshots or screencast
N/A