[lexical-markdown][lexical-playground] Bug Fix: Convert tabs in TabNode at import#8211
Conversation
Signed-off-by: Simon Bauchet <simon@vivaldi.com>
Signed-off-by: Simon Bauchet <simon@vivaldi.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
| if ($isParagraphNode(child) && child.getTextContent().includes('\t')) { | ||
| for (const paragraphChild of child.getChildren()) { | ||
| $createTabInNode(paragraphChild); | ||
| } | ||
| } |
There was a problem hiding this comment.
This would probably be better written by traversing for all of the TextNodes first, computing paragraphNode.getTextContent() is doing a whole extra traversal of that subtree
if ($isElementNode(child)) {
for (const textNode of child.getAllTextNodes()) {
$normalizeMarkdownTextNode(textNode);
}
}For the simplest correct implementation that would preserve formats and such, the tab replacing code should find the appropriate offsets, use textNode.splitText(), and then replace the nodes that are just tabs with $createTabNode().
Signed-off-by: Simon Bauchet <simon@vivaldi.com>
| while (tabOffset !== -1) { | ||
| const splitNodes = currNode.splitText(tabOffset, tabOffset + 1); | ||
| splitNodes.forEach((node) => { | ||
| if (node.getTextContent() === '\t') { | ||
| node.replace($createTabNode()); | ||
| } | ||
| }); | ||
| currNode = splitNodes[splitNodes.length - 1]; | ||
| tabOffset = currNode.getTextContent().indexOf('\t'); |
There was a problem hiding this comment.
I don't think this logic is correct if the node ends with a tab? It looks like it will loop infinitely. It could also be done a little more clearly if all the tabs are found first and splitText is called at most once
There was a problem hiding this comment.
Could have yes, but it seems there's a trim before because I have no '\t' or space at the end of a string when calling getTextContent().
…replace Signed-off-by: Simon Bauchet <simon@vivaldi.com>
etrepum
left a comment
There was a problem hiding this comment.
I think the array approach here can end up with tab offsets like [0,1,1,2] when you have adjacent tabs which will create and then GC empty TextNodes since the current splitText algorithm doesn't de-dupe the indices.
|
|
||
| // Look in node for '\t' and create a TabNode for each occurrence. | ||
| function $normalizeMarkdownTextNode(textNode: TextNode): void { | ||
| const tabOffsets = []; |
There was a problem hiding this comment.
| const tabOffsets = []; | |
| const tabOffsets: Set<number> = new Set(); |
|
|
||
| // Find all tab occurrences | ||
| while (index !== -1) { | ||
| tabOffsets.push(index, index + 1); |
There was a problem hiding this comment.
| tabOffsets.push(index, index + 1); | |
| tabOffsets.add(index); | |
| tabOffsets.add(index + 1); |
There was a problem hiding this comment.
Indeed, I thought I covered this case in unit test but this test had '\t(space)\t', my bad.
I refactored to use a Set and added a test
Signed-off-by: Simon Bauchet <simon@vivaldi.com>
This is a follow up of #7845. I made a new PR to make it clean.
Reported issue: #7820
To summarize, if you use indent button and then switch to markdown, indent will be lost.
In this fix, once all transformers have been applied, loop on nodes to convert all '\t' in TabNodes.
I decided to do it here and not in
$importBlocksas it was gonna create several nodes and break the function flow which only use one TextNode.Closes #7820