Skip to content
This repository has been archived by the owner on Jun 26, 2020. It is now read-only.

Support for pasting images #28

Merged
merged 29 commits into from
Nov 27, 2018
Merged

Support for pasting images #28

merged 29 commits into from
Nov 27, 2018

Conversation

f1ames
Copy link
Contributor

@f1ames f1ames commented Nov 5, 2018

Suggested merge commit message (convention)

Feature: Support for pasting content with images from Word. Closes ckeditor/ckeditor5#2515. Closes ckeditor/ckeditor5#2529. Closes ckeditor/ckeditor5#2534.


Additional information

This PR implements two mechanisms - one for handling images in Chrome, Firefox and Edge and one for handling images in Safari.

Chrome, Firefox, Edge

When content is pasted from Word, images are represented wit local sources file://. In the browser clipboard apart from text/html data, there is text/rtf data provided which contains entire images encoded as hex strings. These strings are extracted, converted to base64 and used as new image elements sources e.g <img src="data:image/jpeg;base64,..." >.

This is a synchronous process so all content manipulation takes place on a view instance so data inserted on paste is in its final form (no transformations are applied after data is rendered in the editor).

Safari

When content is pasted from Word, images are represented with local blob resource blob://. Such resources are then fetched (via XHR), converted to base64 representation and then used as new image elements sources e.g <img src="data:image/jpeg;base64,..." >.

As fetching blobs is an asynchronous process, first the data with blobs is inserted into the editor (images are already visible) and then when each blob is fetched and converted, images sources are replaced.

Note: As this PR was built on top of t/12 (as it provides whole infrastructure for automatic testing on Safari), it can be merged only after #16 (t/12 PR) is merged.

@coveralls
Copy link

coveralls commented Nov 5, 2018

Pull Request Test Coverage Report for Build 148

  • 79 of 79 (100.0%) changed or added relevant lines in 4 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%

Totals Coverage Status
Change from base Build 141: 0.0%
Covered Lines: 163
Relevant Lines: 163

💛 - Coveralls

@f1ames
Copy link
Contributor Author

f1ames commented Nov 6, 2018

Ready for review 🎉

@Mgsy Mgsy self-requested a review November 16, 2018 07:59
Copy link
Member

@Mgsy Mgsy left a comment

Choose a reason for hiding this comment

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

Everything works fine 👌

@pjasiun
Copy link

pjasiun commented Nov 19, 2018

Make sure that, if the image upload adapter is provided, no base64 data will be inserted in the model. In would break collaboration. Also, ensure that the upload progress displays properly on other collaboration clients (or report a follow-up about it if there is any problem with it).

@f1ames
Copy link
Contributor Author

f1ames commented Nov 20, 2018

I have removed logic responsible for blob extraction (moving it to UploadImage plugin). The snapshot of the branch in the previous state (with blob handling) is available under t/21-b.

@f1ames f1ames requested a review from Reinmar November 20, 2018 08:29
@f1ames
Copy link
Contributor Author

f1ames commented Nov 20, 2018

Just to clarify the previous comment. Moving blob extraction to UploadImage plugin means that without the UploadImage plugin, images pasted from Word in Safari will be represented as blobs and will not be converted to base64. We have agreed with @Reinmar that this is acceptable solution for now, but may also think about improvements in the future if needed.

@f1ames
Copy link
Contributor Author

f1ames commented Nov 23, 2018

This PR contains also some additional space normalization which apparently solves #39 and #40, I have added to Closes commit message.

@Reinmar
Copy link
Member

Reinmar commented Nov 23, 2018

Haha :D While checking the code I thought... shouldn't it fix the issue that @f1ames just reported? :D

src/filters/space.js Outdated Show resolved Hide resolved
@f1ames
Copy link
Contributor Author

f1ames commented Nov 23, 2018

image

😄 👍

@Reinmar
Copy link
Member

Reinmar commented Nov 23, 2018

I've done some testing and it works as expected. I'm going to have a quick look at the code still (especially in ckeditor/ckeditor5-image#248) but we should be able to merge this very soon.

@Reinmar Reinmar self-assigned this Nov 26, 2018
src/filters/image.js Outdated Show resolved Hide resolved
src/filters/image.js Outdated Show resolved Hide resolved
src/filters/image.js Outdated Show resolved Hide resolved
src/filters/utils.js Outdated Show resolved Hide resolved
Copy link
Member

@Reinmar Reinmar left a comment

Choose a reason for hiding this comment

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

Comments.

@f1ames f1ames requested a review from Reinmar November 27, 2018 12:24
@Reinmar Reinmar merged commit 87f7e7c into master Nov 27, 2018
@Reinmar Reinmar deleted the t/21 branch November 27, 2018 19:44
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
5 participants