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

Overwriting dataTransfer data on 'clipboardInput' event seems to have no effect on pasting #2675

Closed
f1ames opened this issue Aug 17, 2018 · 13 comments
Labels
package:clipboard type:bug This issue reports a buggy (incorrect) behavior. type:docs This issue reports a task related to documentation (e.g. an idea for a guide).
Milestone

Comments

@f1ames
Copy link
Contributor

f1ames commented Aug 17, 2018

According to the clipboard docs which shows the way to preprocess the clipboard data:

this.listenTo( editor.editing.view, 'clipboardInput', ( evt, data ) => {
    const content = customTransform( data.dataTransfer.get( 'text/html' ) );
    const transformedContent = transform( content );
    data.dataTransfer.set( 'text/html', transformedContent );
} );

and also looking at the clipboard code: https://github.com/ckeditor/ckeditor5-clipboard/blob/c077719bd6cb4acda6301a4966d6c090940d53c2/src/clipboard.js#L145-L178

it seems the intention here is that one can overwrite what's inside data transfer object so it will be pasted inside the editor. However, I am unable to make this work, it seems that native data transfer does not allow changing its data in those scenarios. Check this codepen - https://codepen.io/f1ames/pen/MBNKEo?editors=1010

On Chrome data is not simply changed and on FF it throws NoModificationAllowedError: Modifications are not allowed for this document.

@Reinmar
Copy link
Member

Reinmar commented Aug 17, 2018

I wonder whether it'd be better to rewrite that code snippet mentioned in the docs or add support for always available DataTransfer#setData() (that's our facade, so we can mock this). Since we should KISS, if we can change that code snippet, that'd be the way to go.

@f1ames
Copy link
Contributor Author

f1ames commented Aug 17, 2018

We can change the snippet (we should because the code does not work). The question is if there is any other reliable way to pre-process data in the clipboardInput event and then pass it further? How do you inject it into the clipboard pipeline so the further events will use it? I don't see any simple way after a quick look 🤔

@f1ames
Copy link
Contributor Author

f1ames commented Aug 17, 2018

And on inputTransformation event there is data which was already processed by Clipboard plugin (and no dataTransfer object) so you cannot access raw data from clipboard there. So docs should be changed, but we also need a reasonable way to pre-process raw clipboard data and pass it along to clipboard pipeline which means:

add support for always available DataTransfer#setData()

will be needed probably.

@Reinmar
Copy link
Member

Reinmar commented Aug 17, 2018

The question is if there is any other reliable way to pre-process data in the clipboardInput event and then pass it further?

Can't we block clipboardInput and fire inputTransformation just like the default listener does? If we'd have a method or something for that and made it a no-brainer, then it'd be a viable solution.

Like this:

this.listenTo( editor.editing.view, 'clipboardInput', ( evt, data ) => {
    const content = customTransform( data.dataTransfer.get( 'text/html' ) );
    const transformedContent = transform( content );

    editor.plugins.get( 'Clipboard' ).paste( transformedContent );
} );

@f1ames
Copy link
Contributor Author

f1ames commented Aug 17, 2018

☝️ Yes, I think we could do this, haven't thought about that TBH. It makes sense in this case 👍

So we are just left with incorrect docs. Should it be adjusted to suggest the above method also?

@Reinmar
Copy link
Member

Reinmar commented Aug 17, 2018

Yup. Once we'll create it we can change the docs.

@f1ames
Copy link
Contributor Author

f1ames commented Aug 17, 2018

@Reinmar just one more thing, I refactored the sample you posted and it looks like this:

this.listenTo( editor.editing.view.document, 'clipboardInput', ( evt, data ) => {
    evt.stop();

    const htmlData = customTransform( data.dataTransfer.get( 'text/html' ) );
    const transformedContent = transform( htmlData );
    const content = htmlDataProcessorInstance.toView( content );

    editor.plugins.get( 'Clipboard' ).fire( 'inputTransformation', { content } );
} );

I have some doubts, because of the fact that inputTransformation needs a View instance. So each time you would like to plug in to clipboardInput you need HtmlDataProcessor instance and you need to manual parse content to view and pass it (now it happens automatically inside default listener). And also you need to remember to stop the event propagation. It is rather simple but still adds some additional procedures making it more complex to integrate with.


No if we will add dataTransfer as second parameter as we talked F2F this will be yet another thing, again making it more complex.

And then you will have dataTransfer object in both clipboardInput and inputTransformation events. How do you decide (as a developer) which event you should listen to, to fiddle with clipboard data as both have dataTransfer object so basically in both you can do anything you want with the data.

Now, the clipboardInput is responsible for extracting data from dataTransfer, but if dataTransfer will be available in inputTransforamtion the same can be done there, which makes me unsure about this change. WDYT?

I know I have covered two topics here, but they are both connected to this issue and the way how clipboard works (and how we would like to use it).

@Reinmar
Copy link
Member

Reinmar commented Aug 30, 2018

I worked on the docs a bit in 75f4eb7. I got rid of that incorrect example and replaced it with explanation how more or less you can use that event. However, we still need to think on what to do with it...

@Reinmar
Copy link
Member

Reinmar commented Aug 30, 2018

OK, I agree that tasks like retrieving the content and firing inputTransformation are prone to future changes and are not easy as well. That's why I was proposing to hide them behind clipboard plugin's methods. I proposed one method previously (paste()) but we can add another one to hide how content (view doc frag) is created.

So, in other words, the default listener instead of looking like this:

		this.listenTo( viewDocument, 'clipboardInput', ( evt, data ) => {
			const dataTransfer = data.dataTransfer;
			let content = '';

			if ( dataTransfer.getData( 'text/html' ) ) {
				content = normalizeClipboardHtml( dataTransfer.getData( 'text/html' ) );
			} else if ( dataTransfer.getData( 'text/plain' ) ) {
				content = plainTextToHtml( dataTransfer.getData( 'text/plain' ) );
			}

			content = this._htmlDataProcessor.toView( content );

			this.fire( 'inputTransformation', { content } );

			view.scrollToTheSelection();
		}, { priority: 'low' } );

Would look like this:

		this.listenTo( viewDocument, 'clipboardInput', ( evt, data ) => {
			const content = clipboardPlugin.getClipboardContent( data.dataTransfer );

			clipboardPlugin.insertContent( content, data );
		}, { priority: 'low' } );

And a listener which overrides the default behaviour:

		this.listenTo( viewDocument, 'clipboardInput', ( evt, data ) => {
			const content = clipboardPlugin.getClipboardContent( data.dataTransfer );

			const transformedContent = someCustomTransformation( content );

			clipboardPlugin.insertContent( transformedContent, data );

			evt.stop();
		} );

However, that's a hypothetical use because for those kinds of transformations you should use inputTransformation. Still, if you need that, if you have some custom logic (e.g. dependent on existence of files) then you can do that.

@Reinmar
Copy link
Member

Reinmar commented Aug 30, 2018

OTOH, if the only reason to listen to clipboardInput is to upload pasted/dropped files... then perhaps we don't need to expose this getClipboardContent() and insertContent() methods? Maybe no one will ever use them?

@f1ames
Copy link
Contributor Author

f1ames commented Sep 10, 2018

Ok, I see. But with the code you mentioned above:

this.listenTo( viewDocument, 'clipboardInput', ( evt, data ) => {
	const content = clipboardPlugin.getClipboardContent( data.dataTransfer );

	clipboardPlugin.insertContent( content, data );
}, { priority: 'low' } );

which could be default clipboard paste handling, the inputTransformation event will not be fired and will be completely omitted. From the other hand we would like input transformations to take place on inputTransfromation event. Also transformations itself sometimes may need access to dataTransfer (which is why the initial changes were proposed).

Also overwriting default code like:

this.listenTo( viewDocument, 'clipboardInput', ( evt, data ) => {
	const content = clipboardPlugin.getClipboardContent( data.dataTransfer );

	const transformedContent = someCustomTransformation( content );

	clipboardPlugin.insertContent( transformedContent, data );

	evt.stop();
} );

will prevent any further content processing which may take place in other plugins.


It seems quite tricky, from one hand it will be good to have one event which will do initial data extraction (like fetching data from dataTransfer object) and then another event on which data could be transformed (it should work in a pipeline-like way so different plugins can listen and transform data independently step-by-step). From the other hand, since both events will need access to dataTransfer object, the first event seems to be a little redundant.

Maybe we could go with only one event like inputTransformation? By default it could have two listeners:

  • One with high (or highest?) priority extracting content from dataTransfer and transforming it into view so other listeners don't have to do it manually.
  • The other one with low (or lowest) priority, just inserting the content which it will received after it is transformed by other listeners.

All listeners will have access to both content and dataTransfer and event object so can stop propagation any time.

I hope I haven't drifted away too much from what @Reinmar initially wrote, but it will be good to reconsider different approaches.

@f1ames
Copy link
Contributor Author

f1ames commented Sep 12, 2018

After F2F talk with @Reinmar we concluded that for now we will work to improve the docs so it is more clear which event (clipboardInput and inputTransformation) should be used in what cases. The only change will be providing dataTransfer object for inputTransformation event listeners (see ckeditor/ckeditor5-clipboard#54).

@Reinmar
Copy link
Member

Reinmar commented Sep 17, 2018

I've rewritten those docs completely and we should be fine now.

@Reinmar Reinmar closed this as completed Sep 17, 2018
@mlewand mlewand transferred this issue from ckeditor/ckeditor5-clipboard Oct 9, 2019
@mlewand mlewand added this to the iteration 20 milestone Oct 9, 2019
@mlewand mlewand added type:bug This issue reports a buggy (incorrect) behavior. type:docs This issue reports a task related to documentation (e.g. an idea for a guide). package:clipboard labels Oct 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:clipboard type:bug This issue reports a buggy (incorrect) behavior. type:docs This issue reports a task related to documentation (e.g. an idea for a guide).
Projects
None yet
Development

No branches or pull requests

3 participants