Avoid unnecessary DOM structure updates in renderer #1424

f1ames · 2018-06-01T11:33:41Z

Suggested merge commit message (convention)

Fix: Avoid unnecessary DOM structure updates in Renderer. Closes ckeditor/ckeditor5#4347. Closes ckeditor/ckeditor5#4340. Closes ckeditor/ckeditor5#4307. Closes ckeditor/ckeditor5#4296. Closes ckeditor/ckeditor5#4033. Closes ckeditor/ckeditor5#3109. Closes ckeditor/ckeditor5#3084. Closes ckeditor/ckeditor5#4354.

Additional information

See the extensive comment about the implemented solution in https://github.com/ckeditor/ckeditor5-engine/issues/1417#issuecomment-393845235.

There is also one issue which I'm unable to reproduce (from the beginning) so we should definitely ping the person who reported it to recheck after this PR gets merged - ckeditor/ckeditor5#748.

coveralls · 2018-06-01T11:41:24Z

Coverage remained the same at 100.0% when pulling 4beb226 on t/1417 into 3e74554 on master.

Reinmar · 2018-06-04T11:04:09Z

@Mgsy, could you check these changes? I'm mainly interested if there are some regressions. Scenarios to cover are hard to describe... all that require bigger DOM updates. From typing in weird places, to backspace/enter on bigger non-collapsed selections, applying block quote, lists. Collaboration too. Pretty much everything.

Mgsy · 2018-06-04T12:03:49Z

Steps to reproduce

Scenario 1

Put the caret at the beginning of a block.
Press Ctrl + K.
Insert new link.
Put the caret at the beginning of the link.

Scenario 2

Open the article sample.
Put the caret at the beginning of the link.
Press Arrow right.
Repeat step 3.
Press Arrow left.

Current result

The editor crashes. In scenario 2, after step 3, a highlight doesn't activate on the link.

Error

renderer.js:540 Uncaught TypeError: Cannot read property 'attributes' of undefined
    at Renderer._updateAttrs (renderer.js:540)
    at Renderer.render (renderer.js:214)
    at View._render (view.js:391)
    at View.on (view.js:151)
    at View.fire (emittermixin.js:196)
    at View.change (view.js:357)
    at Document.EditingController.listenTo (editingcontroller.js:81)
    at Document.fire (emittermixin.js:196)
    at Model.Document.listenTo (document.js:154)
    at Model.fire (emittermixin.js:196)

GIF

(Scenario 1)
Click.

Mgsy · 2018-06-04T13:03:14Z

Steps to reproduce

Set the following data:

editor.setData( '<ol><li>Item 1<ol><li>Item 2</li></ol></li></ol><p>Paragraph</p><ol><li>Item 3<ol><li>Item 4</li></ol></li></ol>' );

Start the selection in Item 3 and move it through the Paragraph to Item 2.
Click on the ordered list button in the toolbar.

Current result

Item 3 hasn't unwrapped properly and remains inside <ol> element:

GIF

Click.

Mgsy · 2018-06-04T14:37:53Z

Firefox

Steps to reproduce

Open the article sample.
Change your keyboard to Hiragana.
Put the caret inside the link.
Start the composition.
Press Enter to finish the composition.

Current result

The editor crashes.

Error

TypeError: domElement is undefined[Learn More] renderer.js:540
	_updateAttrs renderer.js:540
	render renderer.js:214
	_render view.js:391
	View/< view.js:151
	fire emittermixin.js:196
	change view.js:357
	EditingController/< editingcontroller.js:81
	fire emittermixin.js:196
	Document/< document.js:154
	fire emittermixin.js:196
	_runPendingChanges model.js:405
	enqueueChange model.js:207
	execute inputcommand.js:79
	decorate/< observablemixin.js:250
	fire emittermixin.js:196
	decorate/this[methodName] observablemixin.js:254
	execute commandcollection.js:67
	execute editor.js:254
	_handleTextMutation input.js:293
	handle input.js:152
	_handleMutations input.js:106
	init/< input.js:50
	fire emittermixin.js:196
	_onMutations mutationobserver.js:247
	_onMutations self-hosted:977:17

GIF

Click.

Mgsy · 2018-06-05T08:32:46Z

This solution causes problems with the user's selection and markers in the collaborative editing. Most of times the marker isn't rendered and a colour of the selection is incorrect.

GIF
Click.

Another case:

On Client A put the caret in some word.
Press Arrow right.

Result: The marker becomes invisible on Client B.

f1ames · 2018-06-11T12:26:03Z

So there were 2 main issues (3 with markers handling but I will cover this in a separate comment):

The one with list rendering problem - #1424 (comment) was caused by some logic flaw in a function calculating replace actions. In some cases if no elements in a group were matched as similar (when group contained only insert or delete actions), actions from this group were skipped (instead of being added unchanged to the output). It was fixed by ebfacb0 commit.

The second issues is related to attributes rendering (it covers both #1424 (comment) and #1424 (comment)) and it's also analysed in depth in https://github.com/ckeditor/ckeditor5-engine/issues/1427. In short the problem was caused by updating mappings. If new view element was marked to have its attributes updated (renderer.markedAttributes) and then it was removed from mappings during the renderer._updateChildrenMappings() function call (because similar already existing element was found), the renderer._updateAttrs() function was not able to proceed due to missing mapping (no corresponding DOM element). This was covered in 8147685.

I have also completely removed marked elements sorting. It was one of the first things which were added. From this point the solution evolved significantly and it is no longer needed as renderer accurately diffs whole structure and the order doesn't make a difference here (especially that sorting had been the first thing inside the renderer.render() function and then the renderer._updateChildrenMappings() function was adding more elements so the order was not kept anyway).

f1ames · 2018-06-11T14:09:58Z

The problem with markers was more general issue with handling UIElements. The UIElement is specific due to the fact that its children are not stored in a view (#799). Before rendering optimisation was implemented the whole marker (marker is just a  element) was rerendered as a p element child.
Now as the marker element is treated as replaced element, only its contents were rerendered. Since uiElement child are not present in a view, its expected DOM children were always empty so entire contents of such element were removed during rendering. I think it makes sense just to skip such elements so they are processed as before - covered in c974fbb.

f1ames · 2018-06-11T14:24:04Z

Ready for 2nd review 🎉 cc @Mgsy @Reinmar

Reinmar · 2018-06-12T09:09:46Z

src/view/renderer.js

+			return actions;
+		}
+
+		function areSimilar( domNode1, domNode2 ) {


These helper fns should not be defined in this scope. Every execution of this method creates them.

Reinmar · 2018-06-12T09:13:47Z

src/view/renderer.js

@@ -19,6 +19,7 @@ import ObservableMixin from '@ckeditor/ckeditor5-utils/src/observablemixin';
 import CKEditorError from '@ckeditor/ckeditor5-utils/src/ckeditorerror';
 import isText from '@ckeditor/ckeditor5-utils/src/dom/istext';
 import fastDiff from '@ckeditor/ckeditor5-utils/src/fastdiff';
+import isNode from '@ckeditor/ckeditor5-utils/src/dom/isnode';


Move up, to be right after/before isText().

scofalik · 2018-06-12T11:05:22Z

src/view/renderer.js

+				if ( expectedSlice.length && actualSlice.length ) {
+					newActions = newActions.concat( calculateReplaceActions( actualSlice, expectedSlice ) );
+				} else if ( expectedSlice.length || actualSlice.length ) {
+					newActions = newActions.concat( skipActions );


Is this needed? Wouldn't calculateReplaceActions, or rather diff, return [ 'insert', 'insert', ... ] if actualSlice is empty and [ 'delete', 'delete', ... ] if expectedSlice is empty?

Especially since you don't have this below (in line 713) which is the same situation as meeting equal.

Reinmar · 2018-06-12T12:19:55Z

src/view/renderer.js

+	 * @param {Object} options
+	 * @param {module:engine/view/position~Position} options.inlineFillerPosition The position on which the inline
+	 * filler should be rendered.
+	 * @param {Boolean} options.bind If new view elements should be bind to their corresponding DOM elements.


This is smelly. It violates the single responsibility principle – why does diffSth() bind anything?

I noticed this because I couldn't understand the comment above:

// We do not perform any operations on DOM here so there is no need to bind view element or convert its children.

It didn't have any sense for me in that context because I saw that the line is const diff = this._diffElementChildren().

Reinmar · 2018-06-12T12:20:41Z

src/view/renderer.js

+	 * @returns {Array} result.actualDomChildren Current viewElement DOM children.
+	 * @returns {Array} result.expectedDomChildren Expected viewElement DOM children.
+	 */
+	_diffElementChildren( viewElement, options ) {


_diffChildren() would be enough. Or maybe _diffDomChildren() to indicate that this method works on the DOM (OTOH, "DOM children" doesn't sound good and "DOM element's children" is too much :D).

Reinmar · 2018-06-12T12:30:41Z

One generic thing which I don't like in some of the Render's code is that we have methods which don't use this in any way. In such a case I think it'd be better to make them a simple functions.

The same, in fact, applies to the DomConverter where I carelessly added two new methods while they could be simple fns too.

Reinmar · 2018-06-12T12:33:44Z

src/view/renderer.js

+		if ( diff ) {
+			const actions = this._findReplaceActions( diff.actions, diff.actualDomChildren, diff.expectedDomChildren );
+
+			if ( actions.indexOf( 'replace' ) !== -1 ) {


Make it an early return. Easier to read when there's less nesting.

Reinmar · 2018-06-12T12:34:41Z

src/view/renderer.js

+
+			if ( actions.indexOf( 'replace' ) !== -1 ) {
+				const counter = { equal: 0, insert: 0, delete: 0 };
+				for ( const action of actions ) {


There should be a blank line before for()/if()/while/function. Odd that we don't have this covered in ESLint.

I suppose I wasn't fully aware of that, but I will keep this in mind now 👍

Reinmar · 2018-06-12T13:56:27Z

src/view/renderer.js

+						// The 'uiElement' is a special one and its children are not stored in a view (#799),
+						// so we cannot use it with replacing flow (since it uses view children during rendering
+						// which will always result in rendering empty element).
+						if ( viewChild && !viewChild.is( 'uiElement' ) ) {


This function grew a bit long. So, perhaps contents of this if() would make a reasonable function itself. It only accepts 3 params – the 3 vars that you define above. That's why I found it a good place to split the code a bit.

Reinmar · 2018-06-12T14:10:05Z

src/view/renderer.js

@@ -447,6 +501,15 @@ export default class Renderer {
 	 */
 	_updateAttrs( viewElement ) {
 		const domElement = this.domConverter.mapViewToDom( viewElement );
+
+		if ( !domElement ) {


I'm not sure about this – this function should not be called if the mapping is broken. Can't we clean Renderer#markedAttribiutes earlier, when updating the mappings.

In other words – if you call a function, you expect it to do something. You should be more or less sure it makes sense. Otherwise, this function has two responsibilities – do that thing, but also check if it should do it.

AFAIR the main reason was that when we update bindings of a specific element (it is removed from mappings and also all its children mappings are removed). So if you would like to update markedAttributes during updating bindings you will have to iterate:

through all updated element children to check and remove them from markedAttributes,

or through markedAttributes and check if any elements mappings were removed.

It just seemed simpler to check it here if we anyway iterate through markedAttributes (and this is just an equivalent to 2nd point just in different place). WDYT @Reinmar ?

Reinmar · 2018-06-12T14:15:50Z

src/view/renderer.js

+		const diff = this._diffElementChildren( viewElement,
+			{ inlineFillerPosition: options.inlineFillerPosition, bind: true, withChildren: true } );
+
+		if ( diff ) {


Again – early return.

And – how is it possible that we call this method when its children are up to date?

And – how is it possible that we call this method when its children are up to date?

I'm not sure what you meant here? Do you have any particular case in mind?

I meant the logic of this method – you call updateChildren() based on markedElements. My assumption is – if an element is marked its children have changed. And hence, the diff should not be empty in this case.

The existence of this if() here means that my assumption is wrong. But why? We need to have a clarity on:

Why is this method called for up-to-date element in the first place.

Can we prevent calling it for up-to-date element (so to remove this if()).

Eventually, if it's most convenient to have this if() here and it's really needed (there are no other straightforward solutions), we need a comment.

One thing, even in situations when children are equal we mark descendant text nodes to sync so I'm not sure if we may completely skip such situations (if that's what you mean).

Didn't noticed your comment earlier @Reinmar, so my last comment refers to previous ones.

As for the latest comment - diff is empty not when elements are up-to-date (then you will have diff result like [ equal, equal, ... ]), but when the viewElement which is passed to updateChildren() and then diffChildren() does not have corresponding DOM element (it cannot be mapped which usually means that corresponding DOM element was removed from DOM, but also similar cases as with attributes - #1424 (comment)). This is not a new if() here, as it was there before and it was just moved to diffChildren() method together with comment explaining what is happening - https://github.com/ckeditor/ckeditor5-engine/pull/1424/files#diff-4c87133ed830fc4ea0646426deba32cfR611.

Reinmar · 2018-06-12T14:16:19Z

src/view/renderer.js

+			const expectedDomChildren = diff.expectedDomChildren;
+
+			let i = 0;
+			const nodesToUnbind = new Set();


Blank line missing.

Reinmar · 2018-06-12T14:16:47Z

src/view/renderer.js

+			return null;
+		}
+
+		function sameNodes( actualDomChild, expectedDomChild ) {


Don't define helper functions inside other methods unless really necessary.

Reinmar · 2018-06-12T15:15:22Z

OK, I think we've got enough for now. I haven't dug into the logic because I think it may change a bit still.

…rinciple.

f1ames · 2018-06-13T15:31:04Z

I have covered all proposed changes and explained two more complex cases:

I think it is ready for further review. cc @Reinmar @scofalik

Reinmar · 2018-06-18T09:13:40Z

Crash:

[FooBar]
Apply a link

Reinmar · 2018-06-18T09:53:10Z

I miss a test for the last change. We had a bug. A fix for this bug should be tested.

Reinmar · 2018-06-18T10:40:00Z

I made quite a lot of tests and all seems fine. Checking the tests now and if all's fine I'll be merging this. Congrats! 🌮

f1ames added 6 commits May 29, 2018 17:07

Avoid unnecessary structure updates in renderer with smarter diffing.

c457a1b

Rendering flow adjustments.

37f7452

Tests: optimal rendering unit tests.

450e1ee

Tests: complex DOM modifications rendering unit tests added.

7cbfc8a

Handle Safari 'br' edge case.

9e36530

Docs adjustments.

d050901

f1ames added 5 commits June 7, 2018 18:27

Fix for skipped actions in 'actions replace diff'.

ebfacb0

Tests: skipped actions unit test added.

0cee0a1

Fixed attributes rendering for replaced elements.

8147685

Tests: attributes rendering unit tests.

37017e8

Removed marked elements sorting as it is no longer needed.

86423cb

f1ames added 3 commits June 11, 2018 15:04

Fix for 'uiElements' rendering.

c974fbb

Tests: 'uiElement' rerendering unit test added.

3a4ffd4

Merge branch 'master' into t/1417

907e2ca

Reinmar changed the title ~~Avoid unnecessary structure updates in renderer~~ Avoid unnecessary DOM structure updates in renderer Jun 12, 2018

Reinmar requested review from Reinmar and scofalik June 12, 2018 08:48

Reinmar reviewed Jun 12, 2018

View reviewed changes

Improved API docs and code style.

4d6a100

scofalik reviewed Jun 12, 2018

View reviewed changes

Reinmar added 2 commits June 12, 2018 13:50

Wording.

dd11e72

Wording.

3a8085f

Reinmar reviewed Jun 12, 2018

View reviewed changes

Wording.

e097246

Reinmar added the status:review- label Jun 12, 2018

f1ames added 2 commits June 13, 2018 16:02

Renderer refactoring.

8d3a0d1

Make 'renderer._diffChildren()' method follow single responsibility p…

4beb226

…rinciple.

Reinmar removed the status:review- label Jun 18, 2018

Merge branch 'master' into t/1417

140633f

Bind new DOM elements before diffing.

34a1788

Tests: unit test covering 'linking styled content' case.

7e1e963

Reinmar merged commit 457afde into master Jun 18, 2018

Reinmar deleted the t/1417 branch June 18, 2018 11:14

f1ames mentioned this pull request Jun 22, 2018

Japanese input (IME) fails at the first character in a paragraph ckeditor/ckeditor5#748

Closed

Reinmar mentioned this pull request Nov 7, 2018

Disable inline filling character removal during composition #1355

Closed

Avoid unnecessary DOM structure updates in renderer #1424

Avoid unnecessary DOM structure updates in renderer #1424

Conversation

f1ames commented Jun 1, 2018 • edited by pomek Loading

Suggested merge commit message (convention)

Additional information

coveralls commented Jun 1, 2018 • edited Loading

Reinmar commented Jun 4, 2018 • edited Loading

Mgsy commented Jun 4, 2018 • edited Loading

Steps to reproduce

Current result

Error

GIF

Mgsy commented Jun 4, 2018 • edited Loading

Steps to reproduce

Current result

GIF

Mgsy commented Jun 4, 2018

Firefox

Steps to reproduce

Current result

Error

GIF

Mgsy commented Jun 5, 2018 • edited Loading

f1ames commented Jun 11, 2018

f1ames commented Jun 11, 2018

f1ames commented Jun 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Reinmar Jun 12, 2018 • edited Loading

Choose a reason for hiding this comment

Reinmar commented Jun 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Reinmar Jun 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

f1ames Jun 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Reinmar commented Jun 12, 2018

f1ames commented Jun 13, 2018

Reinmar commented Jun 18, 2018

Reinmar commented Jun 18, 2018

Reinmar commented Jun 18, 2018

f1ames commented Jun 1, 2018 •

edited by pomek

Loading

coveralls commented Jun 1, 2018 •

edited

Loading

Reinmar commented Jun 4, 2018 •

edited

Loading

Mgsy commented Jun 4, 2018 •

edited

Loading

Mgsy commented Jun 4, 2018 •

edited

Loading

Mgsy commented Jun 5, 2018 •

edited

Loading

Reinmar Jun 12, 2018 •

edited

Loading

Reinmar Jun 12, 2018 •

edited

Loading

f1ames Jun 13, 2018 •

edited

Loading