Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Calling view.change() and view.render() on 'render' event with low priority #4286

Closed
szymonkups opened this issue Feb 19, 2018 · 2 comments · Fixed by ckeditor/ckeditor5-engine#1313
Assignees
Labels
package:engine type:improvement This issue reports a possible enhancement of an existing feature.
Milestone

Comments

@szymonkups
Copy link
Contributor

During the view refactoring we created a mechanism that prevents changing the view after rendering to the DOM is started. It throws special exception when we call View#render() or View#change() after rendering to DOM is done:

// Listening on `low` priority when rendering to DOM is over.
view.on( 'render' , () => {
   // This will throw because View#render event on `low` priority indicates that
   // rendering is over for this change() block.
   view.render(); // or view.change( () => {} );
}, { priority: 'low' } );

Calling View#render() or View#change() should be possible - it should just start another rendering process.

Motivation:
There are situations when we want to listen to the render event on low priority and re-render the view afterwards. For example: balloon panel wants to re-position or close itself after DOM is rendered and focus back the main editable (focusing means that view should be re-rendered).

CC: @pjasiun.

@szymonkups szymonkups self-assigned this Feb 19, 2018
@pjasiun
Copy link

pjasiun commented Feb 20, 2018

I agree. If change block, focus change or render is called in low or lowest listener of the render event another render should be fired after the current render event is done.

I don't think we need to take care of infinite loops. Usually, this event will be fired under some condition, max 2 times. For instance, if balloon disappears we focus editor what causes another render, but, even if focus will be called again, there will be no more render event, because the editor is already focused.

@szymonkups
Copy link
Contributor Author

After some work on this and f2f talks we came to some conclusions.
Currently, view works like this:

  1. We call view.change() method to make changes in the view. All recursive calls to view.change() are grouped together and one render event is fired.
  2. Listeners to render event on "normal" or higher priorities are called before actual DOM rendering. When some listeners call view.change(), those changes are included in current change block and no new render event is fired.
  3. Listeners to render event on "low" or lower priorities are called after DOM rendering. When some listeners call view.change() there we throw with information that rendering to DOM has finished and we cannot modify the view.

There are two problems with that:

  1. We cannot call view.change() on "low" listeners.
  2. Calling view.change() on "normal" and higher listeners does not fire render events - so there is no information about changes made there.

This is too complicated, we decided to introduce post fixers to the view, similar to what we have in model. View changes should work like this:

  1. We call view.change() method to make changes in the view. All recursive calls to view.change() are grouped together.
  2. Post fixers are called. Same as in model - they made changes to the view and return true when made some changes. All post fixers are called until everything is fixed.
  3. Render event is fired. DOM rendering is performed on high priority. This event is used just to inform that rendering to DOM has finished and UI can be updated there.

pjasiun referenced this issue in ckeditor/ckeditor5-engine Feb 26, 2018
Other: Fix `render()` and `change()` flow. Introduce postfixers in view. Closes #1312.
@mlewand mlewand transferred this issue from ckeditor/ckeditor5-engine Oct 9, 2019
@mlewand mlewand added this to the iteration 14 milestone Oct 9, 2019
@mlewand mlewand added module:view type:improvement This issue reports a possible enhancement of an existing feature. package:engine labels Oct 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:engine type:improvement This issue reports a possible enhancement of an existing feature.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants