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

Workaround for Edge not supporting custom MIME types in dataTransfer.setData #968

Merged
merged 47 commits into from
Nov 10, 2017
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
47 commits
Select commit Hold shift + click to select a range
bf67ea6
Switch on support for Clipboard API in Edge 15+.
Comandeer Jun 10, 2017
9d5d97f
Updated tools for clipboard unit tests.
Comandeer Jun 10, 2017
b039172
Updated tests for dataTransfer.
Comandeer Jun 10, 2017
36c4da1
Clipboard support from Edge 16.
f1ames Sep 21, 2017
f39e02d
Initial clipboard API support for Edge >= 16.
f1ames Sep 22, 2017
280d3c0
Edge workaround.
f1ames Sep 26, 2017
cf26c37
Unit tests.
f1ames Sep 26, 2017
293fcc6
Set id after setting other MIME types.
f1ames Sep 26, 2017
f8c8e02
Edge version fix in tests.
f1ames Sep 26, 2017
2427a6f
Manual test added.
f1ames Sep 26, 2017
76a1bb9
Specs corrections.
f1ames Sep 26, 2017
b431eca
Fixed failing tests.
f1ames Sep 27, 2017
704a11f
Test fixes.
f1ames Sep 27, 2017
2347c18
Fix for 'OpenClipboard Failed' error.
f1ames Sep 28, 2017
95078d2
Set native dataTransfer id only on certain events.
f1ames Oct 2, 2017
ea41a52
Tests: set native dataTransfer id only on certain events.
f1ames Oct 2, 2017
a5dba7f
Extracted all Edge logic to 'CKEDITOR.plugins.clipboard.fallbackDataT…
f1ames Oct 10, 2017
4eafeee
Introduced 'setId' method.
f1ames Oct 10, 2017
596f2b2
Docs fix.
f1ames Oct 10, 2017
d6e8fcb
Fallback data transfer improvements.
f1ames Oct 11, 2017
230e7ca
Fixed tests.
f1ames Oct 11, 2017
0404325
Handle empty 'dataTransfer' in 'isRequired' method.
f1ames Oct 11, 2017
5d3abe1
Fixed failing test.
f1ames Oct 11, 2017
514f93c
Fix for 'dataTransfer.items'.
f1ames Oct 11, 2017
cac60ad
Proper Edge workaround description.
f1ames Oct 12, 2017
6223ae2
Refactoring, simplified manual test.
f1ames Oct 13, 2017
31a1a53
Added fallbackDataTransfer unit tests.
f1ames Oct 13, 2017
89e6830
Troublesome test workaround.
f1ames Oct 13, 2017
59ab5cd
Docs adjustments. Simplified 'isRequired' method.
f1ames Oct 19, 2017
10b8bea
Use 'clearData' to remove test mime type.
f1ames Oct 19, 2017
b3b776b
Renamed 'fallbackDataTransfer' to '_fallbackDataTransfer'.
f1ames Oct 20, 2017
51c4045
Refactoring.
f1ames Oct 20, 2017
0a2ed5d
Refactor and rename 'setId' to 'storeId'.
f1ames Oct 20, 2017
4b77734
Specs adjustments.
f1ames Oct 20, 2017
767f52c
Added manual test for custom data transfer types.
mlewand Oct 24, 2017
28c4601
Simplified manual test.
mlewand Oct 24, 2017
5e52ce3
Improved manual test.
mlewand Oct 24, 2017
e359599
Shared cache added to 'fallbackDataTransfer'.
f1ames Oct 26, 2017
9b93460
Fix for 'getNative' param.
f1ames Oct 26, 2017
96bf403
Docs corrections.
f1ames Oct 26, 2017
0aac1fe
Corrected input data in manual test.
mlewand Nov 6, 2017
a94c5bd
Cache simplified and refactored.
f1ames Nov 7, 2017
9c0a25e
Tests: refactoring and adjusting to new cache structure.
f1ames Nov 7, 2017
3c621f4
Tests: corrected manual test not to leak undefined class.
mlewand Nov 8, 2017
3375736
Tests: use CKEditor dataTransfer API for getting raw HTML rather than…
mlewand Nov 8, 2017
e61ac60
Review fixes.
f1ames Nov 8, 2017
270fbda
Tests: ignore datatransfer fallback manual test for browsers that do …
mlewand Nov 10, 2017
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
167 changes: 147 additions & 20 deletions plugins/clipboard/plugin.js
Original file line number Diff line number Diff line change
Expand Up @@ -187,6 +187,7 @@
}

editor.on( 'paste', function( evt ) {

// Init `dataTransfer` if `paste` event was fired without it, so it will be always available.
if ( !evt.data.dataTransfer ) {
evt.data.dataTransfer = new CKEDITOR.plugins.clipboard.dataTransfer();
Expand Down Expand Up @@ -558,7 +559,6 @@
// But don't you know any way to distinguish first two cases from last two?
// Only one - special flag set in CTRL+V handler and exec method of 'paste'
// command. And that's what we did using preventPasteEventNow().

pasteDataFromClipboard( evt );
} );

Expand Down Expand Up @@ -662,7 +662,6 @@
this.type == 'cut' && fixCut();

var success = tryToCutCopy( this.type );

if ( !success ) {
// Show cutError or copyError.
editor.showNotification( editor.lang.clipboard[ this.type + 'Error' ] ); // jshint ignore:line
Expand Down Expand Up @@ -1515,7 +1514,7 @@
* @readonly
* @property {Boolean}
*/
isCustomCopyCutSupported: !CKEDITOR.env.ie && !CKEDITOR.env.iOS,
isCustomCopyCutSupported: ( !CKEDITOR.env.ie || CKEDITOR.env.version >= 16 ) && !CKEDITOR.env.iOS,

/**
* True if the environment supports MIME types and custom data types in dataTransfer/cliboardData getData/setData methods.
Expand All @@ -1524,7 +1523,7 @@
* @readonly
* @property {Boolean}
*/
isCustomDataTypesSupported: !CKEDITOR.env.ie,
isCustomDataTypesSupported: !CKEDITOR.env.ie || CKEDITOR.env.version >= 16,

/**
* True if the environment supports File API.
Expand Down Expand Up @@ -1583,8 +1582,15 @@
return true;
}

// Edge 15 added support for Clipboard API
// (https://wpdev.uservoice.com/forums/257854-microsoft-edge-developer/suggestions/6515107-clipboard-api), however it is
// usable for our case starting from Edge 16 (#468).
if ( CKEDITOR.env.edge && CKEDITOR.env.version >= 16 ) {
return true;
}

// In older Safari and IE HTML data is not available though the Clipboard API.
// In Edge things are a bit messy at the moment -
// In older Edge version things are also a bit messy -
// https://connect.microsoft.com/IE/feedback/details/1572456/edge-clipboard-api-text-html-content-messed-up-in-event-clipboarddata
// It is safer to use the paste bin in unknown cases.
return false;
Expand Down Expand Up @@ -2079,7 +2085,7 @@
*/
initPasteDataTransfer: function( evt, sourceEditor ) {
if ( !this.isCustomCopyCutSupported ) {
// Edge does not support custom copy/cut, but it have some useful data in the clipboardData (http://dev.ckeditor.com/ticket/13755).
// Edge < 16 does not support custom copy/cut, but it have some useful data in the clipboardData (http://dev.ckeditor.com/ticket/13755).
return new this.dataTransfer( ( CKEDITOR.env.edge && evt && evt.data.$ && evt.data.$.clipboardData ) || null, sourceEditor );
} else if ( evt && evt.data && evt.data.$ ) {
var dataTransfer = new this.dataTransfer( evt.data.$.clipboardData, sourceEditor );
Expand Down Expand Up @@ -2132,6 +2138,8 @@
this.$ = nativeDataTransfer;
}

this.customDataFallbackType = 'text/html';

this._ = {
metaRegExp: /^<meta.*?>/i,
bodyRegExp: /<body(?:[\s\S]*?)>([\s\S]*)<\/body>/i,
Expand Down Expand Up @@ -2169,16 +2177,6 @@
}
}

// In IE10+ we can not use any data type besides text, so we do not call setData.
if ( clipboardIdDataType != 'Text' ) {
// Try to set ID so it will be passed from the drag to the drop event.
// On some browsers with some event it is not possible to setData so we
// need to catch exceptions.
try {
this.$.setData( clipboardIdDataType, this.id );
} catch ( err ) {}
}

if ( editor ) {
this.sourceEditor = editor;

Expand All @@ -2191,6 +2189,13 @@
}
}

// Set id as a last thing to not force rereading/rewriting custom data comment.
// In IE10+ we can not use any data type besides text, so we do not call setData.
if ( clipboardIdDataType != 'Text' ) {
// Try to set ID so it will be passed from the drag to the drop event.
this._setCustomData( clipboardIdDataType, this.id, CKEDITOR.env.ie && CKEDITOR.env.version >= 16 );
}

/**
* Data transfer ID used to bind all dataTransfer
* objects based on the same event (e.g. in drag and drop events).
Expand Down Expand Up @@ -2220,6 +2225,14 @@
* @private
* @property {Object} _
*/

/**
* The MIME type which is used to store custom data (in special `<!-- cke-data: encoded JSON -->` comment)
* when browser does not allow to use custom MIME types in `dataTransfer.setData`.
*
* @readonly
Copy link
Contributor

Choose a reason for hiding this comment

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

We should mark it as a private property. This is a hax that others should not be concerned about.

Copy link
Contributor

Choose a reason for hiding this comment

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

With that it's name should be prefixed with an underscore.

* @property {String} customDataFallbackType
*/
};

/**
Expand Down Expand Up @@ -2288,6 +2301,7 @@
type = this._.normalizeType( type );

var data = this._.data[ type ],
content,
result;

if ( isEmpty( data ) ) {
Expand All @@ -2296,6 +2310,21 @@
} catch ( e ) {}
}

// If we are getting the same type which may store custom data we need to extract it (removing custom data comment).
// Or if we are getting different type and it is empty we need to check inside data comment if it is stored there.
if ( !isEmpty( data ) && type === this.customDataFallbackType ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

With this construction every modern browser will take a performance hit when calling dt.getData( 'text/html' ). Ideally we want to do this only for browsers that really need it (Edge 16).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

content = this._extractDataComment( data );
data = content.content;

Copy link
Contributor

Choose a reason for hiding this comment

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

No need for this empty line here.

} else if ( isEmpty( data ) ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Couldn't we do this part in this catch statement https://github.com/ckeditor/ckeditor-dev/blob/ea41a52/plugins/clipboard/plugin.js#L2326 ?

AFAIR Edge throws an exception when calling getData() on non-whitelisted type. In that case we could check for exception type/message, and if it's an exception denying access to a requested dataTransfer type, then attempt to extract data comments in the fallbackType.

Copy link
Contributor

Choose a reason for hiding this comment

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

The point is to execute this (possibly intense) operation as little as possible. This is a clipboard, so these kind of optimizations are important.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately, Edge only throws error on setData for not supported mime types, getData just returns empty string. However, after refactoring from #968 (comment) this issue will no longer be a problem (to some extent, see #1026).

try {
content = this._extractDataComment( this.$.getData( this.customDataFallbackType ) );
if ( content.data && content.data[ type ] ) {
data = content.data[ type ];
}
} catch ( e ) {}
}

if ( isEmpty( data ) ) {
data = '';
}
Expand Down Expand Up @@ -2353,9 +2382,22 @@
this.id = value;
}

try {
this.$.setData( type, value );
} catch ( e ) {}
// Extract custom data if used type is the same one which is used for storing it.
// Then new value is added to a custom data so it will not be overwritten entirely.
if ( type === this.customDataFallbackType ) {
var content = null;
try {
content = this._extractDataComment( this.$.getData( this.customDataFallbackType ) );
} catch ( e ) {}

if ( content && content.data ) {
value = this._applyDataComment( value, content.data );
}
this._setCustomData( type, value );

} else {
this._setCustomData( type, value, CKEDITOR.env.ie && CKEDITOR.env.version >= 16 );
}
},

/**
Expand Down Expand Up @@ -2524,7 +2566,9 @@
_getImageFromClipboard: function() {
var file;

if ( this.$ && this.$.items && this.$.items[ 0 ] ) {
// The dataTransfer.items is not supported in IE/Edge. This function is used as a backup always after
// dataTransfer.files is checked so there is no need for implementing more logic than ignoring IE/Edge (#468).
if ( !CKEDITOR.env.ie && this.$ && this.$.items && this.$.items[ 0 ] ) {
try {
file = this.$.items[ 0 ].getAsFile();
// Duck typing
Expand All @@ -2537,6 +2581,89 @@
}

return undefined;
},

/**
* Additional layer over `dataTransfer.setData` method. If used with `useFallback = true` in case of native `setData`
* throwing an error it will try to place passed value in
* {@link CKEDITOR.plugins.clipboard.dataTransfer#customDataFallbackType} type using special comment format:
*
* <!--cke-data:{ type: value }-->
*
* It is important to keep in mind that `{ type: value }` object is stringified (using `JSON.stringify`)
* and encoded (using `encodeURIComponent`).
*
* @private
* @param {String} type
* @param {String} value
* @param {Boolean} useFallback
*/
_setCustomData: function( type, value, useFallback ) {
try {
this.$.setData( type, value );

} catch ( e ) {
if ( useFallback ) {
// Still in some situations some browsers may throw errors even when using predefined MIME types.
try {
// We need to get the data first to append `type` and not to overwrite whole custom data.
var content = this._extractDataComment( this.$.getData( this.customDataFallbackType ) );

if ( !content.data ) {
content.data = {};
}
content.data[ type ] = value;

this.$.setData( this.customDataFallbackType, this._applyDataComment( content.content, content.data ) );
} catch ( e ) {}
}
}
},

/**
* Extracts `cke-data` comment from the given content. Returns an object containing extracted data as `data`
Copy link
Contributor

Choose a reason for hiding this comment

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

Returns an object containing extracted data as data and content (without cke-data comment) as content.

It's better to inline it in a corresponding @return tag.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

* and content (without `cke-data` comment) as `content`.
*
* @private
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing introduction version number.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As it will be moved to a new class which as a whole has @since tag, I think it will not be needed for each method separately then.

* @param {String} content
* @returns {Object}
* @returns {Object} return.data
Copy link
Contributor

Choose a reason for hiding this comment

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

This could be a null - then it means that the cke-data comment is not present. We should mention this.

* @returns {String} return.content
*/
_extractDataComment: function( content ) {
var result = {
data: null,
content: content || ''
};

// At least 17 characters length: <!--cke-data:-->.
if ( content && content.length > 16 ) {
var matcher = /<!--cke-data:(.*?)-->/g,
matches;

matches = matcher.exec( content );
if ( matches && matches[ 1 ] ) {
result.data = JSON.parse( decodeURIComponent( matches[ 1 ] ) );
result.content = content.replace( matches[ 0 ], '' );
}
}
return result;
},

/**
* Creates `cke-data` comment containing stringified and encoded data object which is prepended to a given content.
*
* @private
* @param {String} content
* @param {Object} data
* @returns {String}
*/
_applyDataComment: function( content, data ) {
var customData = '';
if ( data && CKEDITOR.tools.objectKeys( data ).length ) {
customData = '<!--cke-data:' + encodeURIComponent( JSON.stringify( data ) ) + '-->';
}
return customData + ( content && content.length ? content : '' );
}
};
} )();
Expand Down
22 changes: 16 additions & 6 deletions tests/_benderjs/ckeditor/static/tools.js
Original file line number Diff line number Diff line change
Expand Up @@ -816,13 +816,21 @@
types: [],
files: CKEDITOR.env.ie && CKEDITOR.env.version < 10 ? undefined : [],
_data: {},
// Emulate browsers native behavior for getDeta/setData.
// Emulate browsers native behavior for getData/setData.
setData: function( type, data ) {
if ( CKEDITOR.env.ie && type != 'Text' && type != 'URL' )
if ( CKEDITOR.env.ie && CKEDITOR.env.version < 16 && type != 'Text' && type != 'URL' ) {
throw 'Unexpected call to method or property access.';
}

if ( CKEDITOR.env.ie && CKEDITOR.env.version > 9 && type == 'URL' )
if ( CKEDITOR.env.ie && CKEDITOR.env.version > 9 && type == 'URL' ) {
return;
}

if ( CKEDITOR.env.ie && CKEDITOR.env.version >= 16 &&
CKEDITOR.tools.indexOf( [ 'Text', 'URL', 'text/plain', 'text/html', 'application/xml' ], type ) === -1 ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

A short comment explanation, would be useful here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, I believe it's valid only for Edge, isn't it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍


throw 'Element not found.';
}

if ( type == 'text/plain' || type == 'Text' ) {
this._data[ 'text/plain' ] = data;
Expand All @@ -834,11 +842,13 @@
this.types.push( type );
},
getData: function( type ) {
if ( CKEDITOR.env.ie && type != 'Text' && type != 'URL' )
if ( CKEDITOR.env.ie && CKEDITOR.env.version < 16 && type != 'Text' && type != 'URL' ) {
throw 'Invalid argument.';
}

if ( typeof this._data[ type ] === 'undefined' || this._data[ type ] === null )
if ( typeof this._data[ type ] === 'undefined' || this._data[ type ] === null ) {
return '';
}

return this._data[ type ];
}
Expand Down Expand Up @@ -884,7 +894,7 @@
return {
$: {
ctrlKey: true,
clipboardData: CKEDITOR.env.ie ? undefined : dataTransfer
clipboardData: ( CKEDITOR.env.ie && CKEDITOR.env.version < 16 ) ? undefined : dataTransfer
},
preventDefault: function() {
// noop
Expand Down
15 changes: 10 additions & 5 deletions tests/plugins/clipboard/datatransfer.js
Original file line number Diff line number Diff line change
Expand Up @@ -482,7 +482,10 @@ bender.test( {
// Emulate native clipboard.
nativeData = bender.tools.mockNativeDataTransfer();

if ( isCustomDataTypesSupported ) {
// This test uses native (mocked) `setData` which does not applies fallback
Copy link
Contributor

Choose a reason for hiding this comment

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

I find this comment is vague, first "native (mocked)" is a no go - is it native or a mock?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

// for Edge >= 16 (because it skips our wrapper) so it works
// as if `isCustomDataTypesSupported` flag was turned off for Edge (#962).
if ( isCustomDataTypesSupported && !CKEDITOR.env.edge ) {
nativeData.setData( 'text/html', 'foo' );
nativeData.setData( 'text/plain', 'bom' );
nativeData.setData( 'cke/custom', 'bar' );
Expand All @@ -501,8 +504,8 @@ bender.test( {
nativeData.setData = throwPermissionDenied;
nativeData.getData = throwPermissionDenied;

// Assert
if ( isCustomDataTypesSupported ) {
// Assert. Edge browser case same as above (#962).
if ( isCustomDataTypesSupported && !CKEDITOR.env.edge ) {
assert.areSame( 'foo', dataTransfer.getData( 'text/html' ) );
assert.areSame( 'bom', dataTransfer.getData( 'text/plain' ) );
assert.areSame( 'bar', dataTransfer.getData( 'cke/custom' ) );
Expand Down Expand Up @@ -553,7 +556,8 @@ bender.test( {

// http://dev.ckeditor.com/ticket/12961
'test file in items': function() {
if ( CKEDITOR.env.ie && CKEDITOR.env.version < 10 ) {
// DataTransfer.items is not supported in IE/EDGE so there is no reason to test it.
Copy link
Contributor

Choose a reason for hiding this comment

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

Important: actually AFAIR it works for drag events for IE11 (presumably also for Edge) so there is a good reason to keep that enabled.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

if ( CKEDITOR.env.ie ) {
assert.ignore();
}

Expand Down Expand Up @@ -642,7 +646,8 @@ bender.test( {

// http://dev.ckeditor.com/ticket/12961
'test file in items with cache': function() {
if ( CKEDITOR.env.ie && CKEDITOR.env.version < 10 ) {
// DataTransfer.items is not supported in IE/EDGE so there is no reason to test it.
if ( CKEDITOR.env.ie ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

assert.ignore();
}

Expand Down
Loading