Updated the UI after an image has been loaded in the DOM

[oskarwrobel]

Suggested merge commit message (convention)

Fix: Rendered the view after an <img/> DOM element has been loaded in the DOM root. Closes ckeditor/ckeditor5#5118.

[Reinmar]

imageLoaded?

[Reinmar]

Hm... I forgot that the native image is called load. Still... imageLoad sounds soooo bad.

[oskarwrobel]

My first thought was just load the same as Image#load but load is too general, not only an image fires this event. So I added image prefix. But imageLoaded sounds ok.

[Reinmar]

It's better if this is WeakSet. Or that you clear it up in the destroy().

[Reinmar]

Ideally, both :D

[pjasiun]

Not sure if we can do both. You can not iterate over WeakSet.

[Reinmar]

Very unclear when it's fired. Also an image element or an <img> element.

[oskarwrobel]

Fired when an <img/> DOM element has been loaded in the DOM root.

WDYT?

[Reinmar]

I've got serious doubts about this change. How is it going to work on initial data load? Imagine a content with 100 images. Are we going to call render() 101 times?

Also, we had a rule to not call render at random moments (outside change()). Did it change? It is violated here.

Finally, why do we call render() at all? The view didn't change, so nothing will be rendered. Do we execute it only for the #render event? If so, I'd propose to just fire it and avoid unnecessarily executing all the logic from render().

[oskarwrobel]

I can fire render event instead of calling render(). But still, when there will be a content with 100 images and all of this images will load async then render will be fired 101 times and this is by design. But I understand that you are asking about unnecessary render() call not about render event.

[oskarwrobel]

I used render() method instead of firing render event to avoid situation when render is fired in the middle of the view changes. But I was wrong. This is exactly the opposite, calling view.change() (used by view.render()) in the middle of rendering will throw.

https://github.com/ckeditor/ckeditor5-engine/blob/ec70448ac3dca8314442e304bd3151fc695e07be/src/view/view.js#L323-L338

[oskarwrobel]

I'm afraid that firing render also can lead to exceptions. When render event is fired in the middle of the engine changes it is possible that state of the model will be misaligned with the view and DOM.

[oskarwrobel]

Maybe this is the right moment for introducing ui#update()? ImageLoadObserver should only fire imageLoaded event and Image plugin should listen to this event and call ui#update()?

[Reinmar]

Maybe this is the right moment for introducing ui#update()? ImageLoadObserver should only fire imageLoaded event and Image plugin should listen to this event and call ui#update()?

That would be a cross-layer call. But frankly speaking, we've got a problem here. We want to update the UI on something related closely to the engine.

Therefore, IMO, this observer shouldn't do anything besides firing an event on the engine's "thing". And this event needs to be generic – we want to say that something has changed. Not that we render anything (hence view#render is wrong). That the layout might've changed.

Now, the problem is that this isn't the only reason why the layout may have changed. Resizing the editable is another one. How can someone who allowed resizing the editable let us know that it's changed? Are we back to ui#update()?

Another problem is – do we need two events? One in editor.ui and one in editor.editing.view.document (triggered by editing related changes)? Should the latter trigger the former too? How often should be the latter triggered? Should that happen on editor.editing.view#render too?

[Reinmar]

tl;dr – we need a call for this

[oskarwrobel]

tl;dr – we need a call for this

Call result:

• https://github.com/ckeditor/ckeditor5-engine/issues/1445.
• https://github.com/ckeditor/ckeditor5-core/issues/130

[oskarwrobel]

R- because it's blocked by https://github.com/ckeditor/ckeditor5-core/issues/130.

[oskarwrobel]

Introducing ImageLoadObserver has been moved to the separate ticket to split the engine and the UI part of this fix. #214