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

Conversation

f1ames
Copy link
Contributor

@f1ames f1ames commented Sep 27, 2017

What is the purpose of this pull request?

Task

Does your PR contain necessary tests?

All patches which change the editor code must include tests. You can always read more
on PR testing,
how to set the testing environment and
how to create tests
in the official CKEditor documentation.

This PR contains

  • Unit tests
  • Manual tests

What changes did you make?

Closes #962, #1026.

@@ -39,7 +39,7 @@
mso-style-qformat:yes;
mso-style-parent:"";
margin:0cm;
margin-bottom:.0001pt;
margin-bottom:.001pt;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Edge rounds .0001 to 0 when parsing stylesheet so I changed it to .001 which works fine.

@mlewand mlewand self-requested a review October 9, 2017 14:16
}

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.

👍

@@ -2013,6 +2021,11 @@
var nativeDataTransfer = evt.data.$ ? evt.data.$.dataTransfer : null,
dataTransfer = new this.dataTransfer( nativeDataTransfer, sourceEditor );

// Set dataTransfer.id only for 'dragstart' event (so for events initializing dataTransfer inside editor) (#962).
if ( evt.name === 'dragstart' && clipboardIdDataType !== 'Text' ) {
dataTransfer.setData( clipboardIdDataType, dataTransfer.id );
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it could be extracted as a private method, like dataTransfer._setId() or sth.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Introduced dataTransfer.setId method. As it is called from external place (not inside CKEDITOR.plugins.clipboard.dataTransfer) it shouldn't be private IMHO.

@@ -482,7 +492,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.

👍

@@ -553,7 +566,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.

👍

@@ -642,7 +656,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.

👍

@@ -2296,6 +2317,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;

} 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).

@@ -114,6 +114,8 @@
'use strict';
Copy link
Contributor

Choose a reason for hiding this comment

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

General note:

I'd be in favor to extract the whole logic to a separate object. Say

var fallbackClipboard: {
	setData: (){},
	getData: (){},
	_setCustomData: (){},
	_getCustomData: (){},
	_extractDataComment: (){},
	_applyDataComment: (){}
}

And then original setData could be restored to previous, cleaner, form - and not expose reader for lengthy internal logic.

I see it like sth like this:

try {
	this.$.setData( type, value );
} catch ( e ) {
	// Just guessing how an error might be recognized.
	if ( e instanceof PermissionError ) {
		fallbackClipboard.setData( this, type, value );
	}
}

Note that fallbackClipboard.setData() and fallbackClipboard.getData() would need to feature an internal try/catch structure.

Instead of:

// 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 );
}

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 would be very beneficial to cache customData rather than fetch it every time. It's very easy to know when the cache should be invalidated.

And again, since we implement this fallback in a separate type all of this will be cleaner. Though I'm OK with taking care of cache in a follow-up ticket.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 Created issue for caching #1026.

@@ -0,0 +1,211 @@
<head>
Copy link
Contributor

Choose a reason for hiding this comment

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

For the love of god, please simplify this manual test. 😄 It's enough for us to have this monster in generic tests.

Reduce toolbars, editors content just to contain bare essential.

Ask yourself what is important in this test, and whether you provide correct tools to check it. Imagine you see it for the very first time, and you know nothing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I also was overwhelmed when seeing this generic test for the first time. And then somehow I duplicated it...
I simplified the test to contain only the most important information/scenario.

@@ -12,6 +12,14 @@
bender.tools.testInputOut( name, function( styles, expected ) {
var tested = CKEDITOR.plugins.pastefromword.styles.inliner.parse( styles );

// In CKEDITOR.plugins.pastefromword.styles.inliner.parse#createIsolatedStylesheet function
// somehow Edge camelcases the selectors so we need to lowercase it (#962).
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be troubleshooted, and reported to upstream, sounds like a serious issue. Might be coming from here: https://github.com/ckeditor/ckeditor-dev/blob/ea41a52/plugins/pastefromword/filter/default.js#L810-L820

In which case it would be crucial problem for our styles inlining.

Luckily (😌) I smell something fishy is happening in our codebase 🙂 See the expected value in: https://github.com/ckeditor/ckeditor-dev/blob/ea41a52/tests/plugins/pastefromword/parsestyles.html#L7-L12

It should be camelcased so Edge provides a correct value, the problem is why expected value gets lowercased?

After I have logged it it turns out that chrome also puts the expected value as lowercase, which doesn't make sense. I'll leave it here for you.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I extracted it to a separate issue as it seems to be quite odd behaviour #1042.

Copy link
Contributor

@mlewand mlewand left a comment

Choose a reason for hiding this comment

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

I have created a manual test that fails on latest edge: http://tests.ckeditor.test:1030/tests/plugins/clipboard/manual/customtypes

If you check Edge, it puts only last of custom data types, and discards text/html

Edge missing most of data types

While other browsers works correctly:

Chrome, firefox results

I did some debugging but it took some solid amount of time, without getting the source of the issue.

My initial assumption was that Edge does not return correct results when (native) clipboardData.getData() is called right after setting the data, however after running listener like that:

evt.editor.editable().on( 'copy', function( ckEvt ) {
	var evt = ckEvt.data.$,
		nativeDataTransfer = evt.clipboardData,
		ckDataTransfer = new CKEDITOR.plugins.clipboard.dataTransfer( nativeDataTransfer );

	ckDataTransfer.setData( 'text/plain', 'plain value' );
	ckDataTransfer.setData( 'text/html', 'html value' );
	ckDataTransfer.setData( 'foo-bar', 'foo-bar value' );
	ckDataTransfer.setData( 'baz', 'baz value' );

	console.log( 'plain', ckDataTransfer.getData( 'text/plain' ) );
	console.log( 'html', ckDataTransfer.getData( 'text/html' ) );

	// Prevent DOM event, so that clipboard gets overriden.
	evt.preventDefault();
}, null, null, 100000 );

It works well, so the issue is something other.

* @param {String} value
*/
setData: function( type, value ) {
// Extract custom data if used type is the same one which is used for storing it.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this code is just a bit misleading: it's not only extracting, but also appending this data.

How about something like that?

// In case of setting type that stores custom data, make sure that current custom data gets applied.

@mlewand
Copy link
Contributor

mlewand commented Oct 24, 2017

And the question is also why unit tests didn't catch it 🤔

@f1ames
Copy link
Contributor Author

f1ames commented Oct 25, 2017

The issue is caused by the upstream bug (https://developer.microsoft.com/en-us/microsoft-edge/platform/issues/14110451/). It is not possible to retrieve the data (via nativeDataTransfer.getData) in the same listener (synchronously) in which it was set.

That' the reason it looks like setting custom types overwrites previous values. The mechanism check if something was already checked in merges the data. As the browse incorrectly returns empty data, the new value does not contain previously set data.

And the question is also why unit tests didn't catch it

Unit tests use data transfer mock so native issues may not be visible there.

The solution as we discussed with @mlewand will be to implement cache mechanism for CKEDITOR.plugins.clipboard.fallbackDataTransfer. It should utilize already existing cache mechanism from CKEDITOR.plugins.clipboard.dataTransfer.

@f1ames f1ames requested a review from mlewand October 26, 2017 16:15
@msamsel
Copy link
Contributor

msamsel commented Oct 31, 2017

Extracted #1047.

I've made fix for that, which runs test in separate editors.

Copy link
Contributor

@mlewand mlewand left a comment

Choose a reason for hiding this comment

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

The problem is clipboard is getting bigger and bigger. It has just passed 3k LoC milestone. 😰

I was thinking about something much simpler, instead of adding (backward incompatible) full featured type for caching, why not use "old" and simple cache object? The only thing you need on top of it is a list of custom types that should be handled by _fallbackDataTransfer.

How to do that?

Just keep a simple array, say CKEDITOR.plugins.clipboard.fallbackDataTransfer.customTypes (it make sense to make it a static prop). It's members are collected when:

With that we'd have:

  • 100% backward copatibility
  • substantially less LoC
  • simplified implementation

Additionally:

  • It would be best to stick with the existing class structure, and put cache instance in dataTransfer._.
    • Actually WDYT, does it make sense for us to move fallbackDataTransfer there too?

if ( CKEDITOR.tools.array.indexOf( types, 'text/html' ) != -1 ) {
// Native 'getData' needs to be used here as 'ckDataTransfer.getData'
// strips special comment which we would like to show here too.
output += getRow( 'Native HTML data', ckDataTransfer.$.getData( 'text/html', true ), 'raw' );
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that in fact special comment should be included when calling ckDataTransfer.getData( 'text/html', true ). By passing getData() getNative parameter, you say that you really mean you want to get all the meat - and this is what you should get.

So that being said fallback should respect that and return raw text/html. This parameter was created deliberately for text/html (though it's not properly mentioned in the API docs 😒 #1121).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes sense 👍 , I wasn't fully sure how it should work with fallback html.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Btw. should getNative work only for text/html? In Edge if you call getData( 'cke/id', true ) if we assume getNative should really return what natively sits in dataTransfer you will get empty string.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The other issue with getNative flag is that, it is used inside cacheData:
https://github.com/ckeditor/ckeditor-dev/blob/96bf4037cb3ff812fc9b993bce4a304a884f361a/plugins/clipboard/plugin.js#L2450

so we would like to cache "native value" for all browsers, but for Edge the cache should contain proper type : value pairs without special comment so it shouldn't basically use getNative (fallbackDataTransfer.isRequired() could be use here but it will create another "logic branch", increasing the complexity of already complex plugin 😞 ).

Copy link
Contributor

Choose a reason for hiding this comment

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

Btw. should getNative work only for text/html? In Edge if you call getData( 'cke/id', true ) if we assume getNative should really return what natively sits in dataTransfer you will get empty string.

This is exactly how it should work, I can see that you already implemented it that way. I have corrected this in 3375736.

value = null;

// If we are getting the same type which may store custom data we need to extract content only.
if ( type === this._customDataFallbackType ) {
Copy link
Contributor

@mlewand mlewand Nov 6, 2017

Choose a reason for hiding this comment

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

What about plain text? AFAICT in this construction it will prioritize looking for plain text in the dataComment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it will. But as we don't have hardcoded list of supported types, first checking dataComment is required. Remember that accessing unsupported type in Edge returns empty string (no exception so it can't be distinguished properly). As plain/text won't be there it will simply fallback to regular getData.

Copy link
Contributor

Choose a reason for hiding this comment

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

Make sense 👍

if ( type === this._customDataFallbackType ) {
value = dataComment.content;

// If we are getting different type we need to check inside data comment if it is stored there.
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment should be moved to a proper line, which is before if ( dataComment.data && dataComment.data[ type ] ) {.

if ( dataComment.data && dataComment.data[ type ] ) {
value = dataComment.data[ type ];

// And then fallback to regular `getData`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Similar as above, it would make it more readable to move this comment to else block.

Without this change values were a bit different than expected pattern.
@mlewand
Copy link
Contributor

mlewand commented Nov 6, 2017

I have pushed small change in the manual test in 0aac1fe - without it the values in table were not 100% following the pattern. (it has shown "plain value" instead of "Text value").

@f1ames
Copy link
Contributor Author

f1ames commented Nov 6, 2017

I was thinking about something much simpler, instead of adding (backward incompatible) full featured type for caching, why not use "old" and simple cache object? The only thing you need on top of it is a list of custom types that should be handled by _fallbackDataTransfer.

Makes sense, I extracted it to make it more readable as getData/setData are already a little complicated, but we can try with simple version.

Just keep a simple array, say CKEDITOR.plugins.clipboard.fallbackDataTransfer.customTypes (it make sense to make it a static prop). It's members are collected when:

  • setData fails, due to a custom type,
  • getData (after adding a this._isUnsupportedMimeTypeError check)

We can only mark it on setData as getData does not throw an exception when accesing not supported type (returning empty string). But, it is the same as it works now.

  • It would be best to stick with the existing class structure, and put cache instance in dataTransfer._.
    Actually WDYT, does it make sense for us to move fallbackDataTransfer there too?

Yes, I suppose both cache and fallbackDataTransfer should be in dataTransfer._. "namespace".

@f1ames
Copy link
Contributor Author

f1ames commented Nov 7, 2017

I simplified the cache and added other small changes according to what is described above.

@f1ames f1ames requested a review from mlewand November 7, 2017 10:32
@f1ames
Copy link
Contributor Author

f1ames commented Nov 7, 2017

I updated closes reference of this PR, as after introducing cache it also resolves #1026.

Copy link
Contributor

@mlewand mlewand left a comment

Choose a reason for hiding this comment

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

I must say, I like this view:

150 lines removed, 50 lines added picture 🙂

You have renamed dataTransfer._.data to dataTransfer._.cache. Let's make it dataTransfer._.data back, so any apps that relied on it won't break (though it was private/undocumented but there's no good reason to break it).

I feel these are just last touches here, and we're done with it.

dataTransfer1a = new CKEDITOR.plugins.clipboard.dataTransfer( nativeData1 ),
dataTransfer1b = new CKEDITOR.plugins.clipboard.dataTransfer( nativeData1 ),
dataTransfer2 = new CKEDITOR.plugins.clipboard.dataTransfer( nativeData2 );
dataTransfer1a, dataTransfer1b, dataTransfer2;
Copy link
Contributor

Choose a reason for hiding this comment

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

Vars should be defined each in a separate line.

* @param {Object} cache Cache object used on storing and retrieving data.
* @param {Object} [nativeDataTransfer] A native data transfer object.
*/
CKEDITOR.plugins.clipboard.fallbackDataTransfer = function( cache, nativeDataTransfer ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't it be better if this constructor simply take a (our) dataTransfer instance? Having that you could assign both cache and native data trasnfer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I applied this fix, however then you have this._cache = dataTransfer._.data which means accessing private property outside of dataTransfer class, which shouldn't have place and is not a good practice. WDYT @mlewand, alternatively we could add a getter or leave it as it was?

*
* @private
* @static
* @property {Array}
Copy link
Contributor

Choose a reason for hiding this comment

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

You could doc the type even better, it's an array of strings, so {String[]}.

* @param {String} value
*/
setData: function( type, value ) {
// In case of fallbackDataTransfer, cache does not reflect native data one-to-one. For example, having
Copy link
Contributor

Choose a reason for hiding this comment

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

Actually AFAICS this is no longer the case. Now cache reflects what's stored in given types (also cache does not contain custom data marker).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is still the case, all data is cached in CKEDITOR.plugins.clipboard.dataTransfer#setData which doesn't have any knowledge how fallback works. So the cache contains proper type : value pairs no matter if it is supported by the browser or not.
And then native data transfer contains fallback specific types if needed.

AFAIR, that was one of the reasons for cache, to skip processing the special data comment every time when getData is called.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ahh, sorry my bad, at first I looked at it as if 1st object was data mapping passed to .setData() and the second was our cache. While it is our cache -> browser native clipboard (which is actually documented).

I think we need to get other eyes on it to make sure it was only case for me. 🙂

if ( CKEDITOR.tools.array.indexOf( types, 'text/html' ) != -1 ) {
// Native 'getData' needs to be used here as 'ckDataTransfer.getData'
// strips special comment which we would like to show here too.
output += getRow( 'Native HTML data', ckDataTransfer.$.getData( 'text/html', true ), 'raw' );
Copy link
Contributor

Choose a reason for hiding this comment

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

Btw. should getNative work only for text/html? In Edge if you call getData( 'cke/id', true ) if we assume getNative should really return what natively sits in dataTransfer you will get empty string.

This is exactly how it should work, I can see that you already implemented it that way. I have corrected this in 3375736.

value = null;

// If we are getting the same type which may store custom data we need to extract content only.
if ( 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.

Make sense 👍

Copy link
Contributor

@mlewand mlewand left a comment

Choose a reason for hiding this comment

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

And we made it, LGTM

@mlewand mlewand merged commit 49281a9 into t/468 Nov 10, 2017
@mlewand
Copy link
Contributor

mlewand commented Nov 10, 2017

For the records, I've slightly messed the merge, because I rebased your branch onto latest major but then performed merge on GitHub without pushing.

To fix that I've quickly force-pushed t/468 branch back to where it was before the merge, rebased it onto latest major, and then merged rebased t/468-962 branch.

Now t/468 is fine. 🙂

@mlewand mlewand mentioned this pull request Nov 10, 2017
2 tasks
@f1ames f1ames mentioned this pull request Nov 13, 2017
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants