[HOLD for payment 2022-12-02] [$250] Cursor moves to end after adding space in Room Name reported by @varshamb #12741
Comments
kavimuru
added
Daily
|
Triggered auto assignment to @Christinadobrzyn ( |
|
Christinadobrzyn
added
the
External
|
Current assignee @Christinadobrzyn is eligible for the External assigner, not assigning anyone new. |
|
Triggered auto assignment to Contributor-plus team member for initial proposal review - @Santhosh-Sellavel ( |
melvin-bot
bot
added
the
Help Wanted
|
Triggered auto assignment to @thienlnam ( |
melvin-bot
bot
changed the title
|
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. App/src/components/RoomNameInput.js Lines 59 to 67 in 652a51a |
|
@thienlnam @Santhosh-Sellavel |
|
I guess the room name should single word |
|
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 |
|
Looks like something related to 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 Feel free to drop a note in #expensify-open-source with any questions. |
Proposaldiff --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}>DetailsImplemented Kooha-2022-11-16-20-54-04.mp4@thienlnam Sorry for the ping, can you consider reopening? |
|
@s77rt I closed it since this is intentional and we're freezing accessibility changes from #12741 (comment). What is the reason for your change? |
|
@thienlnam the behavior of replacing |
|
@thienlnam |
|
Yeah that's a good point, @Santhosh-Sellavel could you please review the above proposal? |
|
Actually we might want to use dashes here https://expensify.slack.com/archives/C01GTK53T8Q/p1668113640697309 |
I don't think this is accessibility issue. This bug is open for Cursor position. |
|
@Christinadobrzyn Can you assign this to another C+, I can't get to this sooner. cc: @thienlnam |
Christinadobrzyn
removed
the
External
melvin-bot
bot
removed
the
Help Wanted
|
📣 @s77rt You have been assigned to this job by @thienlnam! |
|
@thienlnam @mollfpr PS: In order to be eligible for the bonus I will have to apply the approved proposal if there is no response about this. |
|
IMO keep on the approved proposal with the platform-specific. What do you think @thienlnam? |
|
Yes please just apply the approved proposal here, and then you can address the other improvement in the other issue |
|
Thanks for the quick response. Working on it |
|
Applied in Upwork (+50% Bonus) |
|
Hired @varshamb as reporter, @s77rt as contributor and @mollfpr as C+ @s77rt I'm confused about the +50% bonus, can you confirm what that relates to? |
|
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 |
|
@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. |
|
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
added
Weekly
|
The solution for this issue has been 🚀 deployed to production 🚀 in version 1.2.31-8 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue: If no regressions arise, payment will be issued on 2022-12-02. 🎊 After the hold period, please check if any of the following need payment for this issue, and if so check them off after paying:
As a reminder, here are the bonuses/penalties that should be applied for any External issue:
|
melvin-bot
bot
changed the title
|
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:
|
|
Bump @Christinadobrzyn it’s payday time 🚀 |
|
I think the bonus also applies to the C+? |
|
Ah yep, you're right, I'll add that bonus for you now. Just added it. |
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. |
|
I've completed the rest of the checklist. This bug was technically added when we implemented User Created Policy Rooms #7028 so it was always there but now that a regression test has been added, that should be enough for this bug. |
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:-
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?
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:
Screen.Recording.2022-11-15.at.9.16.54.AM.mov
Recording.935.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
The text was updated successfully, but these errors were encountered: