Fix placeholder over media embed.

[jodator]

Suggested merge commit message (convention)

Other: The placeholder will not be shown for widgets. Closes ckeditor/ckeditor5#1684.

---

Additional information

@oleq: I've update the PR so the widgets (checked by a custom property) are considered not needing a placeholder.

---

OLD:

I've been fiddling with the placeholder for some time to check what caused the ckeditor/ckeditor5#1684 and it turned out that the placeholder was enabled for "emptish" elements. The "emptish" element is such that has no children or all of the children are UI Elements.

I'm not sure why this was created this way (maybe some collaboration features? but I doubt it) but this check would give false positive for widget with only UIElement children - this is what happens with MediaEmbed.

So taking this into consideration I don't see that UIElement should dictate that element is empty in regards for showing a placeholder.

The other change that we might consider is to tie emptiness check to the model. I didn't go this route as currently the placeholder operates on the view solely.

ps.: The isWidget check was also discarded.

[coveralls]

Coverage remained the same at 100.0% when pulling b772835 on t/ckeditor5/1684 into c057a94 on master.

[oleq]

The isEmptyish check has been introduced by @scofalik in 6a66f6e and solved https://github.com/ckeditor/ckeditor5-engine/issues/1018 which, in turn, was a piece of https://github.com/ckeditor/ckeditor5-engine/issues/1017. Sounds like there was a solid reason for that check (something about markers, which makes sense on second thought).

Can you investigate that? Because it looks like your PR could break some core functionality.

[oleq]

We cannot merge this without the research as pointed out in #1749 (comment).

[jodator]

We cannot merge this without the research as pointed out in #1749 (comment).

Thanks bisect & git log master :D I feel so bad for not digging it properly :( - you're right the check should be better then. I'll check on this and I'll update the PR.

[jodator]

@scofalik & @oleq:
OK so after some fiddling I can see that this check is done for markers case mostly. The placeholder must be visible when there are markers in empty paragraph or empty caption.

The solution for this could done by:

1. CSS override for media embed - do not show placeholder there.
2. Check if element is a widget and consider it always not empty either by:
   a. isWidget() util
   b. checking custom property element.getCustomProperty( 'widget' )

As the markers are hidden in the UI similarly (by CSS) with a placeholder active I think that we might go with CSS solution. But OTOH such solution is not clean enough - it will add the ck-placeholder class to the widget which is not totally OK. So I'd rather go with 2b solution as the engine should not import widget repository.

[oleq]

I'm for 2b. 2a would add a ckeditor5-widget dependency to ckeditor5-engine which we don't need (want?).

[oleq]

On the second thought, that would be the only place in the codebase other than isWidget() from ckeditor5-widget that calls getCustomProperty( 'widget' ) 😛

Maybe we could use ckeditor5-widget as an engine dependency after all. I'm not sure about consequences, though. cc @Reinmar.

[jodator]

Yeah... it might be a cycling dependency then... or widget being an engine feature :P.

At the current state I rather have this check then importing isWidget() but let's see @Reinmar 2¢.

[oleq]

TBH I have nothing against the current state (element.getCustomProperty( 'widget' )) and some improvements in i26 as long as we don't change dependencies in i25.

[oleq]

LGTM.

The only question is whether to isWidget() or to getCustomProperty( 'widget' ).

[jodator]

After F2F talk with @Reinmar I'll updat this PR with checks in the schema so the needsPlaceholder() method will also get editingController.

[jodator]

@Reinmar I'm working on the rewrite so the needsPlaceholder() function would check model, not the view.

This looks like a bigger change because the enablePlaceholder() function and some other functions in this feature must also accept the editing instead of the view.

It is not a big issue in most places but it would require some more work at least in the ImageCaptionEditing because in the downcast conversion it first enables the placeholder and as the next step it binds view and model elements so the needsPlacesholder() fails on checking the schema.

TL;DR: I don't think we can squish the rewrite in the 25 milestone. My suggestion is that we could use the current state (checking for the widget) in 25 and rewrite it in 26.

[scofalik]

To be honest, I think that the problem is using UIElement where we shouldn't. Media embed element is not a UIElement. We use UIElement only because we don't have a better option as far as view element types go. So maybe we should go that way?

[Reinmar]

WDYM? I remember you were proposing introducing new element types. Would that (IIRC) RawElement help here?

[scofalik]

I am not sure why exactly UIElement was used here because I haven't been developing the feature so I don't know what problems EmptyElement or ContainerElement had. All I am saying is that using UIElement for non-UI (read: content meaningful) elements is not the indented use case for UIElement. So saying that "old placeholder solution is wrong because UIElement is used in media embed" might be approaching the problem from an incorrect side. Same with introducing additional checks or extending schema. Although this is something I like because it might be useful for plugins creators.

[Reinmar]

I am not sure why exactly UIElement was used here because I haven't been developing the feature so I don't know what problems EmptyElement or ContainerElement had.

We need custom rendering, without going through our rendering engine.

[jodator]

Guys, so the RawElement would play nice here (https://github.com/ckeditor/ckeditor5-engine/issues/1643). Am I right? From what I can see it would act similarly as UIElement - it should allow creating custom HTML code inside base elment - just like UIElement does but from model POV it will act like any other element.

[scofalik]

I think so.

[Reinmar]

Yep, I think so too. I wasn't sure initially about the semantics of this type of element and the difference between UI and Raw, but I think https://github.com/ckeditor/ckeditor5-engine/issues/1643#issuecomment-514653753 makes it clear and in that case, it would solve this case.

[scofalik]

What is the status of this PR ?

[jodator]

What is the status of this PR ?

I'm going to implement RawElement or whatever it would be called. At the moment the work is staled.