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.

Sign up for GitHub

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-3218] Fixed cursor issue on edit comment box #3434

Merged
merged 1 commit into from
Jun 9, 2021
Merged

[IS-3218] Fixed cursor issue on edit comment box #3434

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions src/pages/home/report/ReportActionItemMessageEdit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,12 +36,26 @@ class ReportActionItemMessageEdit extends React.Component {
this.debouncedSaveDraft = _.debounce(this.debouncedSaveDraft.bind(this), 1000, true);
this.publishDraft = this.publishDraft.bind(this);
this.triggerSaveOrCancel = this.triggerSaveOrCancel.bind(this);
this.onSelectionChange = this.onSelectionChange.bind(this);

this.state = {
draft: this.props.draftMessage,
selection: {
Copy link
Member

Choose a reason for hiding this comment

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

Setting selection without focusing the input internally focuses the input on Safari which is then shifts to other elements causing bugs such as #15105

start: this.props.draftMessage.length,
end: this.props.draftMessage.length,
},
};
}

/**
* Update Selection on change cursor position.
*
* @param {Event} e
*/
onSelectionChange(e) {
Copy link
Member

Choose a reason for hiding this comment

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

Overall looks good but this state change is unnecessary, I have already seen a drawback of this and I am migrating tis unnecessary state change. So my suggestion here would be to remove this as I mentioned over the issue.
cc: @Gonals

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@parasharrajat can you please let me know about the issue?
also, without state change, this issue will not be fixed.

Copy link
Member

Choose a reason for hiding this comment

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

This change is going against our old pattern of not making the Component controlled.

this.setState({selection: e.nativeEvent.selection});
}

/**
* Update the value of the draft in Onyx
*
Expand Down Expand Up @@ -109,6 +123,8 @@ class ReportActionItemMessageEdit extends React.Component {
toggleReportActionComposeView(false);
}}
autoFocus
selection={this.state.selection}
onSelectionChange={this.onSelectionChange}
/>
</View>
<View style={[styles.flexRow, styles.mt1]}>
Expand Down