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

fix: revert the functionality to restore cursor in value update hook #376

Merged
merged 6 commits into from
Jun 11, 2024

Conversation

dominictb
Copy link
Contributor

@dominictb dominictb commented Jun 9, 2024

Details

  • revert the use of contentSelection to restore cursor pos during text update to cursor jumping around funny since contentSelection ref value can get outdated.
  • create func to restore cursor in the ref object

Related Issues

RCA: Expensify/App#43312 (comment)
Solution: Expensify/App#41137 (comment)

Manual Tests

Linked PRs

- revert the use of contentSelection to restore cursor pos during text update to cursor jumping around funny since contentSelection ref value can get outdated.
- create func to restore cursor in the ref object

Signed-off-by: dominictb <[email protected]>
@dominictb dominictb marked this pull request as draft June 9, 2024 19:22
@dominictb dominictb marked this pull request as ready for review June 9, 2024 19:24
@dominictb
Copy link
Contributor Author

Issue: Expensify/App#43312

android-bug-18.mp4

Issue: Expensify/App#43310

android-bug-28.mp4

Issue: Expensify/App#43332

androidb3.webm

@dominictb
Copy link
Contributor Author

dominictb commented Jun 9, 2024

@BartoszGrajdek @Skalakid this should fix #341 (comment) as well

Signed-off-by: dominictb <[email protected]>
@dominictb
Copy link
Contributor Author

Issue: #341 (comment) and Expensify/App#41137 (comment)

android18.mp4

Signed-off-by: dominictb <[email protected]>
Copy link
Collaborator

@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.

@dominictb I see this PR introduces some custom API functions on the web implementation. The idea behind the Live Markdown Input is to make its interface nearly identical to the native TextInput component so people can just replace it. Adding custom API need deeper investigation and planning. If we decide to make something new it would be great to add it on iOS and Android too.
Thank you for your changes, however I think we should try solving the issue with a bit different logic. Have you tried using the currently available features of Live Markdown to fix it on the E/App side? If yes, I think you can try fixing this issue by just changing handlePaste event logic in MarkdownTextInput.web.tsx

@BartoszGrajdek
Copy link
Collaborator

Alright so here's what we're going to do now: We'll revert this PR since a fix for the regression I've mentioned was reverted here. You'll have to make 1 PR with all of these changes and we will do our best to help you along the way with the implementation since it looks like this is a tricky bug. Our goal is to keep react-native-live-markdown lib as bug-free as we can and that way we can properly test it to ensure nothing breaks. 🙌🏻

@dominictb dominictb changed the title chore: create func to restore cursor imperatively fix: revert the functionality to restore cursor in value update hook Jun 11, 2024
@dominictb
Copy link
Contributor Author

@BartoszGrajdek it seems like I couldn't reproduce this Expensify/App#41137 (comment) on 0.1.83 (the published version, not with this change in this PR) with the change on Expensify/App PR: Expensify/App#42622 (without the restoreSelectionPosition part).

It seems like after updating the parser version in react-native-live-markdown, the root cause described here: Expensify/App#41137 (comment) is not relevant anymore since with the correct parser, divRef.current.innerText === processedValue in here always holds true.

So after revert the restoreSelectionPosition change, the only thing left to fix is this comment, which can be done easily on the App repo itself. U guys can merge this PR to fix Expensify/App#41137 (comment), and the rest I will handle in the Expensify/App repo

@dominictb
Copy link
Contributor Author

@Skalakid for u to review the PR and the comment above. I have updated with latest details

Copy link
Collaborator

@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, thanks for investigation. We will close @BartoszGrajdek's PR with the revert

Copy link
Collaborator

@BartoszGrajdek BartoszGrajdek left a comment

Choose a reason for hiding this comment

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

We're basically restoring how it worked before except for setBaseAndExtent so it looks solid to me. I also like the idea of handling the bug in E/App itself

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.

4 participants