Skip to content
This repository has been archived by the owner on Jun 26, 2020. It is now read-only.

Prevented View from firing the render event if there were no changes since the last view.render() call #1661

Merged
merged 20 commits into from
Feb 6, 2019

Conversation

ma2ciek
Copy link
Contributor

@ma2ciek ma2ciek commented Feb 5, 2019

Suggested merge commit message (convention)

Fix: Prevented View from firing the render event if there were no changes since the last rendering. Closes ckeditor/ckeditor5#4473. Closes ckeditor/ckeditor5#4475.


Additional information

  • BREAKING CHANGE: The editing.view.render() method was renamed to editing.view.forceRender(). It should be used with caution as it will re-render editing view and repaint the UI.

@ma2ciek ma2ciek requested a review from pjasiun February 5, 2019 16:29
@ma2ciek
Copy link
Contributor Author

ma2ciek commented Feb 5, 2019

I see that there's one test failing in the ckeditor5-image (image caption placeholder). I'll need to fix it, but it shouldn't block reviewing.

@pjasiun
Copy link

pjasiun commented Feb 6, 2019

Looks promising.

I checked all usage of the render method and all of them uses render with the force flag (except the one in the editing controller). It makes sense, render with no force flag should not be needed since all view changes cause rendering. This is why I propose such additional change: https://github.com/ckeditor/ckeditor5-engine/compare/t/1653-b...t/1653-b2?expand=1

It makes a huge number of tests fail, but mostly because of the missing view.render method. When I've added alias:

	render() {
		this.forceRender();
	}

Only 8 tests are failing (4 in FocusObserver, 2 integration test, 1 SelectionObserver and one view.focus()). Could you check such change?

@ma2ciek
Copy link
Contributor Author

ma2ciek commented Feb 6, 2019

Could you clarify then how the view.change() / view.render() should be called in the EditingController?

@oleq
Copy link
Member

oleq commented Feb 6, 2019

I checked this branch in the context of ckeditor/ckeditor5#479 constellation and it seems to be working.

@pjasiun
Copy link

pjasiun commented Feb 6, 2019

Could you clarify then how the view.change() / view.render() should be called in the EditingController?

I pushed one more commit to my POC.

@ma2ciek
Copy link
Contributor Author

ma2ciek commented Feb 6, 2019

I've just merged your proposal @pjasiun to that branch.

src/view/view.js Outdated Show resolved Hide resolved
@@ -386,7 +409,8 @@ export default class View {
* Throws {@link module:utils/ckeditorerror~CKEditorError CKEditorError} `applying-view-changes-on-rendering` when
* trying to re-render when rendering to DOM has already started.
*/
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you should add to this docs that this method does not need to be used after changes in the view - they will cause rendering anyway. This method is dedicated to special cases when view does not change be need to be rendered anyway (for instance for some observers which overwrites DOM changes if they were not handled).

@pjasiun
Copy link

pjasiun commented Feb 6, 2019

Only a view docs improvements needed.

@ma2ciek
Copy link
Contributor Author

ma2ciek commented Feb 6, 2019

I've fixed the placeholder implementation (wrapped initial changes in the view.change() block) and its tests so all tests (especially the one failing in the ckeditor5-image) are green now.

Co-Authored-By: ma2ciek <ma2ciek@gmail.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Some changes in view are not rendered. Prevent UI from reacting to the conversion in some situations
3 participants