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

[IS-3186] Fixed cursor position issue on Android #3374

Merged
merged 6 commits into from
Jun 8, 2021

Conversation

aliabbasmalik8
Copy link
Contributor

@aliabbasmalik8 aliabbasmalik8 commented Jun 4, 2021

@jboniface

Details

This issue occurs only in the android platform so the solution is android specific. I used Direct Manipulation
to fixed this issue because selection props on textinput have an issue on native platforms https://github.com/facebook/react-native/issues/29063

Fixed Issues

Fixes #3186

Tests

  1. Type some text
  2. Add emoji in the middle of text
  3. Cursor position will display in front of the emoji

QA Steps

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

VIDEO: https://recordit.co/0uUXWucubT

Mobile Web

VIDEO: https://recordit.co/1Y0RirncEZ

Desktop

VIDEO: https://recordit.co/w6lrt5jZqy

iOS

VIDEO: https://recordit.co/vkL2JOqSCL

Android

Screenrecorder-2021-06-04-18-18-44-593_0_COMPRESSED.mp4

@aliabbasmalik8 aliabbasmalik8 requested a review from a team as a code owner June 4, 2021 13:34
@MelvinBot MelvinBot requested review from thienlnam and removed request for a team June 4, 2021 13:34
Copy link
Contributor

@thienlnam thienlnam left a comment

Choose a reason for hiding this comment

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

Thanks for making those changes, this is looking much better! Another thing you'll have to update is to change index.native.js of TextInputFocusable to index.ios.js since you have an android specific one now. I also have a question, what changes here are you making that are specific to android, and would ios break if they had the same changes in them? Because if not, since there is so much overlapping code in the ios.js and android.js files, it would be easier to maintain in the future if they were just both in native.js

@aliabbasmalik8
Copy link
Contributor Author

Thanks for making those changes, this is looking much better! Another thing you'll have to update is to change index.native.js of TextInputFocusable to index.ios.js since you have an android specific one now. I also have a question, what changes here are you making that are specific to android, and would ios break if they had the same changes in them? Because if not, since there is so much overlapping code in the ios.js and android.js files, it would be easier to maintain in the future if they were just both in native.js

TextInput Selection attribute break on IOS but I need it on Android so that's the reason I make a separate index.androd.js file

@thienlnam
Copy link
Contributor

TextInput Selection attribute break on IOS but I need it on Android so that's the reason I make a separate index.androd.js file

Do you mean the Selection prop isn't working on Android? Also, how is it breaking?

@aliabbasmalik8
Copy link
Contributor Author

aliabbasmalik8 commented Jun 8, 2021

Do you mean the Selection prop isn't working on Android? Also, how is it breaking?

Selection prop not working on ios and it's not using in the index.native.js file.
https://github.com/Expensify/Expensify.cash/blob/de1a438527e7d72241a3745516c7e66fd9de8321/src/components/TextInputFocusable/index.native.js#L69

But I need that prop in android to fix this issue.

@thienlnam
Copy link
Contributor

Ah I see what you mean now, although you'll still need to do this step:

Another thing you'll have to update is to change index.native.js of TextInputFocusable to index.ios.js since you have an android specific one now

Additionally, at the top of the iOS / Android file can you explain why they need to be separated and link to the react native issue where this problem is being tracked?

@aliabbasmalik8
Copy link
Contributor Author

Ah I see what you mean now, although you'll still need to do this step:

Another thing you'll have to update is to change index.native.js of TextInputFocusable to index.ios.js since you have an android specific one now

Additionally, at the top of the iOS / Android file can you explain why they need to be separated and link to the react native issue where this problem is being tracked?

DONE, Please have a look

@thienlnam thienlnam merged commit 350991b into Expensify:main Jun 8, 2021
@OSBotify
Copy link
Contributor

OSBotify commented Jun 8, 2021

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

@OSBotify
Copy link
Contributor

OSBotify commented Jun 8, 2021

🚀 Deployed to staging in version: 1.0.65-1🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@OSBotify
Copy link
Contributor

🚀 Deployed to production in version: 1.0.68-4🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@@ -233,6 +233,7 @@ class ReportActionCompose extends React.Component {
* @param {String} newComment
*/
updateComment(newComment) {
this.textInput.setNativeProps({text: newComment});
Copy link
Member

Choose a reason for hiding this comment

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

@aliabbasmalik8 Sorry you ping you this way but I just want to know why did you add this? I am doing some changes to the component and I just want to know was there an issue, that required this. It's pretty urgent we have a couple of deploy blockers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

t's used to update comments ASAP because in android there is some issue with the selection position when we update without this.
What issue you are facing?

Copy link
Member

Choose a reason for hiding this comment

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

There are multiple things I am working on so I was trying to understand the old code. There is an issue when the cursor jumps. There is an issue when the app crashes......so on.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it, this code working perfectly and tested as well. you can mention PR here on which you are working so can help.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm I could be wrong, but this does not seem like a correct usage of setNativeProps()... seems like there are various ways we clear the text input and I'm having a really hard time understanding why... please leave more context in the code in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, we can use some other ways as well but you can see in some other issue conversation reviewer like the way of direct manipulation because uncontrolled input used in the app.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Android - Emoji - Cursor is not in front of the emoji
5 participants