[RN Mobile] Fix Rich-Text setSelection call on Android when BR tags at the end of content#18138
Conversation
…m the Selection indexes on Android
mchowning
left a comment
There was a problem hiding this comment.
This looks good to me. 👍
I tested this by following the directions from your comment on the issue. One thing that tripped me up a bit in testing that was that if you select the paragraph with the extra <br> at the end, then the crash won't happen. It is important to not interact with the paragraph with the extra <br> at the end before you try to merge into it if you want to recreate the crash.
I also tested this in the demo app by manually adding <br> tags to the end of paragraph blocks in the html view, and I was also able to recreate the crash (and verify that this fixes them) that way.
The only suggestion I would have is that this seems like a good candidate for extracting this branch of the if statement into a separate pure function that we could then write some tests around (or possibly even extract all this special selection handling logic into a separate file). I love looking at regex as much as anyone 😉 , but I think a few test cases would make the intent behind this code really obvious.
Strongly agree with this, will make it a lot more easier to compare code with the web too. |
hypest
left a comment
There was a problem hiding this comment.
This fixes the issue! I've left a few comments that would help me understand the specifics here. Let me know if something's not clear Danilo.
…rnmobile/gb-mobile-872-JSApplicationIllegalArgumentException-in-RCTAztecView * 'master' of https://github.com/WordPress/gutenberg: (56 commits) Update: Default gradients. (#18214) Fix: setting a preset color on pullquote default style makes the block invalid (#18194) Fix default featured image size (#15844) Fix postmeta radio regression. (#18183) Components: Switch screen-reader-text to VisuallyHidden (#18165) [rnmobile] Release 1.16.0 to master (#18261) Template Loader: Add theme block template resolution. (#18247) Add a README file for storybook directory (#18245) Add editor-gradient-presets to get_theme_support (#17841) Add "Image Title Attribute" as an editable attribute on the image block (#11070) enables horizontal movers in social blocks (#18234) [RNMobile] Add mobile Spacer component (#17896) Add experimental `ResponsiveBlockControl` component (#16790) Fix mover for floats. (#18230) Rename Component to WPComponent in docstring (#18226) Colors Selector: replace `Aa` text by SVG icon (#18222) Removed gif from README (#18200) makes the submenu items stacked vertically (#18221) Add block navigator to sidebar panel for nav block (#18202) Fix: consecutive updates may trigger a blocks reset (#18219) ...
|
This is ready for another pass. |
There was a problem hiding this comment.
LGTM, thanks for the changes @daniloercoli !
I've restarted a couple of Travis jobs in the meantime and it seems fully green now 🎉
Yes, I agree with you. I've opened a new issue for it here: wordpress-mobile/gutenberg-mobile#1543 and moving forward with the merge of this PR. |
Description
This PR does fix a problem where Aztec Android is out of synch with the JS side when BR tags are available at the end of the content. Currently in Aztec this html
<p>Hello There!<br></p>after being parsed to span and back to html will become<p>Hello There!</p>.This is not a bug, this is how we originally implemented the function that cleans the HTML input in AztecParser->tidy method.
The crash does happen when
setSelectionis called on Aztec with the indexes passed from the JS side, that, for the reasons explained above, may be out of bounds.GB-Mobile PR: wordpress-mobile/gutenberg-mobile#1507
cc @SergioEstevao
Details to reproduce the problem in the GB-mobile above.
Types of changes
Checklist: