Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rich text editor | Support for pasting absolute links when copied from other sources #1559

Merged
merged 24 commits into from
Sep 28, 2023

Conversation

vivinkrishna-ni
Copy link
Contributor

@vivinkrishna-ni vivinkrishna-ni commented Sep 21, 2023

Pull Request

🤨 Rationale

When links are copied from other sources and pasted into the editor, they initially render as plain text. However, they will be transformed into links by adding whitespace or by pressing the Enter key.

Additionally, when hyperlinks (a link behind different text content) are copied and pasted into the editor, only the text content of the link (not the actual URL) is pasted by default or using Ctrl + V. When pasting as plain text or using Ctrl + Shift + V, it still pastes the same text content.

Fixes the following bugs:

  1. Bug 2525823: Rich text editor | Copying a link from Edge browser which is not pasting as a link
  2. Bug 2525827: Rich text editor | Absolute links requires space or enter after pasting the link

👩‍💻 Implementation

  1. By utilizing transformPasted, if the pasted content includes a link mark, the text content of that node is replaced with the href value of the link mark. However, if the URL within the href attribute doesn't start with https or http protocols, then the text content is pasted as plain text within the same node.
  2. In addition to the nimble-anchor tag in parseHTML, added the <a> tag. This is necessary because when copying a link from an external source, the link will typically be enclosed within an <a> tag, which helps the editor identify it as a link and render it within an <a> tag.

🧪 Testing

  1. To simulate the pasting of links into the editor, include the pasteHTMLToEditor function in the page objects.
  2. I have created unit tests to verify that when a valid anchor element is pasted, it is displayed as a link. Additionally, when a hyperlink from an external source is pasted, it remains an absolute link.
  3. I have unit tests to check that when invalid links are pasted, they should be rendered as plain text.

✅ Checklist

  • I have updated the project documentation to reflect my changes or determined no changes are needed.

Co-authored-by: vikisekarNI <94439533+vikisekarNI@users.noreply.github.com>
Co-authored-by: vikisekarNI <94439533+vikisekarNI@users.noreply.github.com>
Comment on lines 365 to 370
const fragmentAsJson = (slice.content.toJSON() as NodeObj[]) ?? [];
updateLinkNodes(fragmentAsJson);
const modifiedFragment = Fragment.fromJSON(
this.tiptapEditor.schema,
fragmentAsJson
);
Copy link
Member

Choose a reason for hiding this comment

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

instead of using toJSON and fromJSON can you use the prose-mirror model apis directly?
The toJSON and fromJSON approach seems to lose a lot of type information when manipulating the fragment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Initially, we tried using the prose-mirror model to manipulate the fragment, but the text content of any node is a read-only property.

The other approach we also tried is, after finding the link mark in a text node, thinking of creating a new text node with the same marks and replacing it in the same object. However, we felt that this operation involves modifying the node replacements which might cause an issue with the other supported formatings. We also couldn't find a way to just update the text content without affecting the existing nodes. So we decided to utilize the JSON formats to update the text nodes without altering the existing nodes.

Copy link
Member

Choose a reason for hiding this comment

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

If this workaround is not temporary it seems like it would be better to understand the prose-mirror model behavior better. I would be surprised if the model cannot handle updating the text for a node.

If this is all a temporary workaround that will not need to be maintained long-term across prose mirror updates that will be scheduled to removed soon then the weaker typing being done here may be a an ok temporary implementation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After exploring deep into the ProseMirror model, found that we can utilize the given APIs to update the Link node value. I have updated the logic without converting to JSON formats and retains all the type information that ProseMirror offers.

We also tested all different kinds of content copied from external sources and verified it worked as expected. So we believe this can be more cleaner approach than the previous one. This method can be updated/removed once we support hyperlinks. I have added the necessary code comments too.

@vivinkrishna-ni vivinkrishna-ni requested review from rajsite and removed request for suseendran-ni September 27, 2023 12:50
@rajsite rajsite enabled auto-merge (squash) September 28, 2023 03:18
@rajsite rajsite merged commit 3d5285d into main Sep 28, 2023
9 checks passed
@rajsite rajsite deleted the users/vivin/paste-link-support branch September 28, 2023 03:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants