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

Introduced deep children text nodes rerendering #1366

Merged
merged 11 commits into from
Mar 28, 2018
Merged

Introduced deep children text nodes rerendering #1366

merged 11 commits into from
Mar 28, 2018

Conversation

f1ames
Copy link
Contributor

@f1ames f1ames commented Mar 16, 2018

Suggested merge commit message (convention)

Other: Introduced deep children text nodes rerendering. Closes ckeditor/ckeditor5#4174. Closes ckeditor/ckeditor5#4156. Closes ckeditor/ckeditor5#3103.


Additional information

The proposed solution is very similar to what was initially proposed by @scofalik - https://github.com/ckeditor/ckeditor5-engine/issues/1125#issuecomment-332791847. The logic marking text nodes is placed in renderer._updateChildren as this is the place where element nodes are modified and based on that modifications we can decide which elements' text nodes should be considered for rerendered.

The mutation observer change is also described here - https://github.com/ckeditor/ckeditor5-engine/issues/1125#issuecomment-373376368.

@coveralls
Copy link

coveralls commented Mar 16, 2018

Coverage Status

Coverage remained the same at 100.0% when pulling c193379 on t/1125 into 679acf6 on master.

@Reinmar
Copy link
Member

Reinmar commented Mar 23, 2018

@Mgsy Could you check this PR? There's a wide range of test cases so we should ba careful.

@Reinmar Reinmar requested review from Mgsy and Reinmar March 23, 2018 09:28
@f1ames
Copy link
Contributor Author

f1ames commented Mar 23, 2018

This PR introduces mechanism which forces rerendering of sibling text nodes of the text node which was modified.

One visible issue of previous behaviour is described in #1093 - in general, renderer failed when changes in one text node required rerendering of some of its sibling (e.g. spaces rerendering in siblings). And the introduced change takes care of that. Here renderer._updateText function was not changed so rendering happens in the same manner (text is rerendered only if it different than existing ones). The only difference is that it is executed for more text nodes than before.

The other change which may have some impact is change in rendering order. Previously, text nodes were rerendered first and element nodes after that. Now due to the logic which was applied (rerender children text nodes of synced element nodes), the order was changed so element nodes are rerendered before text nodes. So it may affect any operation which does more complex changes (so updating text/elements or its attributes at the same time) and probably we should look for any regressions in such cases.

The last thing was a change in processing mutations already described in https://github.com/ckeditor/ckeditor5-engine/issues/1125#issuecomment-373376368. Here it affects how mutations are generated (previously view element was passed with its children to mutation event and now it's passed without it). The scenarios which may get affected by this change are some bigger content modifications (when some bigger part of content, especially with deep structure, gets changed or partially updated) and also cases (I'm not sure if there are any of such situations) when editing one element inside editor changes the other one not in the subtree (so not a child but sibling or any not directly structurally connected element).

@Mgsy
Copy link
Member

Mgsy commented Mar 23, 2018

I've spent some time testing this one. It fixes mentioned issues and I haven't noticed any regressions caused by this fix.

@Reinmar
Copy link
Member

Reinmar commented Mar 26, 2018

There's one thing that would make us feel safer – a test which checks rendering one big batch of various type of changes – in text nodes, removing, adding elements in various places on various levels of a tree. The troublesome part is how text updates mix with element updates due to shifts in indexes.

@f1ames
Copy link
Contributor Author

f1ames commented Mar 27, 2018

a test which checks rendering one big batch of various type of changes – in text nodes, removing, adding elements in various places on various levels of a tree...

Makes perfect sense, I will try to create such test so we could check if the change in rendering order had some unexpected effects.

for ( const child of viewElement.getChildren() ) {
this._markDescendantTextToSync( child );
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder when this else path is taken (what else could viewElement be), but CC is 100%.

Copy link
Contributor Author

@f1ames f1ames Mar 28, 2018

Choose a reason for hiding this comment

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

Theoretically it shouldn't be anything else so simple if - else should be sufficient. But checking it in both cases is more future proof.

@f1ames
Copy link
Contributor Author

f1ames commented Mar 28, 2018

@scofalik after merging master one unit test from model (looks irrelevant to the changes - model/document/should call all already processed callbacks again if a callback returned true) started to fail on Travis 🤔 But it seems master itself passes on Travis.

@scofalik
Copy link
Contributor

scofalik commented Mar 28, 2018

Edit: I haven't seen your comment.

For me the PR looks good. I've pushed some slight changes in docs and merged master.

@f1ames
Copy link
Contributor Author

f1ames commented Mar 28, 2018

And I pushed tests @Reinmar requested, so now we just need to figure out why this one test is failing on Travis.

@f1ames
Copy link
Contributor Author

f1ames commented Mar 28, 2018

This test also fails on ckeditor5-engine#master - https://travis-ci.org/ckeditor/ckeditor5-engine/builds/359328562#L2272 so it is not related to this PR.

@scofalik scofalik merged commit 5c6b6b5 into master Mar 28, 2018
@scofalik scofalik deleted the t/1125 branch March 28, 2018 14:47
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
5 participants