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

Paste replacement #976

Merged
merged 21 commits into from
Oct 25, 2017
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
68 changes: 32 additions & 36 deletions plugins/pastefromwordimage/plugin.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,57 +9,53 @@
CKEDITOR.plugins.add( 'pastefromwordimage', {
requires: 'pastefromword',
init: function( editor ) {
if ( !CKEDITOR.plugins.clipboard.isCustomDataTypesSupported ) {
if ( CKEDITOR.env.ie || CKEDITOR.env.iOS ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Was there any particular reason why it was changed from !CKEDITOR.plugins.clipboard.isCustomDataTypesSupported to CKEDITOR.env.ie || CKEDITOR.env.iOS?

If it is about iOS you may always go with !CKEDITOR.plugins.clipboard.isCustomDataTypesSupported || CKEDITOR.env.iOS which is more flexible (as CKEDITOR.plugins.clipboard.isCustomDataTypesSupported may change at some point and then it will automatically work here too).

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've change wrong file :(
It supposed to be switched in manual test here: https://github.com/ckeditor/ckeditor-dev/blob/aa75e0874a372dd89168314eaae2162c248a1ff9/tests/plugins/pastefromwordimage/manual/pastewordwithimage.html#L3
To avoid problem with loading plugins. As manual test are usually fired only while releasing, then such change should be sufficient there.

return;
}

// Register a proper filter, so that images are not stripped out.
editor.filter.allow( 'img[src]' );

editor.on( 'afterPasteFromWord', this.pasteListener, this );
},
editor.on( 'afterPasteFromWord', pasteListener );
}
} );

pasteListener: function( evt ) {
var pfwi = CKEDITOR.plugins.pastefromwordimage,
imgTags,
hexImages,
newSrcValues = [],
i;

imgTags = pfwi.extractImgTagsFromHtml( evt.data.dataValue );
if ( imgTags.length === 0 ) {
return;
}
function pasteListener( evt ) {
var pfwi = CKEDITOR.plugins.pastefromwordimage,
imgTags,
hexImages,
newSrcValues = [],
i;

hexImages = pfwi.extractImagesFromRtf( evt.data.dataTransfer[ 'text/rtf' ] );
if ( hexImages.length === 0 ) {
return;
}
imgTags = pfwi.extractImgTagsFromHtml( evt.data.dataValue );
if ( imgTags.length === 0 ) {
return;
}

CKEDITOR.tools.array.forEach( hexImages, function( img ) {
newSrcValues.push( this.createSrcWithBase64( img ) );
}, this );
hexImages = pfwi.extractImagesFromRtf( evt.data.dataTransfer[ 'text/rtf' ] );
if ( hexImages.length === 0 ) {
return;
}

// Assumption there is equal amount of Images in RTF and HTML source, so we can match them accordingly to existing order.
if ( imgTags.length === newSrcValues.length ) {
for ( i = 0; i < imgTags.length; i++ ) {
// Replace only `file` urls of images ( shapes get newSrcValue with null ).
if ( ( imgTags[ i ].indexOf( 'file://' ) === 0 ) && newSrcValues[ i ] ) {
evt.data.dataValue = evt.data.dataValue.replace( imgTags[ i ], newSrcValues[ i ] );
}
CKEDITOR.tools.array.forEach( hexImages, function( img ) {
newSrcValues.push( createSrcWithBase64( img ) );
}, this );

// Assumption there is equal amount of Images in RTF and HTML source, so we can match them accordingly to existing order.
if ( imgTags.length === newSrcValues.length ) {
for ( i = 0; i < imgTags.length; i++ ) {
// Replace only `file` urls of images ( shapes get newSrcValue with null ).
if ( ( imgTags[ i ].indexOf( 'file://' ) === 0 ) && newSrcValues[ i ] ) {
evt.data.dataValue = evt.data.dataValue.replace( imgTags[ i ], newSrcValues[ i ] );
}
}
},

createSrcWithBase64: function( img ) {
return img.type ? 'data:' + img.type + ';base64,' + this.hexToBase64( img.hex ) : null;
},

hexToBase64: function( hexString ) {
return CKEDITOR.tools.convertBytesToBase64( CKEDITOR.tools.convertHexStringToBytes( hexString ) );
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I know the above two looks nice and super clear, but hexToBase64 is only used once inside createSrcWithBase64 so maybe it could be merged into one function?

}

} );
function createSrcWithBase64( img ) {
return img.type ? 'data:' + img.type + ';base64,' + CKEDITOR.tools.convertBytesToBase64( CKEDITOR.tools.convertHexStringToBytes( img.hex ) ) : null;
}

/**
* Help methods used by paste from word image plugin.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,6 @@ function createTestCase( options ) {
load( outputPath + deCasher ),
load( specialCasePath + deCasher )
];

if ( options.includeRTF ) {
loadQueue.push( load( inputPathRtf + deCasher ) );
}
Expand All @@ -63,7 +62,7 @@ function createTestCase( options ) {
return;
}
// Single null when RTF is avaialble means that one of 2 required files is missing.
else if ( inputFixtureHtml === null || inputFixtureRtf === null ) {
else if ( options.includeRTF && ( inputFixtureHtml === null || inputFixtureRtf === null ) ) {
resume( function() {
assert.isNotNull( inputFixtureHtml, '"' + inputPathHtml + '" file is missing' );
assert.isNotNull( inputFixtureRtf, '"' + inputPathRtf + '" file is missing' );
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,7 @@
<textarea id="editor" cols="30" rows="10"></textarea>
<script>
Copy link
Contributor

Choose a reason for hiding this comment

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

This test should be ignored in browsers not supporting Clipboard API IMHO.

Copy link
Contributor

Choose a reason for hiding this comment

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

Alternatively you may describe what is the expected behaviour for such browsers.

if ( !CKEDITOR.plugins.clipboard.isCustomDataTypesSupported ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This code should be called at least after pluginsLoaded event as at this point it throws an error
image

because CKEDITOR.plugins.clipboard is not initialized yet.

bender.ignore();
}
CKEDITOR.replace( 'editor' );
</script>
12 changes: 7 additions & 5 deletions tests/plugins/pastefromwordimage/manual/pastewordwithimage.md
Original file line number Diff line number Diff line change
@@ -1,13 +1,15 @@
@bender-tags: bug, 4.8.0, 662, pastefromwordimage
@bender-ui: collapsed
@bender-ckeditor-plugins: wysiwygarea, toolbar, undo, pastefromwordimage, sourcearea, elementspath, newpage
@bender-ckeditor-plugins: wysiwygarea, toolbar, undo, pastefromwordimage, sourcearea, elementspath, newpage, resize

----

1. Using Microsoft Word open a document with images [Offline img](../generated/_fixtures/SimpleImages/SimpleImages.docx).
1. Select the whole document content.
1. Paste it into CKEditor instance.
1. Clear editor with `New page` and repeat above steps for next two files:
1. Open document and copy whole content.
1. Clear editor content.
1. Paste copied content into CKEditor.

### Perform above steps for all files:
* [Offline img](../generated/_fixtures/SimpleImages/SimpleImages.docx).
Copy link
Contributor

Choose a reason for hiding this comment

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

Links doesn't work anymore as folder names were changed.

* [Online and offline img](../generated/_fixtures/MixedOnline/Mixed_drawings_and_online.docx),
* [Document with shape](../generated/_fixtures/MixedOnlineAndShapes/Mixed_drawings_and_online_and_shapes.docx). _Please notice that shape should be ignored while pasting._

Copy link
Contributor

Choose a reason for hiding this comment

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

When you have same scenario for multiple files/editors/etc you may just put a generic steps like:

  1. Open document and copy its whole content.
  2. Clear editor contents / empty editor / etc.
  3. Paste copied content into CKEditor instance.

Perform above scenario for all files:
... list of files

Expand Down