Skip to content

Live Markdown for web refactor#394

Merged
Skalakid merged 82 commits intomainfrom
@Skalakid/web-parser-refactor
Aug 21, 2024
Merged

Live Markdown for web refactor#394
Skalakid merged 82 commits intomainfrom
@Skalakid/web-parser-refactor

Conversation

@Skalakid
Copy link
Contributor

@Skalakid Skalakid commented Jun 20, 2024

Details

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

  • enhancing algorithms and logic behind Markdown Input, to make the project more flexible and enable the possibility of adding new more complex features like inline images or code blocks
  • changing and organizing Markdown input HTML structure, to meet the industry standards, fixing problems with cursor positioning and problems with scrolling into view
  • improving code quality, to make the codebase more understandable and boost the developer experience

During this refactor phase we:

  • changed web live markdown HTML structure so now:

    • every line is represented as paragraph element <p> - it makes structure much clear and organized, and enables us to apply custom logic to specific lines
    • every text is wrapped with <span data-type="text"> tag - by wrapping text with HTML element we enable many useful functions that aren't available in normal text node, for example: scrollIntoView()
    • instead of \n characters inside the input's HTML structure we use <br> tags - br tags give use much more flexibility and fix problem with cursor navigation when using arrow keys when moving between markdown components
    • every <br> tag is wrapped with <span data-type="br"> tag - it fixes many problems connected to cursor positioning

    Thanks to the new structure we enabled the usage of many styles that were generating a lot of bugs in the past, like display: block. Also were able to remove all workarounds connected to cursor positioning.

  • added HTML tree node structure, that represents current HTML dom and stores many important information about every element inside the input. Thanks to this tree structure we can easily interact with the markdown elements and we were able to completely refactor and improve the performance of cursor positioning algorithms

  • replaced all text value representation functions like process value or normalizeValue with one textContent variable that always contains properly formatted text value without any additional newlines from contentEditable

  • split long files of code into smaller ones, and sort functions into utils

  • prepared the code for new, more complex features like inline images

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

Expensify/App#40181 (comment)

@Skalakid Skalakid changed the title @skalakid/web parser refactor Live Markdown for web refactor Jun 20, 2024
@Skalakid Skalakid force-pushed the @Skalakid/web-parser-refactor branch from 701962c to 38693c4 Compare June 26, 2024 13:58
@Skalakid Skalakid requested a review from tomekzaw June 28, 2024 16:22
@Skalakid Skalakid self-assigned this Jun 28, 2024
@Skalakid Skalakid added enhancement New feature or request labels Jun 28, 2024
@Skalakid Skalakid requested a review from BartoszGrajdek June 28, 2024 17:02
@JKobrynski
Copy link

JKobrynski commented Jul 31, 2024

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.

@BartoszGrajdek BartoszGrajdek force-pushed the @Skalakid/web-parser-refactor branch from 17fd595 to a1bf047 Compare August 2, 2024 11:19
@Skalakid Skalakid force-pushed the @Skalakid/web-parser-refactor branch from b5e9b86 to 4e895e9 Compare August 20, 2024 09:07
BartoszGrajdek
BartoszGrajdek previously approved these changes Aug 20, 2024
Copy link
Contributor

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

LGTM! ✅

let shouldAddNewline = false;
const lastNode = target.childNodes[target.childNodes.length - 1];
// Remove the last <br> element if it's the last child of the target element. Fixes the issue with adding extra newline when pasting into the empty input.
if (lastNode?.nodeName === 'DIV' && (lastNode as HTMLElement)?.innerHTML === '<br>') {

Choose a reason for hiding this comment

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

Hi @Skalakid
Can you explain why you check for <div><br></div> instead of just <br> ?
Is there any document on how browsers add <br> when input is empty
Thanks for your help

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hello @drminh2807, it is because contenteditable tries to put everything into <div> elements by default. Especially when you have an empty input, the HTML content looks as follows:

<div><br /></div>

Because of this, when pasting text, a newline appears at the end of the new text. So we can remove it since it's kind of an "invisible" newline. There are more such situations, that's why we have inputUtils that parse contenteditable text content

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there any document on how browsers add
when input is empty

I haven't found one, every browser has different behaviour, so even if there is any article, it may describe only one of them

Choose a reason for hiding this comment

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

Thanks for your information
After debugging, I found that the browser no longer adds <div> wrapper around <br>, just <br> so this line of code no longer runs
Is there any documentation or evidence that this code used to run and now doesn't?
Is it because of browser changes?
Screenshot 2025-07-10 at 16 33 01
Screenshot 2025-07-10 at 16 33 08

Copy link
Contributor Author

@Skalakid Skalakid Jul 14, 2025

Choose a reason for hiding this comment

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

It still wraps line content with <div>, but after every newline, and that when this code is executed :D

Screen.Recording.2025-07-14.at.08.41.54.mov

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As far as I know, we don't have any documentation about parsing behavior. If you see that this condition is generating some issue, please submit a PR with repro steps, and we will review it ;)

Choose a reason for hiding this comment

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

Thanks for your super help
But I can't replicate like you
I just ran latest main 0.1.292, but the log is quite different and no removing the last br fired

Screen.Recording.2025-07-15.at.08.04.47.mov
Screenshot 2025-07-15 at 08 04 27

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

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants