Inserted (dropped, pasted) images uploading support

[Piotrek Koszuliński]

Part of #11437.

Spec (status: under construction)

Main obstacle: uploading is asynchronous when #paste event is synchronous.

Worth mentioning: since we have ACF, we may finally and completely switch to reading pasted data from paste event. The problem was that on Webkit&Blink it contains awful HTML markup with millions of inline styles. This way we'll be able to get rid of pastebin on some browsers. It may be a good opportunity, because most likely we'll have to use data carried in native paste event anyway.

[Piotrek Koszuliński]

See ​http://dev.ckeditor.com/ticket/11437#comment:14.

[Piotr Jasiun]

Problems:

There are some problems related to image upload [[Reinmar: files upload in general]].

In general (e.g. drop file from the file system) we need to handle 2 asynchronous operations:

• get file from disc,
• upload files.

In some cases (e.g. paste from Word, paste from Paint) image is already given as Base64 data in the source of an img element so we need to handle only upload.

During this asynchronous operation (e.g. file is uploading) user can modify the content of the editor. Let's focus on 4 critical cases:

1. undo/redo,
2. getData/switch to the source mode,
3. image modification/dialogs,
4. drag, drop, cut, copy and paste uploading image.

Expected behavior:

1. Undo/Redo should work fine in every case. Scenario:
   • user pastes image which should be uploaded,
   • user modifies some content, creates an undo snapshot, before upload is done,
   • upload is done,
   • user presses undo.
   Content should be restored, but the image that was being uploaded should be replaced with the uploaded image (url instead of data in source).

2. If user calls getData or switches to the source mode before image is fully uploaded, then the image being uploaded should be removed from the HTML content. We should not show Base64 data to user.

3. We can not show Base64 data to the end user in the dialog too. Firstly, it would be a very bad UX, these are definitely data that end user does not care about. Secondly, putting such a big string in the dialog we can hang or break a browser. src field in the dialog or whole dialog should be disabled.

4. Drag, drop, copy, cut and paste should not break an editor or caused second upload of the same image. If user cut or delete image upload should be canceled. [[Reinmar: when user duplicates an image (e.g. copy+paste few times) every image's src should be updated]].

Solution:

Image loading:

For the first asynchronous operation best solution seems to be delay paste event until we get files from hard disc. I measured it with ~1 MB image, SSD and HDD and it takes no more than 20ms for native Ubuntu with Chrome 36 and 100-250ms for virtual machine (Windows 7 with Chrome, Firefox or IE). Keep in mind that:

• the only case when we get files for the hard disc is when dataVaule is empty so there should be no conflicts with other plugins,
• we are talking only about jpg/png images here, these are relatively small files.

By 'delay' I mean we cancel editor.paste event and fire it when files are loaded. Thanks to this:

• we can handle dropped image and pasted images the same way,
• we do not have a problem with mocking image with no data and dimensions.

Of course for file upload we can not use such delay, but for file upload we have none of these problems.

Image uploading:

For the image upload I'd prefer to use a widget. Thanks to the widget system:

• problem 2 can be fixed with 1 line of code (downcast method will return an empty text node),
• problem 3 can be fixed out of the box,
• problem 4 will be partially fixed, because uploading image will be in non-contenteditable so it won't be uploaded for the second time [[Reinmar: Not true. Since on copy we (will) take downcasted widget the copy will... do nothing. It's not that bad, but it means that downcasting on copy/drag should behave differently and we don't yet have a way to achieve this. The plan was to use `dataProcessor.toDataFormat()`, however, it'd not be semantically correct, because data !== HTML, so we should not pack data into clipboard. We need a third processing method in dataProcessor. This reminds me about #10204 and I'm willing to introduce it.]].
• we can encapsulate code related to image uploading: paste event just mark images as "to upload" and widget.upcast method will handle it.

In general uploading image is not just an image, it should be treated differently as a special part of the document so widget seems to be logically a good solution.

Also, with the widgets and with the expected behavior described above we can easily show additional information like upload progress. [[Reinmar: We still have to remember about undo manager, so any solution that involves changes in DOM should be avoided. Though, canvas may be a rescue.]]

Upload manager:

To handle undo/redo we need an upload manager which will keep id's and progress of uploads so when user executes undo command manager will update paths.

Conflicts:

There might be some images in the editor which should not be uploaded and have data in the src. We may marked image as "to be upload" on paste, but we can not prevent user to copy and paste part of the content of the editor. All images in non-contenteditable areas should not be uploaded. Rest of them can use some 'not to upload' flag.

If imageUpload widget will handle uploading then we need also a flag to prevent image2 not to change img into a widget. It could skip all images with data in the src or there could be a flag special-image to mark images that should not be wrapped by image2 widget.

UploadURL:

The one more problem with upload is that now we have at least 2 separate plugins which handle uploading: fileBrowserPlugin and uploadImagePlugin, and:

• fileBrowserPlugin has to work without uploadImagePlugin,
• uploadImagePlugin has to work without fileBrowserPlugin,
• fileBrowserPlugin define uploadURL and imageUploadURL,
• uploadImagePlugin has to work out of the box if user already use fileBrowserPlugin with CKFinder,
• fileBrowserPlugin.uploadURL returns JavaScript to be executed as an response and uploadImagePlugin needs text/JSON response.

To provide the best solution we (me and Wiktor) decided that:

• new config options needs to be provided; they should be placed in the core since there is no single plugin which can handle upload. They will be called: core.uploadURL and core.imageUploadURL, these URLs will return text/JSON responses.
• fileBrowserPlugin will keep use its own config option; it may use core options when it will use new API.
• uploadImagePlugin will use in the order:
  1. core.imageUploadURL,
  2. core.uploadURL,
  3. fileBrowserPlugin.imageUploadURL with response_type=txt added,
  4. fileBrowserPlugin.uploadURL with response_type=txt added.

[Piotrek Koszuliński]

I added some my comments directly in the comment:6. I was sceptical regarding using widgets, because that would create additional dependency - a pretty big one. But after more thinking and your great analysis, I don't see other clean solution. Widgets were designed to be used in such cases and it's awesome that they really help so much. Though, I would like Fred's opinion first, so we have a green light.

My remaining doubts:

• Delaying editor#paste event so the files data are available. I don't like this, because delays are in general extremely unsafe. Even the 0ms timeout we used so pastebin could grab data generates issues, because fast subsequent pastes sometimes were missing pastebin. I don't also see a problem in non delaying the paste event. Pasted images' sources can be updated live in editor after they were inserted.
• Downcasting on copy/cut/drag - see my inline comment for comment:6 mentioning #10204. This is a topic for a separate ticket of course, but the mentioned case affects the way how #10204 needs to be solved. We can discuss this ticket a little bit too.

[Piotr Jasiun]

Unfortunately on Firefox pasted image is available only on native paste (paste bin) and the ClipboardData object is empty so we need to remove Firefox from htmlAlwaysInDataTransfer and keep handle paste bin on Firefox.

[Piotrek Koszuliński]

Did you check it on other OSes than Linux? There was that bug on one of the browsers on Linux only that dropped image's data was empty. It wasn't precisely this, but maybe it had a wider effect.

[Piotr Jasiun]

Yes, I checked on both. The same behavior.

[Piotr Jasiun]

The ticket is ready to review.

What have been done and should works:

• file tools and generic upload widgets with documentation,
• upload image plugin,
• all problem related to asynchronous upload, mentioned above, should be solved,
• tests.

What was moved to the separate tickets:

• Cross domain file uploads (CORS) support,
• dragover - show user that you can not drop on toolbar/ui
• show progress - gray out image,
• responsive width and scale image,
• smart position - image aligned left, block,
• block alignment icons during upload,
• notifications,
• file upload plugin,
• bug: drop to the empty document on Chrome does not work properly.

Changes in ​t/11461.

[Piotrek Koszuliński]

Great job! The architecture looks really nice and I don't think we need to change anything except some cosmetics.

I rebased the branch and pushed many commits to ​branch:t/11461. I advice you to check them for both: good stuff and my possible mistakes.

Here's the review summary:

filetools/plugin.js:

• I don't like changing the FileLoader.abort dynamically.
• ucType -> tools.capitalize.
• it should be CKEDITOR.fileTools.
• it should be CKEDITOR.fileTools.uploadsRepository (but locally the function declaration name should still be UploadsRepository).
• it should be CKEDITOR.fileTools.fileLoader.
• Is fileLoader an appropriate class name at all? It handles loading and uploading, so the wider fileUploader name seems to fit better.
• The fileLoader class misses a step by step usage example pretty badly. It is unclear how it can be used. The reason may also be that all its methods are public, while I believe that only upload, loadAndUpload and load are really useful. If I'm right, then please mark the rest as private. Still - a typical code sample would be great.

uploadwidget/plugin.js:

• What is it for? editor.filter.allow( '*[!data-widget,!data-cke-upload-id]' );
• Hard to say what you tried to say (perhaps: "paste event listener which handles pasted and dropped files"): ​https://github.com/cksource/ckeditor-dev/blob/978a9a514/plugins/uploadwidget/plugin.js#L23
• On pasted what? ​https://github.com/cksource/ckeditor-dev/blob/978a9a514/plugins/uploadwidget/plugin.js#L29
• No idea again what does it mean and when does it happen? ​https://github.com/cksource/ckeditor-dev/blob/978a9a514/plugins/uploadwidget/plugin.js#L53
• What? ​https://github.com/cksource/ckeditor-dev/blob/978a9a514/plugins/uploadwidget/plugin.js#L61
• I have no idea what should go here after reading the docs: ​https://github.com/cksource/ckeditor-dev/blob/978a9a514/plugins/uploadwidget/plugin.js#L66
• Couldn't you use fileToElement there? ​https://github.com/cksource/ckeditor-dev/blob/978a9a514/plugins/uploadwidget/plugin.js#L70
• All the names like onload, load, upload, FileLoader should be links unless you're talking about the thing being currently described.
• wasSelected = this.wrapper.hasClass( 'cke_widget_selected' ) - check editor.widgets.focused.
• If false default behavior will be canceled. - and what's the default behavior? In some cases you described it and in some not.

uploadwidget/plugin.js:

• Wouldn't 1x1 GIF be smaller than 1x1 PNG (I mean the loadingImage property).
• Let's use htmlParser.fragment instead of temp div in the editor#paste listener. HTML parser won't be prone to XSS issues. It means that fileTools.markElement will need to work on htmlParser.element too.
• inEditableBlock() - the name of the function does not fit what it does (where's that block check?). Also, why not node.isReadOnly()?

Tests:

• You shouldn't use assert.isInnerHtmlMatching with editor.getData(). That assert method is for checking innerHTML.

General notes:

• We do not document private functions using /***/ comments, but simple //.
• In the FileLoader class docs you mention status in dozen places but you never link to it. Cross-links are very important part of any documentation. This problem repeats in many places. Be precise and link.
• All onxxx callbacks should be changed to onXxx. I know that native ones are lower-case, so in the FileLoader this convention makes some sense, but unfortunately then these names spread to other classes (e.g. the upload widget definition) and there, without that clear connection to the native API, they look bad.
• You overuse quoted properties names in object literals. There are plenty of cases in tests where you shouldn't use quotes because you don't need to.

Reds:

• ​http://tests.ckeditor.dev:1030/tests/plugins/uploadwidget/uploadwidget one red tests, most likely after ​git:978a9a514c. That's exactly what we call a silent error and that's exactly why we should never add unnecessary silent checks to functions.

[Piotrek Koszuliński]

Three small proposals:

• attachUploadListener -> attachRequestListeners (because we have sendRequest)
• changeStatusAndFire -> changeStatus (because firing a proper event is the obvious part of changing the status)
• The update() method needs a explanation when it should be called. This information should clarify if it should be called when reimplementing some methods of the FileLoader. Of course this may be explained in the update event documentation, but now it points (without a link!) back to the update() method without clearly answering the question when the event is fired.

PS. All FileLoader's events do not need the @member tag.

[Piotr Jasiun]

I also considered firing events from sendRequest and handleResponse, so the behavior of these methods can be changed without overwriting them and by multiple plugins, but I believe that at this point in would be unnecessary complication.

[Piotr Jasiun]

All changes have been done with additional API improvements and docs review. Branch is rebased on the newest major and ready to review. Changes in ​t/11461.

[Piotrek Koszuliński]

Closer :D

• ​https://github.com/cksource/ckeditor-dev/commit/589e3c8602f2dd7e09c01761d59bfb61ded3abb9
• ​https://github.com/cksource/ckeditor-dev/commit/d5f7e99d0913edd89cd666a6f3c0a6efa024e292 - regexp please - will be 3x shorter. evt.data.dataValue.match( /<img|data:/i )
• ​https://github.com/cksource/ckeditor-dev/commit/028291c7cbee8abece862acae25a0a87054dccc6
• loadingType -> loadMethod?
• UploadsRepository#created -> #instanceCreated - there are least two #instanceCreated events so far.
• Please verify if some properties of the FileLoader class don't lack the @readonly tag. I can't tell this by myself.
• ​https://github.com/cksource/ckeditor-dev/commit/bffdb775ae7ea660b1129a2bf74fca39ab2d8663
• ​https://github.com/cksource/ckeditor-dev/commit/c42c25d9a5b0ff0845320afa5571ab81e33d6bed - this needs clarification and test. How is it possible that getSelectedElement returns something else than an element?
• ​http://security.stackexchange.com/questions/50970/is-it-safe-to-use-createhtmldocument-to-sanitize-html and ​http://blog.kotowicz.net/2011/10/sad-state-of-dom-security-or-how-we-all.html and I lost confidence about that document creating.
• ​https://github.com/cksource/ckeditor-dev/commit/64fb022eef3fb97cb4201dd75237a748429d519c
• ​https://github.com/cksource/ckeditor-dev/commit/375a1b3d420a0 - why?

I've checked diffs and run tests on Chrome only.

[Piotrek Koszuliński]

PS. The branch was rebased on latest major.

[Piotrek Koszuliński]

PS. I pushed one commit to the branch.

[Piotr Jasiun]

Replying to Reinmar:

• ​https://github.com/cksource/ckeditor-dev/commit/589e3c8602f2dd7e09c01761d59bfb61ded3abb9
• ​https://github.com/cksource/ckeditor-dev/commit/d5f7e99d0913edd89cd666a6f3c0a6efa024e292 - regexp please - will be 3x shorter. evt.data.dataValue.match( /<img|data:/i )
• ​https://github.com/cksource/ckeditor-dev/commit/028291c7cbee8abece862acae25a0a87054dccc6
• loadingType -> loadMethod?
• UploadsRepository#created -> #instanceCreated - there are least two #instanceCreated events so far.
• Please verify if some properties of the FileLoader class don't lack the @readonly tag. I can't tell this by myself.
• ​https://github.com/cksource/ckeditor-dev/commit/bffdb775ae7ea660b1129a2bf74fca39ab2d8663
• ​http://security.stackexchange.com/questions/50970/is-it-safe-to-use-createhtmldocument-to-sanitize-html and ​http://blog.kotowicz.net/2011/10/sad-state-of-dom-security-or-how-we-all.html and I lost confidence about that document creating. However, ​https://github.com/cure53/DOMPurify/blob/master/purify.js#L185 is using this technique, so maybe it's not that bad still.
• ​https://github.com/cksource/ckeditor-dev/commit/64fb022eef3fb97cb4201dd75237a748429d519c

I've checked diffs and run tests on Chrome only.

Done.

Replying to Reinmar:

• ​https://github.com/cksource/ckeditor-dev/commit/c42c25d9a5b0ff0845320afa5571ab81e33d6bed - this needs clarification and test. How is it possible that getSelectedElement returns something else than an element?

The situation is pretty complex. When undo snapshot is restored the content and selection is set. During setting content, upload widget can be replaced by another element or text node (if upload is done or aborted). Selection is set after that so it can meet different element to select. We decided to put the check into selectBookmarks method.

Replying to Reinmar:

• ​https://github.com/cksource/ckeditor-dev/commit/375a1b3d420a0 - why?

Because by default file loader should works with CKFinder and CKFinder do not encode URL value so the response handler should do it.

[Piotrek Koszuliński]

There are 181 commits in the branch... It's time to close it :D.

[Piotrek Koszuliński]

R+ and merged to major with ​git:51f9bf0 :).

[Piotrek Koszuliński]

We'll create manual tests for these features at later stage, so to make it clear - the API is ok, the automated tests pass, but I haven't seen the features working.

[Piotr Jasiun]

Manual tests ticket: #12805.

[Wim Leers]

AWESOME! Can't wait to try this! :)