Enable <img> inside <p> conversion

[scofalik]

Originally described here: https://github.com/ckeditor/ckeditor5-image/issues/8#issuecomment-263228358

The problem is that <paragraph> do not allow <image> inside it, but it's common to have <p>Foo<img />bar</p> content in pasted data. Such data should be converted to <paragraph>Foo</paragraph><image></image><paragraph>bar</p>.

[Reinmar]

Each element which we'll want to "hoist" from incorrect context by breaking its parent(s) will require additional code. So, we'll need a more general solution to this problem than a special handler in the image feature. A way to mark certain elements that even if they are not in the right context, they should be converted and lead to splitting the parents up to a point where they are allowed.

OTOH, we don't have other cases in our hands currently, so we can take some shortcuts to speed it up.

[scofalik]

Honestly, I don't know how to approach this issue, when there is that issue: https://github.com/ckeditor/ckeditor5-paragraph/issues/11.

Here we want to break paragraph when there is an image inside it, while there we want to make a paragraph on image if it doesn't have it. If we want to come up with something that makes sense we need to agree on some things.

Okay now you can say - but this is block-image case and that issue is inline-image case. But how actually are we going to differentiate between them? External HTML won't give you much clue about it. Consider following examples:

foo<img />bar
<p>foo<img />bar</p>
<p>foo</p><img /><p>bar</p>
<div>foo<img />bar</div>
<div><p>foo</p><img /><p>bar</p></div>

Case 1: Hard to say whether this should be inline or block.
Case 2: Looks like an inline image.
Case 3: Looks like a block image because it is between <p>s.
Case 4: Looks like an inline image because is in same container as text nodes.
Case 5: Is between <p>s but on the other hand is in paragraph-like element.

In case 5, DOM converted to model, without any magic, should end up like this: <paragraph>foo</paragraph><img /><paragraph>bar</paragraph>. So basically it is a block image, as it is not in paragraph.

In case 1 and 4, after autoparagraphing kicks in, we get the same situation as in case 4.

My idea is that if we want inline images, when converting view <img /> we should look at its ancestors (data.context) and decide whether we convert <img /> to block-image or inline-image (we will probably differentiate them somehow in model).

If we don't want inline images in foreseeable future, we can for now forget about the issue I linked above (or instead of <img /> think of other inline element, but now I don't know what kind of empty inline element other than image we could support).

If that's the case let's think how we want to proceed with this. Let's start from noticing, that we could have different kinds of inputs:

foo<img />bar
<div>foo<img />bar</div>
<p>foo<img />bar</p>

Case 1: Works out of the box (after bare <img> conversion is enabled). It's funny because it works because https://github.com/ckeditor/ckeditor5-paragraph/issues/11 this is not enabled :). Will still work after this idea is implemented: https://github.com/ckeditor/ckeditor5-paragraph/issues/10#issuecomment-293889123
Case 2: Should work if above idea is implemented. Now it doesn't work, because <div> is treated like <p> and all elements inside are converted in <paragraph> context. If we change this idea and just convert texts in <paragraph> context the image will be converted.
Case 3: Back to the original issue :)

It seems that with proper autoparagraphing we will only have situations where we either have image in root or in a recognized and converted element, like <heading> or <paragraph>, so let's focus on that.

If we want to fix this on view to model conversion* then we could use the same trick as with autoparagraphing (https://github.com/ckeditor/ckeditor5-paragraph/issues/10#issuecomment-293889123). The trick is to convert image anyway. Optionally, check if it will be allowed at any level of data.context (for example if data.context is $root, blockquote, p check if image can be in $root blockquote or $root). Then, after element is converted, scan it's children and if it has image but cannot have it, split the element and pull the image from it.

* - I've made that comment because I think that it would be also reasonable to do this and even autoparagraphing before view to model conversion. We could use ViewConversionDispatcher#viewCleanup event or insertContent() callbacks. To do this, however, we would probably need to hardcode some rules connected with view elements which isn't nicest (for example we would have to decide that <h2> should be treated like <p> thus <img> should break it no matter what features are enabled and what is set in Schema). This approach, however, might be more efficient and simpler.

[scofalik]

Since it took me like 2 hours to write this and you commented in the meantime - I agree that this could be done as more general solution - both as "autohoist" view-to-model converter (same as autoparagraph for text and inline elements) or - as I described later - corrected straight on view (using some hardcoded view/DOM relations).

OTOH, we don't have other cases in our hands currently, so we can take some shortcuts to speed it up.

Shortcuts can lead us to such problems again. We need a good understanding of how incorrect content should be fixed.

[Reinmar]

Good that I'm on holidays and the weather is bad so I have a lot of time to read this :D

[Reinmar]

Here we want to break paragraph when there is an image inside it, while there we want to make a paragraph on image if it doesn't have it.

Yyy... nope. We don't want to create paragraphs on images because images are allowed in the root while they are not allowed in paragraphs. That's a totally different case than the whole autoparagraphing issues.

[Reinmar]

Okay now you can say - but this is block-image case and that issue is inline-image case. But how actually are we going to differentiate between them? External HTML won't give you much clue about it

It depends on the features you load to the editor. Currently, we support only block images so the image feature converts only them. If we'll ever support inline images this will be a separate feature. Now, which one of them gets the <img> element during conversion should depend on how their converters were registered. The default converters will only try to handle <img> in correct locations. So, I'd say that the order will be like this:

1. Correct block <img> and correct inline <img> converters.
2. Incorrect (<p><img></p>) block <img> and incorrect (<root><img></root>) inline <img> converters.

Actually, the correct block <img> converter listens to <figure> (EDIT: I forgot about the additional listener which this feature introduces – so this would need to have a lower priority). The incorrect <root><img></root> inline converter will be handled by the autoparagraphing feature which uses low priority listeners. So all this will be fine if the incorrect block converter will be added in a way to not conflict with correct inline <img> converter and incorrect inline <img> converter.

[Reinmar]

If we don't want inline images in foreseeable future, we can for now forget about the issue I linked above (or instead of think of other inline element, but now I don't know what kind of empty inline element other than image we could support).

We don't plan to work on inline images in the near future, but, as I wrote above, I don't think that they would be a big issue. There's no magic needed here. If an image is in the right context, its converter should handle it immediately. If not, then it's not that important what happens if someone will ever have two types of images supported in one editor (which is a super ugly scenario anyway).

[Reinmar]

Anyway, for now I can see that all this just boils down to a proper ordering of the listeners. It's sad that this is not trivial, but this is not trivial only if we consider two conflicting features working at the same time – block and inline images. If we considered just one, everything would be simple (EDIT: I should never say that :D).

[scofalik]

Okay, so to sum it up:

1. We don't care about having perfect solution for inline image and block image features running together.
2. We treat this issue https://github.com/ckeditor/ckeditor5-paragraph/issues/11 as "autoparagraphing inline elements" and do not focus on inline images.
3. We would like to introduce "autohoist" converter.
4. Following converters will be added and fired in this order:
   • <img> block converter -- will fire if image can be converted in current context,
   • <img> inline converter -- as above,
   • autohoist converter (<p>x<img />y</p> -> <p>x</p><img /><p>y</p>) -- will fire if image element could be placed in any of it's ancestors but cannot be placed in current context,
   • autoparagraph converter (x< img />y -> <p>x<img />y</p>) -- will fire if image element could be placed in paragraph but cannot be placed in current context.

[Reinmar]

Yep, looks good. Please, remember that the last point (autop inline stuff) has the lowest priority because we don't have any empty inline elements atm. Although, perhaps this issue and losing inline styling when autoping has the same solution anyway.