Expensify / react-native-live-markdown

Drop-in replacement for React Native's TextInput component with Markdown formatting.
https://www.npmjs.com/package/@expensify/react-native-live-markdown
MIT License
781 stars 56 forks source link

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

Closed dominictb closed 3 months ago

dominictb commented 3 months ago

Details

Related Issues

RCA: https://github.com/Expensify/App/issues/43312#issuecomment-2156740534 Solution: https://github.com/Expensify/App/issues/41137#issuecomment-2156739474

Manual Tests

Linked PRs

dominictb commented 3 months ago

Issue: https://github.com/Expensify/App/issues/43312

https://github.com/Expensify/react-native-live-markdown/assets/165644294/45d44e4a-8301-465b-bd1e-e1bb5c86a031

Issue: https://github.com/Expensify/App/issues/43310

https://github.com/Expensify/react-native-live-markdown/assets/165644294/1f86c3cb-b68f-40fe-b622-763d1c047e10

Issue: https://github.com/Expensify/App/issues/43332

androidb3.webm

dominictb commented 3 months ago

@BartoszGrajdek @Skalakid this should fix https://github.com/Expensify/react-native-live-markdown/pull/341#issuecomment-2146805686 as well

dominictb commented 3 months ago

Issue: https://github.com/Expensify/react-native-live-markdown/pull/341#issuecomment-2146805686 and https://github.com/Expensify/App/issues/41137#issuecomment-2122076927

https://github.com/Expensify/react-native-live-markdown/assets/165644294/dc77235f-6c82-43a1-8267-764c1c73d137

BartoszGrajdek commented 3 months ago

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 commented 3 months ago

@BartoszGrajdek it seems like I couldn't reproduce this https://github.com/Expensify/App/issues/41137#issuecomment-2156739474 on 0.1.83 (the published version, not with this change in this PR) with the change on Expensify/App PR: https://github.com/Expensify/App/pull/42622 (without the restoreSelectionPosition part).

It seems like after updating the parser version in react-native-live-markdown, the root cause described here: https://github.com/Expensify/App/issues/41137#issuecomment-2127866960 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 https://github.com/Expensify/App/issues/41137#issuecomment-2153464605, and the rest I will handle in the Expensify/App repo

dominictb commented 3 months ago

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