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

Mutations in UIElements should not be propagated / force render #1811

Merged
merged 5 commits into from
Dec 17, 2019
Merged

Conversation

mlewand
Copy link
Contributor

@mlewand mlewand commented Dec 11, 2019

Suggested merge commit message (convention)

Fix: Change in UIElement view will no longer force a view render nor will it trigger mutations event on the document. Closes ckeditor/ckeditor5#5600.


Additional information

The tests are passing, but this change needs to be carefully tested manually.

I suggest few testing scenario:

  • image style (aligning them, making sure that the toolbar moves properly)
  • inline toolbar for table
  • Cloud Features that use inline toolbar
  • not sure if it might get with toolbar rotator?

Also it's good that we'll be able to merge it at the beginning of iteration, so there's a bigger chance that we'll catch nuances if any appear.

@mlewand mlewand requested a review from Mgsy December 11, 2019 14:13
@Reinmar
Copy link
Member

Reinmar commented Dec 16, 2019

I think it's much more complicated to test than what you described, Marek. This PR is about changes that are not part of the editing pipeline, but instead come from the external logic and go directly to UIElements or changes that do not translate into mutations on the view tree. The problem is – it can be pretty much everything and it can happen at random moments because it may be done by the browser.

Because of that, I'd merge this ASAP and just live with it for some time and see.

@Reinmar
Copy link
Member

Reinmar commented Dec 16, 2019

Of course, Kacper and Filip, you can play with a couple of things but I think there are many more things that can go wrong so keep your eyes open :)

@Reinmar
Copy link
Member

Reinmar commented Dec 16, 2019

One thing's missing – a note in UIElement#render() that when you want to update the UI after re-rendering your element, you should call editor.ui.update().

Copy link
Member

@Reinmar Reinmar left a comment

Choose a reason for hiding this comment

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

As commented – misses one note in the UIElement#render().

mlewand and others added 2 commits December 16, 2019 18:18
@mlewand
Copy link
Contributor Author

mlewand commented Dec 16, 2019

Pushed the changes. I'm not entirely sure though if the wording is right so please double check this.

@mlewand mlewand requested a review from Reinmar December 16, 2019 17:36
src/view/uielement.js Outdated Show resolved Hide resolved
@Reinmar Reinmar merged commit b7e2bfe into master Dec 17, 2019
@Reinmar Reinmar deleted the i/5600 branch December 17, 2019 09:56
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.

Performance improvement in case of empty mutations
2 participants