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

Live Markdown for web refactor #394

Closed Skalakid closed 1 month ago

Skalakid commented 3 months ago

Details

This PR refactors the web implementation of the Live Markdown library. The main goals of it are:

During this refactor phase we:

This refactor was made as a part of inline image feature. After discovering some structure and logic limitations when creating inline image POC we decided to improve Live Markdown for web.

This refactor will fix or unblock following issues:

Related Issues

GH_LINK

Manual Tests

Since it's a big change it would be great to test it in all cases connected to:

  1. Adding text / Markdown
  2. Selection
  3. Cursor positioning
  4. Number of lines
  5. Scroll into view
  6. Placeholder
  7. History - undo/redo
  8. Pasting
  9. Copy / Cut
  10. Composition events
    • Web:
      • Diacritics
      • Text replacement on mac
    • mWeb:
      • Diacritics
      • Autocorrection
  11. Spellcheck
    • Web:
      • Chrome
      • Safari
      • Firefox (not working)
    • mWeb:
      • iOS Safari
      • Android Chrome

Linked PRs

https://github.com/Expensify/App/issues/40181#issuecomment-2180897156

BartoszGrajdek commented 2 months ago

Chrome

Details ### Diacritics after `CMD+A` https://github.com/Expensify/react-native-live-markdown/assets/41169160/57ee6a6a-662a-4d9e-b574-c4933d322bdd

Firefox

Details ### Cursor positioning on focus https://github.com/Expensify/react-native-live-markdown/assets/41169160/f3b7807c-bd9d-4c35-94d5-0a7123f803df ### Weird spellcheck (different than on chrome) spellcheck-ff ### Deleting codeBlock lines with `CMD+backspace` (LOW PRIORITY IMO) https://github.com/Expensify/react-native-live-markdown/assets/41169160/b82da7fc-1f8c-40bb-90bb-d59b36116a4f

Safari

Details ### Autocorrect cursor positioning https://github.com/Expensify/react-native-live-markdown/assets/41169160/1025c81e-b7b7-4834-ba01-f530bbf62667 ### Cursor position after `CMD+A` -> `Composition` -> `CMD+Z` (LOW PRIORITY - REALLY AN EDGE CASE https://github.com/Expensify/react-native-live-markdown/assets/41169160/3cda01ed-f79f-4ad5-b905-b7c009afeaf3
Skalakid commented 2 months ago

Firefox: Cursor positioning on focus

It's not a bug. It happens because in App.tsx we are passing the selection prop with index 0. The same happens on main right now. If you delete the prop, everything works fine and the cursor is positioned at the end of the text

Skalakid commented 2 months ago

Firefox: Weird spellcheck (different than on Chrome)

I think it depends on your default keyboard language. I'm not sure if we can change anything in this case since is browser-native functionality. On my side, it looks like this:

Screenshot 2024-07-10 at 12 28 02

Skalakid commented 2 months ago

@BartoszGrajdek I just fixed all reported bugs 🫡

dangrous commented 2 months ago

Not sure how far along we are with this but wanted to ping about https://github.com/Expensify/App/issues/45345 - looks like something we might be able to fix here!

Skalakid commented 2 months ago

@dangrous The Refactor PR is still in progress and now we are focused on fixing bugs reported by the QA team in https://github.com/Expensify/App/pull/45150. This refactor fixes the issue you linked, however, it will take some time until we merge this into E/App. If you need the fix quicker we can do it in a separate PR. What's your opinion?

EDIT: I can't reproduce this issue, https://github.com/Expensify/App/pull/44958 might have fixed it

dangrous commented 2 months ago

oh nice! I don't think this is high priority, so we can wait (unless https://github.com/Expensify/App/pull/44958 fixed it!)

JKobrynski commented 2 months ago

I've been trying to fix this Android web issue (you can see some of my takeaways in the comments). It led me to parseText method in parserUtils.ts, more specifically these lines:

 if (!rootSpan || rootSpan.innerHTML !== dom.innerHTML) {
      targetElement.innerHTML = '';
      targetElement.innerText = '';
      target.appendChild(dom);

      if (BrowserUtils.isChromium) {
        moveCursor(isFocused, alwaysMoveCursorToTheEnd, cursorPosition, target);
      }
 }

I also contacted @BartoszGrajdek (shout out to him) to discuss it and he suggested to give this PR a try, as it refactors the web implementation. Here is what I discovered on both web and Android web:

(I had to remove TEST_CONST.EXAMPLE_CONTENT as the bug that I'm trying to fix only occurs for an empty input)

Case 1

- const [value, setValue] = React.useState(TEST_CONST.EXAMPLE_CONTENT);
+ const [value, setValue] = React.useState();

Voice recognition works, markdown doesn't work

Case 2

- const [value, setValue] = React.useState(TEST_CONST.EXAMPLE_CONTENT);
+ const [value, setValue] = React.useState('');

Voice recognition doesn't work, markdown works

To test the voice recognition, I started from an empty input and dictated a sentence, to test markdown I simply typed bold or something similar.