Expensify / App

Welcome to New Expensify: a complete re-imagination of financial collaboration, centered around chat. Help us build the next generation of Expensify by sharing feedback and contributing to the code.
https://new.expensify.com
MIT License
2.99k stars 2.5k forks source link

[HOLD setSelection refactor] [$2000] Android - Cursor is placed in wrong position after write/paste the emoji between the word Hello and nice - Reported by: @AndreasBBS #12854

Closed kbecciv closed 7 months ago

kbecciv commented 1 year ago

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


Issue found when executing PR https://github.com/Expensify/App/pull/12632

Action Performed:

  1. Launch the app
  2. Log in with any account
  3. Go to any chat
  4. Start with a string Hello nice to meet you
  5. Write the emoji :wave: in the string Hello nice to meet you between the word Hello and nice
  6. Start with a string Hello nice to meet you
  7. Paste the string :wave: friend in the string Hello nice to meet you between the word Hello and nice

Expected Result:

Describe what you think should've happened

Actual Result:

The resulting string has the emoji converted and the cursor is placed after the word friend/ or after the emoji

Workaround:

Unknown

Platform:

Where is this issue occurring?

Version Number: 1.2.29.2

Reproducible in staging?: Yes

Reproducible in production?: No

Email or phone of affected tester (no customers):

Logs: https://stackoverflow.com/c/expensify/questions/4856

Notes/Photos/Videos: Any additional supporting documentation

https://user-images.githubusercontent.com/93399543/202586022-096c2294-9027-4a31-baf7-d96fc7c5dbec.mp4

Expensify/Expensify Issue URL:

Issue reported by: Applause - Internal Team

Slack conversation:

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~0134ac93ef23b19fbf
  • Upwork Job ID: 1594665437635485696
  • Last Price Increase: 2022-12-16
melvin-bot[bot] commented 1 year ago

Triggered auto assignment to @dylanexpensify (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

rushatgabhane commented 1 year ago

This is a known bug and reported by @AndreasBBS over here

@dylanexpensify could you please add them as the reporter so that they get reporting bonus, thank you!

roryabraham commented 1 year ago

Can we please add some automated UI tests to cover this behavior with the next PR to fix this?

rushatgabhane commented 1 year ago

@roryabraham if i remb correctly, this issue is due to poor performance and not correctness

Can we please add some automated UI tests

sure!

AndreasBBS commented 1 year ago

@roryabraham You can check my analysis of this problem here. In my opinion is something that needs to be solved at the level of the react-native library, the onSelectionChange has erratic behavior on Android.

melvin-bot[bot] commented 1 year ago

@dylanexpensify Whoops! This issue is 2 days overdue. Let's get this updated quick!

dylanexpensify commented 1 year ago

reviewing today!

dylanexpensify commented 1 year ago

@AndreasBBS added you to title for reporting.

Upwork job: https://www.upwork.com/jobs/~013f07f27493ef4401

melvin-bot[bot] commented 1 year ago

Current assignee @dylanexpensify is eligible for the External assigner, not assigning anyone new.

melvin-bot[bot] commented 1 year ago

Job added to Upwork: https://www.upwork.com/jobs/~0134ac93ef23b19fbf

melvin-bot[bot] commented 1 year ago

Triggered auto assignment to Contributor-plus team member for initial proposal review - @mollfpr (External)

melvin-bot[bot] commented 1 year ago

Triggered auto assignment to @MariaHCD (External), see https://stackoverflow.com/c/expensify/questions/7972 for more details.

Puneet-here commented 1 year ago

Hi, I had reported this bug here.

s77rt commented 1 year ago

Proposal

diff --git a/src/components/Composer/index.android.js b/src/components/Composer/index.android.js
index 603bdf829..83dc6be58 100644
--- a/src/components/Composer/index.android.js
+++ b/src/components/Composer/index.android.js
@@ -24,12 +24,6 @@ const propTypes = {
     /** Prevent edits and interactions like focus for this input. */
     isDisabled: PropTypes.bool,

-    /** Selection Object */
-    selection: PropTypes.shape({
-        start: PropTypes.number,
-        end: PropTypes.number,
-    }),
-
     /** Whether the full composer can be opened */
     isFullComposerAvailable: PropTypes.bool,

@@ -51,10 +45,6 @@ const defaultProps = {
     autoFocus: false,
     isDisabled: false,
     forwardedRef: null,
-    selection: {
-        start: 0,
-        end: 0,
-    },
     isFullComposerAvailable: false,
     setIsFullComposerAvailable: () => {},
     isComposerFullSize: false,
diff --git a/src/components/Composer/index.ios.js b/src/components/Composer/index.ios.js
index b31a5b462..ee3abf32b 100644
--- a/src/components/Composer/index.ios.js
+++ b/src/components/Composer/index.ios.js
@@ -24,12 +24,6 @@ const propTypes = {
     /** Prevent edits and interactions like focus for this input. */
     isDisabled: PropTypes.bool,

-    /** Selection Object */
-    selection: PropTypes.shape({
-        start: PropTypes.number,
-        end: PropTypes.number,
-    }),
-
     /** Whether the full composer can be opened */
     isFullComposerAvailable: PropTypes.bool,

@@ -51,10 +45,6 @@ const defaultProps = {
     autoFocus: false,
     isDisabled: false,
     forwardedRef: null,
-    selection: {
-        start: 0,
-        end: 0,
-    },
     isFullComposerAvailable: false,
     setIsFullComposerAvailable: () => {},
     isComposerFullSize: false,
@@ -94,9 +84,6 @@ class Composer extends React.Component {
     render() {
         // On native layers we like to have the Text Input not focused so the
         // user can read new chats without the keyboard in the way of the view.
-        // On Android, the selection prop is required on the TextInput but this prop has issues on IOS
-        // https://github.com/facebook/react-native/issues/29063
-        const propsToPass = _.omit(this.props, 'selection');
         return (
             <RNTextInput
                 autoComplete="off"
@@ -108,7 +95,7 @@ class Composer extends React.Component {
                 textAlignVertical="center"
                 style={this.state.propStyles}
                 /* eslint-disable-next-line react/jsx-props-no-spreading */
-                {...propsToPass}
+                {...this.props}
                 editable={!this.props.isDisabled}
             />
         );
diff --git a/src/pages/home/report/ReportActionCompose.js b/src/pages/home/report/ReportActionCompose.js
index 2335864f9..fbc7ca93a 100644
--- a/src/pages/home/report/ReportActionCompose.js
+++ b/src/pages/home/report/ReportActionCompose.js
@@ -47,6 +47,7 @@ import ExceededCommentLength from '../../../components/ExceededCommentLength';
 import withNavigationFocus from '../../../components/withNavigationFocus';
 import * as EmojiUtils from '../../../libs/EmojiUtils';
 import reportPropTypes from '../../reportPropTypes';
+import setSelection from '../../../libs/setSelection';

 const propTypes = {
     /** Beta features list */
@@ -323,13 +324,11 @@ class ReportActionCompose extends React.Component {
         const emojiWithSpace = `${emoji} `;
         const newComment = this.comment.slice(0, this.state.selection.start)
             + emojiWithSpace + this.comment.slice(this.state.selection.end, this.comment.length);
-        this.setState(prevState => ({
-            selection: {
-                start: prevState.selection.start + emojiWithSpace.length,
-                end: prevState.selection.start + emojiWithSpace.length,
-            },
-        }));
-        this.updateComment(newComment);
+        const newSelection = {
+            start: this.state.selection.start + emojiWithSpace.length,
+            end: this.state.selection.start + emojiWithSpace.length,
+        };
+        this.updateComment(newComment, false, newSelection);
     }

     /**
@@ -381,21 +380,19 @@ class ReportActionCompose extends React.Component {
      * @param {String} comment
      * @param {Boolean} shouldDebounceSaveComment
      */
-    updateComment(comment, shouldDebounceSaveComment) {
+    updateComment(comment, shouldDebounceSaveComment, selection) {
+        const oldComment = this.state.value;
         const newComment = EmojiUtils.replaceEmojis(comment);
-        this.setState((prevState) => {
-            const newState = {
-                isCommentEmpty: !!newComment.match(/^(\s|`)*$/),
-                value: newComment,
-            };
-            if (comment !== newComment) {
-                const remainder = prevState.value.slice(prevState.selection.end).length;
-                newState.selection = {
-                    start: newComment.length - remainder,
-                    end: newComment.length - remainder,
-                };
+        this.setState({
+            isCommentEmpty: !!newComment.match(/^(\s|`)*$/),
+            value: newComment,
+        }, () => {
+            if (selection) {
+                setSelection(this.textInput, selection.start, selection.end);
+            } else if (newComment !== comment) {
+                const remainder = oldComment.slice(this.state.selection.end).length;
+                setSelection(this.textInput, newComment.length - remainder, newComment.length - remainder);
             }
-            return newState;
         });

         // Indicate that draft has been created.
@@ -669,7 +666,6 @@ class ReportActionCompose extends React.Component {
                                         shouldClear={this.state.textInputShouldClear}
                                         onClear={() => this.setTextInputShouldClear(false)}
                                         isDisabled={isComposeDisabled || isBlockedFromConcierge}
-                                        selection={this.state.selection}
                                         onSelectionChange={this.onSelectionChange}
                                         isFullComposerAvailable={this.state.isFullComposerAvailable}
                                         setIsFullComposerAvailable={this.setIsFullComposerAvailable}
diff --git a/src/pages/home/report/ReportActionItemMessageEdit.js b/src/pages/home/report/ReportActionItemMessageEdit.js
index 2f7fbc29d..92a53bee8 100644
--- a/src/pages/home/report/ReportActionItemMessageEdit.js
+++ b/src/pages/home/report/ReportActionItemMessageEdit.js
@@ -22,6 +22,7 @@ import * as EmojiUtils from '../../../libs/EmojiUtils';
 import reportPropTypes from '../../reportPropTypes';
 import ExceededCommentLength from '../../../components/ExceededCommentLength';
 import CONST from '../../../CONST';
+import setSelection from '../../../libs/setSelection';

 const propTypes = {
     /** All the data of the action */
@@ -100,18 +101,18 @@ class ReportActionItemMessageEdit extends React.Component {
      *
      * @param {String} draft
      */
-    updateDraft(draft) {
+    updateDraft(draft, selection) {
+        const oldDraft = this.state.draft;
         const newDraft = EmojiUtils.replaceEmojis(draft);
-        this.setState((prevState) => {
-            const newState = {draft: newDraft};
-            if (draft !== newDraft) {
-                const remainder = prevState.draft.slice(prevState.selection.end).length;
-                newState.selection = {
-                    start: newDraft.length - remainder,
-                    end: newDraft.length - remainder,
-                };
+        this.setState({
+            draft: newDraft,
+        }, () => {
+            if (selection) {
+                setSelection(this.textInput, selection.start, selection.end);
+            } else if (newDraft !== draft) {
+                const remainder = oldDraft.slice(this.state.selection.end).length;
+                setSelection(this.textInput, newDraft.length - remainder, newDraft.length - remainder);
             }
-            return newState;
         });

         // This component is rendered only when draft is set to a non-empty string. In order to prevent component
@@ -180,13 +181,11 @@ class ReportActionItemMessageEdit extends React.Component {
         const emojiWithSpace = `${emoji} `;
         const newComment = this.state.draft.slice(0, this.state.selection.start)
             + emojiWithSpace + this.state.draft.slice(this.state.selection.end, this.state.draft.length);
-        this.setState(prevState => ({
-            selection: {
-                start: prevState.selection.start + emojiWithSpace.length,
-                end: prevState.selection.start + emojiWithSpace.length,
-            },
-        }));
-        this.updateDraft(newComment);
+        const newSelection = {
+            start: this.state.selection.start + emojiWithSpace.length,
+            end: this.state.selection.start + emojiWithSpace.length,
+        };
+        this.updateDraft(newComment, newSelection);
     }

     /**
@@ -243,7 +242,6 @@ class ReportActionItemMessageEdit extends React.Component {
                             this.setState({isFocused: false});
                             toggleReportActionComposeView(true, VirtualKeyboard.shouldAssumeIsOpen());
                         }}
-                        selection={this.state.selection}
                         onSelectionChange={this.onSelectionChange}
                     />
                     <View style={styles.editChatItemEmojiWrapper}>

Details

  1. Removed selection prop since it's not needed and it's also the main cause. We don't need to constantly control the selection just on updateComment, and in my proposal the update is done via setSelection lib.
melvin-bot[bot] commented 1 year ago

Looks like something related to react-navigation may have been mentioned in this issue discussion.

As a reminder, please make sure that all proposals are not workarounds and that any and all attempt to fix the issue holistically have been made before proceeding with a solution. Proposals to change our DeprecatedCustomActions.js files should not be accepted.

Feel free to drop a note in #expensify-open-source with any questions.

mollfpr commented 1 year ago

@s77rt Thanks for the proposal!

Could you attach the result of your proposal?

s77rt commented 1 year ago

@mollfpr

https://user-images.githubusercontent.com/16493223/203107825-612c6615-455d-427a-9059-6e9f537418ff.mp4

mollfpr commented 1 year ago

I’ll start reviewing it in the morning, thanks!

grgia commented 1 year ago

@mollfpr I believe this was fixed by https://github.com/Expensify/App/pull/12632

mollfpr commented 1 year ago

Thanks, @grgia!

This issue is from that PR. You can check the comment here https://github.com/Expensify/App/issues/12854#issuecomment-1319571877

grgia commented 1 year ago

oops my bad! We should remove the Help Wanted label when an issue is being worked on

mollfpr commented 1 year ago

Thanks for the reminder! We are still in the middle of accepting proposals here.

mollfpr commented 1 year ago

Need more time to review this...

Pujan92 commented 1 year ago

Proposal

Find the last index of the unmatched character in comment and newComment string, that index should be the selectionIndex/cursor index.

https://github.com/Expensify/App/blob/532787af6185b463ae040ae3429fa54cb00d21c0/src/pages/home/report/ReportActionCompose.js#L391-L397

if (comment !== newComment) {
   const index = this.getLastUnmatchedIndex(comment, newComment);
   newState.selection = {
        start: index,
        end: index,
    };
}

New Function

   getLastUnmatchedIndex(text, modifiedText) {
        for (let i = text.length - 1, j = modifiedText.length - 1; i >= 0; i--, j--) {
            if (text[i] !== modifiedText[j]) {
                return j;
            }
        }
    }

Working Example 43a2604c-8abf-4441-ac7d-c36cfbd13d22.webm

Karim-30 commented 1 year ago

Proposal

I'm so sorry for this bug, I know it should be covered in my earlier PR.

Anyway, we should make the replaceEmojis function smarter enough to determine the writing cursor position not only replacing the :emoji_name: with its emoji, so it should return the newComment + cursorPosition.

Code :

In /src/libs/EmojiUtils.js :

/**
 * Replace any emoji name in a text with the emoji icon
 * @param {String} text
 * @param {Object} selection
 * @returns {Object}
 */
function replaceEmojis(text, selection) {
    let newText = text;
    let cursorPosition;
    const emojiData = text.match(CONST.REGEX.EMOJI_NAME);
    if (!emojiData || emojiData.length === 0) {
        return {newComment: text, selection};
    }
    for (let i = 0; i < emojiData.length; i++) {
        const checkEmoji = emojisTrie.search(emojiData[i].slice(1, -1));
        if (checkEmoji && checkEmoji.metaData.code) {
            cursorPosition = newText.search(emojiData[i]) + 2;
            newText = newText.replace(emojiData[i], checkEmoji.metaData.code);
        }
    }
    return {newComment: newText, selection: {start: cursorPosition, end: cursorPosition}};
}

In /src/pages/home/report/ReportActionCompose.js :

    updateComment(comment, shouldDebounceSaveComment) {
        const {newComment, selection} = EmojiUtils.replaceEmojis(comment, this.state.selection);
        this.setState({
            isCommentEmpty: !!newComment.match(/^(\s|`)*$/),
            value: newComment,
            selection,
        });

in /src/pages/home/report/ReportActionItemMessageEdit.js :

    updateDraft(draft) {
        const {newComment: newDraft, selection} = EmojiUtils.replaceEmojis(draft, this.state.selection);
        this.setState({
            draft: newDraft,
            selection,
        });

Result:

https://user-images.githubusercontent.com/108357004/203421699-96a60a85-a8ea-49d8-b3e8-f37a39fb6272.mov

PLEASE IGNORE THIS PROPOSAL IT WILL NOT WORK ON ANDROID

mvtglobally commented 1 year ago

Issue not reproducible during KI retests. (First week)

AndreasBBS commented 1 year ago

@Karim-30 @Pujan92 This issue was solved in all platforms except Android. I don't think the proposed solutions will address the problem in Android but if you think it does please post a video of the test in the Android app which is the only platform where this bug is present.

@s77rt approach seems to me the correct one and the one I advised for. Removing the selection prop avoids the onSelectionChange erratic behavior. I'd like to ask you if there's a specific reason you kept the onSelectionChange functionality on your proposal?

s77rt commented 1 year ago

@AndreasBBS onSelectionChange is kept because we need to keep a track of the current cursor position so we know how to act.

AndreasBBS commented 1 year ago

@s77rt Yeah you're right thanks for clarifying. I like your solution, good job.

Pujan92 commented 1 year ago

@Karim-30 @Pujan92 This issue was solved in all platforms except Android. I don't think the proposed solutions will address the problem in Android but if you think it does please post a video of the test in the Android app which is the only platform where this bug is present.

@AndreasBBS I missed reading the platform in the description, my solution isn't for android specific and not working in android too. So that proposal needs to be ignored.

Karim-30 commented 1 year ago

This issue was solved in all platforms except Android.

@AndreasBBS Well done. I using an older version of ExpensifyApp so I didn't see the new code.

AndreasBBS commented 1 year ago

@Karim-30

When developing for the Android platform you can run on an emulator with npm run android. This will build the App for Android with your current changes, spin up an emulator and get it running in an emulator. Then you need to run npx react-native start --port=8083 to connect the App that's running in the emulator. It helps if you have Android Studio installed because it installs all the relevant dependencies for the Android Emulators. If you need help message me on slack, U049U570QPM.

mollfpr commented 1 year ago

@s77rt I just try your proposal, the result looks fine on Android, but not on iOS. When typing the :wave: emoji between text, the cursor should be placed next to the emoji instead the next letter.

https://user-images.githubusercontent.com/25520267/203613223-47cdc778-9ed1-4f71-b7d5-0545f1cbd4b5.mov

Could you check this and update your proposal? Thanks!

mollfpr commented 1 year ago

@Pujan92 Could you attach the result for the Android and iOS? Thanks!

s77rt commented 1 year ago

That's another bug in react-native https://github.com/facebook/react-native/issues/28865 @mollfpr Should we make this platform specific?

s77rt commented 1 year ago

@mollfpr If platform-specific component is not needed here; we can on the same file:

  1. Use two state values: selection and prevSelection
  2. On updateComment we use this.state.prevSelection for iOS since the intended selection was already changed and this.state.selection for the rest.

on iOS the events callback are invested, the first call is onSelectionChange and then onChangeText

mollfpr commented 1 year ago

Checking again…

Pujan92 commented 1 year ago

@Pujan92 Could you attach the result for the Android and iOS? Thanks!

I have a hacky way which isn't the preferred way but it can work for android too. The issue we are facing in the android is onSelectionChange call which executes after we set the selection state in the updateComment which causes infinite cursor flickering/positioning.

Hacky Solution Extra state property shouldAllowOnSelectionChange

In update emoji comment update the shouldAllowOnSelectionChange property https://github.com/Expensify/App/blob/8c86a77a69bef6f4939c4642e33ff413ab97be07/src/pages/home/report/ReportActionCompose.js#L391-L397 newState.shouldAllowOnSelectionChange = false;

In the case of emoji addition we won't update the selection state from onSelectionChange and apply the minor delay to avoid flickering.

https://github.com/Expensify/App/blob/8c86a77a69bef6f4939c4642e33ff413ab97be07/src/pages/home/report/ReportActionCompose.js#L196-L198

   onSelectionChange(e) {
        if (this.state.shouldAllowOnSelectionChange) {
            this.setState({selection: e.nativeEvent.selection});
        } else {
            setTimeout(() => {
                this.setState({shouldAllowOnSelectionChange: true});
            }, 100);
        }
    }
mollfpr commented 1 year ago

@s77rt Sorry for the wait. How about we create a function lib similar to setSelection to set the selection for composer input? Like setComposerSelection and return empty on iOS?

@Pujan92 Thanks for the update! We avoid using setTimeout for a solution.

s77rt commented 1 year ago

@mollfpr I don't see the point of this? We do need to keep a track of the previous selection The new selection is based on the old selection.

The platform specific code is only one line maybe we can make an exception for it: const caretPosition = Platform.OS === "ios" ? {...this.state.prevSelection} : {...this.state.selection};

mollfpr commented 1 year ago

@s77rt Could you put the solution into the full proposal? I'm still not convinced with the usage of this.state.prevSelection.

s77rt commented 1 year ago

@mollfpr on iOS the selection on this.state.selection gets updated before calling the updateComment that's why we need to store another selection (prevSelection)

will post the updated proposal in few mins

s77rt commented 1 year ago

Proposal (Updated)

diff --git a/src/components/Composer/index.android.js b/src/components/Composer/index.android.js
index 603bdf829e..83dc6be587 100644
--- a/src/components/Composer/index.android.js
+++ b/src/components/Composer/index.android.js
@@ -24,12 +24,6 @@ const propTypes = {
     /** Prevent edits and interactions like focus for this input. */
     isDisabled: PropTypes.bool,

-    /** Selection Object */
-    selection: PropTypes.shape({
-        start: PropTypes.number,
-        end: PropTypes.number,
-    }),
-
     /** Whether the full composer can be opened */
     isFullComposerAvailable: PropTypes.bool,

@@ -51,10 +45,6 @@ const defaultProps = {
     autoFocus: false,
     isDisabled: false,
     forwardedRef: null,
-    selection: {
-        start: 0,
-        end: 0,
-    },
     isFullComposerAvailable: false,
     setIsFullComposerAvailable: () => {},
     isComposerFullSize: false,
diff --git a/src/components/Composer/index.ios.js b/src/components/Composer/index.ios.js
index b31a5b462f..ee3abf32b3 100644
--- a/src/components/Composer/index.ios.js
+++ b/src/components/Composer/index.ios.js
@@ -24,12 +24,6 @@ const propTypes = {
     /** Prevent edits and interactions like focus for this input. */
     isDisabled: PropTypes.bool,

-    /** Selection Object */
-    selection: PropTypes.shape({
-        start: PropTypes.number,
-        end: PropTypes.number,
-    }),
-
     /** Whether the full composer can be opened */
     isFullComposerAvailable: PropTypes.bool,

@@ -51,10 +45,6 @@ const defaultProps = {
     autoFocus: false,
     isDisabled: false,
     forwardedRef: null,
-    selection: {
-        start: 0,
-        end: 0,
-    },
     isFullComposerAvailable: false,
     setIsFullComposerAvailable: () => {},
     isComposerFullSize: false,
@@ -94,9 +84,6 @@ class Composer extends React.Component {
     render() {
         // On native layers we like to have the Text Input not focused so the
         // user can read new chats without the keyboard in the way of the view.
-        // On Android, the selection prop is required on the TextInput but this prop has issues on IOS
-        // https://github.com/facebook/react-native/issues/29063
-        const propsToPass = _.omit(this.props, 'selection');
         return (
             <RNTextInput
                 autoComplete="off"
@@ -108,7 +95,7 @@ class Composer extends React.Component {
                 textAlignVertical="center"
                 style={this.state.propStyles}
                 /* eslint-disable-next-line react/jsx-props-no-spreading */
-                {...propsToPass}
+                {...this.props}
                 editable={!this.props.isDisabled}
             />
         );
diff --git a/src/pages/home/report/ReportActionCompose.js b/src/pages/home/report/ReportActionCompose.js
index 2335864f91..90c571e009 100644
--- a/src/pages/home/report/ReportActionCompose.js
+++ b/src/pages/home/report/ReportActionCompose.js
@@ -4,6 +4,7 @@ import {
     View,
     TouchableOpacity,
     InteractionManager,
+    Platform,
 } from 'react-native';
 import _ from 'underscore';
 import lodashGet from 'lodash/get';
@@ -47,6 +48,7 @@ import ExceededCommentLength from '../../../components/ExceededCommentLength';
 import withNavigationFocus from '../../../components/withNavigationFocus';
 import * as EmojiUtils from '../../../libs/EmojiUtils';
 import reportPropTypes from '../../reportPropTypes';
+import setSelection from '../../../libs/setSelection';

 const propTypes = {
     /** Beta features list */
@@ -142,6 +144,10 @@ class ReportActionCompose extends React.Component {
                 start: props.comment.length,
                 end: props.comment.length,
             },
+            prevSelection: {
+                start: props.comment.length,
+                end: props.comment.length,
+            },
             maxLines: props.isSmallScreenWidth ? CONST.COMPOSER.MAX_LINES_SMALL_SCREEN : CONST.COMPOSER.MAX_LINES,
             value: props.comment,

@@ -194,7 +200,12 @@ class ReportActionCompose extends React.Component {
     }

     onSelectionChange(e) {
-        this.setState({selection: e.nativeEvent.selection});
+        this.setState(prevState => {
+            return {
+                selection: e.nativeEvent.selection,
+                prevSelection: {...prevState.selection},
+            };
+        });
     }

     /**
@@ -323,13 +334,11 @@ class ReportActionCompose extends React.Component {
         const emojiWithSpace = `${emoji} `;
         const newComment = this.comment.slice(0, this.state.selection.start)
             + emojiWithSpace + this.comment.slice(this.state.selection.end, this.comment.length);
-        this.setState(prevState => ({
-            selection: {
-                start: prevState.selection.start + emojiWithSpace.length,
-                end: prevState.selection.start + emojiWithSpace.length,
-            },
-        }));
-        this.updateComment(newComment);
+        const newSelection = {
+            start: this.state.selection.start + emojiWithSpace.length,
+            end: this.state.selection.start + emojiWithSpace.length,
+        };
+        this.updateComment(newComment, false, newSelection);
     }

     /**
@@ -381,21 +390,24 @@ class ReportActionCompose extends React.Component {
      * @param {String} comment
      * @param {Boolean} shouldDebounceSaveComment
      */
-    updateComment(comment, shouldDebounceSaveComment) {
+    updateComment(comment, shouldDebounceSaveComment, selection) {
+        // We require the caret position just before updating the comment
+        // on iOS the position gets updated before calling the onChangeText callback (updateComment)
+        // i.e. onSelectionChange => onChangeText. Thus the intended selection is overwritten already that's why we use prevSelection
+        // on the rest of the platforms the callback flow is onChangeText => onSelectionChange. The intended selection is still saved on state.
+        const caretPosition = Platform.OS === "ios" ? {...this.state.prevSelection} : {...this.state.selection};
+        const oldComment = this.state.value;
         const newComment = EmojiUtils.replaceEmojis(comment);
-        this.setState((prevState) => {
-            const newState = {
-                isCommentEmpty: !!newComment.match(/^(\s|`)*$/),
-                value: newComment,
-            };
-            if (comment !== newComment) {
-                const remainder = prevState.value.slice(prevState.selection.end).length;
-                newState.selection = {
-                    start: newComment.length - remainder,
-                    end: newComment.length - remainder,
-                };
+        this.setState({
+            isCommentEmpty: !!newComment.match(/^(\s|`)*$/),
+            value: newComment,
+        }, () => {
+            if (selection) {
+                setSelection(this.textInput, selection.start, selection.end);
+            } else if (newComment !== comment) {
+                const remainder = oldComment.slice(caretPosition.end).length;
+                setSelection(this.textInput, newComment.length - remainder, newComment.length - remainder);
             }
-            return newState;
         });

         // Indicate that draft has been created.
@@ -669,7 +681,6 @@ class ReportActionCompose extends React.Component {
                                         shouldClear={this.state.textInputShouldClear}
                                         onClear={() => this.setTextInputShouldClear(false)}
                                         isDisabled={isComposeDisabled || isBlockedFromConcierge}
-                                        selection={this.state.selection}
                                         onSelectionChange={this.onSelectionChange}
                                         isFullComposerAvailable={this.state.isFullComposerAvailable}
                                         setIsFullComposerAvailable={this.setIsFullComposerAvailable}
diff --git a/src/pages/home/report/ReportActionItemMessageEdit.js b/src/pages/home/report/ReportActionItemMessageEdit.js
index 2f7fbc29db..5a41f6fec0 100644
--- a/src/pages/home/report/ReportActionItemMessageEdit.js
+++ b/src/pages/home/report/ReportActionItemMessageEdit.js
@@ -1,6 +1,6 @@
 import lodashGet from 'lodash/get';
 import React from 'react';
-import {InteractionManager, View} from 'react-native';
+import {InteractionManager, View, Platform} from 'react-native';
 import PropTypes from 'prop-types';
 import _ from 'underscore';
 import ExpensiMark from 'expensify-common/lib/ExpensiMark';
@@ -22,6 +22,7 @@ import * as EmojiUtils from '../../../libs/EmojiUtils';
 import reportPropTypes from '../../reportPropTypes';
 import ExceededCommentLength from '../../../components/ExceededCommentLength';
 import CONST from '../../../CONST';
+import setSelection from '../../../libs/setSelection';

 const propTypes = {
     /** All the data of the action */
@@ -82,6 +83,10 @@ class ReportActionItemMessageEdit extends React.Component {
                 start: draftMessage.length,
                 end: draftMessage.length,
             },
+            prevSelection: {
+                start: draftMessage.length,
+                end: draftMessage.length,
+            },
             isFocused: false,
         };
     }
@@ -92,7 +97,12 @@ class ReportActionItemMessageEdit extends React.Component {
      * @param {Event} e
      */
     onSelectionChange(e) {
-        this.setState({selection: e.nativeEvent.selection});
+        this.setState(prevState => {
+            return {
+                selection: e.nativeEvent.selection,
+                prevSelection: {...prevState.selection},
+            };
+        });
     }

     /**
@@ -100,18 +110,23 @@ class ReportActionItemMessageEdit extends React.Component {
      *
      * @param {String} draft
      */
-    updateDraft(draft) {
+    updateDraft(draft, selection) {
+        // We require the caret position just before updating the comment
+        // on iOS the position gets updated before calling the onChangeText callback (updateComment)
+        // i.e. onSelectionChange => onChangeText. Thus the intended selection is overwritten already that's why we use prevSelection
+        // on the rest of the platforms the callback flow is onChangeText => onSelectionChange. The intended selection is still saved on state.
+        const caretPosition = Platform.OS === "ios" ? {...this.state.prevSelection} : {...this.state.selection};
+        const oldDraft = this.state.draft;
         const newDraft = EmojiUtils.replaceEmojis(draft);
-        this.setState((prevState) => {
-            const newState = {draft: newDraft};
-            if (draft !== newDraft) {
-                const remainder = prevState.draft.slice(prevState.selection.end).length;
-                newState.selection = {
-                    start: newDraft.length - remainder,
-                    end: newDraft.length - remainder,
-                };
+        this.setState({
+            draft: newDraft,
+        }, () => {
+            if (selection) {
+                setSelection(this.textInput, selection.start, selection.end);
+            } else if (newDraft !== draft) {
+                const remainder = oldDraft.slice(caretPosition.end).length;
+                setSelection(this.textInput, newDraft.length - remainder, newDraft.length - remainder);
             }
-            return newState;
         });

         // This component is rendered only when draft is set to a non-empty string. In order to prevent component
@@ -180,13 +195,11 @@ class ReportActionItemMessageEdit extends React.Component {
         const emojiWithSpace = `${emoji} `;
         const newComment = this.state.draft.slice(0, this.state.selection.start)
             + emojiWithSpace + this.state.draft.slice(this.state.selection.end, this.state.draft.length);
-        this.setState(prevState => ({
-            selection: {
-                start: prevState.selection.start + emojiWithSpace.length,
-                end: prevState.selection.start + emojiWithSpace.length,
-            },
-        }));
-        this.updateDraft(newComment);
+        const newSelection = {
+            start: this.state.selection.start + emojiWithSpace.length,
+            end: this.state.selection.start + emojiWithSpace.length,
+        };
+        this.updateDraft(newComment, newSelection);
     }

     /**
@@ -243,7 +256,6 @@ class ReportActionItemMessageEdit extends React.Component {
                             this.setState({isFocused: false});
                             toggleReportActionComposeView(true, VirtualKeyboard.shouldAssumeIsOpen());
                         }}
-                        selection={this.state.selection}
                         onSelectionChange={this.onSelectionChange}
                     />
                     <View style={styles.editChatItemEmojiWrapper}>

Update

Support iOS

https://user-images.githubusercontent.com/16493223/204034172-391d43a8-29df-472d-aedc-b751b2efafe9.mp4

mollfpr commented 1 year ago

The current proposal from @s77rt is suggesting handling both input selections for iOS and Android. It's required to create 2 states for handling each iOS and Android input selection because this RN issue https://github.com/facebook/react-native/issues/28865.

Should we hold this until the issue on RN is fixed (I doubt it will be fixed in near future) or accepting a workaround for that?

cc @MariaHCD @dylanexpensify

dylanexpensify commented 1 year ago

Sorry, @rushatgabhane it looks like @Puneet-here noted he reported this bug earlier than @AndreasBBS did - (the 7th vs the 11th), if so, we'd issue Puneet the reporting bonus. Can you confirm?

rushatgabhane commented 1 year ago

wait, i don't have any context for this issue or the bug report.

im not sure how i can help

dylanexpensify commented 1 year ago

@rushatgabhane I'm referring to this comment!

rushatgabhane commented 1 year ago

@dylanexpensify thank you. @Puneet-here reported the bug earlier than @AndreasBBS

@Puneet-here reported the bug on Nov 8 over here https://expensify.slack.com/archives/C01GTK53T8Q/p1667850534873129?thread_ts=1667850534.873129&cid=C01GTK53T8Q

AndreasBBS commented 1 year ago

@dylanexpensify thank you. @Puneet-here reported the bug earlier than @AndreasBBS

@Puneet-here reported the bug on Nov 8 over here https://expensify.slack.com/archives/C01GTK53T8Q/p1667850534873129?thread_ts=1667850534.873129&cid=C01GTK53T8Q

I cannot open this slack archive but I only reported the bug on November 11 so I think @Puneet-here should get this one. Sorry for the confusion, I was not aware of this previous reporting.

dylanexpensify commented 1 year ago

@MariaHCD any thoughts on @mollfpr's comment?