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
3.51k stars 2.87k forks source link

[HOLD for payment 2024-04-05] [$500] mWeb - Cursor jumps back to the start when adding an emoji in edit #28563

Closed kbecciv closed 6 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!


Action Performed:

  1. Open any report on staging ND
  2. Send a text message
  3. Hold on the message and tap on edit the message
  4. Click the emoji button in the text input
  5. select an emoji

Expected Result:

The cursor should stay after the emoji when an emoji is added

Actual Result:

The cursor jumps to the start of the text when an emoji is added while editing

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

Version Number: 1.3.75.8 Reproducible in staging?: y Reproducible in production?: n If this was caught during regression testing, add the test name, ID and link from TestRail: Email or phone of affected tester (no customers): Logs: https://stackoverflow.com/c/expensify/questions/4856 Notes/Photos/Videos: Any additional supporting documentation

https://github.com/Expensify/App/assets/93399543/62c422e0-4123-4310-97f4-d9b2b153d38d

https://github.com/Expensify/App/assets/93399543/ce03feb3-a78b-444e-8051-4d24d5e46207

Expensify/Expensify Issue URL: Issue reported by: @jo-ui Slack conversation: https://expensify.slack.com/archives/C049HHMV9SM/p1696174769247349

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~01edb4567dee0b0a57
  • Upwork Job ID: 1708949538648821760
  • Last Price Increase: 2023-10-16
OSBotify commented 1 year ago

:wave: Friendly reminder that deploy blockers are time-sensitive ⏱ issues! Check out the open StagingDeployCash deploy checklist to see the list of PRs included in this release, then work quickly to do one of the following:

  1. Identify the pull request that introduced this issue and revert it.
  2. Find someone who can quickly fix the issue.
  3. Fix the issue yourself.
melvin-bot[bot] commented 1 year ago

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

mountiny commented 1 year ago

I cannot repro this in ios

neil-marcellini commented 1 year ago

I'll check if I can reproduce it on Android Chrome now.

neil-marcellini commented 1 year ago

I can't reproduce this on staging so we can close this.

https://github.com/Expensify/App/assets/26260477/ab92a6b9-1d82-40ad-8cd0-3c1a6a232d3d

mountiny commented 1 year ago

Thanks!

jo-ui commented 1 year ago

@neil-marcellini @mountiny It is still reproducible on android chrome but not on ios. Please check again with cleared cookies.

melvin-bot[bot] commented 1 year ago

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

melvin-bot[bot] commented 1 year ago

Bug0 Triage Checklist (Main S/O)

mountiny commented 1 year ago

Reopening to confirm

sakluger commented 1 year ago

Since @jo-ui confirmed it is broken on android only, and Applause confirmed it's reproduceable on android, I'm going to add labels.

melvin-bot[bot] commented 1 year ago

Job added to Upwork: https://www.upwork.com/jobs/~01edb4567dee0b0a57

melvin-bot[bot] commented 1 year ago

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

sakluger commented 1 year ago

My bad, I missed the deploy blocker. I can't test via Browserstack - I just get an endless spinner after entering my email and tapping continue.

@neil-marcellini could you please help trying to reproduce again?

neil-marcellini commented 1 year ago

Yeah I'll test again to see if I can reproduce after clearing cookies as @jo-ui said.

neil-marcellini commented 1 year ago

@sakluger @jo-ui I'm not able to reproduce this on Android Chrome even after clearing all site data. I'm pretty sure it's not a problem. @jo-ui please upload a video reproducing the bug, if you still can. Maybe the test steps need to be more clear.

jo-ui commented 1 year ago

@neil-marcellini https://github.com/Expensify/App/assets/57154082/3a7adb2b-4725-434e-b4d2-3d3896ace72b

neil-marcellini commented 1 year ago

Ah yes I see thank you. I'm able to reproduce as well if I add the emoji by clicking the button in the text input. I'll update the test steps to be extra clear lol.

melvin-bot[bot] commented 1 year ago

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

jo-ui commented 1 year ago

@neil-marcellini it is also reproducible on the latest production 1.3.76-6 https://github.com/Expensify/App/assets/57154082/4f15b9fe-4bc7-4f8c-ba5c-55089a45448a

tienifr commented 1 year ago

Proposal

Please re-state the problem that we are trying to solve in this issue.

The cursor jumps to the start of the text when an emoji is added while editing

What is the root cause of that problem?

When we add the emoji, the selection of the input is manually changed here, and after that, when the EmojiPicker modal closes, the input is focused here.

In Android Chrome, when the selection of an input is changed while that input is currently blurred, it's native behavior that when the focus is returned, it will have selection 0 by default.

What changes do you think we should make in order to solve the problem?

We should manually update the input selection only after the input is focused, not when it's already out of focus.

  1. Add a selectionAfterFocus ref to ReportActionItemMessageEdit to store the selection so we can focus on it afterwards.
    const selectionAfterFocus = useRef();
  2. In addEmojiToTextBox, replace this block with
    selectionAfterFocus.current = {
    start: selection.start + emoji.length + CONST.SPACE_LENGTH,
    end: selection.start + emoji.length + CONST.SPACE_LENGTH,
    }
  3. In this useEffect that runs when isFocused is changed, set the selection if exists, then clear the selection
    if (isFocused && selectionAfterFocus.current) {
    setSelection(selectionAfterFocus.current);
    selectionAfterFocus.current = undefined;
    }

What alternative solutions did you explore? (Optional)

In step 3, an alternative is we can set the selection inside onSelectionChange of the Composer, if we detect that the Composer is resetting the selection to 0 by native behavior.

if (selectionAfterFocus.current && e.nativeEvent.selection.start === 0 && e.nativeEvent.selection.end === 0) {
    setSelection(selectionAfterFocus.current);
    selectionAfterFocus.current = undefined;
    return;
}

But I think relying on the focus state is more intuitive.

abdulrahuman5196 commented 1 year ago

Will review tonight

abdulrahuman5196 commented 1 year ago

TAL now

abdulrahuman5196 commented 1 year ago

@tienifr 's solution here https://github.com/Expensify/App/issues/28563#issuecomment-1746252206 feels like a workaround to avoid the mentioned native behaviour like added a new selectionAfterFocus ref. So I am not aligned on it totally, will wait for better proposals.

abdulrahuman5196 commented 1 year ago

To contributors, I am curious, is this bug happening recently due to any changes?

bernhardoj commented 1 year ago

I don't know why, but the user-select: none style from this PR causing this issue.

melvin-bot[bot] commented 1 year ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

melvin-bot[bot] commented 1 year ago

@sakluger, @neil-marcellini, @abdulrahuman5196 Whoops! This issue is 2 days overdue. Let's get this updated quick!

abdulrahuman5196 commented 1 year ago

There isn't any new proposals to review yet. cc: @sakluger

neil-marcellini commented 1 year ago

Not overdue. I'll post to open source to get more eyes on it. Posted here

sakluger commented 1 year ago

Thanks Neil. Hopefully we get some more proposals!

@bernhardoj it looks like you made a bit of progress but couldn't find the root cause. Any chance you were able to figure out what the issue was here?

bernhardoj commented 1 year ago

@sakluger sadly no, I still don't know what the root cause is

s-alves10 commented 1 year ago

Proposal

Please re-state the problem that we are trying to solve in this issue.

Cursor moves to the first position when adding an emoji in edit composer box in mobile chrome

What is the root cause of that problem?

Emoji item is a Pressable with style user-select: 'none'. This looks like a bug of Android chrome: When clicking an element with css user-select: 'none', it disables the text selection of the element, but it causes the cursor of the last focused input element jump to the start position.

What changes do you think we should make in order to solve the problem?

We can fix this issue by preventing default action of mouse events of Pressable element Add the following props to the EmojiPickerMenuItem https://github.com/Expensify/App/blob/fe282b45cb13e01519282ccc023e5bfbd7714158/src/components/EmojiPicker/EmojiPickerMenuItem/index.js#L72

        onMouseDown={(e) => e.preventDefault()}
        onMouseUp={(e) => e.preventDefault()}

This update works as expected

Result https://github.com/Expensify/App/assets/126839717/e59fdf25-9f64-439e-8732-aef20977c2d6

What alternative solutions did you explore? (Optional)

We can prevent onSelectionChange event handler when composer is not focused

abdulrahuman5196 commented 1 year ago

@bernhardoj @s-alves10

It seems you had mentioned the addition of user-select: 'none' is causing this issue.

Why is the style added and what is required for? Will it cause any issue if we remove. It seems you added that style @s-alves10

@s-alves10 Regarding the proposal here https://github.com/Expensify/App/issues/28563#issuecomment-1762647105 it seems to be hacky without proper root cause. I think we should investigate on root cause and avoid any regressions in future.

melvin-bot[bot] commented 1 year ago

@sakluger @neil-marcellini @abdulrahuman5196 this issue was created 2 weeks ago. Are we close to approving a proposal? If not, what's blocking us from getting this issue assigned? Don't hesitate to create a thread in #expensify-open-source to align faster in real time. Thanks!

melvin-bot[bot] commented 1 year ago

📣 It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? 💸

abdulrahuman5196 commented 1 year ago

As far as now we don't have any proposals to approve. @sakluger

sakluger commented 1 year ago

Asked for help in #expensify-callstack: https://expensify.slack.com/archives/C03UK30EA1Z/p1697478673562329.

pac-guerreiro commented 1 year ago

I think this issue is similar to this one - https://github.com/Expensify/App/issues/21727

But the root cause is definitely different. I would debug the state of the TextInput from react-native-web when the emoji is introduced and check what is getting changed and force it to keep the position.

pac-guerreiro commented 1 year ago

I'm able to reproduce this on desktop chrome if I toggle the device toolbar from the inspect menu!

https://github.com/Expensify/App/assets/48553277/08e47c07-9d96-455d-9301-86afff92127f

sakluger commented 1 year ago

Started a discussion in Slack, hopefully we can get closer to understanding the root cause.

sakluger commented 1 year ago

I think this bug is pretty low priority since it only affects Android Chrome, and the only bad side effect is that the user needs to move the cursor again. I'm going to close the issue so we can focus on higher priority bugs. Thanks everyone for looking into this one!

jo-ui commented 1 year ago

@sakluger @neil-marcellini Shouldn't the reporting bonus be paid for this legit bug?

situchan commented 1 year ago

If the bug is not fixed, not eligible for reporting bonus

sakluger commented 1 year ago

Okay, opening back up because we're still discussing whether we should fix or not. But moving to weekly.

melvin-bot[bot] commented 1 year ago

@sakluger @neil-marcellini @abdulrahuman5196 this issue is now 3 weeks old. There is one more week left before this issue breaks WAQ and will need to go internal. What needs to happen to get a PR in review this week? Please create a thread in #expensify-open-source to discuss. Thanks!

sakluger commented 1 year ago

Asked CallStack for help to see if they can think of any solutions that fix both bugs: https://expensify.slack.com/archives/C03UK30EA1Z/p1698092827271159

roksanaz commented 1 year ago

I am Roksana from Callstack - expert contributor group. I’d like to work on this job.

roksanaz commented 1 year ago

Sorry for no update yesterday. I'm searching for the root cause of the problem. As suggested, I'm investigating the TextInput component. Moreover, I'm also looking at differences in implementation of TextInput between new message composer (https://github.com/Expensify/App/blob/main/src/pages/home/report/ReportActionCompose/ReportActionCompose.js) - as the behavior here is correct - and edit message composer (https://github.com/Expensify/App/blob/main/src/pages/home/report/ReportActionItemMessageEdit.js).

sakluger commented 1 year ago

Thanks @roksanaz! Appreciate you digging into this one, and understand that it may take a little while to figure out. Keep us updated.