Custom upcasting for image widget

[Comandeer]

What is the purpose of this pull request?

New feature for 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?

I've created custom upcast menu for image widgets created by imagebase plugin, which loops through all functions passed in upcasts property and does additional checks (at the moment it checks only if the element is not a fake object) before upcasting.

[mlewand]

tl;dr It looks like unnecessary overcomplication at this point.

I believe we should not further customize upcast/upcasts combo just for this widget. Actually I think we don't need this feature at this point.

What you wanted to do here is to add a common handling for all element types in imagebase definition, and allow extending plugin to easily provide upcast for just any element.

Doing this is still a straightforward task:

• Extract common task to CKEDITOR.plugins.imagebase.imageWidgetDefinition method, say _isValidImageElement().
• Simply use this function in your deriving widgetDefinition, say easyImageWidgetDefinition.

Take a look at t/932-2970-upcast-simplified branch.

However it required one issue fix in 6232b54, as currently widget upcast methods are executed in a totally random context.

[Comandeer]

The issue with the solution you suggested is the fact that _isValidImageElement() performs only a check on given element. In the real world scenario upcasting elements often needs to alter the whole HTML structure, as it's done currently in image2 plugin.

IMO it would be a very common scenario to load content into the editor with no figure tags, but with img ones, which should be also handled by our plugin. And without wrapping img in figure, we can't provide the new functionality (e.g. dynamic caption). Solution with only a check is not sufficient for such case.

[mlewand]

@Comandeer I agree that upcast needs often to alter the markup, but it does so after checking if the input markup/element is OK.

At this point element is not yet changed.

Am I missing something?

[Comandeer]

Ok, let's think about img case:

1. upcast method is invoked with img tag.
2. _isValidImageElement method returns true as it sees img.
3. default upcast method returns false (img is not figure) – so widget won't be upcasted.
4. overwriting upcast means copying check logic and adding whole logic connected with wrapping img in figure.

And that overwrite covers only one case. What about other ones? E.g. image wrapped in link? If widget would allow linking, then links with images should have additional upcast method. If we're trying to create universal base plugin, adding new upcasting method alongside new feature integration should be easier than requesting to overwrite upcast method.

At this point element is not yet changed.

I don't understand it. upcast method is the method, which changes DOM to appropriate shape. Note that upcast often returns widget's element, not boolean value.

[mlewand]

You're not forced to duplicate and copy the code.

If you want to upcast a link that could contain a figure or img directly (note that for EI we're looking only for the first case) you can reuse necesarry members from upcasts, here first use upcasts.a, which (if succeed) also checks for ucpasts.figure/ucpasts.img.

Look at my recent push to the branch, especially b31e9d4.

This implementation looks even cleaner that I thought, and it reuses existing API.

Assuming that #1094 is fixed upcast whould be simplified just to CKEDITOR.tools.objectKeys( widgetDef.upcasts ).join( ',' ) (string) so it doesn't have to be updated every time.

Thus I don't think we need to introduce custom upcasting logic, as we can solve nicely this problem with existing mechanisms.

[Comandeer]

Actually your workaround is not really a workaround, because upcasts does not behave in a such way. If upcast property is not a string with member names of upcasts, then widget could not be upcasted at all (https://github.com/ckeditor/ckeditor-dev/blob/e9b72c216006e12290532c716b9dca12191872c0/plugins/widget/plugin.js#L1967-L1976 ) – that's why I added my own logic at the first place.

I'd also move it into imagebase plugin. Then we could eliminate _isValidImageElement and do all validation inside upcast methods.

[Comandeer]

After talk with @mlewand we decided to go with CKEDITOR.tools.objectKeys().join solution.