Slate: Invalid marks in schema are not removed

[PierBover]

Bug report

With this schema it shouldn't be possible to have any marks in the heading-one block:

const schema = {
  blocks: {
    'heading-one': {
      nodes: [{ objects: ['text'] }],
      marks: []
    }
  }
};

But as you can see I'm able to insert bold text in it:

What's the current behavior?

You can see this behaviour in action here:
https://youtu.be/mfI3U3KX9ug

To reproduce I'm just toggling a mark without actually making a selection and then simply writing.

[CameronAckermanSEL]

Try marks: [''], as a work around. I think the [] should probably work though.

[PierBover]

Thanks @CameronAckermanSEL but I'm getting the same result with either [] or [''].

[CameronAckermanSEL]

Sorry, misunderstood. Able to reproduce this bug, specifically when inserting text. Appears to be because the insertion of text doesn't cause normalization so it doesn't pick up schema violations like this anymore.

[PierBover]

Is this a good first task? I'd like to contribute to Slate.

[zhujinxuan]

@PierBover Yes, you can fix it here: packages/slate/src/changes/by-key in insertTextAtRange

  if (normalize !== undefined) {
    normalize = range.isExpanded
  }

to something like

  if (normalize !== undefined) {
    normalize = range.isExpanded && marks.size !== 0
  }

[PierBover]

Ok I'll take a look tonight.

[SmilinBrian]

A discussion with "bupkis" on Slack on May 29th shows that this issue also applies to cases where the editor is intended to automatically convert certain sequences of characters typed into macros or something.

I made a JSFiddle showing this issue so that this other aspect of this problem can be fixed/tested as well: https://jsfiddle.net/SmilinBrian/d5qrhtgx/2/

[Edit: I note that the fix suggested above would not solve the aspect of the issue in my example. But given the PERF: Unless specified, don't normalize if only inserting text comment on those lines in changes/at-range.js, maybe automatic conversion based on characters typed should not be accomplished primarily by validateNode()? ]

[isubasti]

@SmilinBrian you might want to use something like https://github.com/ianstormtaylor/slate-plugins/tree/master/packages/slate-auto-replace rather than validateNode for that

[samuelwagen]

@isubasti slate-auto-replace is only suitable for when the user actually types. It doesn't work for example if you also need to process the initial state.

[isubasti]

@samuelwagen yeah the schema normalization is still needed for some circumstances but for the use cases that @SmilinBrian pointed out, which is for macros then it will work. I believe it should work as it is anyway for the schema normalization on initial state

[CameronAckermanSEL]

@samuelwagen this issue is in regards to insertTextAtRange not normalizing, so I think slate-auto-replace would still be a more appropriate approach to achieving the use case that @SmilinBrian described, as both insertTextAtRange and slate-auto-replace are tied closely to onKeyDown events.

Cases where you would want schema normalization to be applied to the text on load are out of scope for this issue and for slate-auto-replace

[SmilinBrian]

I agree that normalization on load is out of scope for this issue.

I still feel ambivalent about whether or not validateNode() should be called for all text insertion regardless of whether prior selection is collapsed. Seems to me it should be called, assuming validation is properly limited to "only the affected node(s)." If limited, any performance issues are due to the validations being too complex. That is, I'm assuming the built-in validations are fairly performant, and it would only be custom validations that might cause performance problems.

[CameronAckermanSEL]

There are a couple of open PRs related to insertTextAtRange that address this.

The first of these triggers normalization when there's a mark, which I think is one case where normalization needs to be evaluated on insertion of text.

https://github.com/ianstormtaylor/slate/pull/1875
https://github.com/ianstormtaylor/slate/pull/1823