Fix android copy paste menu doens't show up in compose box #7717

K4tsuki · 2022-02-12T14:11:22Z

Details

Fix copy paste context menu won't show up in android.
Change TextInput of compose box into uncontrolled TextInput that will improve performance of TextInput

Fixed Issues

$ #6876

Tests

Type something in compose box in Android
Select/highlight part of text
Verify that Copy / Paste menu context show up in Android
Set selection/cursor at certain part of text
Input an emoji
Verify emoji placement, cursor position and text change is correct after input an emoji

Verify that no errors appear in the JS console

QA Steps

Type something in compose box in Android
Select/highlight part of text
Verify that Copy / Paste menu context show up in Android
Set selection/cursor at certain part of text
Input an emoji
Verify emoji placement, cursor position and text change is correct after input an emoji

Verify that no errors appear in the JS console

Tested On

Screenshots

Web

web.mp4

Mobile Web

m_web.mp4

Desktop

desktop_compose.mp4

iOS

ios_compose.mp4

Android

android_cp.mp4

parasharrajat · 2022-02-12T14:30:41Z

src/pages/home/report/ReportActionCompose.js

        if (this.comment.length === 0 && newComment.length !== 0) {
+            if (this.state.isCommentEmpty) {
+                this.setState({isCommentEmpty: false});
+            }
            Report.setReportWithDraft(this.props.reportID.toString(), true);
        }

        // The draft has been deleted.
        if (newComment.length === 0) {
+            if (!this.state.isCommentEmpty) {
+                this.setState({isCommentEmpty: true});
+            }


I didn't understand this part. Also, I would like to ask that your original solution is very different from your proposal. Why so?

Because I realize updateCommnet is handler for onChangeText and put setNativePropse(text: ) in there isn't doing anything because when user type a character setNativeProps is being called even though the textInput is already changed by user type (you don't need setNativeProps there).

As for setState, each time user type a character setState is being called then ReportActionCompose is being rendered. I am guarding setState of isCommentEmpty so it is not being called unnecessarily.

https://stackoverflow.com/questions/24718709/reactjs-does-render-get-called-any-time-setstate-is-called

Here I am attaching performance improvement, the first compose box is build using developement build but it is faster than second compose box that is current production release.

ft.mp4

Ok. I think we can refactor this into a small function so that it does not look confusing.

setCommentEmpty(isEmpty){ if(this.state.isCommentEmpty === isEmpty}{ return; } this.setState({isCommentEmpty: isEmpty}); }

parasharrajat · 2022-02-12T14:33:47Z

src/pages/home/report/ReportActionCompose.js

+     * @param {Object} selection: {start: number, end: number}
+     */
+    setTextAndSelectionOfTextInput(newText, selection) {
+        if (Platform.OS === 'web') {


We don't use this. Check this.

Alright, I will try other way.

parasharrajat · 2022-02-12T14:57:12Z

src/pages/home/report/ReportActionCompose.js

+            // react-native-web
+            // because setNativeProps in react-native-web is slow and doesn't have good support
+            this.textInput.value = newText;
+            this.textInput.setSelectionRange(selection.start, selection.end);
+        } else {
+            this.textInput.setNativeProps({text: newText, selection});
+
+            // Relasing selection handling to system
+            setTimeout(() => this.textInput.setNativeProps({selection: {start: undefined, end: undefined}}), 0);
+        }


Let's discuss this first. I have a couple of questions.

why because setNativeProps in react-native-web is slow and doesn't have good support? Any reasons to support your comment.

Use of setTimeout. Why is this needed?

It is setTimeout 0, you can read more about it here:
https://stackoverflow.com/questions/779379/why-is-settimeoutfn-0-sometimes-useful
For this code it is to ensure that the previous setNativeProps is completed/done then do second setNativeProps.

As for setNativeProps for react-native-web, please wait.

And this is very confusing that you are first settings the selection and then releasing it.

If we don't do that, the selection will stay or come back at that position. Maybe will adds some comments.

parasharrajat

There is an issue with IOS. Where after adding the emoji the cursor is moved to the start of the text.

Please test it on IOS and Destop and add videos for those.

K4tsuki · 2022-02-18T09:50:16Z

Add dekstop and ios videos, and new commit to fix IOS issue.

K4tsuki · 2022-02-19T00:39:15Z

I am able to find another solution for IOS without second setNativeProps and without setTimeout / Promise resolve.

K4tsuki · 2022-02-19T09:49:38Z

@stitesExpensify for update, the setTimeout 0 could be replaced with Promise/resolve.

parasharrajat · 2022-02-21T18:39:58Z

I have raised some concerns on the slack. I will revisit this in some days.

parasharrajat · 2022-02-25T15:02:02Z

src/libs/TextInputUtils/index.ios.js

+* @param {String} text
+* @param {Object} selection: {start: number, end: number}
+*/
+function setTextAndSelection(textInputElement, text, selection) {


@stitesExpensify This function is implemented differently on Android|IOS|Web. What do you think about this imlementation?

I think I'm missing some context. Why does this need to be implemented in 3 different ways @K4tsuki ?

@stitesExpensify for the web, setNativeProps for selection doesn't work, because to set the selection of text input in web the property name is selectionStart and selectionEnd. So it has different name than native selection property.

For IOS, at first I am using android code, but there is a bug, like what @parasharrajat mention above.

There is an issue with IOS. Where after adding the emoji the cursor is moved to the start of the text.

The native behavior of setting selection using setNativeProps is different in IOS and android, so I am just adjusting the code based on native behavior of android and IOS.

Also the setTimeouts can be replaced by Promise-resolve.

@stitesExpensify Hope you had a nice vacation. Please share your thoughts on the PR. I have raised some review comments that need your attention. Thanks.

parasharrajat

Issue (Web): NumberLines is not updated which means the necessary events are not fired when we update the value via setTextAndSelection.

Steps:

Write a multiline comment for about 6 lines.
Select the text on lines 3-5.
add an emoji.
Notice that emoji is placed fine but there are extra lines on the text input.
But when we start typing the lines are recalc.

Lines should update when we update the value on input.

parasharrajat

Ok. The solution started to become messy and hacky. I will try to do some digging first. But We are just trying to patch in code to fix issues that are going to introduce more issues.

I would not suggest going too deep with platform-specific code.
As events are not firing, it means that we are doing it all wrong. Although we can put in all the vanilla js in react we should avoid it. Instead, do things react way.

Let me know if you have other ideas as well. I would hear them out first.

K4tsuki · 2022-03-08T01:49:04Z

@parasharrajat What is the ideal solution? Should I don't use platform specific code at all for this issue? and Should I don't use setNativeProps too?
The TextInputFocusable is using platform specific code, only in web version of TextInputFocusable it modifies numberOfLines of TextInput.
Should I move the TextInputUtils.setTextAndSelection logic into this component instead? So I just continuing what already exist of platform specific code and I don't create new platform specific code.
and what are prohibited codes / functions that I must avoid when making PR?

K4tsuki · 2022-03-09T09:41:16Z

@stitesExpensify What is the ideal solution here? Could I use my code inside TextInputFocusable, because the component have platform specific code, so I am not making new platform specific code, but I will use the existing component.

Also, Are setTimeout 0 and platform specific code strictly prohibited here?
Could you give me some advice please? thank you.

parasharrajat · 2022-03-09T10:26:12Z

I will give it a shot and see if something else can be done here. It is fine to use platform-specific code but this should only be used when there are no other choices. setNativeProps is necessary to use. If you remember the whole problem is that the menu does not show due to controlled prop.

Apart from it, the problem is not that we are using platform-specific code instead of the way it is used.

stitesExpensify · 2022-04-05T22:28:53Z

Closing this since K4tsuki is no longer a contributor

github-actions · 2022-04-05T22:29:09Z

CLA Assistant Lite bot:
Thank you for your submission, we really appreciate it. Like many open-source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution. You can sign the CLA by just posting a Pull Request Comment same as the below format.

I have read the CLA Document and I hereby sign the CLA

Katsuki seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
_{You can retrigger this bot by commenting recheck in this Pull Request}

Fix android copy paste menu doens't show up in compose box

0da21d2

K4tsuki requested a review from a team as a code owner February 12, 2022 14:11

MelvinBot requested review from parasharrajat and stitesExpensify and removed request for a team February 12, 2022 14:11

parasharrajat reviewed Feb 12, 2022

View reviewed changes

parasharrajat suggested changes Feb 13, 2022

View reviewed changes

Katsuki added 2 commits February 17, 2022 13:16

Merge branch 'main' into compose_android_context

0dbb3b4

Fix IOS selection and split setTextandSelection

68731ee

parasharrajat reviewed Feb 25, 2022

View reviewed changes

parasharrajat suggested changes Mar 6, 2022

View reviewed changes

Katsuki added 2 commits March 7, 2022 22:52

update row height in settextandselection.web

3561275

set ios settimeout

a6f8698

parasharrajat reviewed Mar 7, 2022

View reviewed changes

stitesExpensify closed this Apr 5, 2022

parasharrajat mentioned this pull request Aug 10, 2023

[Ready for payment via newdot][$16000] Android - Copy / Paste / Cut menu is not displayed when selecting text in compose box - Reported by: @parasharrajat #8349

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix android copy paste menu doens't show up in compose box #7717

Fix android copy paste menu doens't show up in compose box #7717

K4tsuki commented Feb 12, 2022 •

edited

Loading

parasharrajat Feb 12, 2022

K4tsuki Feb 12, 2022

parasharrajat Feb 12, 2022 •

edited

Loading

K4tsuki Feb 12, 2022

parasharrajat Feb 12, 2022

K4tsuki Feb 12, 2022

parasharrajat Feb 12, 2022

K4tsuki Feb 12, 2022 •

edited

Loading

parasharrajat Feb 12, 2022

K4tsuki Feb 12, 2022 •

edited

Loading

parasharrajat left a comment

K4tsuki commented Feb 18, 2022 •

edited

Loading

K4tsuki commented Feb 19, 2022

K4tsuki commented Feb 19, 2022

parasharrajat commented Feb 21, 2022

parasharrajat Feb 25, 2022

stitesExpensify Feb 25, 2022

K4tsuki Feb 26, 2022 •

edited

Loading

parasharrajat Mar 14, 2022

parasharrajat left a comment

parasharrajat left a comment •

edited

Loading

K4tsuki commented Mar 8, 2022 •

edited

Loading

K4tsuki commented Mar 9, 2022 •

edited

Loading

parasharrajat commented Mar 9, 2022 •

edited

Loading

stitesExpensify commented Apr 5, 2022

github-actions bot commented Apr 5, 2022

Fix android copy paste menu doens't show up in compose box #7717

Fix android copy paste menu doens't show up in compose box #7717

Conversation

K4tsuki commented Feb 12, 2022 • edited Loading

Details

Fixed Issues

Tests

QA Steps

Tested On

Screenshots

Web

Mobile Web

Desktop

iOS

Android

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parasharrajat Feb 12, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

K4tsuki Feb 12, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

K4tsuki Feb 12, 2022 • edited Loading

Choose a reason for hiding this comment

parasharrajat left a comment

Choose a reason for hiding this comment

K4tsuki commented Feb 18, 2022 • edited Loading

K4tsuki commented Feb 19, 2022

K4tsuki commented Feb 19, 2022

parasharrajat commented Feb 21, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

K4tsuki Feb 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

parasharrajat left a comment

Choose a reason for hiding this comment

parasharrajat left a comment • edited Loading

Choose a reason for hiding this comment

K4tsuki commented Mar 8, 2022 • edited Loading

K4tsuki commented Mar 9, 2022 • edited Loading

parasharrajat commented Mar 9, 2022 • edited Loading

stitesExpensify commented Apr 5, 2022

github-actions bot commented Apr 5, 2022

K4tsuki commented Feb 12, 2022 •

edited

Loading

parasharrajat Feb 12, 2022 •

edited

Loading

K4tsuki Feb 12, 2022 •

edited

Loading

K4tsuki Feb 12, 2022 •

edited

Loading

K4tsuki commented Feb 18, 2022 •

edited

Loading

K4tsuki Feb 26, 2022 •

edited

Loading

parasharrajat left a comment •

edited

Loading

K4tsuki commented Mar 8, 2022 •

edited

Loading

K4tsuki commented Mar 9, 2022 •

edited

Loading

parasharrajat commented Mar 9, 2022 •

edited

Loading