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

Edge 17 throws an error when pasting into editor #2192

Merged
merged 9 commits into from
Jul 5, 2018
Merged

Edge 17 throws an error when pasting into editor #2192

merged 9 commits into from
Jul 5, 2018

Conversation

engineering-this
Copy link
Contributor

@engineering-this engineering-this commented Jun 29, 2018

What is the purpose of this pull request?

Fixes

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?

Testing for custom mime types (just by trying setData with such type) makes dataTransfer inaccessible in Edge, so as a workaround for upstream issue I've added if statement which will omit such test on Edge.

Note:
I have different error in console, than described in original issue, but with my fix it looks to work fine.

Closes #2169.

Copy link
Member

@Comandeer Comandeer left a comment

Choose a reason for hiding this comment

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

Isn't there any more elegant way to do it in Edge than browser sniffing? I'm wondering about using window.DataTransfer to create new DataTransfer object and test on it instead of already existing instance.

If it works, I'll add also unit test using the same technique.

@@ -2825,6 +2825,11 @@

fallbackDataTransfer._isCustomMimeTypeSupported = false;

// It looks like after our custom mime type test Edge 17 is denying access on nativeDataTransfer.
if ( CKEDITOR.env.edge && CKEDITOR.env.version > 16 ) {
return false;
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't it return true? As it's stated in comment to this method:

If true is returned, it means custom MIME types are not supported in the current browser

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, that negated return, good call. But now I am surprised that change actually makes it work.

@@ -2825,6 +2825,11 @@

fallbackDataTransfer._isCustomMimeTypeSupported = false;

// It looks like after our custom mime type test Edge 17 is denying access on nativeDataTransfer.
Copy link
Member

Choose a reason for hiding this comment

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

Please add reference to the issues in our and Edge trackers.

@engineering-this
Copy link
Contributor Author

I've tried such test for custom mime types on Edge:

var testTransfer = new DataTransfer();

testTransfer.setData( testType, testValue );
fallbackDataTransfer._isCustomMimeTypeSupported = testTransfer.getData( testType ) === testValue;
testTransfer.clearData( testType );

But such test still corrupts original data transfer, and Edge denies access to it's items. Also DataTransfer methods are poorly supported and it doesn't help in creating proper unit test.

CHANGES.md Outdated
@@ -7,6 +7,10 @@ Fixed Issues:

* [#2114](https://github.com/ckeditor/ckeditor-dev/issues/2114): [Autocomplete](https://ckeditor.com/cke4/addon/autocomplete) cannot be initialized before [`instanceReady`](https://docs.ckeditor.com/ckeditor4/latest/api/CKEDITOR_editor.html#event-instanceReady).

Fixed Issues:
Copy link
Member

Choose a reason for hiding this comment

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

There is already "Fixed Issues` section.

@@ -2825,6 +2825,12 @@

fallbackDataTransfer._isCustomMimeTypeSupported = false;

// It looks like after our custom mime type test Edge 17 is denying access on nativeDataTransfer (#2169).
// Upstream issue: https://developer.microsoft.com/en-us/microsoft-edge/platform/issues/18089287/
if ( CKEDITOR.env.edge && CKEDITOR.env.version > 16 ) {
Copy link
Member

Choose a reason for hiding this comment

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

CKEDITOR.env.version is a float, not integer. In this case this conditional is targeting also 16.x Edge.

Copy link
Member

@Comandeer Comandeer left a comment

Choose a reason for hiding this comment

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

LGTM. I've just updated changelog entry and fixed a typo in comment.

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.

None yet

2 participants