From e8b1e50fd4d189815417e4f48f91a4c04f88287b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotrek=20Koszuli=C5=84ski?= Date: Thu, 20 Jul 2017 14:08:30 +0200 Subject: [PATCH 1/2] Filter out mutations which undid themselves. (and result with a before==after change). --- src/view/observer/mutationobserver.js | 26 ++++++++++++++++---------- 1 file changed, 16 insertions(+), 10 deletions(-) diff --git a/src/view/observer/mutationobserver.js b/src/view/observer/mutationobserver.js index a00c115dd..bad9c694c 100644 --- a/src/view/observer/mutationobserver.js +++ b/src/view/observer/mutationobserver.js @@ -12,6 +12,7 @@ import Observer from './observer'; import ViewSelection from '../selection'; import { startsWithFiller, getDataWithoutFiller } from '../filler'; +import isEqual from '@ckeditor/ckeditor5-utils/src/lib/lodash/isEqual'; /** * Mutation observer class observes changes in the DOM, fires {@link module:engine/view/document~Document#event:mutations} event, mark view @@ -204,16 +205,21 @@ export default class MutationObserver extends Observer { for ( const viewElement of mutatedElements ) { const domElement = domConverter.mapViewToDom( viewElement ); - const viewChildren = viewElement.getChildren(); - const newViewChildren = domConverter.domChildrenToView( domElement ); - - this.renderer.markToSync( 'children', viewElement ); - viewMutations.push( { - type: 'children', - oldChildren: Array.from( viewChildren ), - newChildren: Array.from( newViewChildren ), - node: viewElement - } ); + const viewChildren = Array.from( viewElement.getChildren() ); + const newViewChildren = Array.from( domConverter.domChildrenToView( domElement ) ); + + // It may happen that as a result of many changes (sth was inserted and then removed), + // both elements haven't really changed. #1031 + if ( !isEqual( viewChildren, newViewChildren ) ) { + this.renderer.markToSync( 'children', viewElement ); + + viewMutations.push( { + type: 'children', + oldChildren: viewChildren, + newChildren: newViewChildren, + node: viewElement + } ); + } } // Retrieve `domSelection` using `ownerDocument` of one of mutated nodes. From 852c390902ddb6962b15363127ddcde6df490f2f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotrek=20Koszuli=C5=84ski?= Date: Sun, 23 Jul 2017 22:47:01 +0200 Subject: [PATCH 2/2] Added tests and fixed text nodes comparison. --- src/view/observer/mutationobserver.js | 23 +++++++++++++++-- tests/view/observer/mutationobserver.js | 34 +++++++++++++++++++++++++ 2 files changed, 55 insertions(+), 2 deletions(-) diff --git a/src/view/observer/mutationobserver.js b/src/view/observer/mutationobserver.js index bad9c694c..d5b70eccf 100644 --- a/src/view/observer/mutationobserver.js +++ b/src/view/observer/mutationobserver.js @@ -12,7 +12,7 @@ import Observer from './observer'; import ViewSelection from '../selection'; import { startsWithFiller, getDataWithoutFiller } from '../filler'; -import isEqual from '@ckeditor/ckeditor5-utils/src/lib/lodash/isEqual'; +import isEqualWith from '@ckeditor/ckeditor5-utils/src/lib/lodash/isEqualWith'; /** * Mutation observer class observes changes in the DOM, fires {@link module:engine/view/document~Document#event:mutations} event, mark view @@ -210,7 +210,7 @@ export default class MutationObserver extends Observer { // It may happen that as a result of many changes (sth was inserted and then removed), // both elements haven't really changed. #1031 - if ( !isEqual( viewChildren, newViewChildren ) ) { + if ( !isEqualWith( viewChildren, newViewChildren, sameNodes ) ) { this.renderer.markToSync( 'children', viewElement ); viewMutations.push( { @@ -250,6 +250,25 @@ export default class MutationObserver extends Observer { // If nothing changes on `mutations` event, at this point we have "dirty DOM" (changed) and de-synched // view (which has not been changed). In order to "reset DOM" we render the view again. this.document.render(); + + function sameNodes( child1, child2 ) { + // First level of comparison (array of children vs array of children) – use the Lodash's default behavior. + if ( Array.isArray( child1 ) ) { + return; + } + + // Elements. + if ( child1 === child2 ) { + return true; + } + // Texts. + else if ( child1.is( 'text' ) && child2.is( 'text' ) ) { + return child1.data === child2.data; + } + + // Not matching types. + return false; + } } /** diff --git a/tests/view/observer/mutationobserver.js b/tests/view/observer/mutationobserver.js index cec8c271d..18f2a3a97 100644 --- a/tests/view/observer/mutationobserver.js +++ b/tests/view/observer/mutationobserver.js @@ -291,6 +291,40 @@ describe( 'MutationObserver', () => { expect( lastMutations[ 0 ].newText ).to.equal( 'foo ' ); } ); + it( 'should ignore child mutations which resulted in no changes – when element contains elements', () => { + viewRoot.appendChildren( parse( '' ) ); + + viewDocument.render(); + + const domP = domEditor.childNodes[ 2 ]; + const domY = document.createElement( 'y' ); + domP.appendChild( domY ); + domY.remove(); + + mutationObserver.flush(); + + expect( lastMutations.length ).to.equal( 0 ); + } ); + + // This case is more tricky than the previous one because DOMConverter will return a different + // instances of view text nodes every time it converts a DOM text node. + it( 'should ignore child mutations which resulted in no changes – when element contains text nodes', () => { + const domP = domEditor.childNodes[ 0 ]; + const domText = document.createTextNode( 'x' ); + domP.appendChild( domText ); + domText.remove(); + + const domP2 = domEditor.childNodes[ 1 ]; + domP2.appendChild( document.createTextNode( 'x' ) ); + + mutationObserver.flush(); + + // There was onlu P2 change. P1 must be ignored. + const viewP2 = viewRoot.getChild( 1 ); + expect( lastMutations.length ).to.equal( 1 ); + expect( lastMutations[ 0 ].node ).to.equal( viewP2 ); + } ); + it( 'should not ignore mutation with br inserted not on the end of the paragraph', () => { viewRoot.appendChildren( parse( 'foo' ) );