Text deletion event includes erroneous retain attribute

[ezeidman]

Describe the bug
Deleting text can lead to a YEvent which includes an extra attribute update

To Reproduce
Steps to reproduce the behavior:

1. Create an XmlText which contains in order (1) some text with formatting and (2) an embedded XmlText
2. observeDeep events on the parent XmlText
3. Delete the text prefix from the XmlText

Or in code:

  const doc =  new Y.Doc()
  const root = doc.get("text", Y.XmlText) as Y.XmlText;
  const p1 = new Y.XmlText();
  p1.setAttribute("type", "paragraph")
  root.insertEmbed(0, p1);
  root.insert(0, "Text", {bold: true});

  root.observeDeep((events) => {
    events.forEach(event => {
      console.log("Event", JSON.stringify(event.changes, null, 4));
    })
  })
  root.delete(0, 4);

Expected behavior
Expect to observe an event with a delta that has one delete with a length equal to however long the text was.

Actual behavior
For some reason the event includes a retain that tries to null out the formatting attribute on the embedded XmlText

Environment Information

• Yjs version = 13.5.45

Additional context
Ran into this problem while using Slate Yjs, which turns the retain into set_node Slate operation over here: https://github.com/BitPhinix/slate-yjs/blob/966b9325ff312058023d017f857fbb6ac9a7a8a4/packages/core/src/applyToSlate/textEvent.ts#L88. This then tries to update the representation of the paragraph in Slate based on formatting attributes that the text had, which doesn't really make sense and leads to an invalid state.

There probably are a couple workarounds for this issue but it seems odd that deleting text would result in a retain signaling a change to content that didn't actually change. Is there any reason to structure the YEvents this way?

From debugging the issue it seems when we're creating the YTextEvent, when we process the first deleted ContentFormat, we set the attribute to null in attributes but we don't remove it when processing the second deleted ContentFormat. This is around

yjs/src/types/YText.js

Lines 718 to 727 in e0a2f11

	} else if (this.deletes(item)) {
	oldAttributes.set(key, value)
	const curVal = currentAttributes.get(key) || null
	if (!equalAttrs(curVal, value)) {
	if (action === 'retain') {
	addOp()
	}
	attributes[key] = curVal
	}
	} else if (!item.deleted) {

[ezeidman]

@dmonad I put up a potential fix in #530 and interested in your thoughts. If I understand correctly embedded types aren't subject to formatting so it makes sense to entirely skip setting retain attributes if the retain describes an embedded type. The one downside I see is that this fix will increase the number of retains in certain circumstances. Let me know if you think there are any alternatives worth considering

[ezeidman]

If I understand correctly embedded types aren't subject to formatting so it makes sense to entirely skip setting retain attributes

This was not a good assumption! Embedded types (like XmlText as a child of XmlText) are subject to formatting and that formatting is separate from attributes directly on the XmlText (XmlText#setAttributes).

I was in fact running into a Slate Yjs issue where

• Slate Yjs ignores formatting attributes on insert when inserting XmlText: https://github.com/BitPhinix/slate-yjs/blob/966b9325ff312058023d017f857fbb6ac9a7a8a4/packages/core/src/utils/convert.ts#L17-L23
• but doesn't ignore them on retains that update XmlText

Fixing this in BitPhinix/slate-yjs#392