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.55k stars 2.9k forks source link

[$250] Room - Pasting long text in description not scrolled to bottom & cursor shown at middle. #45885

Closed izarutskaya closed 1 week ago

izarutskaya commented 3 months 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!


Version Number: 9.0.10 Reproducible in staging?: Y Reproducible in production?: Y Logs: https://stackoverflow.com/c/expensify/questions/4856 Issue reported by: Applause-Internal team

Action Performed:

  1. Go to https://staging.new.expensify.com/home
  2. Tap on any room chat (#admin)
  3. Paste any long text in compose box and note user scrolled to end showing cursor at end.
  4. Tap on header
  5. Tap room description
  6. Paste the same long text in description field and note user not scrolled to end and cursor not shown at end.
  7. Tap private notes
  8. Paste the same long text in Private Notes field and note user scrolled to end and cursor shown at end.

Expected Result:

Pasting long text in description must scrolled to bottom & cursor must be shown at end.

Actual Result:

Pasting long text in description not scrolled to bottom & cursor is shown in middle of text instead of end.

This happens in task description and in other description field across the app.

Workaround:

Unknown

Platforms:

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

Screenshots/Videos

https://github.com/user-attachments/assets/5a995426-49ea-4adc-ae42-a15bccdc1c97

View all open jobs on GitHub

Upwork Automation - Do Not Edit
  • Upwork Job URL: https://www.upwork.com/jobs/~013fba32c1ac87d746
  • Upwork Job ID: 1818439983332130443
  • Last Price Increase: 2024-09-18
Issue OwnerCurrent Issue Owner: @mollfpr
melvin-bot[bot] commented 3 months ago

Triggered auto assignment to @mallenexpensify (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.

izarutskaya commented 3 months ago

We think this issue might be related to the #vip-vsb

abzokhattab commented 3 months ago

Edited by proposal-police: This proposal was edited at 2024-08-13 07:50:10 UTC.

Proposal

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

Pasting long text in description not scrolled to bottom

What is the root cause of that problem?

In the private notes page, we use useHtmlPaste on the input ref, which handles moving the cursor when pasting from clipboard. however we don't do that in the description page

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

To ensure consistency, we should call the useHtmlPaste(reportDescriptionInputRef); inside the room and task description pages as well.

We can also do that in other places if needed.

alternatively

For a global solution, we can use the useHtmlPaste inside the TextInput and we should add a prop disableUseHtmlPaste that would disable the hook call ... this would be useful for the compose where we pass the onPaste function to the hook, this prop can be used in other cases as well.

Another solution

OR more specifically since the issue occurs when the isMarkdownEnabled is enabled then we can enable the useHtmlPaste hook when the markdown is enabled.

mallenexpensify commented 3 months ago

Checking a couple other cursor-related issues

kpadmanabhan commented 3 months ago

Proposal

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

Pasting long text in description not placing cursor at end of text

What is the root cause of that problem?

Cursor positon is not corrected while pasting text similar to Composer text field. The text input is neither subscribed to useHtmlPaste hook nor having a similar logic for correction of cursor while pasting from clipboard.

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

useHtmlPaste hook to be used in description text field similar to composer and notes text fields. However, if the approach is an overkill in case certain characters are not permitted in description, we may reuse the logic from useHtmlPaste to address the issue here.

What alternative solutions did you explore? (Optional)

N.A.

melvin-bot[bot] commented 3 months ago

@mallenexpensify Whoops! This issue is 2 days overdue. Let's get this updated quick!

kpadmanabhan commented 3 months ago

@mallenexpensify : I would like to work on this issue, based on previous work in this area. Please assign this to me. Proposal.

melvin-bot[bot] commented 3 months ago

@mallenexpensify Still overdue 6 days?! Let's take care of this!

melvin-bot[bot] commented 3 months ago

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

melvin-bot[bot] commented 3 months ago

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

mallenexpensify commented 3 months ago

@mollfpr can you review @kpadmanabhan proposal above and the issues I linked above too? https://github.com/Expensify/App/issues/45885#issuecomment-2243843578

abzokhattab commented 3 months ago

this proposal is the same as mine ... can u please let me know your opinion about my proposal?

mollfpr commented 3 months ago

I agree that the @kpadmanabhan solution is the same as the @abzokhattab proposal. It seems using useHtmlPaste is fixing the issue, but can we seek a universal solution so it can fix the issue in another place?

abzokhattab commented 3 months ago

Updated Proposal: added an alternative solution

kpadmanabhan commented 3 months ago

@abzokhattab : didnt notice your proposal earlier. sorry for that.

@mollfpr : according to me useHtmlPaste itself is a universal solution as it is the central handling for all paste actions for the control passed in. We can add this by default to any text input, unless it is unsubscribed explicitly.

melvin-bot[bot] commented 3 months ago

@mallenexpensify @mollfpr 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 3 months ago

@mallenexpensify, @mollfpr Huh... This is 4 days overdue. Who can take care of this?

mollfpr commented 3 months ago

according to me useHtmlPaste itself is a universal solution as it is the central handling for all paste actions for the control passed in. We can add this by default to any text input, unless it is unsubscribed explicitly.

I mean a solution that can be fixed without applying useHtmlPaste every time we want the multiline input.

kpadmanabhan commented 3 months ago

I mean a solution that can be fixed without applying useHtmlPaste every time we want the multiline input.

An option here is to create useHtmlPaste as a global hook for all components that accepts pasting of content from clipboard. An approach similar to https://medium.com/swlh/decouple-your-logic-from-ui-by-creating-your-own-react-hooks-d291fd2d60dc

This will make sure that we handle the paste by default for all components unless we explicitly disable that.

mollfpr commented 3 months ago

I was thinking that the useHtmlPaste could be used in InputWrapper instead reapply it in another component that uses InputWrapper, because I see that the issue only appears on multiline input with InputWrapper.

@kpadmanabhan Could you update your proposal based on your last comment? Thanks!

abzokhattab commented 3 months ago

I think the issue is no longer reproducible... are u still able to reproduce it @mollfpr?

mollfpr commented 3 months ago

I am still able to reproduce the issue on web chrome.

Edit: also still reproduce on the desktop.

abzokhattab commented 3 months ago

Thanks ... i removed the autoGrowHeight, maxAutoGrowHeightfrom the room description page to see whether this happens when the multiline is only enabled and the same issue is happening it doesn't move the index focus to the last char

also we shall clean the private notes to use this prop as well instead of using the hook

mollfpr commented 3 months ago

Thanks ... i removed the autoGrowHeight, maxAutoGrowHeightfrom the room description page to see whether this happens when the multiline is only enabled and the same issue is happening it doesn't move the index focus to the last char

So it might be the issue-only case for InputWrapper?

abzokhattab commented 3 months ago

I think it happens regardless of whether its inputwapper or just the textinput ... to validate that i used the textinput and passed the same params to it .. same result in both https://github.com/user-attachments/assets/9d879bc8-971c-4796-ac14-0ec4d96b4215

mollfpr commented 3 months ago

I think it happens regardless of whether its inputwapper or just the textinput ... to validate that i used the textinput and passed the same params to it .. same result in both

You're right!

It seems we haven't determined the correct RCA yet. I found out this issue only happens when isMarkdownEnabled is true.

melvin-bot[bot] commented 3 months 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 3 months ago

@mallenexpensify, @mollfpr Huh... This is 4 days overdue. Who can take care of this?

abzokhattab commented 3 months ago

Proposal updated

added a third solution. please let me know what you think @mollfpr

melvin-bot[bot] commented 3 months 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 3 months ago

@mallenexpensify, @mollfpr Still overdue 6 days?! Let's take care of this!

mollfpr commented 3 months ago

@abzokhattab Could you explain why the issue is with isMarkdownEnabled?

melvin-bot[bot] commented 2 months ago

@mallenexpensify @mollfpr this issue is now 4 weeks old, please consider:

Thanks!

mallenexpensify commented 2 months ago

@abzokhattab can you reply to @mollfpr 's comment above? Thx.

Could you explain why the issue is with isMarkdownEnabled ?

melvin-bot[bot] commented 2 months ago

@mallenexpensify, @mollfpr Huh... This is 4 days overdue. Who can take care of this?

melvin-bot[bot] commented 2 months ago

๐Ÿ“ฃ It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? ๐Ÿ’ธ

dominictb commented 2 months ago

Edited by proposal-police: This proposal was edited at 2024-08-22 09:15:34 UTC.

Proposal

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

[Web platform]: Pasting long text in description not scrolled to bottom & cursor is shown in middle of text instead of end. This is happening for some markdown input field like RoomDescriptionForm

What is the root cause of that problem?

RoomDescriptionPage is using TextInput with isMarkdownEnabled=true, and that means under the hood the react-native-live-markdown component will be used. However, the way react-native-live-markdown handle the cursor position after pasting is inaccurate: it uses document.execCommand(insertText) here to set the new text, and it causes the \n character to be stripped in the preSelectionRange.toString() here. Therefore, if we copy the multiline text, the cursor position will be wrong.

The useHtmlPaste hook in the app is used to extend the paste mechanism of the text input, for example, we want to handle pasting image in the Composer, or pasting raw HTML and convert that into correct markdown format. Probably the reason we don't want to opt in useHtmlPaste by default for the BaseTextInput is because in some case we want to keep the copied HTML text as is instead of converting back to our markdown format.

The reason useHtmlPaste handles the cursor correctly is because of this implementation, then it dispatch 2 extra events PasteEvent with empty clipboard, and InputEvent to handle cursor positioning. Because this implementation will allow the preSelectionRange.toString() to include the newline characters instead of document.execCommand('insertText'), the cursor positioning calculated here is correct.

And in RoomDescriptionPage, since we don't use useHtmlPaste, the react-native-live-markdown will handle the pasting itself, which can put the cursor in the wrong position in case of multiline paste text.

Also, when we paste the text, the document.execCommand(insertText) will trigger onChangeText event, and we are parsing the text first here before updating the selection of the target here. While parsing text, we also move the cursor, after setting the cursor position, we will scroll the cursor into view. However, the parseText happens before updating the selection as mentioned above, in case the input is empty, this !target.value condition will be true, and there's no scroll at all.

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

We should bring this implementation to the handlePaste, and change this to target.innerText. This will make sure the cursor positioning and scrolling correct without using useHtmlPaste in the BaseTextInput, which entails some other features like this which we might not want to opt-in BaseTextInput.

What alternative solutions did you explore? (Optional)

dominictb commented 2 months ago

https://github.com/Expensify/App/issues/45885#issuecomment-2304140694 Proposal modified to update the main solution

dominictb commented 2 months ago

Please note that we have a new refactor PR for web markdown here. I tried to test out the solution with the new version of react-native-live-markdown but it doesn't seem to work, so we might need to provide a patch in E/App before the new version of react-native-live-markdown is deemed stable.

melvin-bot[bot] commented 2 months ago

@mallenexpensify, @mollfpr Still overdue 6 days?! Let's take care of this!

melvin-bot[bot] commented 2 months ago

@mallenexpensify, @mollfpr 10 days overdue. I'm getting more depressed than Marvin.

mallenexpensify commented 2 months ago

@mollfpr , ๐Ÿ‘€ on the above proposal from @dominictb ?

melvin-bot[bot] commented 2 months ago

๐Ÿ“ฃ It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? ๐Ÿ’ธ

mollfpr commented 2 months ago

Reviewing ๐Ÿ‘€

mollfpr commented 2 months ago

Sorry for the delay ๐Ÿ™

It's a great explanation for RCA from @dominictb!

Please note that we have a new refactor PR for web markdown https://github.com/Expensify/react-native-live-markdown/pull/394. I tried to test out the solution with the new version of react-native-live-markdown but it doesn't seem to work, so we might need to provide a patch in E/App before the new version of react-native-live-markdown is deemed stable.

Regarding the above comment, what patch we should add to make your solution work?

dominictb commented 2 months ago

I will provide a new proposal since the live markdown refactor PR is merged and updated on the E/App.

melvin-bot[bot] commented 2 months ago

@mallenexpensify, @mollfpr Eep! 4 days overdue now. Issues have feelings too...

melvin-bot[bot] commented 2 months ago

๐Ÿ“ฃ It's been a week! Do we have any satisfactory proposals yet? Do we need to adjust the bounty for this issue? ๐Ÿ’ธ

dominictb commented 2 months ago

This is reproducible on latest main. I'll provide a new proposal shortly.

melvin-bot[bot] commented 2 months ago

@mallenexpensify, @mollfpr 6 days overdue. This is scarier than being forced to listen to Vogon poetry!