Bug fixes for PFW Image with rotation, reflection, alternative text #1158
Conversation
| @@ -13,6 +13,9 @@ | |||
| 2. [Online_and_offline_image.docx](../generated/_fixtures/Online_and_offline_image/Online_and_offline_image.docx) | |||
| 3. [Shapes_and_online_and_offline_image.docx](../generated/_fixtures/Shapes_and_online_and_offline_image/Shapes_and_online_and_offline_image.docx) | |||
| 4. [Wrapped_image.docx](../generated/_fixtures/Wrapped_image/Wrapped_image.docx) | |||
| 5. [Image_alternative_text.docx](../generated/_fixtures/Image_alternative_text/Image_alternative_text.docx) | |||
| 6. [Image_reflection.docx](../generated/_fixtures/Image_reflection/Image_reflection.docx) | |||
| 7. [Image_rotation.docx](../generated/_fixtures/Image_rotation/Image_rotation.docx) | |||
There was a problem hiding this comment.
When pasting rotated image it is pasted in its original form (not rotated). I'm not sure if it is a proper behaviour or not (as for previous .docx images are pasted exactly as they appear in the document). If this is an intended behaviour it should be explain that rotated images are pasted, but without rotation.
| @@ -13,6 +13,9 @@ | |||
| 2. [Online_and_offline_image.docx](../generated/_fixtures/Online_and_offline_image/Online_and_offline_image.docx) | |||
| 3. [Shapes_and_online_and_offline_image.docx](../generated/_fixtures/Shapes_and_online_and_offline_image/Shapes_and_online_and_offline_image.docx) | |||
| 4. [Wrapped_image.docx](../generated/_fixtures/Wrapped_image/Wrapped_image.docx) | |||
| 5. [Image_alternative_text.docx](../generated/_fixtures/Image_alternative_text/Image_alternative_text.docx) | |||
| 6. [Image_reflection.docx](../generated/_fixtures/Image_reflection/Image_reflection.docx) | |||
| 7. [Image_rotation.docx](../generated/_fixtures/Image_rotation/Image_rotation.docx) | |||
|
|
|||
| **Expected:** Images are pasted to the editor.<br> | |||
| Note: Keep in mind that shapes (like last object in `Shapes_and_online_and_offline_image.docx`) will not be pasted. | |||
There was a problem hiding this comment.
To be honest I cannot recall what was the status of handling shapes. Now I see that for shapes (e.g. in Shapes_and_online_and_offline_image.docx), img element with invalid path (pointing to some local/temporary resources) is inserted. Shouldn't it just be skipped? Same happens for canvas elements.
There was a problem hiding this comment.
I know that this PR is about images, but as a whole implementation of v:shape filter changed it influences shapes also.
There was a problem hiding this comment.
It's a good point. Honestly I don't know. I remain them untouched. I'll talk with @mlewand what to do with it.
There was a problem hiding this comment.
Ok so img which earlier appear after pasting shapes are now removed. More details how it works is below in main line comment.
… to have no img tags belong to shapes in expected file. Little description update for manual test.
|
Ok so shapes, or more specific img tags belong to shapes, are now removed from PFW output. I change line which tag them with |
There was a problem hiding this comment.
Looks good 👍 Just one manual tests needs updating after shapes handling change was introduced - /tests/plugins/pastefromword/manual/shapesaretagged, because current behaviour doesn't match the description.
I found one edge case with canvases: if you copy image together with canvas (empty or containing other shapes), the image is pasted properly and canvas removed. However, if canvas contains other image, the canvas element itself is removed, but image (the one outside canvas) is pasted without being converted to base64:
I would ask you to extract it to separate issue as it is not directly related with this fix.
|
Extracted issue here: #1183 |
| **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 (Edge, 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. | ||
| **Expected:** Single image is pasted to the editor. Shape present in word document is removed during paste process. |
There was a problem hiding this comment.
You simplified it too much IMHO. It works as described in Chrome, FF, Safari. On IE11 both shape and image are pasted and on older IEs again only second image.
Additionally AFAIR or Safari and IEs < 11, the image is pasted, but it has invalid src, so basically the broken image is pasted (hard to tell if it is valid behaviour based on description).
TBH, I haven't thought about this earlier, but this test (shapesaretagged) was created to check if shapes are tagged with special attribute. Now shapes are removed so there is no purpose in keeping it and it could be simply removed, WDYT @msamsel? The case with removing shapes is already covered in other manual tests.
Sorry for the confusion, but I think it will be the best way, no sense in duplicating similar test.
What is the purpose of this pull request?
Bug fix
Does your PR contain necessary tests?
yes
This PR contains
What changes did you make?
v:shapeare process by PFW filter.nullin case of shape detection. Currently shapes are filtered out earlier, so this part is not required any longer (this cause one of the errors).related to: #662