[HOLD for Payment 2024-09-23][$100] Web - Chat - New line added when selecting "Edit comment" with arrow and enter key

[IuliiaHerets]

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

---

Version Number: v9.0.28-0 Reproducible in staging?: Y Reproducible in production?: Y If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/4912531 Email or phone of affected tester (no customers): applausetester+vd_web090224@applause.expensifail.com Issue reported by: Applause Internal Team

Action Performed:

1. Go to https://staging.new.expensify.com/
2. Go to any chat with messages (or send some messages).
3. Right click on any message.
4. Click on "Edit comment".
5. Note that the cursor is at the end of the message.
6. Cancel the edit.
7. Right click on the message.
8. With the arrows and enter keys, select "Edit comment".
9. Note that a new line is added, cursor is on a new line.

Expected Result:

No new line should be added when selecting "Edit comment" with arrows and enter keys.

Actual Result:

A new line is added when selecting "Edit comment" with arrows and enter keys.

Workaround:

Unknown

Platforms:

• [ ] Android: Native
• [ ] Android: mWeb Chrome
• [ ] iOS: Native
• [ ] iOS: mWeb Safari
• [x] MacOS: Chrome / Safari
• [ ] MacOS: Desktop

Screenshots/Videos

https://github.com/user-attachments/assets/c8be3199-28da-46ee-a3aa-62f111456c7e

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~021831491720281537308
• Upwork Job ID: 1831491720281537308
• Last Price Increase: 2024-09-05

Issue Owner

Current Issue Owner: @parasharrajat

[melvin-bot[bot]]

Triggered auto assignment to @adelekennedy (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

[IuliiaHerets]

@adelekennedy FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

[abzokhattab]

Edited by proposal-police: This proposal was edited at 2024-09-03 11:12:31 UTC.

Proposal

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

When pressing the "Edit" option in the context menu for a report action, the Enter key press propagates to the compose box, resulting in a new line being inadvertently inserted. This behavior is unintended and disrupts the editing flow.

What is the root cause of that problem?

The root cause is that the Enter key press used to select the "Edit" option in the context menu propagates to the compose box, causing it to register as an input and insert a new line. https://github.com/Expensify/App/blob/e1a886e5b5f78cd89b963600bf0b7fcd59ac6f45/src/pages/home/report/ContextMenu/ContextMenuActions.tsx#L255-L259

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

We should use InteractionManager.runAfterInteractions to ensure that the edit action is only triggered after all interactions, including the closing of the context menu, have fully completed. This will prevent the Enter key press from propagating to the compose box and inserting an unwanted new line.

hideContextMenu(false, InteractionManager.runAfterInteractions(() => editAction()));

or we can add this InteractionManager.runAfterInteractions when calling the callback value in the hideContextMenu itself

POC

https://github.com/user-attachments/assets/caebc609-41d7-477c-960f-974d6d066eb9

What alternative solutions did you explore? (Optional)

• an alternative solution is to use preventDefault on the enter event this can be done by changing preventDefault to true however this will bring the following regression
• so i suggest to add a new prop (shouldPreventDefault) in the context actions where if this value is passed then use it if not the default is false
• and pass this value as true in the editAction context and any other context if needed.

[Krishna2323]

Edited by proposal-police: This proposal was edited at 2024-09-03 11:27:53 UTC.

Proposal


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

Web - Chat - New line added when selecting "Edit comment" with arrow and enter key

What is the root cause of that problem?

shouldPreventDefault is set to false when subscribing to enter key press. https://github.com/Expensify/App/blob/4fe86eb6baf6d71aa8757b2362a788d20ee42109/src/pages/home/report/ContextMenu/BaseReportActionContextMenu.tsx#L245-L261

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


Update shouldPreventDefault to true.

What alternative solutions did you explore? (Optional)

We can call event?.preventDefault(); inside the callback function because shouldPreventDefault: false was added in the config to add the emojis on keypress. I also think that we can remove event.stopPropagation(); from the callback if we are adding event?.preventDefault();.

Result

[Krishna2323]

Proposal Updated

• Added alternative

[abzokhattab]

Proposal updated

added an alternative proposal

[bernhardoj]

Proposal

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

New line is added when we edit a message using keyboard ENTER.

What is the root cause of that problem?

In the context menu component, we have an ENTER keyboard shortcut that will open the edit composer. https://github.com/Expensify/App/blob/aa693719e19c468983a090e0c8a7bb2cec8ee76c/src/pages/home/report/ContextMenu/BaseReportActionContextMenu.tsx#L245-L261

Because the context menu option is focused, pressing ENTER will also by default trigger the element onPress which will open the edit composer. But because it's already handled by the ENTER keyboard shortcut, the enter event is captured by the edit composer which adds the unexpected new line.

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

Enable the ENTER shortcut for native only. We don't need the ENTER shortcut for the web because the item is focused when we navigate using the arrow key.

[adelekennedy]

I can reproduce - however this seems like a pretty small bug to me so I'm reducing the bounty https://github.com/user-attachments/assets/16189b95-d7a1-4433-8ad7-ea81ca3ebf59

[melvin-bot[bot]]

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

[melvin-bot[bot]]

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

[melvin-bot[bot]]

Upwork job price has been updated to $100

[chevgan]

Proposal:

To address the issue where pressing "Enter" during the "Edit Comment" action adds an unintended new line, I propose adjusting the keyboard input handling logic in the component responsible for this functionality.

Technical Explanation:

• I will modify the event handling for the "Enter" key to ensure that when it's pressed during the "Edit Comment" action, it prevents the default behavior that adds a new line.

• The solution will ensure that pressing "Enter" correctly triggers the editing action without introducing any additional lines in the text input.

• This change will involve refining how the keyboard input is managed in the context of the edit action, ensuring that only the intended behavior occurs when the "Enter" key is pressed.

https://github.com/user-attachments/assets/a6f067e3-5b4b-491a-a8bb-33a43cd067f0

[melvin-bot[bot]]

📣 @chevgan! 📣 Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork. Please follow these steps:

1. Make sure you've read and understood the contributing guidelines.
2. Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
3. Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
4. Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details. Format:

   Contributor details
   Your Expensify account email: <REPLACE EMAIL HERE>
   Upwork Profile Link: <REPLACE LINK HERE>

[chevgan]

Contributor details Your Expensify account email: danil.chevgan.00@gmail.com Upwork Profile Link: https://www.upwork.com/freelancers/chevgan

[melvin-bot[bot]]

✅ Contributor details stored successfully. Thank you for contributing to Expensify!

[parasharrajat]

@adelekennedy Can we please set the price to 125 like we normally do? I think 100 is less.

[parasharrajat]

@Krishna2323's alternate solution sounds good to me. It was suggested first. It will require fewer changes and we don't have to go through the hassle of config changes.

:ribbon: :eyes: :ribbon: C+ reviewed

[melvin-bot[bot]]

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

[Krishna2323]

@parasharrajat, can you please let me know which regression are you talking about?

Also, @abzokhattab update their proposal after mine.

[parasharrajat]

Yes, the regression that @abzokhattab mentioned in their alternative solution.

Also, @abzokhattab update their proposal after mine.

Saw that. But I believe it is important to present refined solution. There is no use if a solution is causing another regression. So IMO, it is important to ensure that a solution will not cause any regression.

[Krishna2323]

We can call event?.preventDefault(); inside the callback function because shouldPreventDefault: false was added in the config to add the emojis on keypress. I also think that we can remove event.stopPropagation(); from the callback if we are adding event?.preventDefault();.

@parasharrajat, I have already talked about that before the other proposal and my alternative proposal doesn't cause any regression, let me check again.

[Krishna2323]

@parasharrajat, here is the result after applying my alternative solution.

https://github.com/user-attachments/assets/6ae3bb2b-6229-42aa-bcd4-c6d81f1aa8a3

[parasharrajat]

Oops, sorry. I misread the timings. Yes, in that case, your proposal is better.

[parasharrajat]

@Krishna2323 When you say callback function which one are you talking about? I think we don't need to remove StopPropagation from the logic. How will you make sure that emoji keypress will still work?

[Krishna2323]

When you say callback function which one are you talking about? I think we don't need to remove StopPropagation from the logic.

Inside this callback: https://github.com/Expensify/App/blob/aa693719e19c468983a090e0c8a7bb2cec8ee76c/src/pages/home/report/ContextMenu/BaseReportActionContextMenu.tsx#L247-L259

How will you make sure that emoji keypress will still work?

Sorry, but I didn't understand what do you mean by this.

[parasharrajat]

This callback. fires for all context items which include emoji reactions so If we add this in the callback, will it still work emoji reactions?

[Krishna2323]

This callback. fires for all context items which include emoji reactions so If we add this in the callback, will it still work emoji reactions?

Yes, this video is recorded after applying the change.

[abzokhattab]

What about wrapping the callback function with InteractionManager.runAfterInteractions inside the hideContextMenu itself (here) as presented in the first solution ... i think this is a better solution as we should avoid using the preventDefault

This ensures the callback is only used after all interactions are finished.

[parasharrajat]

This will delay the Execution. It could prevent focus from working in some cases. We should focus in the same event callback which is triggered by the user.

[lakchote]

@parasharrajat what was your reasoning for choosing @Krishna2323's alternative proposal?

IMO, @bernhardoj's proposal LGTM. It effectively address in the first place the problem, since we don't need to have the ENTER shortcut when on web from what it seems.

Letting the shortcut event there trigger while we don't need it, to then prevent its effect doesn't sound that good to me.

[adelekennedy]

Just noting for pricing we don't have a standard reduced priced - our SO says to set it at whatever we think is fair and then increase if we don't see traction

[parasharrajat]

@lakchote I initially thought we needed it. I was worried that we would not miss any edge cases that end up as a regression. But it looks like we don't it. In that case, I am fine with this approach too.

[melvin-bot[bot]]

@lakchote, @parasharrajat, @adelekennedy Whoops! This issue is 2 days overdue. Let's get this updated quick!

[parasharrajat]

Following https://github.com/Expensify/App/issues/48451#issuecomment-2334531979, @lakchote I agree we can move the handler to native only.

[lakchote]

Thank you @parasharrajat, assigning @bernhardoj then!

[bernhardoj]

PR is ready

cc: @parasharrajat

[parasharrajat]

I think the dates are wrong here. It should be ready to be paid in 2 days.

[adelekennedy]

Payouts due:

• [x] Contributor: $100 @bernhardoj NewDot
• [x] Contributor+: $100 @parasharrajat NewDot

[bernhardoj]

Requested in ND.

[parasharrajat]

@adelekennedy We can close this issue. Thanks

[JmillsExpensify]

$100 approved for @bernhardoj

[parasharrajat]

Payment requested as per https://github.com/Expensify/App/issues/48451#issuecomment-2367713607

[JmillsExpensify]

$100 approved for @parasharrajat