[HOLD for payment 2022-12-02] [$250] Cursor moves to end after adding space in Room Name reported by @varshamb

kavimuru 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!

Action Performed:-

Navigate to Room
Click header to open details of Room
goto Room Settings
Add space in Room Name and observe the Cursor

Expected Result:

Cursor should not moves to end after adding space in Room Name.

Actual Result:

Cursor is moving to end after adding space in Room Name.

Workaround:

unknown

Platform:

Where is this issue occurring?

Web

Version Number: 1.2.27-4 Reproducible in staging?: y Reproducible in production?: y Email or phone of affected tester (no customers): Logs: https://stackoverflow.com/c/expensify/questions/4856 Notes/Photos/Videos:

https://user-images.githubusercontent.com/43996225/201992973-cbac3e76-33c7-4148-ada5-7537bfb73502.mov

https://user-images.githubusercontent.com/43996225/201993005-b88faa43-945f-4ef9-a0f6-8ae1ee620084.mp4

Expensify/Expensify Issue URL: Issue reported by: @varshamb Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1668484634763459

View all open jobs on GitHub

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~01b4bebba6c8508e8c
Upwork Job ID: 1593456078477520896

melvin-bot[bot] commented 1 year ago

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

Christinadobrzyn commented 1 year ago

[X] Can this be reproduced? Yes
I can reproduce this on the Production desktop app v 1.2.27-4 (1.2.27-4)
I can reproduce this on staging and mobile app staging v1.2.28-0
[X] Any other Ghs relate to this? No
I don't see any other GHs related to this issue
[X] Does this need to be fixed? I don't know
Thinking more about this, I'm not actually sure we need to change anything. When a space is added, it creates a _ so it's not like it doesn't work but the display is different than what a user might expect. But is there any reason to have a space in a room name? I'm not 100% sold on why this needs to be fixed. Going to assign the Exernal label to get more thoughts.

2022-11-16_10-53-32 (1)

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

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

Pujan92 commented 1 year ago

I can see it is the intentional behavior from the code, on room name change we are applying the modifyRoomName function to replace space(' ') with underscore('_') and etc. As we are replacing the string on each keystroke cursor will moves to the end only.

https://github.com/Expensify/App/blob/652a51ac2bfbe3164b807dc5efaa4e0156ae0bdf/src/components/RoomNameInput.js#L59-L67

varshamb commented 1 year ago

@thienlnam @Santhosh-Sellavel Is there any specific reason for not allowing Room Name with space?

Santhosh-Sellavel commented 1 year ago

I guess the room name should single word

thienlnam commented 1 year ago

Yeah, this is intentional - we could update it so that it keeps the cursor where the underscore was added. Though, similar to https://github.com/Expensify/App/issues/11146#issuecomment-1317198685 we're going close since it's related to keyboard navigation / accessibility

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.

s77rt commented 1 year ago

Proposal

diff --git a/src/components/RoomNameInput.js b/src/components/RoomNameInput.js
index 95e016cd6..07d025613 100644
--- a/src/components/RoomNameInput.js
+++ b/src/components/RoomNameInput.js
@@ -17,6 +17,15 @@ const propTypes = {
     /** Error text to show */
     errorText: PropTypes.string,

+    /** Update selection position on change */
+    onSelectionChange: PropTypes.func,
+
+    /** Selection Object */
+    selection: PropTypes.shape({
+        start: PropTypes.number,
+        end: PropTypes.number,
+    }),
+
     ...withLocalizePropTypes,

     /** A ref forwarded to the TextInput */
@@ -28,6 +37,11 @@ const defaultProps = {
     value: '',
     disabled: false,
     errorText: '',
+    onSelectionChange: () => {},
+    selection: {
+        start: 0,
+        end: 0,
+    },
     forwardedRef: () => {},
 };

@@ -46,8 +60,34 @@ class RoomNameInput extends Component {
         const roomName = event.nativeEvent.text;
         const modifiedRoomName = this.modifyRoomName(roomName);
         this.props.onChangeText(modifiedRoomName);
+
+        // If we made a modification to the room name, update the cursor position
+        // to avoid cursor jump behaviour
+        const roomNameWithHash = `${CONST.POLICY.ROOM_PREFIX}${roomName}`;
+        if (modifiedRoomName !== roomNameWithHash) {
+            const offset = roomNameWithHash.length - modifiedRoomName.length;
+            // customEvent must have the same signature as react-native TextInput onSelectionChange's argument
+            const customEvent = {
+                nativeEvent: {
+                    selection: {
+                        start: event.target.selectionStart - offset,
+                        end: event.target.selectionEnd - offset,
+                    },
+                },
+            };
+            this.setSelection(customEvent);
+        }
     }

+    /**
+     * Calls the onSelectionChange callback with the updated selection
+     * @param {Event} event
+     */
+    setSelection(event) {
+        this.props.onSelectionChange(event);
+    }
+
+
     /**
      * Modifies the room name to follow our conventions:
      * - Max length 80 characters
@@ -76,6 +116,8 @@ class RoomNameInput extends Component {
                 placeholder={this.props.translate('newRoomPage.social')}
                 onChange={this.setModifiedRoomName}
                 value={this.props.value.substring(1)} // Since the room name always starts with a prefix, we omit the first character to avoid displaying it twice.
+                selection={this.props.selection}
+                onSelectionChange={this.setSelection}
                 errorText={this.props.errorText}
                 autoCapitalize="none"
             />
diff --git a/src/pages/ReportSettingsPage.js b/src/pages/ReportSettingsPage.js
index 4eb29ef28..8fdf33c5f 100644
--- a/src/pages/ReportSettingsPage.js
+++ b/src/pages/ReportSettingsPage.js
@@ -71,6 +71,10 @@ class ReportSettingsPage extends Component {

         this.state = {
             newRoomName: this.props.report.reportName,
+            newRoomNameSelection: {
+                start: 0,
+                end: 0,
+            },
             errors: {},
         };

@@ -130,6 +134,16 @@ class ReportSettingsPage extends Component {
         }));
     }

+    /**
+     * @param {String} inputKey
+     * @param {Object} selection
+     */
+    setSelection(inputKey, selection) {
+        this.setState({
+            [`${inputKey}Selection`]: selection,
+        });
+    }
+
     render() {
         const shouldShowRoomName = !ReportUtils.isPolicyExpenseChat(this.props.report);
         const shouldDisableRename = ReportUtils.isDefaultRoom(this.props.report)
@@ -184,9 +198,11 @@ class ReportSettingsPage extends Component {
                                                 <RoomNameInput
                                                     ref={el => this.roomNameInputRef = el}
                                                     value={this.state.newRoomName}
+                                                    selection={this.state.newRoomNameSelection}
                                                     policyID={linkedWorkspace && linkedWorkspace.id}
                                                     errorText={this.state.errors.newRoomName}
                                                     onChangeText={newRoomName => this.clearErrorAndSetValue('newRoomName', newRoomName)}
+                                                    onSelectionChange={event => this.setSelection('newRoomName', event.nativeEvent.selection)}
                                                     disabled={shouldDisableRename}
                                                 />
                                             )}
diff --git a/src/pages/workspace/WorkspaceNewRoomPage.js b/src/pages/workspace/WorkspaceNewRoomPage.js
index 654e2ce8c..8bfc1fe75 100644
--- a/src/pages/workspace/WorkspaceNewRoomPage.js
+++ b/src/pages/workspace/WorkspaceNewRoomPage.js
@@ -49,6 +49,10 @@ class WorkspaceNewRoomPage extends React.Component {

         this.state = {
             roomName: '',
+            roomNameSelection: {
+                start: 0,
+                end: 0,
+            },
             policyID: '',
             visibility: CONST.REPORT.VISIBILITY.RESTRICTED,
             errors: {},
@@ -128,6 +132,16 @@ class WorkspaceNewRoomPage extends React.Component {
         }));
     }

+    /**
+     * @param {String} inputKey
+     * @param {Object} selection
+     */
+    setSelection(inputKey, selection) {
+        this.setState({
+            [`${inputKey}Selection`]: selection,
+        });
+    }
+
     focusRoomNameInput() {
         if (!this.roomNameInputRef) {
             return;
@@ -162,7 +176,9 @@ class WorkspaceNewRoomPage extends React.Component {
                             policyID={this.state.policyID}
                             errorText={this.state.errors.roomName}
                             onChangeText={roomName => this.clearErrorAndSetValue('roomName', roomName)}
+                            onSelectionChange={event => this.setSelection('roomName', event.nativeEvent.selection)}
                             value={this.state.roomName}
+                            selection={this.state.roomNameSelection}
                         />
                     </View>
                     <View style={styles.mb5}>

Details

Implemented selection and onSelectionChange on RoomNameInput

https://user-images.githubusercontent.com/16493223/202281306-ae9ae2eb-7f29-4b37-99b0-2b4aadf3d0a7.mp4

@thienlnam Sorry for the ping, can you consider reopening?

thienlnam commented 1 year ago

@s77rt I closed it since this is intentional and we're freezing accessibility changes from https://github.com/Expensify/App/issues/12741#issuecomment-1317545251. What is the reason for your change?

varshamb commented 1 year ago

@thienlnam the behavior of replacing space with _ is intentional. As cursor is moving to end after space, we should consider opening it.

s77rt commented 1 year ago

@thienlnam Replacing spaces with underscores is intentional. Moving the cursor to the end is not <-- my proposal fixes this.

thienlnam commented 1 year ago

Yeah that's a good point, @Santhosh-Sellavel could you please review the above proposal?

mountiny commented 1 year ago

Actually we might want to use dashes here https://expensify.slack.com/archives/C01GTK53T8Q/p1668113640697309

varshamb commented 1 year ago

@thienlnam

Yeah, this is intentional - we could update it so that it keeps the cursor where the underscore was added. Though, similar to #11146 (comment) we're going close since it's related to keyboard navigation / accessibility

I don't think this is accessibility issue. This bug is open for Cursor position.

Santhosh-Sellavel commented 1 year ago

@Christinadobrzyn Can you assign this to another C+, I can't get to this sooner.

cc: @thienlnam

melvin-bot[bot] commented 1 year ago

Current assignee @Christinadobrzyn 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/~01b4bebba6c8508e8c

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

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

mollfpr commented 1 year ago

@s77rt Can you add context for your proposal? Why do we need to handle the selection value in the ReportSettingsPage and WorkspaceNewRoomPage? Could handling the selection in RoomNameInput is enough?

I also couldn't find any details about the cause.

s77rt commented 1 year ago

@mollfpr If I remember correctly, I think it must be done that way otherwise the last change will occur at ReportSettingsPage and RoomNameInput via clearErrorAndSetValue this will change the value and automatically reset the cursor.

mollfpr commented 1 year ago

I just test this proposal and here's the review.

I got an error on native iOS and Android, so I guess we should create a platform-specific component for RoomNameInput and let the RN handle the selection (I didn't find any problem on native iOS or Android).

https://user-images.githubusercontent.com/25520267/202905879-7d565d43-52eb-4ba2-80c9-df16a9aab0fa.mov

Why we can't use the selection value from the current event? The offset itself always has a 0 value for me.

+            const offset = roomNameWithHash.length - modifiedRoomName.length;
+            // customEvent must have the same signature as react-native TextInput onSelectionChange's argument
+            const customEvent = {
+                nativeEvent: {
+                    selection: {
+                        start: event.target.selectionStart - offset,
+                        end: event.target.selectionEnd - offset,
+                    },
+                },
+            };

I don't think we need to handle the selection value separately in ReportSettingsPage or WorkspaceNewRoomPage. I did some tweaks to your proposal and that can solve this issue by only saving the selection value from onChange and passing it to the input.

I guess we can improve your proposal and fix this issue with small changes @s77rt

s77rt commented 1 year ago

Proposal (Updated)

diff --git a/src/components/RoomNameInput.js b/src/components/RoomNameInput.js
index 95e016cd6..ff7483a0d 100644
--- a/src/components/RoomNameInput.js
+++ b/src/components/RoomNameInput.js
@@ -36,6 +36,11 @@ class RoomNameInput extends Component {
         super(props);

         this.setModifiedRoomName = this.setModifiedRoomName.bind(this);
+        this.setSelection = this.setSelection.bind(this);
+
+        this.state = {
+            selection: {start: 0, end: 0},
+        };
     }

     /**
@@ -46,6 +51,32 @@ class RoomNameInput extends Component {
         const roomName = event.nativeEvent.text;
         const modifiedRoomName = this.modifyRoomName(roomName);
         this.props.onChangeText(modifiedRoomName);
+
+        // Prevent cursor jump behaviour:
+        // Check if newRoomNameWithHash is the same as modifiedRoomName
+        // If it is then the room name is valid (does not contain unallowed characters); no action required
+        // If not then the room name contains unvalid characters and we must adjust the cursor position manually
+        // Read more: https://github.com/Expensify/App/issues/12741
+        const oldRoomNameWithHash = this.props.value || '';
+        const newRoomNameWithHash = `${CONST.POLICY.ROOM_PREFIX}${roomName}`;
+        if (modifiedRoomName !== newRoomNameWithHash) {
+            const offset = modifiedRoomName.length - oldRoomNameWithHash.length;
+            const selection = {
+                start: this.state.selection.start + offset,
+                end: this.state.selection.end + offset,
+            }
+            this.setSelection(selection);
+        }
+    }
+
+    /**
+     * Set the selection
+     * @param {Object} selection
+     */
+    setSelection(selection) {
+        this.setState({
+            selection: selection,
+        });
     }

     /**
@@ -76,6 +107,8 @@ class RoomNameInput extends Component {
                 placeholder={this.props.translate('newRoomPage.social')}
                 onChange={this.setModifiedRoomName}
                 value={this.props.value.substring(1)} // Since the room name always starts with a prefix, we omit the first character to avoid displaying it twice.
+                selection={this.state.selection}
+                onSelectionChange={(event) => this.setSelection(event.nativeEvent.selection)}
                 errorText={this.props.errorText}
                 autoCapitalize="none"
             />

Update

The changes only effect the component itself RoomNameInput

@mollfpr Sorry I didn't test on native, fixed now. However it seems to be a little buggy, or maybe that's just due to simulator lag + dev mode?... Anyway, I guess native devices does not suffer from this issue so maybe we should apply this fix only for web? It works pretty smooth on web.

The offset is used to cover other cases such as special chars, enter a special char, the cursor will get moved without entering anything. Using offset fixes this (cursor won't move forward if you type a disallowed char)

We can't and we shouldn't save the selection from onChange (the selection is only available on web). Instead I'm saving the selection from onSelectionChange.

mollfpr commented 1 year ago

Thanks for the update @s77rt!

Overall your proposal works well on the web, mWeb, and desktop. 👍

I found an issue on native when I type a special char really fast continuously the cursor is moving forward. Probably we should make the RoomNameInput platform-specific component, where we do nothing for the native component since this issue does not happen on the native app.

🎀 👀 🎀 C+ reviewed!

cc @thienlnam

s77rt commented 1 year ago

@mollfpr Since you are reviewing the other issue, I think we should apply the same approach here to fix the buggy behaviour on native. However this means that we have to edit the other pages as well to get a callback after setState is done. But the good part is that we won't have to make the component platform specific.

thienlnam commented 1 year ago

Thank you @mollfpr, proposal looks good

melvin-bot[bot] commented 1 year ago

📣 @s77rt You have been assigned to this job by @thienlnam! Please apply to this job in Upwork and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻 Keep in mind: Code of Conduct | Contributing 📖

s77rt commented 1 year ago

@thienlnam @mollfpr Should I apply the approved proposal? There is a possibility of improvement https://github.com/Expensify/App/issues/12741#issuecomment-1322335993

PS: In order to be eligible for the bonus I will have to apply the approved proposal if there is no response about this.

mollfpr commented 1 year ago

IMO keep on the approved proposal with the platform-specific. What do you think @thienlnam?

thienlnam commented 1 year ago

Yes please just apply the approved proposal here, and then you can address the other improvement in the other issue

s77rt commented 1 year ago

Thanks for the quick response. Working on it

s77rt commented 1 year ago

Applied in Upwork (+50% Bonus) PR is ready.

Christinadobrzyn commented 1 year ago

Hired @varshamb as reporter, @s77rt as contributor and @mollfpr as C+ Upwork job here - https://www.upwork.com/jobs/~01b4bebba6c8508e8c

@s77rt I'm confused about the +50% bonus, can you confirm what that relates to?

mollfpr commented 1 year ago

I think @s77rt referring to CONTRIBUTING.md where there will be a bonus if the PR is merged within 3 days after the issue assigned. @Christinadobrzyn

s77rt commented 1 year ago

@Christinadobrzyn as @mollfpr stated it's about the bonus if the PR is merged within 3 days. I submitted the PR on the same day of being assigned so it's more likely to be merged within that period.

Christinadobrzyn commented 1 year ago

Ah sounds good! Thank you for confirming @s77rt and @mollfpr! I'll hire for that amount so I don't forget about the 50% bonus but when we come to the payment, I'll change to pay $250 for the job and add the $125 as a bonus. Thanks!

Christinadobrzyn commented 1 year ago

I think this can be moved to weekly while we review the PR? I'm going to move it but feel free to move it back to daily if that's best.

melvin-bot[bot] commented 1 year ago

The solution for this issue has been :rocket: deployed to production :rocket: in version 1.2.31-8 and is now subject to a 7-day regression period :calendar:. Here is the list of pull requests that resolve this issue:

https://github.com/Expensify/App/pull/12899

If no regressions arise, payment will be issued on 2022-12-02. :confetti_ball:

After the hold period, please check if any of the following need payment for this issue, and if so check them off after paying:

[X] External issue reporter YES @varshamb
[X] Contributor that fixed the issue YES @s77rt
[X] Contributor+ that helped on the issue and/or PR @mollfpr

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

Merged PR within 3 business days of assignment - 50% bonus
Merged PR more than 9 business days after assignment - 50% penalty

melvin-bot[bot] commented 1 year ago

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

[x] [@mollfpr / @thienlnam] The PR that introduced the bug has been identified. Link to the PR: https://github.com/Expensify/App/pull/7028
[x] [@mollfpr / @thienlnam] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment: (This was added as part of User Created Policy Rooms, and did not come out of another PR)
[x] [@mollfpr / @thienlnam] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion: (Not needed, a regression test has been added and it's not a large regression)
[x] [@Christinadobrzyn] A regression test has been added or updated so that the same bug will not reach production again. Link to the GH issue for creating the test here: https://github.com/Expensify/Expensify/issues/245919

mollfpr commented 1 year ago

Bump @Christinadobrzyn it’s payday time 🚀

Christinadobrzyn commented 1 year ago

Paid:

@varshamb $250 reporting
@mollfpr $250 C+
@s77rt $375 ($250 + $125 bonus)

Job closed in Upwork, @mollfpr and @thienlnam can you please complete these steps and close out this GH?

mollfpr commented 1 year ago

I think the bonus also applies to the C+?

Christinadobrzyn commented 1 year ago

Ah yep, you're right, I'll add that bonus for you now. Just added it.

mollfpr commented 1 year ago

[@mollfpr / @thienlnam] The PR that introduced the bug has been identified. Link to the PR: [@mollfpr / @thienlnam] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment: [@mollfpr / @thienlnam] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:

I feel this issue is not related to any PR but is something we need to take care of when working with the input component and ensure that all platforms are working consistently.

cc @Christinadobrzyn @thienlnam

thienlnam commented 1 year ago

I've completed the rest of the checklist. This bug was technically added when we implemented User Created Policy Rooms https://github.com/Expensify/App/pull/7028 so it was always there but now that a regression test has been added, that should be enough for this bug.

Expensify / App