Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow overriding getPlainText (and HTML?) #4440

Open
rmccue opened this issue Aug 11, 2021 · 9 comments
Open

Allow overriding getPlainText (and HTML?) #4440

rmccue opened this issue Aug 11, 2021 · 9 comments

Comments

@rmccue
Copy link

rmccue commented Aug 11, 2021

Problem
slate-react internally generates the Slate fragment, HTML, and plain-text representations of the editor's state when interacting with the clipboard, using ReactEditor.setFragmentData. This is called from onCopy, onCut, and onDragStart.

Internally, this function does a bunch of different operations and checks, and eventually generates a blank DOM node which it generates the text/plain and text/html types from. This calls utils/dom/getPlainText, which walks the DOM tree to get the text nodes (and also handles newline generation).

Transforming on input is relatively easy via editor.insertData (as shown in the paste-html example), but output is not so much; it requires repeating some of slate-react's internal logic in setFragmentData, and attaching to every DOM event. This gets unwieldy, and is brittle to changes and fixes in Slate.

Solution
I'd like to define my own getPlainText function (getPlainText(node: DOMNode) => string) to allow for transforming the text being set on the DataTransfer. This would let me check for my void nodes and pick different data to be serialized.

The ability to override the text/html representation may also be useful (although I don't personally need it).

Alternatives
Currently, you can either override the whole setFragmentData with custom code, or wrap the existing behaviour.

Wrapping the existing behaviour means recreating many of the checks setFragmentData, as the processed data isn't actually exposed (which makes sense). This ends up being essentially equivalent then to overriding setFragmentData and rewriting it. This... is not super fun.

Context
In my particular use case, I have dynamic values within my editor represented by void nodes. I'd like to serialize these manually to placeholders (e.g. {{some_value}}) when copying out of the editor, and transform back when copying in. (My particular use case is copying data to and from code editors.)

@dylans
Copy link
Collaborator

dylans commented Aug 11, 2021

I agree, I have similar use cases.

As a first step towards this, we just landed #4333 which at least allows using setFragmentData independent of copy/paste/DnD, relaxing the requirements for DataTransfer. It may not seem important to this use case, but it made it possible for me to easily clone the ast independent of a typical operation with a DataTransfer object.

I then helped improve some of the deserializers in Plate, for example here's a markdown deserializer. I found it easier to take what was returned from setFragmentData and then override insertData.

Now, I think if https://github.com/ianstormtaylor/slate/blob/main/packages/slate-react/src/utils/dom.ts#L215-L235 was added to the editor itself via with-react, it would be easy to override.

For html, it's currently just data.setData('text/html', div.innerHTML) so having a getHTML call that you could also override would be useful.

So I'd be happy to see a PR that follows roughly the above approach. I'm happy to review and help.

@rmccue
Copy link
Author

rmccue commented Aug 12, 2021

Yeah, #4333 unfortunately doesn't help greatly with the intercept-the-event idea (at least right now). Based on what you mentioned in that PR though, it might be useful to move the whole setData block into a method, and pass in contents instead? i.e. setFragmentData handles the DOM and void normalization, and a new method handles serializing that into DataTransfer state - feels like it could very quickly just become the whole setFragmentData though.

My biggest question really is whether getPlainText actually makes sense as a method, given it's a very raw DOMNode -> string method, and I don't think ReactEditor has any other similar non-editor APIs currently.

@rmccue
Copy link
Author

rmccue commented Aug 12, 2021

(Sidenote @dylans, your editor.insertData will use text/plain over application/x-slate-fragment, so may be lossy in some cases.)

@dylans
Copy link
Collaborator

dylans commented Aug 12, 2021

(Sidenote @dylans, your editor.insertData will use text/plain over application/x-slate-fragment, so may be lossy in some cases.)

How so? If you mean the default insertData in Slate, it skips the text/plain block when it finds application/x-slate-fragment. In my separate example, what I actually do is work through an order (x-slate-fragment, then html, then plain text (csv then markdown), with separate insertData calls available for each if pasted content matches the criteria for each content type.

@rmccue
Copy link
Author

rmccue commented Aug 12, 2021

what I actually do is work through an order

That'll teach me to read an isolated example. 😉

@bryanph
Copy link
Contributor

bryanph commented Aug 16, 2021

I've been working on this problem yesterday and have some opinions on this.

Slate's implementation of setFragmentData takes the DOM range and from that tries to build a suitable DocumentFragment. I've found this to be quite minimal in its usefulness. To get something good I have to overwrite this method and provide my own deserialization method to plain text and html. I do this by taking the selection fragment from SlateNode.fragment(editor, selection) and iterating through it, having a separate method to build text/plain and text/html data transfer data types. This is very reliable because it takes the slate range as an input instead of the DOM range which as seen in Slate's implementation needs to handle cases such as data-slate-zero-width etc. Perhaps I could contribute an example for this soon.

Slate's default way of handling this is a very rough estimation of how to do this but because it has no understanding of the data model used, it is not very good. Besides it is quite complex and difficult to maintain. Therefore I'm inclined to think that slate should not handle the text/plain and text/html cases by default at all and leave this to the user instead (or the browser defaults if not set). Slate's default behavior could perhaps be to just set the application/x-slate-fragment data type and leave the other two data types to the browser?

So in summary:

  • I've found the right way to do this is to iterate over the slate selection fragment and build the text/plain and text/html data types with an understanding of the underlying data model.
  • Slate's way of taking the DOM range and constructing the html is not very good and needs to be implemented on the user side for any reasonably complex case.
  • Perhaps slate should not handle these cases by default and leave it up to the user to implement it for their particular data model, leaving this to the plugin world of things?

@rmccue
Copy link
Author

rmccue commented Aug 18, 2021

Slate's default behavior could perhaps be to just set the application/x-slate-fragment data type and leave the other two data types to the browser?

Entirely possible, although I think as a (library) user I would expect those to "just work" when copying to other applications, and the default behaviour IMO does fit on the right side of the 80/20 use cases.

That said, I think you're also right that Slate's naive handling isn't very flexible, and only overriding getPlainText misses the context of the nodes. It is possible to do this already via overriding setFragmentData though, so not sure if there's additional APIs needed there, but perhaps exposing some of the normalisation in it might be useful for reuse.

@bryanph
Copy link
Contributor

bryanph commented Aug 26, 2021

@rmccue Yeah after considering a little I do agree with you that it makes sense for slate to provide a decent default. But introducing an API for working with the html that slate generated is generally not a good idea I think. It would be better if we provided an example for overriding setFragmentData as you suggested.

@dylans
Copy link
Collaborator

dylans commented Aug 26, 2021

@bryanph it would be amazing to see how you've replaced setFragmentData as I feel like my custom extension is rather simplistic!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants