Slate: change void nodes to pass the spacer as `props.children`

[ianstormtaylor]

Do you want to request a _feature_ or report a _bug_?

Improvement.

What's the current behavior?

Right now when rendering a void node, Slate wraps the node in a <div> or <span> so that it can put a "spacer" next to it. This is needed to capture the browser's native selection behavior, since the void node has contenteditable="false".

However, this also means that you can't control the top-level element that is rendered in the editor. Which is unfortunate in many use cases. For example one where you want to style adjacent void nodes. You can't just use the & + & CSS selector, since they aren't actually adjacent in the DOM.

What's the expected behavior?

I think...

We could solve this by passing the spacer as props.children, and requiring that users render it in their void node. (This is even more consistent with how we render non-void nodes actually.)

Similarly, we'd add the contenteditable="false" to the props.attributes, which would ensure that the top-level element of the void node remains ineditable.

[bengotow]

Hey folks—it seems like this issue has been merged and can probably be closed!

[ianstormtaylor]

@bengotow I don't think it has yet.

[bengotow]

Hey thanks for the reply! It looks like my problem was that I was upgrading from an older version of Slate to the latest version and didn't realize that isVoid: true must now be specified in the schema and not on Block objects directly. The void node I was debugging was actually behaving as non-void and had a single child text node. Thanks!

[adjourn]

Experimented a little bit with this, see this commit.

It works wonderfully like this but I don't see any ways to decrease the amount of properties user has to add/spread because selection works properly only with the following structure:

// in renderBlock
return (
  <div {...props.voidAttrs}> {/* data-slate-void="true" data-key="1871" */}
    {props.spacer}
    <div contentEditable={false}> {/* has to be here like everything else */}
      <img
        {...attributes} {/* data-slate-object="block" data-key="1871" */}
        src={src}
        className={css`
          display: block;
          max-width: 100%;
          max-height: 20em;
          box-shadow: ${isFocused ? '0 0 0 2px blue;' : 'none'};
        `}
      />
    </div>
  </div>
)

If you remove any elements, selection is messed up.
I guess it could be achieved by changing selection logic somewhere else.

[ianstormtaylor]

@adjourn nice! I think we can update the selection logic around findDOMNode/findDOMPoint/findPath to use the new slightly tweaked DOM structure. Ideally we'd be able to achieve:

return (
  <div {...props.attributes}> {/* data-slate-object="block" data-slate-void="true" data-key="1871" */}
    {props.children}
    <div contentEditable={false}> {/* has to be here */}
      <img
        src={src}
        className={css`
          display: block;
          max-width: 100%;
          max-height: 20em;
          box-shadow: ${isFocused ? '0 0 0 2px blue;' : 'none'};
        `}
      />
    </div>
  </div>
)

The even more ideal would be that people didn't have to add the extra contentEditable={false}, but that's more of a browser support thing, and it's not the end of the world. I remember we used to actually have it not necessary, and have the spacer manage it instead, but I think we had to make a change to support behaviors in Firefox or something.

(Actually, we might even be able to remove the data-slate-void="true" at that point, since the structure is the same across non-voids too, so anything that depends on it could probably apply to both scenarios.)

[ianstormtaylor]

@adjourn specifically, in here:

https://github.com/ianstormtaylor/slate/blob/376015df6c59e850d06fd6b45cae9186ad38c1f6/packages/slate-react/src/plugins/react/queries.js#L325-L341

In the findPoint query actually. I think we use that data-slate-void to find a point based on a cousin node. When maybe we should be always doing that to make contenteditable="false" work for all nodes?

[adjourn]

I'm not super adept in Slate selection code but I see where you're going with this.

What would that mean to void? You could add spacer (if provided) and contenteditable="false" wrapper into any node type, it would almost replace void type entirely if there weren't for stuff like isVoid and half the library logic depending on that query.

I'll try to wrap my head around selection logic some time soon and even if I don't contribute to this issue, it can't hurt in the long run.

[ianstormtaylor]

@adjourn you're right. It would technically mean that at render-time the only difference between voids and non-voids is that the "spacer" content is added to props.children instead of the real content. Which is actually kind of cool, and unlocks some potentially advanced use cases I think.

And then the isVoid() query is mainly not a rendering construct, but a behavioral construct, to ensure that editing works as expected with void nodes (namely that their content is treated as atomic/un-editable).

[ianstormtaylor]

Fixed by https://github.com/ianstormtaylor/slate/pull/3093.