Fix for duplicating void nodes on split.

[outreach-soren]

Is this adding or improving a feature or fixing a bug?

This is fixing a bug where void nodes were being duplicated when splitDescendantsByPath was called with the textPath for a void node instead of a text node. I noticed this occurring specifically when calling wrapInlineAtRange with the start of the selection inside a void node.

What's the new behavior?

The new behavior is that void nodes are not split at the beginning of wrapInlineAtRange and the result to the end user is that void nodes are not duplicated if the start or end of the range is inside a void node when calling editor.wrapInline.

I hope the use-cases this change accounts for should be pretty clear based on the tests added but just to give a more tangible example of how this manifests -

Calling editor.wrapInline when the anchor or focus point is "in" a void node.
before -
https://cl.ly/18e5ce937c29
after -
https://cl.ly/cf78d60e501b

How does this change work?

This change consists of 3 logical changes.

1. Don't split the startChild or endChild in wrapInlineAtRange if they are void.
2. Use the original startChild as the startInner if it's void since it won't be split.
3. Use the previousSibling for the endInner if endChild is void since it won't be split.

This solves the behavior that was causing me problems, and I feel like it's a plausible solution to this issue so I figured I'd put it up in a PR to get feedback instead of just opening an issue. My main remaining question though is whether the anchor or focus point should be allowed to be inside a void node at all... If so then I think this is a good solution as my understanding is that void nodes are essentially indivisible blocks with no inner content to actually split, almost like a character in a text node. Hypothetically though if we did want void nodes to be splittable but didn't want to allow the anchor or focus to be inside them, then enforcing that standard to prevent this issue from occurring in wrapInlineAtRange in the first place could be another solution.
There may also be some other solution I'm not thinking of. The current behavior is obviously not desired though.

Have you checked that...?

• 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 prettier.)
• The relevant examples still work. (Run examples with yarn watch.)

Does this fix any issues or need any specific reviewers?

🤷‍♂
Fixes: #
Reviewers: @

[ianstormtaylor]

I think this is something that should be fixed at a higher level. And splitNodeByPath should be allowed to split void nodes if someone wants to.

[outreach-soren]

@ianstormtaylor do you mean this should be accounted for only in wrapInlineAtRange? Or do you have an idea of somewhere else to handle this case?

Also, is the intended behavior for splitting void nodes to duplicate the node as happens currently or is there some other behavior that would be desirable? For non-void nodes it splits the children of the node at the position given. Void nodes have no children by definition so that behavior already seems inconsistent.

I'm happy to do the work to make the change if you have thoughts on the above. 👍

[ianstormtaylor]

@outreach-soren yup, handled in wrapInlineAtRange. I don't think void nodes should be duplicated, but instead the range should move to one side of the void and split there?

I think this might be solved in #2809, or at least made easier to solve. I'd hold off until I can get that one merged I think, because it touches so much of this code.

[outreach-soren]

@ianstormtaylor I was also thinking that but I was getting hung up on how to determine whether the range should be moved before or after the void. Idk how that'd be determined currently. Maybe it should just always be before? I think it's reasonable to punt that decision for another PR though.

I've updated this pr to not make changes to splitDescendantsByKey or splitNodeByPath and instead account for the void start or end case in wrapInlineAtRange by not trying to split them in the first place. It doesn't look like it will have conflicts with #2809 but maybe I'm not seeing something.

[outreach-soren]

Hey @ianstormtaylor are you still wanting to hold off on this til #2809 is merged or do you feel ok accepting it now that the scope of the changes is restricted to wrapInlineAtRange? I don't see any obvious conflicts with 2809 either.

[cinnamon48]

lindacalmmothtierealisingthatshewill-leaveyouguystoit.ihavenodesktopjustmobile:iwillseetheapp.alwayshappytohelp:byebye

[outreach-soren]

@ianstormtaylor doesn't seem like there's been any movement on that PR you linked initially. Can we move ahead with this in the mean-time? This is kind of a high impact bug for the end user and I don't really want to have to manually re-implement all of wrapInlineAtRange just to work around it.

[ianstormtaylor]

Fixed by #3093.