typewriter-editor / typewriter

A rich text editor based off of Quill.js and Ultradom, and using Svelte for UI.
MIT License
394 stars 32 forks source link

New lines create new line IDs instead of moving them. #77

Closed taylorhadden closed 3 years ago

taylorhadden commented 3 years ago

This change modifies the handling of newline insertions in TextChange so that the leading text retains the current line ID and the trailing text is given a new line ID.

Background

In the current implementation (without this change), inserting a new line actually moves the current line ID to the newly created line. In this example (using ^ as cursor position), editing :

Hello, I am a fancy line
                        ^

...to...

Hello, I am a fancy line

^

...you would see a change in line ids from something like ['xhfkdf'] to ['hreoff', 'xhfkdf']. The new line takes the old line's ID.

This is caused by how lines are re-parsed out of a new Delta document and where Delta stores line information (on the ending \n character). The system loops through each line of the delta and either uses the existing id or makes a new id (see Line.fromDelta() and Delta.eachLine()).

When you hit enter in the middle of a line, this inserts a new \n character, with no attributes. This means when lines are parsed out, the content after the insertion retains the existing line id while the content before the insertion has a new ID generated for it.

When a new line is inserted at the very end of a paragraph, your cursor is technically just before the ending newline of the paragraph. The existing paragraph is then interpreted as a new paragraph, while the new paragraph is interpreted as if the old paragraph had its content deleted.

When the rendering pass finally happens, this creates problems with custom elements. Because of the line ids, the existing content is seen as brand new, and all child elements are destroyed and recreated. For basic text, this is instant and invisible. For complex custom elements, they must be reinitialized. This can cause noticeable flicker. It is most easily diagnosed by logging on the custom element's connectedCallback() method; new lines will cause that method to fire.

Implementation

This change works by applying the current ID (following the same concept as applying the current line format) to the new \n character. The subsequent formatLine() call is then automatically adjusted to target the existing newline. Reseting its id makes the mechanism in Line.fromDelta() interpret that line as a new line that needs a new ID.

With this change, everything preceding the new \n is left alone, and custom elements are able to remain stable. This greatly enhances the sensation of stability when editing text with such elements.

jacwright commented 3 years ago

We actually had this solution in place before, but it caused issues when saving changes to disk or sending them across a socket, because the id is meant to be ephemeral for optimization in-memory. Once the changes start including IDs, they become permanent. The easy fix was to remove it, but I see the problem with rendering custom elements.

Is this something we can do in the TextDocument class? Whenever a \n is added via a change operation in TextDocument.apply() it could keep the ids on the previous lines. Perhaps we even remove id from Line.attributes and have it reside directly on Line instead to keep the separation clear. I can take a stab at this.

taylorhadden commented 3 years ago

Those are good points. I suppose you could remove the ids on serialization, but that's not ideal.

I like the idea of moving the id out of the line attributes. I did notice that the line id is used in lines.ts. Would this be something that would be broken / have to be fixed if line ids were moved?

Another location where it might make sense to do this is in Line.fromDelta, but only if ids stay in the attributes. Otherwise, the TextDocument approach seems like a good one.

jacwright commented 3 years ago

Fixed with https://github.com/typewriter-editor/typewriter/pull/79.