Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[RN Mobile] Fix Rich-Text setSelection call on Android when BR tags at the end of content #18138

Merged
merged 5 commits into from
Nov 5, 2019
Merged

[RN Mobile] Fix Rich-Text setSelection call on Android when BR tags at the end of content #18138

merged 5 commits into from
Nov 5, 2019

Conversation

daniloercoli
Copy link
Contributor

@daniloercoli daniloercoli commented Oct 28, 2019 •
edited
Loading

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 setSelection is 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:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.

@daniloercoli daniloercoli added the Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change) label Oct 28, 2019
@daniloercoli daniloercoli self-assigned this Oct 28, 2019
@daniloercoli daniloercoli mentioned this pull request Oct 28, 2019
Merged
@SergioEstevao SergioEstevao self-requested a review October 29, 2019 16:38
mchowning
mchowning approved these changes Oct 29, 2019
View reviewed changes
Copy link
Contributor

@mchowning mchowning left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@SergioEstevao
Copy link
Contributor

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.

@daniloercoli daniloercoli mentioned this pull request Oct 30, 2019
Merged
hypest
hypest reviewed Oct 30, 2019
View reviewed changes
Copy link
Contributor

@hypest hypest left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@daniloercoli
Merge branch 'master' of https://github.com/WordPress/gutenberg into …
e21e76e 
…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)
  ...
@daniloercoli
Use the same RegExp of Aztec
ac205a5
@daniloercoli
Add comment and use grouping on the matching regexp
2fd801f
@daniloercoli
Make the logic that re-set the selection indexes more clear
d9dd14c
@daniloercoli
Copy link
Contributor Author

This is ready for another pass.
Tests are failing as often happen. Tried to restart the job several times with no luck.

hypest
hypest approved these changes Nov 5, 2019
View reviewed changes
Copy link
Contributor

@hypest hypest left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for the changes @daniloercoli !

I've restarted a couple of Travis jobs in the meantime and it seems fully green now 🎉

@daniloercoli daniloercoli mentioned this pull request Nov 5, 2019
Open
@daniloercoli
Copy link
Contributor Author

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.

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.

@daniloercoli daniloercoli merged commit 711f1f7 into master Nov 5, 2019
@daniloercoli daniloercoli deleted the rnmobile/gb-mobile-872-JSApplicationIllegalArgumentException-in-RCTAztecView branch November 5, 2019 16:35
@youknowriad youknowriad added this to the Gutenberg 6.9 milestone Nov 11, 2019
@hypest hypest mentioned this pull request Nov 11, 2019
Closed
6 tasks
This was referenced Jan 31, 2020
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mchowning mchowning mchowning approved these changes

@hypest hypest hypest approved these changes

@marecar3 marecar3 Awaiting requested review from marecar3

@SergioEstevao SergioEstevao Awaiting requested review from SergioEstevao

Assignees

@daniloercoli daniloercoli

Labels
Mobile App - i.e. Android or iOS Native mobile impl of the block editor. (Note: used in scripts, ping mobile folks to change)
Projects
None yet
Milestone
Gutenberg 6.9
Development

Successfully merging this pull request may close these issues.
5 participants
@daniloercoli @SergioEstevao @hypest @mchowning @youknowriad