Skip to content

[WEB] Fix cursor moves several characters when deleting markdowns on multiline message #656

Merged
Skalakid merged 2 commits intoExpensify:mainfrom
VickyStash:bugfix/50687-cursor-jump
Apr 15, 2025
Merged

[WEB] Fix cursor moves several characters when deleting markdowns on multiline message #656
Skalakid merged 2 commits intoExpensify:mainfrom
VickyStash:bugfix/50687-cursor-jump

Conversation

@VickyStash
Copy link
Contributor

@VickyStash VickyStash commented Apr 3, 2025

Details

Fix cursor moves several characters on android web platform when deleting markdowns on multiline message.

Related Issues

Expensify/App#50687
Expensify/App#57024
Proposal: Expensify/App#50687 (comment)

Manual Tests

  1. Enter a bold text in multiline.
  2. Delete * at the end of the text.
  3. Make sure the cursor position is expected.
  4. Experiment with typing quickly when there are a lot of markdown styles inside the input to make sure there are no side affects due to updates.
WEB
web.mp4
WEB iOS
ios-web.mp4
WEB Android
android-web.mp4

Linked PRs

N/A

@VickyStash VickyStash marked this pull request as ready for review April 4, 2025 12:35
Copy link
Contributor

@Skalakid Skalakid left a comment

Choose a reason for hiding this comment

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

I see that these changes break quick typing a bit, since now we need to pass a slight delay to our tests. I'm worried that on slower devices the required delay will increase, and some bugs may appear :/
Also, there are some more places in our library where the input structure is modified, for example, the insertText function in the main web component. Maybe we also need to add runAfterInteractions there. Can we check if we can remove this delay in any way?

});

test('fast type cursor position', async ({page}) => {
test.setTimeout(60000);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The default test timeout is 30sec, with added delay this test takes ~40sec to run, so I needed to increase the timeout.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we use fake timers instead? https://jestjs.io/docs/timer-mocks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can we use fake timers instead? https://jestjs.io/docs/timer-mocks

It's a Playwright, not a jest test.
Maybe we can try to customize the pressSequentially implementation, but I don't feel really strongly about the idea...

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oops, you're right.

@VickyStash
Copy link
Contributor Author

I see that these changes break quick typing a bit, since now we need to pass a slight delay to our tests. I'm worried that on slower devices the required delay will increase, and some bugs may appear :/

The delay I've added is a simulation of human typing. But I understand you concern about slow-performant devices.

Also, there are some more places in our library where the input structure is modified, for example, the insertText function in the main web component. Maybe we also need to add runAfterInteractions there.

I'll check it!

Can we check if we can remove this delay in any way?

I've tried to do it on Friday but had no luck unfortunately.

@VickyStash
Copy link
Contributor Author

Also, there are some more places in our library where the input structure is modified, for example, the insertText function in the main web component. Maybe we also need to add runAfterInteractions there.

It looks like all of the places that update the input structure use the updateInputStructure method anyway.

For example insertText calls handleOnChangeText => handleOnChangeText calls parseText or other methods that later calls parseText => parseText calls updateInputStructure.

I've also done some testing on the real low-performant device (Android - Redmi Note 7) and haven't had problems, but we def will need to keep an eye on it.

@VickyStash VickyStash requested review from Skalakid and tomekzaw April 10, 2025 06:58
Copy link
Contributor

@Skalakid Skalakid left a comment

Choose a reason for hiding this comment

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

LGTM

@Skalakid Skalakid merged commit e6c3752 into Expensify:main Apr 15, 2025
5 checks passed
@os-botify
Copy link
Contributor

os-botify bot commented Apr 15, 2025

🚀 Published to npm in 0.1.265 🎉

@Skalakid
Copy link
Contributor

Hello @VickyStash, unfortunately, InteractionManager.runAfterInteractions causes the following issue in Expensify NewDot App. I think we need to revert your changes, since it will be a deploy blocker

@VickyStash
Copy link
Contributor Author

VickyStash commented Apr 18, 2025

Hello @VickyStash, unfortunately, InteractionManager.runAfterInteractions causes the following issue in Expensify NewDot App. I think we need to revert your changes, since it will be a deploy blocker

It feels like this PR was reverted by mistake, as the original issue existed on E/App main, but the updates of this PR never reached the main.
@Skalakid Have you tested the issue with/without these PR updates to confirm?

@Skalakid
Copy link
Contributor

Yes, I've tested the issue with and without your PR changes. I've also tested different versions of the Live Markdown, and only when InteractionManager.runAfterInteractions is added the issue appears. Here is more info

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants