Feature/ck editor easy image

[skamphuis]

This PR Fixes #4404

Summary

This PR implements the easyimage plugin in the CKEditor provider, as described in #4404.
We needed/chose to use a CKEditor download that includes this plugin, which is also why a large number of files have changed.

The PR is build upon #4101 which upgrades the CKEditor version.

[sleupold]

would it be much work to implement a second toolbar button for this great plugin?
for admins, we would still need the additional image options, when inserting a picture.

[skamphuis]

would it be much work to implement a second toolbar button for this great plugin?
for admins, we would still need the additional image options, when inserting a picture.

A second button is not possible. I would be possible to use the easyimage plugin only for non-admins, or otherwise configurable roles. But each user would have either the standard one, or the easyimage one. I chose this solution for now, so we have something to build on.

[mitchelsellers]

Although this is an option and something that would need to be enabled by the users, I'm not 100% sure that this is a good idea for us to implement.

Removal of the ability to set attributes such as alt on the image via the dialog will encourage poor Accessibility and we have been making strides as a platform to improve accessibility.

[skamphuis]

Although this is an option and something that would need to be enabled by the users, I'm not 100% sure that this is a good idea for us to implement.

Removal of the ability to set attributes such as alt on the image via the dialog will encourage poor Accessibility and we have been making strides as a platform to improve accessibility.

Setting the alt on the image is actually even easier with this plugin:
right click to select the option:

and enter the new alt:

[mitchelsellers]

Ok, that is great then! I'll continue review here

[bdukes]

Can you explain the workflow here? I'm having trouble following the code changes without doing a deeper dive.

1. There's a new CloudServices Web API controller that just returns "dummytoken", is that needed for this to work?
2. It looks like you've changed the ImageUpload command to Browser.aspx to just write {"default":null} instead of a script block with the JsUploadCode. What other changes to the current image upload workflow happened?
3. You've added a couple of console.log statements, can those be removed, please?
4. (nit) Can you rename ImageButton.cs to ImageButtonType.cs (to match the name of the type it contains)?
5. You've commented out the file browser URL settings (in EditorControl.cs), is that intentional? Should that only happen when the Easy Upload button used? Can the file browser be used with the easy upload?

[skamphuis]

Hi, thanks for asking. Let me try to answer each question below:

1. There's a new `CloudServices` Web API controller that just returns `"dummytoken"`, is that needed for this to work?

Yes, that's required for this to work. The Easy Image plugin was built to have images upload to a CDN that's operated by CKEditor. For this upload to work, they added this service call to handle security/authentication to their API. We don't need that, but the plugin requires a working service call to even work. I'm not sure if returning an empty string would work too, but I chose to return "dummytoken" to express that it's actually a dummy token.

2. It looks like you've changed the `ImageUpload` command to `Browser.aspx` to just write `{"default":null}` instead of a `script` block with the `JsUploadCode`.  What other changes to the current image upload workflow happened?

It looks like I missed a bit of cleanup here. Returning the {"default":null} was part of an attempt to find and fix an issue I was having. The changed return value should only apply to uploads that actually come from the Easy Image plugin. The current image upload workflow shouldn't be changed at all. This was needed because the current upload indeed expects a script block, but the easy image expects a JSON object with property "default" and a value containing the relative path to the newly uploaded image.

I'll commit a fix for this, later on today to make sure I didn't change existing behavior. However, I haven't found any way in which this return value is actually used, since the current workflow of uploading uses FileUploader.ashx to post the files to. This only now makes me think I probably should have done that too, so that Browser.aspx can be cleaned later on, by removing all unused code. What do you think?

3. You've added a couple of `console.log` statements, can those be removed, please?

Yes, of course, sorry about that.

4. (nit) Can you rename `ImageButton.cs` to `ImageButtonType.cs` (to match the name of the type it contains)?

Yes. I made it the same as "BrowserType" which is in a file "Browser", but I'll rename that one too, since it's the only one where the classname is not matching the file name. Bad pick...

5. You've commented out the file browser URL settings (in `EditorControl.cs`), is that intentional?  Should that only happen when the Easy Upload button used?  Can the file browser be used with the easy upload?

Well, this is related to "2". These settings seem to be overwritten in Browser.aspx to use FileUploader.ashx and thus unused. Which is why I didn't notice I still had this commented out while testing (everything was working).
What's your opinion? Clean up now, or undo the change to keep existing behavior, and clean up later?

[bdukes]

Sounds good to me. I think I'd prefer to leave the current image upload code as-is, and then do clean up in a separate PR (but not a strong preference, so if it's easier to do it in this PR, I won't complain)

[skamphuis]

Sounds good to me. I think I'd prefer to leave the current image upload code as-is, and then do clean up in a separate PR (but not a strong preference, so if it's easier to do it in this PR, I won't complain)

I agree. Just pushed a commit with these cleanups.
Also, I noticed (and remembered) that the file browser URL settings (in EditorControl.cs) were commented out because they caused a conflict (double POST requests for uploads) with the EasyImage plugin. I restored original workflow now, and only apply these for the standard image upload button.

[bdukes]

👍🏻