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

yTextToSlateElement strips properties from empty text nodes #415

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

BrentFarese
Copy link
Contributor

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
Copy link

changeset-bot bot commented Jul 21, 2023

⚠️ No Changeset found

Latest commit: 22ed98e

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
Copy link
Contributor Author

@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
Copy link
Owner

BitPhinix commented Jul 24, 2023

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 "abc" 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
Copy link
Contributor Author

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 "abc" 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).

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.

2 participants