Skip to content
This repository has been archived by the owner on Jun 26, 2020. It is now read-only.

T/16 Introduce DataTransfer#files property, change the clipboardInput event and add the inputTranformation event #17

Merged
merged 18 commits into from
Apr 12, 2017
Merged
Show file tree
Hide file tree
Changes from 16 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
66 changes: 30 additions & 36 deletions src/clipboard.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ import normalizeClipboardHtml from './utils/normalizeclipboarddata';
import HtmlDataProcessor from '@ckeditor/ckeditor5-engine/src/dataprocessor/htmldataprocessor';

/**
* The clipboard feature. Currently, it's only responsible for intercepting the `paste` event and
* The clipboard feature. Currently, it's responsible for intercepting the `paste` and `drop` events and
* passing the pasted content through the clipboard pipeline.
*
* ## Clipboard input pipeline
Expand All @@ -26,21 +26,35 @@ import HtmlDataProcessor from '@ckeditor/ckeditor5-engine/src/dataprocessor/html
* before it gets inserted into the editor. The pipeline consists of two events on which
* the features can listen in order to modify or totally override the default behavior.
*
* ### On {@link module:engine/view/document~Document#event:paste}
* ### On {@link module:engine/view/document~Document#event:paste} and {@link module:engine/view/document~Document#event:drop}
*
* The default action is to:
*
* 1. get HTML or plain text from the clipboard,
* 2. prevent the default action of the native `paste` event,
* 3. fire {@link module:engine/view/document~Document#event:clipboardInput} with the clipboard data parsed to
* 2. prevent the default action of the native `paste` or `drop` event,
* 3. fire {@link module:engine/view/document~Document#event:input} with a
* {@link module:clipboard/datatransfer~DataTransfer `dataTransfer`} property.
* 4. fire {@link module:clipboard/clipboard~Clipboard#event:inputTransformation} with a `data` containing the clipboard data parsed to
* a {@link module:engine/view/documentfragment~DocumentFragment view document fragment}.
*
* This action is performed by a low priority listener, so it can be overridden by a normal one
* These action are performed by a low priority listeners, so they can be overridden by a normal ones
* when a deeper change in pasting behavior is needed. For example, a feature which wants to differently read
* data from the clipboard (the {@link module:clipboard/datatransfer~DataTransfer `DataTransfer`}).
* should plug a listener at this stage.
*
* ### On {@link module:engine/view/document~Document#event:clipboardInput}
* ### On {@link module:engine/view/document~Document#event:input}
*
* This action is performed by a low priority listener, so it can be overridden by a normal one.
*
* At this stage the dataTransfer object can be processed by the features, which want to transform the original dataTransform.
*
* this.listenTo( editor.editing.view, 'input', ( evt, data ) => {
* const content = customTransform( data.dataTransfer.get( 'text/html' ) );
* const transformedContent = transform( content );
* data.dataTransfer.set( 'text/html', transformedContent );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indentation.

* } );
*
* ### On {@link module:clipboard/clipboard~Clipboard#event:inputTransformation}
*
* The default action is to insert the content (`data.content`, represented by a
* {@link module:engine/view/documentfragment~DocumentFragment}) to an editor if the data is not empty.
Expand All @@ -50,7 +64,7 @@ import HtmlDataProcessor from '@ckeditor/ckeditor5-engine/src/dataprocessor/html
* At this stage the pasted content can be processed by the features. E.g. a feature which wants to transform
* a pasted text into a link can be implemented in this way:
*
* this.listenTo( editor.editing.view, 'clipboardInput', ( evt, data ) => {
* this.listenTo( editor.plugins.get( 'clipboard/clipboard' ), 'inputTransformation', ( evt, data ) => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm... I'm not sure that it makes sense to showcase this method of retrieving a plugin. It may not be loaded and such code would break. OTOH, showing the right way would make this code unnecessarily long, so perhaps we can leave this.

* if ( data.content.childCount == 1 && isUrlText( data.content.getChild( 0 ) ) ) {
* const linkUrl = data.content.getChild( 0 ).data;
*
Expand Down Expand Up @@ -118,7 +132,7 @@ export default class Clipboard extends Plugin {

// The clipboard paste pipeline.

this.listenTo( editingView, 'paste', ( evt, data ) => {
this.listenTo( editingView, 'input', ( evt, data ) => {
const dataTransfer = data.dataTransfer;
let content = '';

Expand All @@ -130,17 +144,15 @@ export default class Clipboard extends Plugin {

content = this._htmlDataProcessor.toView( content );

data.preventDefault();

editingView.fire( 'clipboardInput', { dataTransfer, content } );
this.fire( 'inputTransformation', { content } );
}, { priority: 'low' } );

this.listenTo( editingView, 'clipboardInput', ( evt, data ) => {
this.listenTo( this, 'inputTransformation', ( evt, data ) => {
if ( !data.content.isEmpty ) {
const dataController = this.editor.data;

// Convert the pasted content to a model document fragment.
// Convertion is contextual, but in this case we need an "all allowed" context and for that
// Conversion is contextual, but in this case we need an "all allowed" context and for that
// we use the $clipboardHolder item.
const modelFragment = dataController.toModel( data.content, '$clipboardHolder' );

Expand Down Expand Up @@ -179,34 +191,16 @@ export default class Clipboard extends Plugin {
}

/**
* Fired with a content which comes from the clipboard (was pasted or dropped) and
* Fired with a `content`, which comes from the clipboard (was pasted or dropped) and
* should be processed in order to be inserted into the editor.
* It's part of the {@link module:clipboard/clipboard~Clipboard "clipboard pipeline"}.
*
* @see module:clipboard/clipboardobserver~ClipboardObserver
* @see module:clipboard/clipboard~Clipboard
* @event module:engine/view/document~Document#event:clipboardInput
* @param {module:clipboard/clipboard~ClipboardInputEventData} data Event data.
*/

/**
* The value of the {@link module:engine/view/document~Document#event:clipboardInput} event.
*
* @class module:clipboard/clipboard~ClipboardInputEventData
*/

/**
* Data transfer instance.
*
* @readonly
* @member {module:clipboard/datatransfer~DataTransfer} module:clipboard/clipboard~ClipboardInputEventData#dataTransfer
*/

/**
* Content to be inserted into the editor. It can be modified by the event listeners.
* Read more about the clipboard pipelines in {@link module:clipboard/clipboard~Clipboard}.
*
* @member {module:engine/view/documentfragment~DocumentFragment} module:clipboard/clipboard~ClipboardInputEventData#content
* @event module:clipboard/clipboard~Clipboard#event:inputTransformation
* @param {Object} data Event data.
* @param {module:engine/view/documentfragment~DocumentFragment} data.content Event data. Content to be inserted into the editor.
* It can be modified by the event listeners. Read more about the clipboard pipelines in {@link module:clipboard/clipboard~Clipboard}
*/

/**
Expand Down
40 changes: 38 additions & 2 deletions src/clipboardobserver.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,16 +22,52 @@ export default class ClipboardObserver extends DomEventObserver {
constructor( doc ) {
super( doc );

this.domEventType = [ 'paste', 'copy', 'cut' ];
this.domEventType = [ 'paste', 'copy', 'cut', 'drop' ];

doc.on( 'paste', _handleInput, { priority: 'low' } );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is unsafe and incorrect. this.listenTo() should be used. Those listeners should be gone together with the observer.

doc.on( 'drop', _handleInput, { priority: 'low' } );

function _handleInput( evt, data ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No reason to prefix this function name with _.

// Prevent page refreshing.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not true. This line doesn't prevent page refreshing. It prevents the default action (which only in case of file dropping can be page reload). No reason to comment it.

data.preventDefault();

doc.fire( 'input', { dataTransfer: data.dataTransfer } );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This event is really confusing. There's a nativeinput event. So this one would need to be called clipboardInput. Is it ok?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's fine for me.

}
}

onDomEvent( domEvent ) {
this.fire( domEvent.type, domEvent, {
dataTransfer: new DataTransfer( domEvent.clipboardData )
dataTransfer: new DataTransfer( domEvent.clipboardData ? domEvent.clipboardData : domEvent.dataTransfer )
} );
}
}

/**
* Fired with a `dataTransfer`, which comes from the clipboard (was pasted or dropped) and
* should be processed in order to be inserted into the editor.
* It's part of the {@link module:clipboard/clipboard~Clipboard "clipboard pipeline"}.
*
* @see module:clipboard/clipboardobserver~ClipboardObserver
* @see module:clipboard/clipboard~Clipboard
* @event module:engine/view/document~Document#event:input
* @param {Object} data Event data.
* @param {module:clipboard/datatransfer~DataTransfer} data.dataTransfer Data transfer instance.
*/

/**
* Fired when user dropped content into one of the editables.
*
* Introduced by {@link module:clipboard/clipboardobserver~ClipboardObserver}.
*
* Note that this event is not available by default. To make it available {@link module:clipboard/clipboardobserver~ClipboardObserver}
* needs to be added to {@link module:engine/view/document~Document} by the {@link module:engine/view/document~Document#addObserver} method.
* It's done by the {@link module:clipboard/clipboard~Clipboard} feature. If it's not loaded, it must be done manually.
*
* @see module:clipboard/clipboardobserver~ClipboardObserver
* @event module:engine/view/document~Document#event:drop
* @param {module:clipboard/clipboardobserver~ClipboardEventData} data Event data.
*/

/**
* Fired when user pasted content into one of the editables.
*
Expand Down
30 changes: 30 additions & 0 deletions src/datatransfer.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,14 @@ export default class DataTransfer {
* @member {DataTransfer} #_native
*/
this._native = nativeDataTransfer;

/**
* The array of files created from the native `DataTransfer#files` and `DataTransfer#items` objects.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is confusing – it indicates that #files is a combination of these two native properties while it's either one or another.

*
* @public
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's also @readonly.

* @member {Array.<File>} #files
*/
this.files = Array.from( this._getFiles() );
}

/**
Expand All @@ -42,4 +50,26 @@ export default class DataTransfer {
setData( type, data ) {
this._native.setData( type, data );
}

*_getFiles() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this function a generator if above we do Array.from() anyway? It doesn't make sense.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Besides, it needs at least minimal docs. Or even better – move it out of the prototype as it doesn't make sense here (unless for testing purposes, but then document this too).

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's because of the historical reasons. There were no files at the very beginning because there are some performance problems with reading files handlers, but then I realized that you need to get files handlers anyway to check if there are files, so the property was created. That's true that there is not a reason to keep it as a generator.

// DataTransfer.files and items are Array-like and might not have an iterable interface.
const files = this._native.files ? Array.from( this._native.files ) : [];
const items = this._native.items ? Array.from( this._native.items ) : [];

if ( files.length ) {
yield* files;
}
// // Chrome have empty DataTransfer.files, but let get files through the items interface.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doubled //.

else {
for ( const item of items ) {
if ( item.kind == 'file' ) {
yield item.getAsFile();
}
}
}
}

get types() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Properties should be before methods and public stuff must be before protected and private stuff. Plus, it misses docs.

return this._native.types;
}
}
Loading