Bug fixing for shape processing and inserting linked images #1088

msamsel · 2017-10-25T08:10:31Z

What is the purpose of this pull request?

Bug fix

This PR contains

Unit tests
Manual tests

What changes did you make?

fix for 2 significant problem. Introduce both changes, because both contains modification of PFW filter and partially common fragment (v:shape element).
- problem with processing grouped and adjacent shapes
  1. There is change in PFW filter to tag img which are shapes with additional data attribute.
  2. Next in PFW Image was add recognition of such attribute. This allows on ignore img tags belong to shapes.
  3. Also it was possible to simplify RTF parser to detect only images in it.
  4. And a little clean up was required to fix test, because after parser change shapes are not returned from RTF.
- pasting images with link twice:
  1. There is change in PFW filter for v:shape processing.
  2. Old behaviour (which is probably left for IE8 compatibility, but I have not 100% sure about it) was triggered always
  3. Now it's triggered for shapes without o:gfxdata attribute. Only shapes poses this attribute, so in case of processing image old mechanism will be preserved. Alternative might be browser detection.
  4. In old behaviour was changed way how sibling tags are process. Now are searched recursively. In case of linked image, we don't have directly an img tag, but span tag which contains adequate img tag.
  5. This prevent of inserting images in Chrome and Firefox.

close #995 , close #1069

…in PFW Image to ignore shape images.

…mages on PFW output.

f1ames

The PFW filter now adds data-cke-is-shape attribute to shape nodes. However this attribute may not be allowed by ACF (that's probably why it was added to a ACF in a manual test via config). When there is a situation that specific plugin requires some attribute to work it is good practice to add it to ACF on plugin initialization so it will not be removed breaking the implementation (same as it is already done in pastefromwordimage).

I found one case with groups in which pasted output was different than expected. When I copy/paste group which doesn't have any image inside (empty or only with other shapes), the img tag doesn't have data-cke-is-shape attribute. Strangely it works fine for the Shape_Nested_Groups.docx fixture. See provided docx to reproduce - Failing.groups.docx.txt.

f1ames · 2017-10-30T14:47:31Z

plugins/pastefromword/filter/default.js

@@ -110,6 +128,16 @@
 						element.attributes.alt && element.attributes.alt.match( /^https?:\/\// ) ) {
 						element.attributes.src = element.attributes.alt;
 					}
+
+					var imgShapes = element.attributes[ 'v:shapes' ] ? element.attributes[ 'v:shapes' ].split( ' ' ) : [];


I would rename it to imgShapesIds.

f1ames · 2017-10-30T15:44:17Z

plugins/pastefromword/filter/default.js

+
+					var imgShapes = element.attributes[ 'v:shapes' ] ? element.attributes[ 'v:shapes' ].split( ' ' ) : [];
+					// Check if every single name is recognised as shape before, then add additional attribute.
+					var isShapeFromList = CKEDITOR.tools.array.every( imgShapes, function( item ) {


And then here item to shapeId.

f1ames · 2017-10-30T16:26:41Z

plugins/pastefromword/filter/default.js

-		filter = new CKEDITOR.htmlParser.filter( {
+		var shapesIds = [];
+
+		function shapeTagging( element ) {


You may move shapesIds initialization to the beginning of this function (right after msoListsDetected ). Also shapeTagging definition could be placed there (right after vars declarations).

f1ames · 2017-10-31T10:30:18Z

tests/plugins/pastefromword/manual/shapesaretagged.html

+
+</textarea>
+<button onclick="getHtml()">Get HTML</button>
+<pre id="output" style="white-space: pre-wrap;background-color:aquamarine;">


You could add word-wrap: break-word here so long base64 string will be nicely wrapped inside pre tag.

f1ames · 2017-10-31T10:37:50Z

tests/plugins/pastefromwordimage/manual/pasteimagewithlink.md

@@ -0,0 +1,16 @@
+@bender-tags: bug, 4.8.0, 662, pastefromwordimage
+@bender-ui: collapsed
+@bender-ckeditor-plugins: wysiwygarea, toolbar, undo, pastefromwordimage, sourcearea, elementspath, newpage, resize


This test fails. Image is inserted, but it is not wrapped with any link. Probably you are missing link plugin dependency here?

f1ames · 2017-10-31T10:46:38Z

tests/plugins/pastefromword/manual/shapesaretagged.md

@@ -0,0 +1,10 @@
+@bender-tags: bug, pastefromword, 4.8.0, 662


I think the Expected description should be altered for IE browsers as there is no data-cke-is-shape="true" for the first shape (should there be?).

Also for IEs shape is pasted as base64 image and visible and for other browsers there is blank space. It will be good to shortly note that too.

f1ames · 2017-10-31T10:48:24Z

tests/plugins/pastefromword/manual/shapesaretagged.md

@@ -0,0 +1,10 @@
+@bender-tags: bug, pastefromword, 4.8.0, 662
+@bender-ui: collapsed
+@bender-ckeditor-plugins: wysiwygarea, toolbar, undo, basicstyles, pastefromword, pastefromwordimage, sourcearea, elementspath, sourcearea


basicstyles plugin is not needed here from what I see.

f1ames · 2017-10-31T11:10:24Z

tests/plugins/pastefromword/generated/_helpers/pfwTools.js

@@ -12,7 +12,7 @@
 			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}'
+			disallowedContent: 'td{vertical-align};img[data-cke-is-shape]'


I see only one unit test in pastefromword is failing because of data-cke-is-shape. Maybe it will be better to adjust this test (as data-cke-is-shape is also an expected output there) instead of filtering it?

f1ames · 2017-10-31T12:20:57Z

tests/plugins/pastefromword/generated/_fixtures/Shape_adjacent_image/expected.html

@@ -0,0 +1 @@
+<p style="margin-left:0in; margin-right:0in"></p><table style="width:100%"><tbody><tr><td><div><p>hello world</p></div></td></tr></tbody></table><p><br /></p><p><br /></p><table align="left"><tbody><tr><td><br /></td><td><br /></td><td><br /></td></tr><tr><td><br /></td><td colspan="2" valign="top"><img data-cke-is-shape="true" src="file:///c:\users\foobar\appdata\local\temp\msohtmlclip1\01\clip_image001.png" style="height:106px; width:348px" /></td></tr><tr><td><br /></td></tr><tr><td><br /></td><td valign="top"><img data-cke-is-shape="true" src="file:///c:\users\foobar\appdata\local\temp\msohtmlclip1\01\clip_image002.png" style="height:106px; width:120px" /></td></tr></tbody></table><p style="margin-left:0in; margin-right:0in"><br /></p><p style="margin-left:0in; margin-right:0in"><br /></p><p style="margin-left:0in; margin-right:0in"><br /></p><p style="margin-left:0in; margin-right:0in"><br /></p><p style="margin-left:0in; margin-right:0in"><br /></p><p style="margin-left:0in; margin-right:0in"><br /></p><p style="margin-left:0in; margin-right:0in"><br /></p><p><br /></p>


I wonder why expected output has only two img tags here as there are 3 shapes in the docx and input files? When you remove text shape from the docx, expected output has 3 img tags. Looks like textbox causing some additional content after it is removed or not properly processed?

This test work a little bit different.

Textbox is treat as a shape. So there is 4 shapes in source doc.

Because 3 first shapes are adjacent, they are treat as a single img in the output. Take a look here: https://github.com/ckeditor/ckeditor-dev/blob/534af88e2b35d7e4906c38b44957940d77e2c5d8/tests/plugins/pastefromword/generated/_fixtures/Shape_adjacent_image/word2013/chrome.html#L1084
It shows that this single img tag belong to 3 shapes with given names. Every new shape is separate by a space character.

If you remove textbox and remain other shapes untouched, this will generate 3 img tags in the output, because 3 top shapes will stop being adjacent.

Makes sense, haven't noticed that detail. So this means the expected output is valid in this case, isn't it? 👍

I think it's valid here :)

f1ames · 2017-10-31T12:26:04Z

tests/plugins/pastefromword/generated/shapesdetection.js

+			'Shape_single_image': true,
+			'Shape_nested_groups': true,
+			'Shape_adjacent_image': true,
+			'Shape_and_image': true


Maybe you could gather all shapes related fixtures to one folder like:

Shapes/Single Shapes/Nested_groups Shapes/Adjacent_shapes|text <- not sure about this name Shapes/Shape_and_image

?

… canvas example.

msamsel · 2017-11-03T12:55:15Z

I've update test cases with Chrome browser from MacOS. Firefox has exactly the same markup so there is no need to duplicate it and thats why is missing in tests.
Canvas test case doesn't return img tag in text/html clipboard on MacOS. That's why those aren't present in the test cases. Having test without img tag in output, where Windows have 4 of them, might be confusing for analysing this test case it in the future.
There is update in condition which apply old behaviour of v:shape parsing. Unfortunately simple checking of CKEDITOR.env doesn't allow on proper work of autogenerated tests. Right now v:shape (without o:gfxdata attribute) can be processed with an old way (IE8 way), if fulfil 2 additional conditions. Those condition are required to detect canvas usage in text/html markup. Canvas generate empty v:shapes which should not be transform into img with an old mechanism.
- https://github.com/ckeditor/ckeditor-dev/blob/aee832046414f46a3ddea923c3c696bfe5513293/plugins/pastefromword/filter/default.js#L397
- https://github.com/ckeditor/ckeditor-dev/blob/aee832046414f46a3ddea923c3c696bfe5513293/plugins/pastefromword/filter/default.js#L402-L406

f1ames

I found one case with groups in which pasted output was different than expected. When I copy/paste group which doesn't have any image inside (empty or only with other shapes), the img tag doesn't have data-cke-is-shape attribute. Strangely it works fine for the Shape_Nested_Groups.docx fixture. See provided docx to reproduce - Failing.groups.docx.txt.

When pasting canvas shapes the is still no data-cke-is-shape (also expected fixture for canvas test does not have it). I recall we discussed to maybe extract canvas case to separate issue, but I'm not sure of the final output? Could you elaborate on that a little?

I see some inconsistent naming in fixtures again, there are word2013 and osx (df5775e#diff-523e73c28fd12a04f84635f5df5b521bR27) fixtures, the one for osx doesn't have word version specified. My proposition is the same as in pastefromwordimage fixture, to have word2013_win and wordYYYY_osx.

f1ames · 2017-11-06T10:31:46Z

plugins/pastefromword/filter/default.js

@@ -110,6 +127,16 @@
 						element.attributes.alt && element.attributes.alt.match( /^https?:\/\// ) ) {
 						element.attributes.src = element.attributes.alt;
 					}
+
+					var imgShapesId = element.attributes[ 'v:shapes' ] ? element.attributes[ 'v:shapes' ].split( ' ' ) : [];


Sorry to be such perfectionist but it should be imgShapesIds (alternatively imgShapeIds) as this is an array storing multiple ids (in most cases).

f1ames · 2017-11-06T10:38:15Z

tests/plugins/pastefromword/manual/shapesaretagged.md

+1. Click button `getHtml` below editor.
+
+**Expected:**
+  * For browsers with good clipboard processing (Firefox, Chrome): First image tag had additional attribute `data-cke-is-shape="true"`. Second image is pasted without this attribute.


As you are listing browsers, please add all of them to make it clear:

For browsers with good clipboard processing (Firefox, Chrome, Safari)...

And

For other browsers (Edge, IE, Mobile)...

f1ames · 2017-11-06T10:55:50Z

tests/plugins/pastefromword/manual/shapesaretagged.html

+</pre>
+<script>
+	var editor = CKEDITOR.replace( 'editor', {
+		extraAllowedContent: 'img[*]'


I think it is not needed here as data-cke-is-shape allow rule was added to pasteformword (https://github.com/ckeditor/ckeditor-dev/pull/1088/files#diff-9609bf210ad60faf5b8f8f446baf4e1eR21) ?

Ahh, I see, there is no image plugin in bender-plugins. Was there any particular reason for using extraAllowedContent: img[*] instead of including image plugin?

msamsel · 2017-11-07T11:08:58Z

I fix those minor stuff and rename test. When I start to analyse canvas issue and try to describe it as separate one, I found possible solution. It seem like changing one if solve the case. Currently canvas shapes are properly tagged. :)

f1ames

Looks good, just last touch needed and it will be ready :)

I can see that Safari doesn't expose RTF in the clipboard, could you create an issue for future reference to analyze this in more depth (upstream issue marked as resolved, however doesn't seem to work in Safari 11 or Preview 43).

f1ames · 2017-11-07T15:53:33Z

tests/plugins/pastefromwordimage/manual/pasteimagewithlink.md

+### Perform above steps for all files:
+  1. [Link_image.docx](../generated/_fixtures/Link_image/Link_image.docx)
+
+**Expected:** Images is visible and pasted only once. Image is wrapped with an anchor link.


Unfortunately, this is not true for Safari. While the implementation works fine, the image is not visible (not sure why it doesn't have a proper size) so this description is misleading:

It appear that image plugin was missing in those test. Because PFW Image register img tag as allowed, the img appear but had stripped away width and height.
Test is now also ignored to not confuse user with missing picture after paste (Safari doesn't allow for reading RTF clipboard).

f1ames · 2017-11-07T15:56:56Z

tests/plugins/pastefromword/manual/shapesaretagged.md

+
+**Expected:**
+  * For browsers with good `text/html` clipboard processing (Firefox, Chrome, Safari): First image tag had additional attribute `data-cke-is-shape="true"`. Second image is pasted without this attribute.
+  * For other browsers (IE, Mobile): There won't be `data-cke-is-shape="true"`. Browser will behave in an old way: display blank place or embed image by it's own.


I still suggest to add Edge here:

For other browsers (Edge, IE, Mobile)...

When #468 and #662 will be delivered, we will be able to test if everything works together and update this test.

I forgot that Edge exists :D.

f1ames · 2017-11-07T16:07:23Z

plugins/pastefromword/filter/default.js

+
+						// Canvas leaves empty `v:shape`, which should not be converted into img tag.
+						// These empty `v:shape` contains 2 attributes which helps distinguish it.
+						child = element.getFirst( 'v:imagedata' );


This can be assigned on initialization, 5 lines above. And then the comment will fit better, right after if which it describes.

I prefer not to move comment after if, as it might imply relation to shapeTagging method. All other if description in entire file seems to be described just before it.

@msamsel I fully agree, my mistake here, what I meant was

And then the comment will fit better, right before if which it describes

By merging var initialization you achieved just what I was thinking about so it is ok 👍

msamsel · 2017-11-08T10:45:58Z

I've added an issue on our side to remember about checking Safari in future: #1134.
I've replaced ignore condition:
from:

!CKEDITOR.plugins.clipboard.isCustomDataTypesSupported || CKEDITOR.env.iOS

to:

!CKEDITOR.plugins.clipboard.isCustomDataTypesSupported || CKEDITOR.env.safari

iOS is tested inside isCustomDataTypesSupported, so there is no need to double check it here.

…ignore condition. Little code improvements.

f1ames

LGTM 👍

msamsel force-pushed the t/662 branch from 734f957 to d244233 Compare October 25, 2017 12:20

Mateusz Samsel added 8 commits October 25, 2017 14:25

Update PFW to tag shape img with additional attribute. Change method …

c45e88c

…in PFW Image to ignore shape images.

Fix PFW test and update fixtures, because now shapes are not returned.

d4e5d78

Adding new unit test for PFW. Check if proper attribute is added to i…

18d8a22

…mages on PFW output.

Fix PFW filter to not duplicate Linked Images.

8be7307

Update test names for PFW shapes.

5cc1028

Manual test for tagging shapes in PFW output

3a0b389

Add new unit test for group shapes and linked images.

5e91a6c

Manual test for pasting images with link.

534af88

msamsel force-pushed the t/662-3119 branch from 51f8493 to 534af88 Compare October 25, 2017 12:30

f1ames self-requested a review October 31, 2017 12:29

f1ames suggested changes Oct 31, 2017

View reviewed changes

Mateusz Samsel added 3 commits November 3, 2017 13:30

Update autogeenrated test cases for shapes, extend with osx case, add…

df5775e

… canvas example.

Other test related to PFW fixes

aef44f1

Other review fixes: code clean up, update pfw filter, etc.

aee8320

f1ames self-requested a review November 6, 2017 10:28

f1ames suggested changes Nov 6, 2017

View reviewed changes

Mateusz Samsel added 3 commits November 7, 2017 09:57

Review fixes. Manual test improvements, variable rename.

98976b6

Review fixes: rename test cases.

8eaebec

Fix canvas recognition by PFW.

344c648

msamsel requested a review from f1ames November 7, 2017 12:29

Rewording.

a088ed4

f1ames suggested changes Nov 7, 2017

View reviewed changes

Fix minor stuff: tests case description and included plugins. Update …

9bd5337

…ignore condition. Little code improvements.

msamsel requested a review from f1ames November 8, 2017 10:51

f1ames approved these changes Nov 8, 2017

View reviewed changes

f1ames merged commit e9c2331 into t/662 Nov 8, 2017

f1ames deleted the t/662-3119 branch November 8, 2017 12:40

msamsel restored the t/662-3119 branch December 15, 2017 11:54

msamsel deleted the t/662-3119 branch December 15, 2017 12:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug fixing for shape processing and inserting linked images #1088

Bug fixing for shape processing and inserting linked images #1088

msamsel commented Oct 25, 2017 •

edited

Loading

f1ames left a comment

f1ames Oct 30, 2017

f1ames Oct 30, 2017

f1ames Oct 30, 2017

f1ames Oct 31, 2017

f1ames Oct 31, 2017

f1ames Oct 31, 2017

f1ames Oct 31, 2017

f1ames Oct 31, 2017

f1ames Oct 31, 2017

msamsel Nov 2, 2017 •

edited

Loading

f1ames Nov 2, 2017

msamsel Nov 2, 2017

f1ames Oct 31, 2017

msamsel commented Nov 3, 2017

f1ames left a comment

f1ames Nov 6, 2017

f1ames Nov 6, 2017

f1ames Nov 6, 2017

msamsel commented Nov 7, 2017 •

edited

Loading

f1ames left a comment

f1ames Nov 7, 2017

msamsel Nov 8, 2017 •

edited

Loading

f1ames Nov 7, 2017

msamsel Nov 8, 2017

f1ames Nov 7, 2017

msamsel Nov 8, 2017 •

edited

Loading

f1ames Nov 8, 2017

msamsel commented Nov 8, 2017

f1ames left a comment

		@@ -0,0 +1,10 @@
		@bender-tags: bug, pastefromword, 4.8.0, 662

		@@ -0,0 +1 @@
		<p style="margin-left:0in; margin-right:0in"></p><table style="width:100%"><tbody><tr><td><div><p>hello world</p></div></td></tr></tbody></table><p><br /></p><p><br /></p><table align="left"><tbody><tr><td><br /></td><td><br /></td><td><br /></td></tr><tr><td><br /></td><td colspan="2" valign="top"><img data-cke-is-shape="true" src="file:///c:\users\foobar\appdata\local\temp\msohtmlclip1\01\clip_image001.png" style="height:106px; width:348px" /></td></tr><tr><td><br /></td></tr><tr><td><br /></td><td valign="top"><img data-cke-is-shape="true" src="file:///c:\users\foobar\appdata\local\temp\msohtmlclip1\01\clip_image002.png" style="height:106px; width:120px" /></td></tr></tbody></table><p style="margin-left:0in; margin-right:0in"><br /></p><p style="margin-left:0in; margin-right:0in"><br /></p><p style="margin-left:0in; margin-right:0in"><br /></p><p style="margin-left:0in; margin-right:0in"><br /></p><p style="margin-left:0in; margin-right:0in"><br /></p><p style="margin-left:0in; margin-right:0in"><br /></p><p style="margin-left:0in; margin-right:0in"><br /></p><p><br /></p>

Bug fixing for shape processing and inserting linked images #1088

Bug fixing for shape processing and inserting linked images #1088

Conversation

msamsel commented Oct 25, 2017 • edited Loading

What is the purpose of this pull request?

This PR contains

What changes did you make?

f1ames left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msamsel Nov 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msamsel commented Nov 3, 2017

f1ames left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msamsel commented Nov 7, 2017 • edited Loading

f1ames left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msamsel Nov 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msamsel Nov 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msamsel commented Nov 8, 2017

f1ames left a comment

Choose a reason for hiding this comment

msamsel commented Oct 25, 2017 •

edited

Loading

msamsel Nov 2, 2017 •

edited

Loading

msamsel commented Nov 7, 2017 •

edited

Loading

msamsel Nov 8, 2017 •

edited

Loading

msamsel Nov 8, 2017 •

edited

Loading