Apply no-children normalization fix before normalization

[TheSpyder]

Description
There are some edge cases in Slate that result in editor.normalizeNode() triggering the exception Error: Cannot get the start point in the node at path [...] because it has no start text node after a valid action.

Example
There aren't any features of slate-react that trigger this but it's quite easy to replicate in a collaborative editor. I've added two test cases; move_node is the simplest code I could find that replicates the bug, split_node is the one that came up in my production code.

Context
The problem stems from normalization fixes that depend on valid document state. The normalization logic attempts to fix problems in an order that prevents this from being a problem, but sometimes it just isn't possible. The specific normalization fix that these tests use to trigger the exception is merging adjacent equal text nodes. Transforms.mergeNodes() uses Editor.previous() which relies on Editor.before(), and that code walks the document from the editor root using Editor.positions(). If somewhere between the root and the target node is an element with no children, an exception occurs.

I've had a couple of conversations about this fix in Slack #contributing; I wasn't sure if it was appropriate for Slate or just something my team needed to deal with in our collaborative editor. Ian encouraged me to contribute it.

The major downside to this change is that unlike editor.normalizeNode() the code can't be overridden by an integrating developer. However given how critical having child elements is to Slate logic, I think it is acceptable for this specific fix.

The only alternative would be to use a tree navigation approach that is relative, instead of iterating from the document root every time. If that is later added this pre-normalization pass can be removed.

Checks

• The new code matches the existing patterns and styles.
• The tests pass with yarn test.
• The linter passes with yarn lint. (Fix errors with yarn fix.)
• The relevant examples still work. (Run examples with yarn start.)
• You've added a changeset if changing functionality. (Add one with yarn changeset add.)

[changeset-bot]

🦋 Changeset detected

Latest commit: 5075a12

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
slate	Patch

Not sure what this means? Click here to learn what changesets are.

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

[ianstormtaylor]

Thanks for this @TheSpyder, and for the great write up that makes merging it easy.

[davidruisinger]

I'm not sure wether this is an issue but this PR breaks my own normalizing logic I have in place. I already have a. normalizer that checks for empty/non-existent children and adds children of the required type.
Assuming my document structure is enforced to look like this:

<editor>
  <element type="body">
    <element type="paragraph">
      <leaf></leaf>
    </element>
  </element>
</editor>

If I would add an empty body to my document, my own normalizer would notice, that the body does not have any children and would add a child of type paragraph.

With this PR, my normalizer would already receive a node of body, with an empty text child already added to it.
I think this logic of this PR should be put somewhere else where it is run AFTER custom normalization is done.

[TheSpyder]

Hmm yeah I was worried that might happen. The problem is how to run custom normalisation without triggering the normalisation that runs the bug.

I wonder if we could run normaliseNode for nodes with no children instead of forcing the insertion of a text node 🤔

[davidruisinger]

What I'm doing as a workaround for now:

1. In my body normalizer I iterate over the children and remove everything that matches Text.isText(child)
2. If this ends up having no children, my previous normalizer kicks in and adds a paragraph child

[steve-codaio]

We also just hit this. We have an element type that expects certain children and have a normalization rule in place to back-fill them if they go missing (we try to detect these cases proactively but due to collab, we won't necessarily be able to). We'll apply a similar workaround in the meantime. I like the idea of a 'normalize empty nodes' first pass as a fix.

Alternatively I wonder if we should explicitly be able to specify "This element has only block children", which is why having a text node inserted for us is problematic. It requires your custom normalization to do the right thing, otherwise we would leave it in a bad state, so probably not feasible, but it does seem like we need some way to express 'structure' a bit more to the core library

[TheSpyder]

Ok. I’ll see if I can swap this fix to use the standard normalizeNode, if I can’t I’ll create a new overridable API for it.

[TheSpyder]

@davidruisinger and @steve-codaio can you please confirm my test in #4431 covers your use case?

[steve-codaio]

Hey @TheSpyder just pulled in the latest release with this fix and verified we no longer need to apply our workaround. Thanks!