[$250] Markdown - Cursor position moves to start when inserting a new break in a hyperlink

[lanitochka17]

If you haven’t already, check out our contributing guidelines for onboarding and email [email protected] to request to join our Slack channel!

---

Version Number: 1.4.85-0
Reproducible in staging?: Y
Reproducible in production?: Y
If this was caught during regression testing, add the test name, ID and link from TestRail: https://expensify.testrail.io/index.php?/tests/view/4641558
Issue reported by: Applause - Internal Team

Action Performed:

1. Navigate to staging.new.expensify.com
2. Open any conversation
3. Send any hyperlink using format
4. Get into editing mode on the hyperlink sent on step 3
5. Position the cursor to be at the end of the text content right before the last "]" and insert a line break

Expected Result:

The cursor positions itself in the beginning of the new line

Actual Result:

The cursor positions itself in the beginning of the first line

Workaround:

Unknown

Platforms:

Which of our officially supported platforms is this issue occurring on?

• Android: Native
• Android: mWeb Chrome
• iOS: Native
• iOS: mWeb Safari
• MacOS: Chrome / Safari
• MacOS: Desktop

Screenshots/Videos

Add any screenshot/video evidence

Bug6517276_1718718866342.bandicam_2024-06-18_16-47-50-581.mp4

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~019e1861b7a8f9e8fd
• Upwork Job ID: 1803124326197693470
• Last Price Increase: 2024-06-18

Issue Owner

Current Issue Owner: @ikevin127

[melvin-bot]

Triggered auto assignment to @sakluger (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details. Please add this bug to a GH project, as outlined in the SO.

[lanitochka17]

@sakluger FYI I haven't added the External label as I wasn't 100% sure about this issue. Please take a look and add the label if you agree it's a bug and can be handled by external contributors

[lanitochka17]

We think that this bug might be related to #vip-vsp

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~019e1861b7a8f9e8fd

[melvin-bot]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @ikevin127 (External)

[huult]

Proposal

Please re-state the problem that we are trying to solve in this issue.

Cursor position moves to start when inserting a new break in a hyperlink

What is the root cause of that problem?

When the user inputs a newline character at the end of a Markdown string, it disappears after parsing. Therefore, when comparing conditions, this discrepancy can lead to incorrect evaluations

What changes do you think we should make in order to solve the problem?

Trim the Markdown input prior to condition checks to mitigate whitespace discrepancies.

In parseExpensiMarkToRanges at parser, should add .trim() like in the code snippet below:

function parseExpensiMarkToRanges(markdown: string): Range[] {
  try {
    const html = parseMarkdownToHTML(markdown);
    const tokens = parseHTMLToTokens(html);
    const tree = parseTokensToTree(tokens);
    const [text, ranges] = parseTreeToTextAndRanges(tree);

-   if (text !== markdown) {
+   if (text !== markdown.trim()) { 

POC

Screen.Recording.2024-06-19.at.04.06.39.mov

What alternative solutions did you explore? (Optional)

[ikevin127]

@huult Thanks for your proposal. A few clarifications before I can reach a point where I can verify your proposal:

1. The root cause of your proposal is not clear enough. Can you expand on the why of the following statements ?
   • When the user inputs a newline character at the end of a Markdown string, it disappears after parsing
   • when comparing conditions, this discrepancy can lead to incorrect evaluations
2. I wasn't able to fix the issue applying the suggested solution. Can you detail how you implemented your solution in the Expensify app such that it works ?

Note: I applied the suggested changes in the react-native-live-markdown repo, compiled and used the compiled react-native-live-markdown-parser.js to replace in Expensify/node_modules/@expensify/react-native-live-markdown both parser and lib/parser folders -> my Expensify web app reloaded but I can still reproduce the issue.

Here's a console log of the variables after implementing the solution:

Given these results, it would make sense that the error is still thrown from that if condition. So I'm curious if you left out a certain part of the solution from your proposal or if not - how come the issue is fixed on your side ? 🤔

Note: Looks like the difference between original / processed input is \n regardless of trimming or not.

[badeggg]

Proposal

Please re-state the problem that we are trying to solve in this issue.

Cursor move to beginning when insert trailing \n to a link text.

What is the root cause of that problem?

There are two errors thrown out actually.

The first error is because g1.trim() (we should care this trim too) has trimmed link text which cause it's impossible to recover to original comment-text from parsed html with code here, which cause condition text !== markdown to true.

The first error can somehow trigger this condition if (nextChar !== '\n'), which lead to the second error, which finally cause cursor moving unexpectedly. The second error is actually a duplication of this.

What changes do you think we should make in order to solve the problem?

Fix the first error
To fix the first error, we can either change g1.trim() to g1:
from

                    return `<a href="${Str.sanitizeURL(g2)}" data-raw-href="${g2}" data-link-variant="labeled" target="_blank" rel="noreferrer noopener">${g1.trim()}</a>`;

to

                    return `<a href="${Str.sanitizeURL(g2)}" data-raw-href="${g2}" data-link-variant="labeled" target="_blank" rel="noreferrer noopener">${g1}</a>`;

Similar change apply to this line

Fix the second error
The second error is actually a duplication of this. And if the first error is fixed, the second error will not be triggered in this issue case. So I do not recommend fix the second error here.

What alternative solutions did you explore? (Optional)

N/A

[Amoralchik]

Proposal

Please re-state the problem that we are trying to solve in this issue.

Cursor position moves to start when inserting a new break in a hyperlink

What is the root cause of that problem?

We set cursor to the front of text in setCursorPosition at react-native-live-markdown-parser: here

What changes do you think we should make in order to solve the problem?

I got stable and successful results changed the first "if" statement conditions to:

if (nextChar !== '\n' && textNodes.length > 1) {
  range.setStart(textNodes[i], 0);
} 

[badeggg]

@Amoralchik Hi, gently remind, your suggesting code change is trying to fix the second error mentioned in my proposal, which is a duplication of this. And I personally do not recommend change if (nextChar !== '\n') { to if (nextChar !== '\n' && textNodes.length > 1) {.

[Amoralchik]

@badeggg
Thank you for noticing. I have tested my code using actions from #43239, and everything is working fine, I haven't encountered any errors in my console.
I also see a message from the 'react-native-live-markdown' team, so let's wait for their response.

[huult]

Hi @ikevin127 ,

Firstly, I would like to thank you for reviewing my proposal.

When the user wants to enter a line where the value is text, a newline character (\n) is added to the value. This value is then sent to the parseExpensiMarkToRanges function of react-native-live-markdown. Like the image below:

So I trimmed the markdown to remove \n from it, but that was incorrect. Accidentally, this issue was resolved because the selection did not reset to {start: 0, end: 0}

Thanks,

[ikevin127]

🙏 Thanks everybody for your interest in fixing this issue!

Currently @Amoralchik's solution is the only one that fixes the issue. Performing the action still throws the match related error (for obvious reasons), but the cursor position is not being reset anymore -> which fulfils our issue's expected result:

Screen.Recording.2024-06-19.at.18.51.58.mov

What's Next ?

Regarding the proposed solution:

-    if (nextChar !== '\n') {
+    if (nextChar !== '\n' && textNodes.length > 1) {
        range.setStart(textNodes[i], 0);
    } 

I'm afraid that the change might cause regressions even though all react-native-live-markdown tests are still passing with the change. Given this, before moving on with hiring somebody here, I'd appreciate if @BartoszGrajdek could chime in regarding the proposed solution (as they suggested here).

[badeggg]

@ikevin127 Hi, have you checked my proposal?

[ikevin127]

Yes, I did check all proposals and the issue was still reproducible in all of them except Amoralchik's.

Keep in mind that we're not looking to get rid of the first error since:

1. In our case, it's inevitable for the error to show up because the original vs processed input is different once we add the newline \n.
2. The first error is not what causes cursor reset (what this issue is about).

[badeggg]

Then the second error has same root cause of this issue: #43239, and I do posted my proposal there, and have linked it in my proposal for this issue.

[ikevin127]

Different issues, (slightly) different solutions. Additionally mallenexpensify notified you in the other issue to:

hang tight for now plz while we get a better handle on managing markdown improvements more holistically.

So don't worry, I also tagged BartoszGrajdek in #43922 (comment) for this issue and I mentioned that:

before moving on with hiring somebody here, I'd appreciate if BartoszGrajdek could chime in regarding the proposed solution

Whenever we get feedback, if they think that the other issue's solution would fix this one as well -> then nobody will get hired for this one, otherwise they might decide that we want to fix them separately.

[badeggg]

@ikevin127 Code changes in my proposal actually can fix this issue. Fix the first error indeed lead the second error disappears for this issue.

I kind don't agree that:

The first error is not what causes cursor reset (what this issue is about).

To confirm code changes in my proposal can fix this issue:

1. From root folder of react-native-live-markdown
2. yarn install
3. cd parser/node_modules/expensify-common/dist/
4. Edit file "ExpensiMark.js"(get rid of the two trim() in rule for 'link')
5. From root folder of react-native-live-markdown
6. cd parser
7. npm run build to build new react-native-live-markdown-parser.js which is in current parser folder
8. From root folder of expensify-app
9. cd node_modules/@expensify/react-native-live-markdown
10. Copy the built react-native-live-markdown-parser.js to parser/react-native-live-markdown-parser.js
11. Copy the built react-native-live-markdown-parser.js to lib/parser/react-native-live-markdown-parser.js
12. From root folder of expensify-app
13. cd node_modules/expensify-common/dist
14. Edit file "ExpensiMark.js"(get rid of the two trim() in rule for 'link')
15. Start expensify-app and verify

@ikevin127 Maybe you don't replace all react-native-live-markdown-parser.js files?

[BartoszGrajdek]

Hi! Thank you for tagging me here 🙌🏻

So I've looked through quite a few issues in recent days. I think this one is related to:

1. [$500] Chat - Console error and email changes to "a%20href=" for multiline email hyperlinks #43386
2. [$250] When editing comment, copy and paste text that has trailing \n, console error appears and send button changed to grey, can not send #43239 (just like you've said @ikevin127)

What @badeggg solution is trying to fix is actually the 1st issue linked. I agree that this is something we should aim to resolve, but I'm not sure if it's the best way to do it. This is a matter of preference that I would rather discuss with the Expensify Team. IMO there are 3 solutions to this problem:

1. Keep trimming the text content of a link and fix the bug directly in react-native-live-markdown (not a preferred option)
2. Stop trimming text content in expensify-common (this is what @badeggg suggested)
3. Ignore any links that contain line-breaks (I think this one makes the most sense)

What I'm going to do next is talk with the react-native-live-markdown team @ SWM to ask if there was a reason for multiline links to even be considered and confirm which of these options are valid (considering our current setup).

Next, we'll discuss it with the Expensify team to have a clear picture of how we want it to behave. 🔜

I'll keep you guys updated 🙏🏻

[badeggg]

@BartoszGrajdek Hi, about the 3rd solution:

Ignore any links that contain line-breaks

Be cautious, links that have leading or trailing spaces are causing same error, we can not recover to original comment text from parsed html after trimming.

[ikevin127]

Code changes in my proposal actually can fix this issue.

No, it doesn't fix our issue.

Maybe you don't replace all react-native-live-markdown-parser.js files?

Yes, I did.

Not sure if your solution works on any of the other issues, the only thing that I'm certain of is that it's not fixing this issue.

Checkout the video below where I'm demonstrating applying your solution and the result proving that it doesn't change / fix our issue (cursor position reset), nor the match related error which you stated that your solution will fix even though I stated in #43922 (comment) that:

The first error is not what causes cursor reset (what this issue is about).

which you said you disagree with, without actually providing any reasoning as to why 🤔

Getting back to your solution

1. I removed both .trim() from this expensify-common/ExpensiMark.ts block line 255 and line 261.

2. I added .trim() to the markdown variable from this react-native-live-markdown/parser/index.ts block line 246.

Note: I did this from E/App node_modules only for the files in the directory which actually impact the web app, this is proven by seeing the app reload whenever saving both file changes.

Watch video

Screen.Recording.2024-06-20.at.13.mp4

cc @badeggg

Conclusion

Regardless of your solution working or not for this issue (which it doesn't, as proven above), I would still have massive reservations in selecting it simply because those 2 .trim() were added there for a reason, which I wouldn't want to mess with.

The reason why I'm leaning towards Amoralchik's solution is because:

1. It actually works, fixing our issue according to the issue's expected result.
2. The proposed change is quite narrow in scope and primarily affects the treatment of newlines within the text, therefore is less likely to cause regressions compared to removing 2 instances of .trim() from the ExpensiMark function which handles all types of links (broader scope).

Even if we decide to proceed with their solution, I still have some reservations when it comes to the change potentially causing regressions as mentioned in #43922 (comment), reason for which I decided to include BartoszGrajdek in the discussion for this issue as well since they are part of the team which created and maintain the react-native-live-markdown library, meaning they have more context on the piece of code we're looking at changing.

[badeggg]

@ikevin127 The video you provided, from 42sec to 48sec, that's not the change I suggested. My suggested change is only delete two trim()(this one and this one) of expensify-common, no change with if (text !== markdown) {. In my original comment, I did suggest change to if (text !== markdown) {, but I delete that change right away(less than 5 minutes).

To change node_modules/@expensify/react-native-live-markdown/lib/parser/react-native-live-markdown-parser.js file, you need goto react-native-live-markdown source code and change expensify-common dependency and run command to build it, then copy to expensify-app. Details in my this comment, step 1 to step 11.

[badeggg]

About this:

which you said you disagree with, without actually providing any reasoning as to why 🤔

I believe the error trigger path is this:

1. Since that two trim(), we can not recover to original comment from parsed html
2. So condition if (text !== markdown) met
3. Error [react-native-live-markdown] Parsing error: the processed text does not match the original Markdown input... thrown.
4. Program not process as expected to following codes
5. Error 2 appears.
6. Cursor move unexpectedly

Trigger path is linear. You can't say the first error is not the root cause.

[badeggg]

Just in case, I have to say, file node_modules/@expensify/react-native-live-markdown/lib/parser/react-native-live-markdown-parser.js rely on markdown parser from 'expensify-common'. Deleting two trim() needs reflect to it. That's why we need rebuild it.

[ikevin127]

Hey, did you know that you can edit your previous comment instead of posting a new one everytime you have something else to add to the discussion ?

There's clearly a language barrier since I wasn't able to figure out from your proposal that when you say "remove the 2 .trim() from expensify-common" you actually mean:

• apply these changes to the package within node_modules of the react-native-live-markdown library, then rebuild react-native-live-markdown and pass the generated .js script to the Expensify/App node_modules/@expensify/react-native-live-markdown.

But regardless, I stand by what I said here:

I would still have massive reservations in selecting it simply because those 2 .trim() were added there for a reason, which I wouldn't want to mess with.

Even if that solution works, I'd still pick the other one because removing 2 instances of .trim() from the ExpensiMark function handles all types of links, therefore having a broader scope -> more prone to regressions.

[badeggg]

did you know that you can edit your previous comment

I know this, I will amend my previous comment if adding one more comment is disturbing others.

language barrier ... "remove the 2 .trim() from expensify-common" you actually mean ...

I thought you know react-native-live-markdown relying on expensify-common. My mistake.

I would still have massive reservations in selecting it simply because those 2 .trim() were added there for a reason, which I wouldn't want to mess with.

If you say so.

Just try to clarify that code changes in my proposal can work. 🤝

[sakluger]

I've temporarily removed the Help Wanted label while we wait for @BartoszGrajdek to come back with the results of speaking with SWM. @BartoszGrajdek - is there a Slack link where I can we that discussion, or are you discussing offline?

[ikevin127]

I know this, I will amend my previous comment if adding one more comment is disturbing others.

Wouldn't call it disturbing but it's good to know that most of us that are participating to this issue (7 currently) are getting notified everytime somebody posts a new comment - you posted 3 in a row and that's why I mentioned the alternative 👍

I thought you know react-native-live-markdown relying on expensify-common. My mistake.

You are forgiven! For your convenience, here are some of the raw proposal template recommendations:

For Contributors

Reminder: Please use plain English, be brief and avoid jargon. Feel free to use images, charts or pseudo-code if necessary. Do not post large multi-line diffs or write walls of text.

For Reviewers

You are the first line of defense in making sure every proposal has a clear and easily understood problem with a "root cause". Do not approve any proposals that lack a satisfying explanation to the first two prompts. It is CRITICALLY important that we understand the root cause at a minimum even if the solution doesn't directly address it. When we avoid this step, we can end up solving the wrong problems entirely or just writing hacks and workarounds.

I want to highlight that it's paramount, if you want your proposals to be considered, that you are very specific with both the root cause of the issue, the proposed solution and where / how should this solution be implemented because:

1. Shows that you have a decent understanding of the codebase and what the issue is about ✅
2. Avoids assuming things, which can lead to miscomunication and time wasted ❌

If you say so.

Here's an example of regression your proposed solution will cause by removing only one .trim() from ExpensiMark:

• Issue: [HOLD for payment 2022-12-02] [$250] Link with trailing spaces doesn't got trimmed reported by @kerupuksambel #12511 check the issue's actual result and screenshots
• PR which fixed the issue (added the .trim()): trim link title when convert markdown to html expensify-common#471 proves that implementing your solution would basically revert this PR and reintroduce the issue

Again, regardless of whether your solution can work -> that doesn't mean much if it's going to revert already-fixed issues as our goal here is to improve the functionality with every PR.

Tip

1. If you feel that your proposals are not being understood / misconstrued, I suggest using tools like ChatGPT in order to format your final proposals when you are ready to post them in order to make it easier to understand for everybody that's going to review.
2. Before proposing a solution which removes some existing code / methods which can work and seems to be working, I'd suggest learning to use github blame which means you can view the line-by-line revision history for a file / piece of code. Note: This is how I found the above example of regression that your solution code change would revert back to if implemented.
3. If still interested -> I suggest taking on issues with less cognitive complexity in order to gradually get a deeper understanding of the codebase and the libraries we use as part of it.

cc @badeggg

[BartoszGrajdek]

Hello everyone! 👋🏻

A couple of things have changed since I last updated you here. Let me go through them one by one:

Should we support multiline links?

@BartoszGrajdek - is there a Slack link where I can have that discussion, or are you discussing offline?

I've spoken to the SWM team, and according to my colleagues, we already had a brief discussion about it earlier. The Expensify Team was leaning towards handling multiline links, so I don't think that's something we should be changing right now.

Cursor positioning problem

This has been fixed. If you go onto staging the cursor should be positioned correctly when creating a multiline link. That's due to this PR, which is patching react-native-live-markdown. These changes will be also added to the react-native-live-markdown lib later this week.

Unfortunately, this still doesn't fix our markdown / ExpensiMark problem.

Multiline link markdown

There are currently 2 issues that are related to problems with multiline hyperlinks. This one and one of the issues I've linked previously. Both of them are different but could conflict with one another in terms of solutions, so I'll let you decide if we want to keep both of them or maybe merge them @ikevin127 🙌🏻

As for fixing multiline hyperlink problems with .trim() I agree with @ikevin127

those 2 .trim() were added there for a reason, which I wouldn't want to mess with

I will try to research that problem a bit, with a more holistic approach, maybe inside react-native-live-markdown lib. If anyone has a valid solution we're still open to that, so waiting for more proposals 🙏🏻

[ikevin127]

Thank you for the detailed breakdown BartoszGrajdek, will get back on this soon!

[BartoszGrajdek]

@thienlnam this is also an issue related to live markdown, could we add it to the project? 🙏🏻

[melvin-bot]

@sakluger, @ikevin127 Uh oh! This issue is overdue by 2 days. Don't forget to update your issues!

[ikevin127]

Cursor positioning problem

This has been fixed. If you go onto staging the cursor should be positioned correctly when creating a multiline link. That's due to #44168, which is patching react-native-live-markdown. These changes will be also added to the react-native-live-markdown lib later this week.

@sakluger I can confirm that this issue is not reproducible anymore:

Video

Screen.Recording.2024-06-28.at.12.21.14.mov

Screen.Recording.2024-06-28.at.12.21.14.mov

Given that this issue's expected result was fulfilled (IMO), I think the other react-native-live-markdown related issues are out of scope here and will most likely handled by SWM as they continue working on and improving the library.

Note: I think this can be closed as fixed if no objections are raised.

There are currently 2 issues that are related to problems with multiline hyperlinks. This one and #43386 I've linked previously. Both of them are different but could conflict with one another in terms of solutions, so I'll let you decide if we want to keep both of them or maybe merge them @ikevin127 🙌🏻

As mentioned above, I think we should keep them separate as this one was already fixed while the other one is still reproducible. This being said, I'd be good to keep this one in mind when testing the fix for the other one to make sure the regression (this issue) is not reintroduced.

[sakluger]

Hey @ikevin127, thanks so much for retesting and confirming that it's no longer reproducible. I agree that we can close out this issue.

Thanks everyone!