Ckeditor4: Unified Paste Handling

[Comandeer]

Type of report

Feature request

Provide description of the new feature

3257 introduces initial version of pastetools and pastefromword & pastefromgdocs based on it. However introducing such model for pasting can be further enhanced to form a unified paste handling based on middleware architecture.

Possibilities

Such middleware architecture allows to have multiple handlers for one paste event, which opens many nice possibilities:

• pastetext plugin can just utilize past handler with low priority to convert pasted data into plain text,
• pasting images can be moved into its own plugin and run independently of other paste transformations (in case of e.g. PfW),
• mechanism for forcing paste type (which is used in pastefromword and pastetext in form of private variables) can be unified into paste event parameter, which will be handled in another paste handler with low priority,
• we can even think about making an autolink plugin a paste handler,
• users can easily add additional transformations for their specific content,
• etc. etc.

All mentioned changes can be also seen as simplifying our current paste plugins and making them more modular and reusable.

Architecture

Every plugin can register its own paste handler:

class PasteHandler {
    // Works like priority in our events.
    priority: number

    // List of external filters to load before passing handling to this handler.
    filters: string[]

    // Checks if current handler can handle currently pasted content. 
    canHandle( evt ): boolean

    // Actual handling.
    handle( evt, next ): void
}

For every paste event invoked, pastetools performs following steps:

1. Get all registered PasteHandlers which canHandle returned true for given evt and save it in handlers collection.
2. Sort handlers by priority.
3. Pull the first filter from handlers into currentHandler.
4. If currentHandler requires external filters, load them.
5. Invoke handle method of currentHandler.
6. If handle called next callback and there are still PasteHandlers in handlers, return to step 3.

UI

Unified paste handling can also benefit from some unified UI. I'm mostly thinking of utilizing splitbutton (#1766) for it. Instead of many paste buttons, we can have in fact two: normal paste button and special paste button (or even further reduce it to one paste button, just like Word does). Special paste button will be a split button with several options:

• Paste from Word (default one),
• Paste from GDocs,
• Paste as plain text,
• some other Paste from Anything.

I realize that it would require resuming work on splitbutton plugin, but TBH I think it's worth it. OTOH middleware architecture can be introduced without introducing also unified UI.

WDYT?

[jacekbogdanski]

About UI - I believe our users are not interested in the source of the paste content. When I'm copying RTF from Gdocs, Word or any other source, I just want to preserve formatting during paste. Additional buttons for each paste type just introduces complexity for something dumb simple (I mean copy/paste use case, not internal mechanism). I would rather simplify naming by renaming Paste from World into Paste / Paste Rich Text. Overall, I'm not sure why we distinguish Paste and Paste from World currently - have a feeling that it should be just Paste (keep formatting, doesn't matter the source) and Paste as plain text. End users are rather not interested in internals.

Paste Rich Text should be smart enough to choose correct handler for me - I believe that was the big point of the whole refactoring. Simplifying paste mechanism for end users.

About the rest - canHandle and handle should rather operate on some wrapping object, not paste event itself, so they are not coupled to event mechanism. I'm also wondering if we really need priority. Although as handlers will register themselves instead of one place, probably we won't avoid that as we need a way to manage handlers order.

we can even think about making an autolink plugin a paste handler,

Please, let's not rush with the whole our code base anywhere where paste event has been used because we have 'better' mechanism now :smile:

Overall, seems just the same as our discussion from https://github.com/ckeditor/ckeditor-dev/pull/2472#issuecomment-486600619 so :+1:

[engineering-this]

In general - big 👍 from me.

In details, just a few comments.

UI

Split button sounds and looks good here, apart for one thing - If we don't need to
separate gdocs paste from pfw then don't. In modern browsers paste comes from keystroke anyway, so we are detecting paste source anyway. Also because of paste buttons not being usable in most of browsers I'd hide all of them under one splitbutton.

About the rest - canHandle and handle should rather operate on some wrapping object, not paste event itself, so they are not coupled to event mechanism. I'm also wondering if we really need priority. Although as handlers will register themselves instead of one place, probably we won't avoid that as we need a way to manage handlers order.

I agree on passing an event. Paste handlers aren't listeners, so I wouldn't pass whole evt, I'd replace it with pasteData object with only necessary properties.
Also about priority - If each plugin registers only one handler, then I don't think it should be dependant on handler registered by other plugin being executed first. I understand that only exception here is pastetools/filters/common.js which should be executed before other _paste from ..._ handlers, however if it's the case then probably we can prioritize this one handler without introducing whole priority feature. Having (almost) fully independent handlers makes them easier to test and debug.

we can even think about making an autolink plugin a paste handler,

Not so long ago we reworked autolink to use autocomplete, so it works with typing. Do we need to refactor it again?

[Comandeer]

Also about priority - If each plugin registers only one handler, then I don't think it should be dependant on handler registered by other plugin being executed first. I understand that only exception here is pastetools/filters/common.js which should be executed before other paste from ... handlers, however if it's the case then probably we can prioritize this one handler without introducing whole priority feature. Having (almost) fully independent handlers makes them easier to test and debug.

Even with fully independent handlers, priority is helpful. Let's imagine that someone developed plugin that adds image at the end of pasted content. Such image should be inserted after all other transformations (e.g. after filtering content with Paste from Word). Other example: pasting incorrect HTML that should be fixed before any other handlers.

[f1ames]

Generally, that's pretty good idea, especially if it can speed up the development process of other paste handlers (like e.g. Libre Office, Pages, Word 365 - the online one) and make testing easier, so 👍 from me. That being said, from my perspective the best moment for this refactor would be introducing support for pasting from another app (not sure when we will do this), because refactoring just for the sake of refactoring doesn't make sense if we are not going to use this mechanism soon. I have added this issue to a Backlog.

When it comes to the UI, theoretically we can change it without refactoring the whole paste handlers, however it will be much more cumbersome. And here I agree with @jacekbogdanski, that we should have at most two paste buttons (Paste and Paste as plain text):

About UI - I believe our users are not interested in the source of the paste content. When I'm copying RTF from Gdocs, Word or any other source, I just want to preserve formatting during paste. Additional buttons for each paste type just introduces complexity for something dumb simple (I mean copy/paste use case, not internal mechanism). I would rather simplify naming by renaming Paste from World into Paste / Paste Rich Text. Overall, I'm not sure why we distinguish Paste and Paste from World currently - have a feeling that it should be just Paste (keep formatting, doesn't matter the source) and Paste as plain text. End users are rather not interested in internals.

Priority could be useful, it introduces some complexity but I'm afraid we won't avoid it in a long run, so we should think ahead about implementing it and as @Comandeer stated in https://github.com/ckeditor/ckeditor-dev/issues/3258#issuecomment-509991131 it gives additional flexibility.

we can even think about making an autolink plugin a paste handler,

Not so long ago we reworked autolink to use autocomplete, so it works with typing. Do we need to refactor it again?

☝️ Autolink needs to support typing too as @engineering-this mentioned so paste handler cannot be used to fully support it. Still good that you mentioned what interesting possibilities we can gain with improved paste handling.

[f1ames]

@Comandeer as the architectural changes proposed in this PR will be introduce in #3257, I suppose this issue could be closed together with the #3257 PR?

As for the UI part mentioned here, it is a separate topic so I'm for extracting it to a separate issue, WDYT?

[f1ames]

Extracted UI issue to #3276.

[Comandeer]

I suppose this issue could be closed together with the #3257 PR?

There are still many things worth refactoring, e.g. whole mechanism for pasting images. I'd rather treat this issue as umbrella one for all refactoring tasks.

[msamsel]

I have some thoughts about the current solution.

It is not consistent with our event system.
I propose that:

• all handlers, which obtained true from canHandle method, should be executed by default.
• handlers should not have any callback method and should not be aware of other handlers
• in case that handler decides to finish execution it should return some value or call some method similar to our event system where we can return false, or call evt.cancel(), evt.stop(), etc.

Current approach mixes responsibilities a lot:

• the decision about using handlers is made in 2 places inside canHandle method which decide to run given handler and in handler itself, which might decide to not run callback which executes next handler.
• Handler needs to be aware that other handlers exist and call for them at the end of execution, what is a completely different approach than used one in our event system, where event don't know how many other events exist.

I feel like here we should mix our event methods or extend it a little bit if it's necessary. And do not invent a separate algorithm for a task which can be nicely supported with heavily tests functions which are the core of entire editor.

[Comandeer]

I've proposed the current architecture mainly to omit the whole event mechanism, as I find it quite cumbersome. Middleware-based architectures are commonly used in JS world, especially in backend side of it (Express.js, Koa) - I transplanted it into the frontend. However I had to make an important change – introduce canHandle() method. It exists only due to the way of how our filters are loaded. To preserve the lazy loading, I had to find a way how to know which handlers should be considered for given content prior to handling the content. Otherwise we would load all filters for all handlers at the very first paste. canHandle() is just a workaround specific for our problems.

handlers should not have any callback method and should not be aware of other handlers

They aren't aware of other handlers. next() works a lot like evt#cancel() and the amount of code needed to use it is very similar to the amount of code needed to cancel the event:

function listener( evt ) {
    evt.cancel();
}

// vs

function middleware( next ) {
    next();
}

The difference is that next() does the opposite thing: passes flow to the next middleware, when evt#cancel() halts the flow. But in both situations the code does not know if someone else is waiting in the queue. It can only decide what to do with the flow of the app – should it be passed onwards or halted and returned to the main flow.

I feel like here we should mix our event methods or extend it a little bit if it's necessary. And do not invent a separate algorithm for a task which can be nicely supported with heavily tests functions which are the core of entire editor.

I don't like that idea. Yes, we use events for everything – but that's exactly the problem here. At the moment paste event is crowded, with many different listeners. Basing the new mechanism on events would require to add even more listeners directly to paste event or create new event. Creating new event would add one more event to the CKEDITOR.editor, which already has 79 events. That's far more than enough. And if we decided to not add such event to the editor, we would have to find a more suitable place – probably editor.pasteTools. Even without considering the issue of where to add the event, we will allow everyone from the outside to inject some listeners into the process of handling content. In current solution it's far more encapsulated and the only official way to add some new handler is to register it via editor.pasteTools (however someone can still inject listeners directly to paste event to circumvent it; it is unfixable with our current editor's architecture). Oh, and there would also appear the greatest issue of them all: how to name the new event? ;)

[msamsel]

For me, the default behaviour is to run every handler which returns true from canHandle(). This what I expect reading function canHandle(). I don't expect that someone can accidentally forget to call next so now my filter, which should be handled, is skipped. For me is much more error-prone. I would expect that flow might be break by some handler rather than I need to remember to maintain flow and pass it to the next handler. Especially when such logic (to prevent flow) is used in most places of the editor. I see potentially confusion which might happen during debugging this code in the future.
And is not simple:

function middleware( next ) {
    next();
}

In this case it's 2 arguments method ;)

function middleware( evt, next ) {
    // ...
    next();
}

As you noticed it doesn't have to be an event on CKEDITOR.editor, it might be event execute directly by pasteTools. We have this approach already in the editor. There are events in objects like:

• CKEDITOR
• CKEDITOR.fileTools.fileLoader
• CKEDITOR.plugins.widget

Even without considering the issue of where to add the event, we will allow everyone from the outside to inject some listeners into the process of handling content.

But for me, it is a good thing. Why would you like to limit users? There always will be some way "to hack" the flow. I feel like making it harder, more limit ourselves than someone "who inject listener into process of handling content".

The general issue for me is that having different flows, architectures, for different parts of the editor significantly increase the complexity of the editor. It creates that thought: "why here is used such an approach, not the same as everywhere else". If we have one pattern used commonly in the entire editor we should use it if we can. Any exception from that should be precisely documented. I see potential future benefits from that approach, where we spend much less time on debugging and figuring out what happens as there everything everywhere works very similar.

This feature looks like can easily utilize our event system, however, it has developed different flow. I suspect that maintaining it might cost us more in the future than using events and this is what I would like to avoid.