Improve URL regex and separator detection in AutoLinkPlugin

NozomiSugiyama commented 1 year ago

close #3546

What this PR does:

This PR updates the URL detection method in the AutoLinkPlugin with the following changes:

Allow URLs with extensions (e.g., .png, .jpeg)
Allow URLs that do not start with "www" (e.g., gist.github.com)
Disallow domains ending with a dot (e.g., example.com.)
Disallow TLDs with fewer than 2 characters (e.g., example.c)
Make URL detection case-insensitive (e.g., https://example.com/)

Before:

After:

Please review and let me know if you have any suggestions or concerns.

vercel[bot] commented 1 year ago

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
lexical	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jul 6, 2023 3:15am
lexical-playground	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jul 6, 2023 3:15am

NozomiSugiyama commented 1 year ago

I've made additional corrections. Please review them.

Changes

instead of no longer considering delimiters, AutoLinkNode is now generated or edited by determining whether to combine the TextContent of adjacent TextNode and match it to the URL when generating AutoLinkNode. This change makes it AutoLinkNode even when text like url is input by multiple TextNodes

https://github.com/facebook/lexical/assets/19605052/869566b0-4bfb-4401-b142-ba6b6b8b0a60

When converting from Text to Markdown, AutoLinkNode will now work even if it contains an AutoLinkNode.

https://github.com/facebook/lexical/assets/19605052/b71bde4a-b226-475a-a46a-e4f6da9e1caa

ivailop7 commented 12 months ago

@NozomiSugiyama the integration tests are failing, if you are still looking after this PR

ivailop7 commented 11 months ago

@NozomiSugiyama are you planning to update this one, happy to help on this one if needed.

NozomiSugiyama commented 11 months ago

@ivailop7 My apologies for the delayed response. I have rebased this onto the latest main branch and fixed the issues with the findLast method that were causing the tests to fail.

ivailop7 commented 11 months ago

@ivailop7 My apologies for the delayed response. I have rebased this onto the latest main branch and fixed the issues with the findLast method that were causing the tests to fail.

Reran the tests, but unfortunately only the collab tests are passing, the rest are still failing.

NozomiSugiyama commented 11 months ago

@ivailop7 Ah! I missed that, my apologies. I've now fixed the behavior of the auto link node when a LineBreakNode comes at the end in plain-text mode. I have confirmed that the following tests are working correctly on my end:

env E2E_EDITOR_MODE=plain-text E2E_EVENTS_MODE=legacy-events npm run test-e2e-chromium
env E2E_EDITOR_MODE=plain-text E2E_EVENTS_MODE=modern-events npm run test-e2e-chromium
env E2E_EDITOR_MODE=rich-text E2E_EVENTS_MODE=legacy-events npm run test-e2e-chromium
env E2E_EDITOR_MODE=rich-text E2E_EVENTS_MODE=modern-events npm run test-e2e-chromium

Please check the changes again. Thank you.

ivailop7 commented 11 months ago

Thank you @NozomiSugiyama for addressing the comments. @fantactuka or @zurfyx could any of you scan this through again, it looks OK to me, but I think someone a bit more familiar with that part of the code would be nice to sign off as well.

ivailop7 commented 11 months ago

Hey @zurfyx and @fantactuka can you have a look if this looks good to you too, looks mergeable to me.

ivailop7 commented 10 months ago

PR should solve: https://github.com/facebook/lexical/issues/4692

ivailop7 commented 10 months ago

@NozomiSugiyama have you had a chance to look into Acy's comments?

ivailop7 commented 10 months ago

Hey @acywatson , @NozomiSugiyama hasn't replied recently, so I addressed your comments and pushed them to their branch. Could you have have a look. I changed the .getLatest() to .getWritable() and fixed the double reverse thing. the splitNodes method is used to split the before and after text away from the text converted to a link.

robfig commented 9 months ago

I would love to get this in for the next version. The current AutoLinkPlugin is close but not quite able to be used in production for our use case. This would do it.

ivailop7 commented 8 months ago

@NozomiSugiyama let's split this PR into Markdown and non-Markdown, in order to move things forward with it.

NozomiSugiyama commented 7 months ago

@ivailop7 I apologize for the repeated delays in my responses. Regarding splitting the PR into Markdown and non-Markdown components, would it be appropriate to extract the changes from packages/lexical-markdown/src/MarkdownShortcuts.ts into a separate PR?

However, I'd like to point out that due to the changes in the AutoLinkPlugin in this PR, the following E2E tests will not pass. Hence, splitting them in a way that ensures all tests pass might be challenging:

[chromium] › packages/lexical-playground/__tests__/e2e/Markdown.spec.mjs:683:5 › Markdown › can convert "hello [world](https://www.test.com/)!" shortcut 
[chromium] › packages/lexical-playground/__tests__/e2e/Markdown.spec.mjs:858:3 › Markdown › can type text with markdown 
[chromium] › packages/lexical-playground/__tests__/e2e/Markdown.spec.mjs:960:3 › Markdown › can adjust selection after text match transformer

The main reason I made modifications to the MarkdownShortcuts is that previously, due to the absence of delimiters like spaces, certain text wasn't recognized as URLs. With the current changes, they are now identified as AutoLinks. Consequently, the MarkdownShortcuts does not function correctly when AutoLinks are included in the evaluated text.

ivailop7 commented 7 months ago

@ivailop7 I apologize for the repeated delays in my responses. Regarding splitting the PR into Markdown and non-Markdown components, would it be appropriate to extract the changes from packages/lexical-markdown/src/MarkdownShortcuts.ts into a separate PR?

However, I'd like to point out that due to the changes in the AutoLinkPlugin in this PR, the following E2E tests will not pass. Hence, splitting them in a way that ensures all tests pass might be challenging:
[chromium] › packages/lexical-playground/__tests__/e2e/Markdown.spec.mjs:683:5 › Markdown › can convert "hello [world](https://www.test.com/)!" shortcut 
[chromium] › packages/lexical-playground/__tests__/e2e/Markdown.spec.mjs:858:3 › Markdown › can type text with markdown 
[chromium] › packages/lexical-playground/__tests__/e2e/Markdown.spec.mjs:960:3 › Markdown › can adjust selection after text match transformer
The main reason I made modifications to the MarkdownShortcuts is that previously, due to the absence of delimiters like spaces, certain text wasn't recognized as URLs. With the current changes, they are now identified as AutoLinks. Consequently, the MarkdownShortcuts does not function correctly when AutoLinks are included in the evaluated text.

Hey! Right, that's part of the problem with this PR, we can't couple the Markdown Shortcuts Plugin to the AutoLinks Plugin, the two should be able to operate completely independently of each other an not rely on behaviour of the other as much as possible. Let's make the Markdown Shortcuts mergeable, so extracting stuff to packages/lexical-markdown/src/MarkdownShortcuts.ts is OK and the tests need to pass for the Markdown PR and then we can revisit the logic for the Autolinking.

arp commented 5 months ago

@ivailop7 @NozomiSugiyama is there any further progress on this, possibly in a different PR? The problem is still happening on the Playground. Maybe I could help to move this forward?

robfig commented 2 months ago

I tested out this PR and it works a lot better than the stock AutoLinkPlugin. I did find one issue. URLs include trailing punctuation in the link, which is undesirable. A common usage scenario is putting a URL in a sentence, e.g.

"Open http://localhost:5560/r/#/."

If you type "Open ", paste the URL, and type ".", the dot is linkified. I think a heuristic of excluding any trailing punctuation from the link would be best.

Thanks

ivailop7 commented 2 months ago

I tested out this PR and it works a lot better than the stock AutoLinkPlugin. I did find one issue. URLs include trailing punctuation in the link, which is undesirable. A common usage scenario is putting a URL in a sentence, e.g.

"Open http://localhost:5560/r/#/."

If you type "Open ", paste the URL, and type ".", the dot is linkified. I think a heuristic of excluding any trailing punctuation from the link would be best.

Thanks

This PR is quite old now and it's unlikely to get merged, so I'm going to close it. If you are interested in taking the AutoLink part-only and cleaning it up and raising a new one, happy to give it another go.

facebook / lexical