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.

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

Web - After adding the emoji, cursor is not in the front of the message #2601

Closed
isagoico opened this issue Apr 27, 2021 · 25 comments · Fixed by #3080
Closed

Web - After adding the emoji, cursor is not in the front of the message #2601

isagoico opened this issue Apr 27, 2021 · 25 comments · Fixed by #3080
Assignees
Labels
Engineering External Added to denote the issue can be worked on by a contributor

Comments

@isagoico
Copy link

isagoico commented Apr 27, 2021

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


Expected Result:

Emoji is added to the text input field and cursor moves forward

Actual Result:

Cursor does not move after adding an emoji and user has to manually move the cursor in front of the emoji

Action Performed:

  1. Go to staging.expensify.cash and login with credentials
  2. Tap any contact from the LHN
  3. In the text input field type some text then select an emoji

Workaround:

User has to manually move the cursor

Platform:

Where is this issue occurring?

Web ✔️ (MacOS Safari only)
iOS
Android
Desktop App
Mobile Web

Version Number: 1.0.32-0

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

Notes/Photos/Videos: Any additional supporting documentation

Bug5042372_Cursor.mp4

Expensify/Expensify Issue URL:

view this job on upwork here

View all open jobs on Upwork

@isagoico isagoico added the AutoAssignerTriage Auto assign issues for triage to an available triage team member label Apr 27, 2021
@MelvinBot
Copy link

Triggered auto assignment to @jboniface (AutoAssignerTriage), see https://stackoverflow.com/c/expensify/questions/4749 for more details.

@MelvinBot MelvinBot removed the AutoAssignerTriage Auto assign issues for triage to an available triage team member label Apr 27, 2021
@jboniface jboniface removed their assignment Apr 27, 2021
@MelvinBot
Copy link

Triggered auto assignment to @marcaaron (Engineering), see https://stackoverflow.com/c/expensify/questions/4319 for more details.

@marcaaron
Copy link
Contributor

Looks like there's some kind of bug @stitesExpensify maybe will know if this was happening already or is a regression. I'm not sure if we tested this much on Safari.

@marcaaron marcaaron removed their assignment Apr 27, 2021
@marcaaron marcaaron added the External Added to denote the issue can be worked on by a contributor label Apr 27, 2021
@MelvinBot
Copy link

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

@stitesExpensify
Copy link
Contributor

I don't think this is a regression. It was probably already happening on safari when we initially released the emoji picker :( Safari is the worst.

@ninjadev007
Copy link

ninjadev007 commented Apr 28, 2021

PROPOSAL

need to update selection on update comment and for this add this code in TextInputFocusable component

if (prevProps.value !== this.props.value) {
    this.setState({selection: {start: this.props.value.length, end: this.props.value.length}});
}

https://github.com/Expensify/Expensify.cash/blob/5ed9e261867aec89018d295fe67775b9d7d621b6/src/components/TextInputFocusable/index.js#L126

and also add value attribute in TextInputFocusable component when we add in ReportActionCompose component
https://github.com/Expensify/Expensify.cash/blob/5ed9e261867aec89018d295fe67775b9d7d621b6/src/pages/home/report/ReportActionCompose.js#L355

value={this.comment}

VIDEO

Screen.Recording.2021-04-28.at.12.41.15.AM.mov

@isagoico
Copy link
Author

isagoico commented May 9, 2021

Issue reproducible during today's KI retests

@jboniface
Copy link

job posted here: https://www.upwork.com/jobs/~019cc8e9b14b76f110

@marcaaron marcaaron removed their assignment May 10, 2021
@MelvinBot
Copy link

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

@iwiznia
Copy link
Contributor

iwiznia commented May 10, 2021

Argh damnit, not sure if it is too late, but I also noticed that the emoji is not inserted where the cursor is, instead it is always inserted at the end of the text (I tested on web chrome)

@ninjadev007
Copy link

Here is my PROPOSAL
#2601 (comment)

Thanks

@isagoico
Copy link
Author

Issue reproducible during today's KI retests

@jboniface
Copy link

@aliabbasmalik11 sorry for the delay, let me flag an engineer to review this

@tgolen
Copy link
Contributor

tgolen commented May 17, 2021

@aliabbasmalik11 Can you please verify if your proposal works when you put the cursor in the middle of a message to add an emoji?

Also, can you please explain why the value attribute needs added to TextInputFocusable ?

@tgolen
Copy link
Contributor

tgolen commented May 17, 2021

@aliabbasmalik11 I was also curious why this is a different account than @aliabbasmalik8 which has done several issues for us before? https://github.com/aliabbasmalik8. Is that just a coincidence???

@ninjadev007
Copy link

ninjadev007 commented May 17, 2021

@tgolen, @jboniface There is one more issue when we add emoji in mid of text it'll add at the end, you can see this issue in this video https://recordit.co/arBHs9WvqB and this issue also fixed in this latest proposal

VIDEO AFTER FIXED ISSUES: https://recordit.co/jpFReGrGap

UPDATED PROPOSAL

https://github.com/Expensify/Expensify.cash/blob/c1000111ec29f0b0313730bd2cf0c21309819e21/src/pages/home/report/ReportActionCompose.js#L118
add selection state here,

selection: {
                start: props.comment.length,
                end: props.comment.length,
            },

https://github.com/Expensify/Expensify.cash/blob/c1000111ec29f0b0313730bd2cf0c21309819e21/src/pages/home/report/ReportActionCompose.js#L270

update addEmojiToTextBox function like this way

addEmojiToTextBox(emoji) {
        this.hideEmojiPicker();
        const {selection} = this.state;
        this.textInput.value = this.comment.slice(0, selection.start)
            + emoji + this.comment.slice(selection.start, this.comment.length);
        const updatedSelection = {
            start: selection.start + emoji.length,
            end: selection.end + emoji.length,
        };
        this.setState({selection: updatedSelection});
        this.setIsFocused(true);
        this.updateComment(this.textInput.value);
}

added onSelectionChange function in ReportActionCompose component.

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

https://github.com/Expensify/Expensify.cash/blob/c1000111ec29f0b0313730bd2cf0c21309819e21/src/pages/home/report/ReportActionCompose.js#L392
also add these values in TextInputFocusable component

selection={this.state.selection}
 onSelectionChange={this.onSelectionChange}    

removed selection state from TextInputFocusable component
https://github.com/Expensify/Expensify.cash/blob/c1000111ec29f0b0313730bd2cf0c21309819e21/src/components/TextInputFocusable/index.js#L91

update <Textinput /> like this way

<TextInput
               ref={el => this.textInput = el}
               selection={this.props.selection}
               onChange={() => {
                   this.updateNumberOfLines();
               }}
               numberOfLines={this.state.numberOfLines}
               style={propStyles}
               /* eslint-disable-next-line react/jsx-props-no-spreading */
               {...propsWithoutStyles}
               disabled={this.props.isDisabled}
               onSelectionChange={this.props.onSelectionChange}
           />

at here https://github.com/Expensify/Expensify.cash/blob/c1000111ec29f0b0313730bd2cf0c21309819e21/src/components/TextInputFocusable/index.js#L223

@ninjadev007
Copy link

@aliabbasmalik11 I was also curious why this is a different account than @aliabbasmalik8 which has done several issues for us before? https://github.com/aliabbasmalik8. Is that just a coincidence???

I'm the same aliabbasmalik8

@tgolen
Copy link
Contributor

tgolen commented May 17, 2021

I'm the same aliabbasmalik8

OK, and you're still able to accept the job and be paid via UpWork?

@ninjadev007
Copy link

I'm the same aliabbasmalik8

OK, and you're still able to accept the job and be paid via UpWork?

yes

@tgolen
Copy link
Contributor

tgolen commented May 17, 2021

OK, thanks for clarifying that for me!

I looked at your updated proposal and it looks like it would properly handle the mid-sentence edit now. As far as refactoring TextInputFocusable to have the selection passed as props, will that affect any other uses of TextInputFocusable (outside of ReportActionCompose)?

Maybe TextInputFocusable should default to the old selection start/end values if no selection props are passed?

@ninjadev007
Copy link

Maybe TextInputFocusable should default to the old selection start/end values if no selection props are passed?

Okay, will remain selection state in TextInputFocusable and update state selection if we have it in props and updated in componentDidUpdate lifecycle

if (prevProps.selection !== this.props.selection) {
            this.setState({selection: this.props.selection});
        }

@tgolen
Copy link
Contributor

tgolen commented May 17, 2021

I like that better, yep. Thanks!

Proposal LGTM ✅

@ninjadev007
Copy link

@jboniface Can I start work on this issue?

@jboniface
Copy link

Yep!

@isagoico
Copy link
Author

Issue reproducible today during KI retests

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Engineering External Added to denote the issue can be worked on by a contributor
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants