Widget upload feature for Easy Image #1483

mlewand · 2018-01-22T14:48:48Z

What is the purpose of this pull request?

New feature

Does your PR contain necessary tests?

All patches which change the editor code must include tests. You can always read more
on PR testing,
how to set the testing environment and
how to create tests
in the official CKEditor documentation.

This PR contains

Unit tests
Manual tests

What changes did you make?

This pull request adds a widget upload feature.

It can be hooked to any widget.

Allowed accepted file types can specified using supportedTypes property in widget definition (similarly as it is in uploadwidget).

Also allows for easy progress bar customization by progressReporterType property in widget definition.

It also fixes several other issues:

Misplaced progress bar (tp3350).
Misplaced balloon toolbar after initial image load.

I have targetted this PR into a upload widget feature branch, as it still needs undo integration before being merged into EI feature.

…ept with some logs and ugly commented code.

…et upload feature.

…dget feature.

…e plugin.It's going to be extracted later. This type will be passed into Upload Widget Feature. The Idea is to have the ability to implement a different progress indicators for other widgets.

…erage.

…est.

…ion of progress indicator with Easy Image itself.

…get upload feature. Extracted custom progress indicators types and styles into a common files.

Also removed good old conole logs.

… filled in 100% instead of being empty.

…g CIRCLE_THICKNESS const. Also removed stray CSS code.

There were also a couple minor code style corrections.

…ered.

…y. Adjusted API docs.

This is done as a temp workaround, as the objects that we're passing as definition to features, are copies of the original definition. In other words any modifications to definition done at runtime (by other features setUps or at a widget runtime) are not available in setUp function. Because of that definition.name was not available (because it's assigned by the widget system at a widget construction time).

…d test coverage.

…y Image upload.

…ure by dropping onto readonly editor.

…pload feature.

…ite passes faster.

mlewand · 2018-01-25T09:14:43Z

Pushed a set of fixes that also brings:

Fixes requested in review summary/comments.
Support for pasting images from clipboard in IE11 tp#3390
Support for pasting a single image file in Firefox tp#3390
Support for converting any pasted base64 encoded image.
- This also handles images pasted from Word.

This PR does not cover following cases:

No improved support for switching to source mode and back during upload, so that following is not yet satisfied:

Switching to Source mode during upload also results in widget with not updated blob URL (previously it caused errors so it is an improvement).
No handing for cross editor copy and paste widget during upload to other editors (it will remain a blob reference).

These limitations will be addressed in future releases.

f1ames

Just few things to polish:

Language files were not regenerated.
As support for PFW was added, it would be nice to add some tests covering it (manual for sure, unit test maybe, WDYT?):

Support for converting any pasted base64 encoded image.

This also handles images pasted from Word.

Small issue with circle progress reporter - as it creates some overlay over image during upload it is not possible to edit caption during upload.
Direct image paste works funny on OS X, but I suppose it is an OS "feature":

f1ames · 2018-01-25T10:30:45Z

dev/langtool/meta/ckeditor.plugin-easyimage/meta.txt

@@ -4,3 +4,4 @@
 altText = Label for button converting image to assign an alternative text.
 fullImage = Label for button converting image to a full width.
 sideImage = Label for button converting image to be a side image.


Shouldn't it be commands.altText, commands. fullImage , commands. sideImage?

That's out of scope of this issue, but it is a good catch, please extract it.

f1ames · 2018-01-25T10:31:58Z

plugins/easyimage/lang/en.js

@@ -8,5 +8,6 @@ CKEDITOR.plugins.setLang( 'easyimage', 'en', {
 		fullImage: 'Full Size Image',
 		sideImage: 'Side Image',
 		altText: 'Change image alternative text'
-	}
+	},
+	uploadFailed: 'Your image could not be downloaded due to network error.'


Your image could not be downloaded due to network error

or rather

Your image could not be uploaded due to a network error

?

f1ames · 2018-01-25T10:35:02Z

plugins/easyimage/plugin.js

+						} );
+					}
+
+					function setImageWidth( widget, height ) {


It somehow a little less consistent, but from practical point of view makes much sense. Let's leave it as is 👍

f1ames · 2018-01-25T10:49:23Z

plugins/imagebase/plugin.js

+			 * @param {String} blobUrl Blob URL of an image.
+			 * @param {Boolean} [finalize=true] If `false` widget will not be automatically finalized (added to {@link CKEDITOR.plugins.widget.repository})
+			 * but returned as a {@link CKEDITOR.dom.element} instance.
+			 * @returns {CKEDITOR.plugins.widget/CKEDITOR.dom.element} The widget instance or {@link CKEDITOR.dom.element} of a widget wrapper if `finalize` was set to `false`.


No docs for data param.

All other methods docs are public /**, only this one is private /*, is there any specific reason for this? (same for upload* events below)

f1ames · 2018-01-25T11:44:55Z

plugins/imagebase/plugin.js

+		/**
+		 * Widget feature dedicated for handling seamless file uploads.
+		 *
+		 * This type serves solely as a mixing, and should be added using


Shouldn't it be

This type serves solely as a mixing, and should be...

?

Also, I think we should add @since tag here.

As for @since tag, I don't see much sense for that here as CKEDITOR.plugins.imagebase namespace is added in 4.9.0 so it seems natural to me that this info is not needed.

I'll report a feature request in Umberto to inherit/"cascade" since tag.

FYI: reported as cksource/umberto#466

f1ames · 2018-01-25T12:06:57Z

plugins/imagebase/plugin.js

+			 *
+			 * @private
+			 * @param {CKEDITOR.editor} editor
+			 * @param {Blob/String} fileOrData See {@link CKEDITOR.fileTools.fileLoader}.


The fileOrData docs param name doesn't match with the file param in the code.

f1ames · 2018-01-25T14:14:44Z

plugins/imagebase/plugin.js

+		updated: function() {},
+
+		/**
+		 * To be called when the progress should be marked as complete.


I was thinking about making it more consistent (Method to be called versus To be called), but I think something like Marks the progress as complete. or Refreshes the progress. sounds better, WDYT?

Great idea! It's simpler. The original text looks redundant compared to that.

f1ames · 2018-01-25T15:00:39Z

tests/plugins/imagebase/features/_helpers/tools.js

+		 * @param {CKEDITOR.editor} editor
+		 * @param {Object} options
+		 * @param {File[]} [options.files=[]] Files to be dropped.
+		 * @param {Function} options.callback Function to be called after the paste event.


I would change order of options.files and options.callback so the required one is first.

f1ames · 2018-01-25T15:17:54Z

tests/plugins/imagebase/features/_helpers/tools.js

+			 * {@link CKEDITOR.plugins.imageBase.progressReporter}.
+			 * @returns {Function}
+			 */
+			getProgressOverlapType: function( editor, ProgressReporter ) {


I have a mixed feelings about ProgressReporter instead of progressReporter, but I see what was the intention here.

Well we don't have an official and strict rule about this. My intention was to uppercase a local variable in case it stores a type. Does it make sense?

Makes sense, it may be a good way to distinguish it 👍

f1ames · 2018-01-25T15:18:13Z

tests/plugins/imagebase/features/_helpers/tools.js

+			/*
+			 * Returns a type that implements a progress indicator that puts a
+			 * circle-shaped progress bar.
+			 *


Missing @param docs.

mlewand · 2018-01-26T15:36:52Z

Language files were not regenerated.

Good point, however our lang tool seems to be broken, as I'm adding easyimage plugin to list of plugins tracked by lang tools and it still doesn't work. I'll extract it to a separate ticket, especially that original entries need a fix too.

As support for PFW was added, it would be nice to add some tests covering it (manual for sure, unit test maybe, WDYT?):

Support for converting any pasted base64 encoded image.

This also handles images pasted from Word.

As for manual test it's a good idea (added in 44e6f43).

As for unit test, TBH I don't find it necessary at this point. We have a good coverage with base64 encoded tests, and this is what our Paste form Word produces.

If we ever have some problem on integration between the two, we'll add a proper unit testing for integration.

Small issue with circle progress reporter - as it creates some overlay over image during upload it is not possible to edit caption during upload.

This is used just for tests, it's purpose is just to visualize the flexibility of this feature. It looks good enough as it is.

We'd have to worry about that if it was our main progress reporter.

Direct image paste works funny on OS X, but I suppose it is an OS "feature":

I don't have a Mac to check it at this point, but I suppose it transforms image into base64-encoded one, thus following feature apply:

Support for converting any pasted base64 encoded image.

Pushed fixes.

f1ames

Looks good, nice work 👍

I don't have a Mac to check it at this point, but I suppose it transforms image into base64-encoded one, thus following feature apply:

Support for converting any pasted base64 encoded image.

Looks like works according to the feature. The image (I mean the icon) is properly uploaded to CS.

f1ames · 2018-01-29T10:22:19Z

tests/plugins/imagebase/features/_helpers/tools.js

+			 * {@link CKEDITOR.plugins.imageBase.progressReporter}.
+			 * @returns {Function}
+			 */
+			getProgressOverlapType: function( editor, ProgressReporter ) {


Makes sense, it may be a good way to distinguish it 👍

…minor doc fixes. Docs: simplified reporter methods description and corrected uploadStarted event docs.

…tion.

mlewand added 30 commits January 22, 2018 15:07

First mocks to get the new upload working. So far it's messy PoC conc…

a6c06eb

…ept with some logs and ugly commented code.

Another round of this fabolous (and dirty) PoC to bring reusable widg…

ad18f2f

…et upload feature.

An attempt to integrate our custom EI progress bar with the upload wi…

0ead4ec

…dget feature.

Added a ProgressBar type, so far it's going to be placed in Easy Imag…

ac5c5cd

…e plugin.It's going to be extracted later. This type will be passed into Upload Widget Feature. The Idea is to have the ability to implement a different progress indicators for other widgets.

Generic loader integration for ProgressBar type. Added unit tests cov…

d0e30ec

…erage.

Added ProgressBar#remove and a basic manual test.

bcd0ba8

Tests: implemented a different type of progress indicator in manual t…

e314594

…est.

Fixed the event subscribed for upload progress. Experimental integrat…

ee02ae0

…ion of progress indicator with Easy Image itself.

Tests: added a manual test for custom progress indicators used in wid…

fe8cb0b

…get upload feature. Extracted custom progress indicators types and styles into a common files.

Time to say farewell to our good old Easy Image custom upload widget.

1017dea

Refactoring: renamed bindToLoader to bindLoader method.

ac319db

Also removed good old conole logs.

Tests: fixed an issue where the circle progress indicator would start…

3845489

… filled in 100% instead of being empty.

Tests: make sure that circle bar thickness is fully configurable usin…

624da0c

…g CIRCLE_THICKNESS const. Also removed stray CSS code.

Refactoring: removed code leftovers.

9f8e160

Upload Widget Feature now supports dropping multiple files at once.

675c83d

There were also a couple minor code style corrections.

Added support for pasting images straight from the clipboard.

8192548

Moved Easy Image specific upload logic to the EI plugin.

3427cfc

Fixed the toolbar positioning for initial Easy Image widget render.

180db78

Added an error message when error occured during the upload.

23941ca

Removed redundant image width setting.

745c94c

Removed a todo note about the toolbar repositioning. It's already cov…

c219678

…ered.

Added a base ProgressReporter type, refactored code to use it properl…

3d8a021

…y. Adjusted API docs.

Added basic tests for Upload Widget Feature.

f4faf57

Added more unit tests for Upload Widget Feature.

44754f9

Tests: minor code style adjustments in upload feature suite.

ecc2b20

Docs: added API docs for uploadFailed event.

16fd417

Moved widget upload feature progress logic back to imagebase. Improve…

e5db4a6

…d test coverage.

Tests: added an integration test for switching editor mode during Eas…

32ac54a

…y Image upload.

Fixed a case where it would be possible to trigger Upload Widget Feat…

1102405

…ure by dropping onto readonly editor.

mlewand added 11 commits January 24, 2018 19:39

Docs: rephased method description.

8ad7486

Tests: removed a todo marker.

59751c9

Docs: rephrased comment.

41ef371

Minor code enhancements, added a new, simple manual test for widget u…

0119fb2

…pload feature.

Added lang meta entry.

0420547

Docs: more API docs for the Widget Upload Feature.

d8685f0

Docs: API docs polishing.

27185cf

Changed the order of _spawnLoader method.

649a874

Refactoring: _loadWidget is no longer used - removing.

4c59ec8

Refactoring: renamed uploadBegan to uploadStarted event.

41921b3

Tests: lowered fake file loader response timings, so that the test su…

e0a5079

…ite passes faster.

mlewand requested a review from f1ames January 25, 2018 09:14

f1ames suggested changes Jan 26, 2018

View reviewed changes

f1ames self-requested a review January 26, 2018 16:15

f1ames approved these changes Jan 29, 2018

View reviewed changes

mlewand and others added 9 commits January 29, 2018 13:03

Docs: improved API docs in widget upload feature.

c06386e

Fixed typos.

2b453d9

Tests: Corrected API docs for Upload Widget Feature test helpers.

077156b

Fixed uploadId not being assigned if uploadStarted was cancelled and …

548f411

…minor doc fixes. Docs: simplified reporter methods description and corrected uploadStarted event docs.

Fixed error message when uploading Easy Image widget fails.

40853b7

Docs: API docs for Image Base plugin corrected.

38aff7f

Tests: added a manual test for Easy Image and Paste from Word integra…

a6dd0c7

…tion.

Tests: adjusted test assert to message change.

b454268

Docs scope adjustments.

83d2e32

f1ames force-pushed the t/932-3389 branch from c92e2fe to 83d2e32 Compare January 29, 2018 12:04

f1ames merged commit d7d36f7 into t/932-3388 Jan 29, 2018

f1ames deleted the t/932-3389 branch January 29, 2018 12:05

mlewand mentioned this pull request Jan 29, 2018

Upload for Easy Image #1515

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Widget upload feature for Easy Image #1483

Widget upload feature for Easy Image #1483

mlewand commented Jan 22, 2018

mlewand commented Jan 25, 2018 •

edited

f1ames left a comment

f1ames Jan 25, 2018

mlewand Jan 26, 2018 •

edited

f1ames Jan 25, 2018

f1ames Jan 25, 2018

f1ames Jan 25, 2018

f1ames Jan 25, 2018

f1ames Jan 25, 2018

f1ames Jan 25, 2018

mlewand Jan 26, 2018

mlewand Jan 27, 2018

f1ames Jan 25, 2018

f1ames Jan 25, 2018

mlewand Jan 26, 2018 •

edited

f1ames Jan 25, 2018

f1ames Jan 25, 2018

mlewand Jan 26, 2018

f1ames Jan 29, 2018

f1ames Jan 25, 2018

mlewand commented Jan 26, 2018 •

edited

f1ames left a comment

f1ames Jan 29, 2018

Widget upload feature for Easy Image #1483

Widget upload feature for Easy Image #1483

Conversation

mlewand commented Jan 22, 2018

What is the purpose of this pull request?

Does your PR contain necessary tests?

This PR contains

What changes did you make?

mlewand commented Jan 25, 2018 • edited

f1ames left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mlewand Jan 26, 2018 • edited

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

Choose a reason for hiding this comment

mlewand Jan 26, 2018 • edited

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

mlewand commented Jan 26, 2018 • edited

f1ames left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mlewand commented Jan 25, 2018 •

edited

mlewand Jan 26, 2018 •

edited

mlewand Jan 26, 2018 •

edited

mlewand commented Jan 26, 2018 •

edited