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.

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

Native mobile: don't set caret when rich-text text will get trimmed #15021

Merged
merged 6 commits into from
Apr 17, 2019

Conversation

hypest
Copy link
Contributor

@hypest hypest commented Apr 17, 2019

Description

Addressing wordpress-mobile/gutenberg-mobile#871 on the native mobile side. The internal representation of rich-text in Aztec and the format-lib are not fully compatible or in-sync and we also use html to establish the communication between the two. That can lead to cases like the issue linked, where Aztec will do some trimming to remove spaces that are unimportant for html rendering, but format-lib won't so, making the caret positioning logic to get out of sync.

This PR will try to detect when such spaces will be removed and avoid telling Aztec where to put the caret and hope for the best 🤞.

How has this been tested?

Using the gutenberg-mobile side PR: wordpress-mobile/gutenberg-mobile#885

Types of changes

Try to detect when such spaces will be removed and avoid telling Aztec where to put the caret. Only affecting Android.

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.

@hypest hypest 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 Apr 17, 2019
@hypest hypest requested a review from marecar3 April 17, 2019 14:42
@@ -624,6 +625,17 @@ export class RichText extends Component {
}
}

willTrimSpaces( html ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a nice opportunity to write some unit test against this method, so that we are always sure it's working like expected, WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea! Will it be OK with you to do it in a separate PR, only to make sure we get this one merged today if possible? (to have at least a couple of days of testing until the code freeze)

Copy link
Contributor

Choose a reason for hiding this comment

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

Yup, of course, I will finish testing very soon so that you can merge this one. @hypest

Copy link
Contributor

@marecar3 marecar3 left a comment

Choose a reason for hiding this comment

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

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

3 participants