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

Improve Lexical -> HTML and Lexical -> Lexical Copy and Paste Data Model Conversion #1996

Merged
merged 6 commits into from
May 3, 2022

Conversation

tylerjbainbridge
Copy link
Contributor

@tylerjbainbridge tylerjbainbridge commented Apr 26, 2022

Solves the problems described in #1957 and the Lexical -> HTML & Lexical -> Lexical (Clone) conversion functions now have identical outputs, which will make debugging issues in the future much easier.

This PR also adds an extractWithChild method to ElementNodes that can be used to ensure there's a tightly coupled relationship between parent and child. This is useful situations where the ElementNode provides important context for its children e.g. Heading -> TextNode or List -> ListItem.

NOTE: Most of the line count is from the new tests in clipboard.test.js.

Lexical -> Lexical (Clone) and Lexical -> HTML are tested individually and then they're both ran through the same processing code and compared to each other ensure the end output is the same.

Screen.Recording.2022-04-26.at.6.28.01.PM.mov

@vercel
Copy link

vercel bot commented Apr 26, 2022

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

Name Status Preview Updated
lexical ✅ Ready (Inspect) Visit Preview May 3, 2022 at 2:54PM (UTC)
lexical-playground ✅ Ready (Inspect) Visit Preview May 3, 2022 at 2:54PM (UTC)

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Apr 26, 2022
@tylerjbainbridge tylerjbainbridge marked this pull request as ready for review April 28, 2022 19:25
@tylerjbainbridge
Copy link
Contributor Author

PR is ready for review.

): boolean {
let shouldInclude = currentNode.isSelected();
const shouldExclude =
$isElementNode(currentNode) && currentNode.excludeFromCopy();
Copy link
Collaborator

Choose a reason for hiding this comment

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

I just noticed the excludeFromCopy logic here (and before) is wrong. We should be ignore extracting the children an element that is excluded from copy, as we still want to copy that, just not the element itself. This will be important for commenting. You can check this today with overflow node.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I fixed this and added a comment, but I think this should actually be changed by updating the cloned "state" data model & the $generateNodes function. The range + nodeMap approach creates some issues when the core structure of the tree is modified while copying. E.g. when parents no longer reference the right children, a node moves up or down the tree, etc. The way "cloned" nodes work is a bit dangerous because if you use .getWritable() you're actually modifying the original node too since it looks it up by key.

I propose that we build an entirely new tree composed of deep clones in memory (never committed to the editor state), that way there's no risk of the actual editor contents being affected during the cloning process.

let shouldInclude = currentNode.isSelected();
const shouldExclude =
$isElementNode(currentNode) && currentNode.excludeFromCopy('html');
let clone = $cloneWithProperties<LexicalNode>(currentNode);
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we clone basically just so we can splitText without mutating the current tree?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that's right, and for when the __children array changes. I noted this above, but I think building a new tree in memory with deep cloned nodes is the safest (and most intuitive) approach moving forward.

const topLevelNode = topLevelChildren[i];
$appendSelectedNodesToHTML(editor, selection, topLevelNode, container);
}
return container.innerHTML;
Copy link
Contributor

Choose a reason for hiding this comment

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

To make this work on the server, I wonder if it'd be better to have exportDOM return some abstract interface that we can then render directly to a string here. For instance, it could be something like:

interface ExportDOMResult = {
  tag: string; // p, div, ul, li, etc...,
  style: {[key: string]: string},
  ...
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this is a step in the right direction, but some nodes add extra elements and move their children around during the exportDOM step e.g. Tables add a colgroup and move the child rows into a tbody.

Maybe exportDOM could return a similar structure to what you proposed above, but one that allows for nested structures? Not sure. Probably out of the scope of this PR.

I do worry that making exportDOM radically different than createDOM is pretty confusing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Related conversation on Maksim's headless PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was worried the signature of exportDOM might be hard to change later, but the return value is abstracted into an object we can control the shape of, so I think we should be OK.

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 long as exportDOM returns something that can be translated into an HTML string we should be able to toy with and refine these APIs as we discover limitations :)

@trueadm
Copy link
Collaborator

trueadm commented May 2, 2022

Any chance for some copy and paste e2e tests? Can’t we check the clipboard contents in e2e tests for everyone we do a copy in each test that does a copy? Like an assertion on the clipboard content?

@tylerjbainbridge
Copy link
Contributor Author

tylerjbainbridge commented May 2, 2022

Any chance for some copy and paste e2e tests? Can’t we check the clipboard contents in e2e tests for everyone we do a copy in each test that does a copy? Like an assertion on the clipboard content?

@trueadm We can do that, but these functions are consumed directly by the ones that add the strings to the clipboard. So in a way we'd be testing that the browser clipboard API works rather than the actual changes introduced in this PR.

Definitely open to doing this in a follow up PR though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants