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

Support for uploading images with base64 source #248

Merged
merged 12 commits into from
Nov 27, 2018
Merged

Support for uploading images with base64 source #248

merged 12 commits into from
Nov 27, 2018

Conversation

f1ames
Copy link
Contributor

@f1ames f1ames commented Nov 14, 2018

Suggested merge commit message (convention)

Feature: Support for uploading images with base64 source. Closes ckeditor/ckeditor5#5172. Closes ckeditor/ckeditor5#2524.


Additional information

See ckeditor/ckeditor5#5172 (and https://github.com/ckeditor/ckeditor5-paste-from-office/issues/24#issuecomment-436186985 which explains the approach a little). ATM Edge browser is not supported, see ckeditor/ckeditor5#5173.

@coveralls
Copy link

coveralls commented Nov 14, 2018

Coverage Status

Coverage remained the same at 100.0% when pulling cd2a8fd on t/246 into cc1e7a3 on master.

@f1ames
Copy link
Contributor Author

f1ames commented Nov 14, 2018

Ready for review.

@f1ames
Copy link
Contributor Author

f1ames commented Nov 21, 2018

I have changed the way upload integrates with base64/blob images. Now imageuploadediting listens to inputTransformation event and finds all images with base64/blob source. The images sources are fetched as File instance (except for Edge, see #247) and then file is passed to a file uploader (from this point it works the same as regular file uploading flow as it reuses its logic too).

Important changes:

  1. As inputTransforamtion is emitted by the Clipboard plugin I have added it as a package dependency (not sure if it is a breaking change for the package?) Alternatively, if we don't want to add it as a dep, the code could check if Clipboard plugin is present and perform image processing only if plugin is available.
  2. The base64 source could be transformed to File in a synchronous manner, however blob cannot. To not make the code too complicated by using different methods for fetching base64 and blob there is the same async method (using Fetch API) used for both. It also guarantees non-blocking fetching (so UI won't freeze).
  3. The above (async fetching) requires inputTransformation to be stopped and re-fired after images are fetched.
  4. I'm quite sad about the Edge browser, especially that Blob's can be also used for upload same as File objects. However, as mentioned in #247, when Blob is send, the filename used for the file during upload is always blob which may not work with some upload adapters (for example CKFinder doesn't play well with it - it seems it does some type checking based on filename). However, if we decide that we can move the responsibility of proper file naming to adapter or server-side uploading we can go with blobs on Edge. ATM images are simply removed as they cannot be uploaded (no File instance available) which is not the best solution (remember we can't keep them with base64 source to not break collaboration).

Slightly different approach for fetching images:

As mentioned in points 2 and 3 above the flow for fetching base64/blob images is:

  1. Listen to inputTransformation event.
  2. Find images with local sources in clipboard data.
  3. If there are any, stop inputTransformation event.
  4. Fetch images (async) and start upload.
  5. Modify clipboard data based on fetch result.
  6. Fire inputTransformation event.

I was wondering if the flow could be altered as:

  1. Listen to inputTransformation event.
  2. Find images with local sources in clipboard data.
  3. Modify images in clipboard data so base64/blob is not propagated to model.
  4. Fetch images (async) and start upload.

So in the second variant, the inputTransformation event is not paused/delayed by fetching images. It means the pasted content will appear faster in the editor. However, as images are not ready the content will appear without image previews. Preview images will appear when fetched which will result in resizing/moving some parts of the content. This is just an alternative which I was thinking of, not sure if it brings any real improvement, but wanted to mention it.

@f1ames f1ames requested a review from Reinmar November 21, 2018 11:26
@Reinmar Reinmar self-assigned this Nov 26, 2018
package.json Outdated
@@ -10,6 +10,7 @@
"ckeditor5-plugin"
],
"dependencies": {
"@ckeditor/ckeditor5-clipboard": "^10.0.3",
Copy link
Member

Choose a reason for hiding this comment

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

That's not necessary. You're not using it anyway. You're only using plugins.get( 'Clipboard' ) which can be satisfied by other implementations of this feature (not only ckeditor5-clipboard).

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 see, wouldn't

this.listenTo( editor.plugins.get( 'Clipboard' ), ... );

fail if there will not be any Clipboard feature in the CKEditor build?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

☝️ cc @Reinmar

Copy link
Member

Choose a reason for hiding this comment

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

Yes, it would fail. But we do the same thing with other feature-feature relations. This simplifies recomposing the editor with e.g. a bit different implementation of one of those features.

We use Plugin#requires() for feature-util and gluefeature-subfeature relations.

src/imageupload/utils.js Outdated Show resolved Hide resolved
src/imageupload/utils.js Outdated Show resolved Hide resolved
src/imageupload/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.

We can simplify a couple of things here.

@f1ames f1ames requested a review from Reinmar November 27, 2018 12:25
@Reinmar Reinmar merged commit 89ab27e into master Nov 27, 2018
@Reinmar Reinmar deleted the t/246 branch November 27, 2018 19:43
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
3 participants