yTextToSlateElement strips properties from empty text nodes

[BrentFarese]

It seems yTextToSlateElement strips properties/marks from empty text nodes. This causes issues b/c, when editor.connect is called it sets the output of yTextToSlateElement to editor.children here.

Stripping properties from empty text nodes results in information loss and bugs (we are experiencing a bug at Aline).

[changeset-bot[bot]]

⚠️ No Changeset found

Latest commit: 22ed98e3a531d2fb4eddab4fb55b488e666b3353

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

[BrentFarese]

@BitPhinix it seems that YText cannot have an empty string in YJS ''? See here in YText.insert for example, which ignores empty strings.

Maybe we should use something like zero-width space to encode empty text nodes and, then on the way out of YJS, turn them into ''. Would that work to preserve empty text nodes?

Is there some way to do empty text nodes in YJS correctly?

[BitPhinix]

I made this trade-off when deciding to "flatten" the slate tree in the yjs representation for better merging characteristics.

Since [{text: "a", bold" true}, {text: "b"}, {text:"c", italic: true}] -> Y.XmlText with content "ab~c~" including empty slate text in the representation is quite tricky / causes a lot of overhead.

Since slate merges empty text nodes by default and treats them as disposable, storing data that isn't for local use on the current selection is an anti-pattern in my eye. In most cases, that state should be handled on the parent element, but I might be overlooking a use case. (Or if that's not possible, it can also be enforced via normalization)

[BrentFarese]

I made this trade-off when deciding to "flatten" the slate tree in the yjs representation for better merging characteristics.

Since [{text: "a", bold" true}, {text: "b"}, {text:"c", italic: true}] -> Y.XmlText with content "ab~c~" including empty slate text in the representation is quite tricky / causes a lot of overhead.

Since slate merges empty text nodes by default and treats them as disposable, storing data that isn't for local use on the current selection is an anti-pattern in my eye. In most cases, that state should be handled on the parent element, but I might be overlooking a use case. (Or if that's not possible, it can also be enforced via normalization)

Here is an example use case we need empty text nodes for <text bold /><link url=google.com><text>Google<text /><link /><text italic />. If you don't preserve the empty text nodes around the link, then you lose formatting information. YJS strips out the empty text nodes => Slate normalization re-inserts empty nodes but without formatting and information is lost.

This is a simple example there are others where the information loss is more severe/causes weirder bugs. In our case, empty text nodes w/ comment information are being stripped out and it causes a bug in our comments implementation.

I think it's completely fine to merge/flatten text nodes w/ the same properties, but not fine to remove all empty text nodes (which is what I am seeing).