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.

Sign up for GitHub

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

Paste replacement #976

Show file tree
Hide file tree
Changes from 16 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
d84763d
Changes in PFW required for images.
Sep 21, 2017
eae8e25
Some working example. Base for further improvements.
Sep 21, 2017
c434d75
Add fixture generator for PFWImage.
Sep 25, 2017
242c8cf
Adding library necessary for auto-generated tests.
Sep 26, 2017
08b1490
Add auto-generated unit tests.
Sep 26, 2017
693e3f1
Small code improvements such as: renaming variables, extracting code …
Sep 28, 2017
091aa2f
Adding manual tests.
Sep 28, 2017
5da4676
Hot fixes after rebasing on t/662-2973.
Oct 3, 2017
22f7e81
Adding new fixtures.
Oct 3, 2017
5d870ea
Little code improvement, adding macos support for test.
Oct 3, 2017
54d2498
Set font-family as disallowed to fix Unit tests on Firefox. Remove fo…
Oct 4, 2017
17cc5dd
Fix test name for helpers.
Oct 17, 2017
d2d8a13
Fix typos.
f1ames Oct 18, 2017
2e852c0
Update PFW autogenerated test to support RTF clipboard. Remove test g…
Oct 19, 2017
ab1ff15
Rename test cases to be more consistent.
Oct 19, 2017
aa75e08
Move PFW Image logic outside of plugin definition. Update Manual tests.
Oct 19, 2017
bd65d9c
Typo fixes.
f1ames Oct 20, 2017
b9b878a
Test updates. Small code improvements in plugin.
Oct 20, 2017
31f7874
Manual test - ignore on loaded.
f1ames Oct 23, 2017
2055b4d
Review: rename tests, fix links
Oct 24, 2017
6388b13
Some rewording.
f1ames Oct 25, 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
1 change: 1 addition & 0 deletions config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ CKEDITOR.editorConfig = function( config ) {
'newpage,' +
'pagebreak,' +
'pastefromword,' +
'pastefromwordimage,' +
'pastetext,' +
'preview,' +
'print,' +
Expand Down
97 changes: 97 additions & 0 deletions dev/pastefromwordimage/getclipboard.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,97 @@
<!DOCTYPE html>
<html>
<head>
<title>Get Clipboard HTML and RTF</title>
<style>

textarea {
border: 1px solid #808080;
float: left;
height: 200px;
width: 100%;
overflow: auto;
margin-bottom: 20px;;
}

</style>
<script src="../../ckeditor.js"></script>
</head>
<body>
<p>Paste Inside the Editor:</p>
<div><textarea id="input"></textarea></div>
<div>
<div style="float: left; width: 49%">
<p>Raw HTML Data Received:</p>
<textarea data-name="input.html" id="rawHtml" readonly="readonly"></textarea>
<button id="htmlData">Save HTML Data</button>
</div>
<div style="float: right; width: 49%">
<p>Raw RTF Data Received:</p>
<textarea data-name="input.rtf" id="rawRtf" readonly="readonly"></textarea>
<button id="rtfData">Save RTF Data</button>
</div>
</div>
<div style="width: 100%; float: left;">
<p>After Paste Processing:</p>
<textarea id="output" readonly="readonly"></textarea>
</div>
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe you could also add a container which will show all images pasted to the CKEditor (in the form already transformed by CKEditor), something like Extracted Images? If it's relatively easy to do (like 0,5h) you could take a look at it. From the other hand you can see pasted images inside editor instance, so not sure if it is needed. WDYT?

Copy link
Contributor Author

@msamsel msamsel Oct 19, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

On this stage of implementation it might be problematic. I could process RTF clipboard, but it might contains also Word Shapes, which are ignored for processing while pasting. That's why preparing similar solution here might be repeating functionality of the plugin, which seems to be not a good thing.
It should be easier, when further changes related to file transfer will be implemented:
https://github.com/ckeditor/ckeditor-dev/blob/128516a52485505176189743962eaf97d52aa067/plugins/pastefromwordimage/plugin.js#L60-L63
There is nice event when will be exposed processed URL and image which is going to be embed. With that implementation it should be like 0,5h of work :)

Copy link
Contributor

Choose a reason for hiding this comment

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

@msamsel Ok, so let's leave it as it is for now.


<script>
var editor = CKEDITOR.replace( 'input', {
height: 100,
allowedContent: true,
plugins: 'pastefromword,pastefromwordimage,wysiwygarea'
} );

editor.on( 'paste', function( evt ) {
var val = evt.data.dataValue;

if ( evt.data.dataTransfer && evt.data.dataTransfer.getData( 'text/html', true ) ) {
val = evt.data.dataTransfer.getData( 'text/html', true );
}
document.getElementById( 'rawHtml' ).value = val;

if ( evt.data.dataTransfer && evt.data.dataTransfer.getData( 'text/rtf', true ) ) {
val = evt.data.dataTransfer.getData( 'text/rtf', true );
}
document.getElementById( 'rawRtf' ).value = val;

}, null, null, -1 );

editor.on( 'paste', function( evt ) {
setTimeout( function() {
document.getElementById( 'output' ).value = editor.getData();
}, 0 );
}, null, null, 999 );

var rtfButton = document.getElementById( 'rtfData' ),
htmlButton = document.getElementById( 'htmlData' );

rtfButton.onclick = save( document.getElementById( 'rawRtf' ) );
htmlButton.onclick = save( document.getElementById( 'rawHtml' ) );

function save( input ) {
return function() {
var textBlob = new Blob( [ input.value ], { type: 'text/plain' } );
var saveLink = document.createElement( 'a' );

saveLink.download = input.dataset.name;
saveLink.innerHTML = 'Save file';
if ( CKEDITOR.env.webkit ) {
saveLink.href = window.URL.createObjectURL( textBlob );
} else {
saveLink.href = window.URL.createObjectURL( textBlob );
saveLink.onclick = function( evt ) {
document.body.removeChild( evt.target )
};
saveLink.style.display = 'none';
document.body.appendChild( saveLink );
}
saveLink.click();
}
}


</script>
</body>
</html>
5 changes: 4 additions & 1 deletion plugins/pastefromword/plugin.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,10 +62,13 @@
var data = evt.data,
dataTransferHtml = CKEDITOR.plugins.clipboard.isCustomDataTypesSupported ?
data.dataTransfer.getData( 'text/html', true ) : null,
// Required in paste from word image plugin (#662).
dataTransferRtf = CKEDITOR.plugins.clipboard.isCustomDataTypesSupported ?
data.dataTransfer.getData( 'text/rtf', true ) : null,
// Some commands fire paste event without setting dataTransfer property. In such case
// dataValue should be used.
mswordHtml = dataTransferHtml || data.dataValue,
pfwEvtData = { dataValue: mswordHtml },
pfwEvtData = { dataValue: mswordHtml, dataTransfer: { 'text/rtf': dataTransferRtf } },
officeMetaRegexp = /<meta\s*name=(?:\"|\')?generator(?:\"|\')?\s*content=(?:\"|\')?microsoft/gi,
wordRegexp = /(class=\"?Mso|style=(?:\"|\')[^\"]*?\bmso\-|w:WordDocument|<o:\w+>|<\/font>)/,
isOfficeContent = officeMetaRegexp.test( mswordHtml ) || wordRegexp.test( mswordHtml );
Expand Down
49 changes: 47 additions & 2 deletions plugins/pastefromwordimage/plugin.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,55 @@

CKEDITOR.plugins.add( 'pastefromwordimage', {
requires: 'pastefromword',
init: function() {}
init: function( editor ) {
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', pasteListener );
}
} );


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

imgTags = pfwi.extractImgTagsFromHtml( evt.data.dataValue );
if ( imgTags.length === 0 ) {
return;
}

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

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 ] );
}
}
}
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 All @@ -36,7 +82,6 @@
rePictureOrShape = new RegExp( '(?:(' + rePictureHeader.source + ')|(' + reShapeHeader.source + '))([\\da-fA-F\\s]+)\\}', 'g' ),
wholeImages,
imageType;

wholeImages = rtfContent.match( rePictureOrShape );
if ( !wholeImages ) {
return ret;
Expand Down
9 changes: 6 additions & 3 deletions tests/plugins/pastefromword/generated/_helpers/assertWordFilter.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,17 +1,20 @@
/* global promisePasteEvent */
/* exported assertWordFilter */
function assertWordFilter( editor, compareRawData ) {
return function( input, output ) {
return function( inputHtml, output, inputRtf ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe you could use an object here input: { 'text/html': ..., 'text/rtf': ... }?

var nativeDataTransfer = bender.tools.mockNativeDataTransfer(),
dataTransfer;

if ( CKEDITOR.plugins.clipboard.isCustomDataTypesSupported ) {
nativeDataTransfer.setData( 'text/html', input );
nativeDataTransfer.setData( 'text/html', inputHtml );
if ( inputRtf ) {
nativeDataTransfer.setData( 'text/rtf', inputRtf );
}
}

dataTransfer = new CKEDITOR.plugins.clipboard.dataTransfer( nativeDataTransfer );

return promisePasteEvent( editor, { dataValue: input, dataTransfer: dataTransfer } )
return promisePasteEvent( editor, { dataValue: inputHtml, dataTransfer: dataTransfer } )
.then( function( data ) {
return [
// Lowercase, since old IE versions paste the HTML tags in uppercase.
Expand Down
40 changes: 29 additions & 11 deletions tests/plugins/pastefromword/generated/_helpers/createTestCase.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,13 @@
* @param {Boolean} [options.compareRawData=false] If `true` test case will assert against raw paste's `data.dataValue` rather than
* what will appear in the editor after all transformations and filtering.
* @param {Array} [options.customFilters] Array of custom filters (like [ pfwTools.filters.font ]) which will be used during assertions.
* @param {Boolean} options.includeRTF Flag infor if RTF clipboard should be loaded in test case
Copy link
Contributor

Choose a reason for hiding this comment

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

Make it optional and false by default @param {Boolean} [options.includeRTF=false]. Also I would update a description to sth like Whether RTF fixture should be loaded.

* @returns {Function}
*/
function createTestCase( options ) {
return function() {
var inputPath = [ '_fixtures', options.name, options.wordVersion, options.browser ].join( '/' ) + '.html',
var inputPathHtml = [ '_fixtures', options.name, options.wordVersion, options.browser ].join( '/' ) + '.html',
Copy link
Contributor

Choose a reason for hiding this comment

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

You may extract [ '_fixtures', options.name, options.wordVersion, options.browser ] like:

var inputPath = [ '_fixtures', options.name, options.wordVersion, options.browser ].join( '/' ),
    inputPathHtml = inputPath + '.html,
    inputPathRtf = options.includeRTF ? inputPath + '.rtf' : null,

Copy link
Contributor

Choose a reason for hiding this comment

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

Also options.includeRTF ? inputPath + '.rtf' : null, is not needed here as further in the code options.includeRTF is checked so the inputPathRtf may be set even when options.includeRTF is false.

inputPathRtf = options.includeRTF ? [ '_fixtures', options.name, options.wordVersion, options.browser ].join( '/' ) + '.rtf' : false,
outputPath = [ '_fixtures', options.name, '/expected.html' ].join( '/' ),
specialCasePath = [ '_fixtures', options.name, options.wordVersion, 'expected_' + options.browser ].join( '/' ) + '.html',
deCasher = '?' + Math.random().toString( 36 ).replace( /^../, '' ), // Used to trick the browser into not caching the html files.
Expand All @@ -34,24 +36,40 @@ function createTestCase( options ) {
} );

return deferred.promise;
};
},
loadQueue = [
load( inputPathHtml + deCasher ),
load( outputPath + deCasher ),
load( specialCasePath + deCasher )
];
if ( options.includeRTF ) {
loadQueue.push( load( inputPathRtf + deCasher ) );
}

Q.all( [
load( inputPath + deCasher ),
load( outputPath + deCasher ),
load( specialCasePath + deCasher )
] ).done( function( values ) {
var inputFixture = values[ 0 ],

Q.all( loadQueue ).done( function( values ) {
var inputFixtureHtml = values[ 0 ],
inputFixtureRtf = options.includeRTF ? values[ 3 ] : null ,
// If browser-customized expected result was found, use it. Otherwise go with the regular expected.
expectedValue = values[ 2 ] !== null ? values[ 2 ] : values[ 1 ];

// null means that fixture file was not found - skipping test.
if ( inputFixture === null ) {
// null means that fixture file was not found in case of regular test - skipping test.
// In case of using RTF clipboard it's required to have both nulls
if ( inputFixtureHtml === null && ( !options.includeRTF || inputFixtureRtf === null ) ) {
resume( function() {
assert.ignore();
} );
return;
}
// Single null when RTF is avaialble means that one of 2 required files is missing.
else if ( options.includeRTF && ( inputFixtureHtml === null || inputFixtureRtf === null ) ) {
resume( function() {
assert.isNotNull( inputFixtureHtml, '"' + inputPathHtml + '" file is missing' );
assert.isNotNull( inputFixtureRtf, '"' + inputPathRtf + '" file is missing' );
} );
}



var nbspListener = editor.once( 'paste', function( evt ) {
// Clipboard strips white spaces from pasted content if those are not encoded.
Expand All @@ -67,7 +85,7 @@ function createTestCase( options ) {

assert.isNotNull( expectedValue, '"expected.html" missing.' );

assertWordFilter( editor, options.compareRawData )( inputFixture, expectedValue )
assertWordFilter( editor, options.compareRawData )( inputFixtureHtml, expectedValue, inputFixtureRtf )
.then( function( values ) {
resume( function() {
nbspListener.removeListener();
Expand Down
6 changes: 4 additions & 2 deletions tests/plugins/pastefromword/generated/_helpers/createTestSuite.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,8 @@ function createTestSuite( options ) {
testData: { _should: { ignore: {} } },
ignoreAll: false,
compareRawData: false,
customFilters: null
customFilters: null,
includeRTF: false
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing docs for includeRTF (could be the same as in createTestCase) in the method @params list.

} );

var testData = options.testData,
Expand Down Expand Up @@ -52,7 +53,8 @@ function createTestSuite( options ) {
wordVersion: wordVersion,
browser: options.browsers[ j ],
compareRawData: options.compareRawData,
customFilters: options.customFilters
customFilters: options.customFilters,
includeRTF: options.includeRTF
} );
}
}
Expand Down
10 changes: 10 additions & 0 deletions tests/plugins/pastefromword/generated/_helpers/pfwTools.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,16 @@
'unicode-bidi,direction,dir,lang,page-break-after};td[valign]',
disallowedContent: 'td{vertical-align}'
},
// Prefered editor config for generated tests with PFW Image.
imageDefaultConfig: {
language: 'en',
removePlugins: 'dialogadvtab,flash,showborders,horizontalrule',
colorButton_normalizeBackground: false,
extraAllowedContent: 'span{line-height,background,font-weight,font-style,text-decoration,text-underline,display,' +
'page-break-before,height,tab-stops,layout-grid-mode,text-justify,-ms-layout-grid-mode,-ms-text-justify,' +
'unicode-bidi,direction,dir,lang,page-break-after};td[valign]',
disallowedContent: 'td{vertical-align};*[data-cke-*];span{font-family}'
},

// Filters for use in compatHtml in tests.
filters: {
Expand Down
Binary file added ...stefromwordimage/generated/_fixtures/OnlineAndOfflineImage/Mixed_drawings_and_online.docx
View file
Open in desktop
Binary file not shown.
1 change: 1 addition & 0 deletions tests/plugins/pastefromwordimage/generated/_fixtures/OnlineAndOfflineImage/expected.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
<p style="margin-left:0in; margin-right:0in"><span style="font-size:11pt"><span style="line-height:107%"><span>Kitty from internet: <img alt="http://placekitten.com/200/305" style="width:200px; height:305px" src="http://placekitten.com/200/305" /></span></span></span></p><p style="margin-left:0in; margin-right:0in"><span style="font-size:11pt"><span style="line-height:107%"><span>My drawing: <img style="width:32px; height:32px" src="data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAACAAAAAgCAIAAAD8GO2jAAAAAXNSR0IArs4c6QAAAAlwSFlzAAAOwgAADsIBFShKgAAAAOdJREFUSEvtljEWgjAQRA0HsLG19P4nsrS18QK6efFBMpMsS1hS+KQDNn+YAbIbntfb5XE/lcf7BRdMp+GMZQIPvLSPPnNAphDYia5qLAI6ne0b678CrWrm2iNNazUBC33Wqz6iEKIA39uEVjSigBc9yTBtMn3h5iK2jgJ94SgPUAi40AHiHBFbGSjgkk9ykKMGOjB/itsK/w5W81oi8uo2sCP92DtwSQkgwyPaaaLSvo5umfWIOnw055JWL+V9sfVP6SPPqMFL97G6H0BB3g9w+O1IX6HLrUkm7Lwijko0hRsd8FqBfwBKE0DK1O0llgAAAABJRU5ErkJggg==" />&nbsp;hehehehe :D <img style="width:32px; height:32px" src="data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAACAAAAAgCAIAAAD8GO2jAAAAAXNSR0IArs4c6QAAAAlwSFlzAAAOwgAADsIBFShKgAAAAU1JREFUSEu9ljEOwjAMRZvuXZhhAiQOADvszNwBcQkugbgDMzvscAAkxNQZBjhAsIjaJnaT2m1p1aWN/Z/tOK3Vsz/qpfcIXR+F33CeE42sXoOx0hq/jeqp59ouxgU0lC5jWICg+vC0ROk/FsdQ2bI8MoBHnepSUS/pxwgBOOo5rxyT6B/ADX++unJaxmdzPkyLpUTHTXsmHMtHxU2C5fhigKjuYkAr6khE6XcRh1kb7Ld2aOnaeaRRU3u7ozrcA1F9Lpsb3L49sKUEGRjRsDRFcgE03tluIu4in0OgGpUMVgYQbH5XKiIDFoCKMusDjgWg4vueQUwq4Tw6PQfVJ1ladDj53gyYVeLXx9kDaaRMe9xFrSRhs+OITEvM0Fhmpf9k4yn69oF98KcP638fW/wMX0LCwcvI/GF0dLuolQ13RWKYsJ1+gOXaGOIL4l+GAZPNqBWlOAAAAABJRU5ErkJggg==" /></span></span></span></p>
Loading