From 1fb2e5088bd77fab7e4501572c73d5001ccaeb73 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oskar=20Wr=C3=B3bel?= Date: Thu, 4 Apr 2019 16:33:35 +0200 Subject: [PATCH 1/2] Fix: Fixed View#render collision when moving focus from a one editable to the other in multi-root editor. --- src/editableui/editableuiview.js | 17 ++++++++++++- tests/editableui/editableuiview.js | 39 ++++++++++++++++++++++++------ 2 files changed, 48 insertions(+), 8 deletions(-) diff --git a/src/editableui/editableuiview.js b/src/editableui/editableuiview.js index 0531445b..a2d303b1 100644 --- a/src/editableui/editableuiview.js +++ b/src/editableui/editableuiview.js @@ -122,7 +122,7 @@ export default class EditableUIView extends View { const editingView = this._editingView; if ( editingView.isRenderingInProgress ) { - editingView.once( 'change:isRenderingInProgress', () => update( this ) ); + updateAfterRender( this ); } else { update( this ); } @@ -135,5 +135,20 @@ export default class EditableUIView extends View { writer.removeClass( view.isFocused ? 'ck-blurred' : 'ck-focused', viewRoot ); } ); } + + // In a case of a multi-root editor, a callback will be attached more than once (one callback for each root). + // While executing one callback the `isRenderingInProgress` observable is changing what causes executing another + // callback and render is called inside the already pending render. + // We need to be sure that callback is executed only when the value has changed from `true` to `false`. + // See https://github.com/ckeditor/ckeditor5/issues/1676. + function updateAfterRender( view ) { + editingView.once( 'change:isRenderingInProgress', ( evt, name, value ) => { + if ( !value ) { + update( view ); + } else { + updateAfterRender( view ); + } + } ); + } } } diff --git a/tests/editableui/editableuiview.js b/tests/editableui/editableuiview.js index 5d15dbf2..36fec6a8 100644 --- a/tests/editableui/editableuiview.js +++ b/tests/editableui/editableuiview.js @@ -80,22 +80,47 @@ describe( 'EditableUIView', () => { } ); // https://github.com/ckeditor/ckeditor5/issues/1530. + // https://github.com/ckeditor/ckeditor5/issues/1676. it( 'should work when update is handled during the rendering phase', () => { + const secondEditingViewRoot = new ViewRootEditableElement( 'div' ); + const secondView = new EditableUIView( locale, editingView ); + const secondEditableElement = document.createElement( 'div' ); + + document.body.appendChild( secondEditableElement ); + + secondEditingViewRoot.rootName = 'second'; + secondEditingViewRoot._document = editingView.document; + editingView.document.roots.add( secondEditingViewRoot ); + + secondView.name = 'second'; + secondView.render(); + + editingView.attachDomRoot( editableElement, 'main' ); + editingView.attachDomRoot( secondEditableElement, 'second' ); + view.isFocused = true; - editingView.isRenderingInProgress = true; + secondView.isFocused = false; - expect( editingViewRoot.hasClass( 'ck-focused' ) ).to.be.true; - expect( editingViewRoot.hasClass( 'ck-blurred' ) ).to.be.false; + expect( editingViewRoot.hasClass( 'ck-focused' ), 1 ).to.be.true; + expect( editingViewRoot.hasClass( 'ck-blurred' ), 2 ).to.be.false; + expect( secondEditingViewRoot.hasClass( 'ck-focused' ), 3 ).to.be.false; + expect( secondEditingViewRoot.hasClass( 'ck-blurred' ), 4 ).to.be.true; + editingView.isRenderingInProgress = true; view.isFocused = false; + secondView.isFocused = true; - expect( editingViewRoot.hasClass( 'ck-focused' ) ).to.be.true; - expect( editingViewRoot.hasClass( 'ck-blurred' ) ).to.be.false; + expect( editingViewRoot.hasClass( 'ck-focused' ), 5 ).to.be.true; + expect( editingViewRoot.hasClass( 'ck-blurred' ), 6 ).to.be.false; + expect( secondEditingViewRoot.hasClass( 'ck-focused' ), 7 ).to.be.false; + expect( secondEditingViewRoot.hasClass( 'ck-blurred' ), 8 ).to.be.true; editingView.isRenderingInProgress = false; - expect( editingViewRoot.hasClass( 'ck-focused' ) ).to.be.false; - expect( editingViewRoot.hasClass( 'ck-blurred' ) ).to.be.true; + expect( editingViewRoot.hasClass( 'ck-focused' ), 9 ).to.be.false; + expect( editingViewRoot.hasClass( 'ck-blurred' ), 10 ).to.be.true; + expect( secondEditingViewRoot.hasClass( 'ck-focused' ), 11 ).to.be.true; + expect( secondEditingViewRoot.hasClass( 'ck-blurred' ), 12 ).to.be.false; } ); } ); } ); From 5f7a35e233f41629ad3c0f0660cd2777b0ce392f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oskar=20Wr=C3=B3bel?= Date: Thu, 4 Apr 2019 18:08:59 +0200 Subject: [PATCH 2/2] Cleaned up in the DOM after test. --- tests/editableui/editableuiview.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/editableui/editableuiview.js b/tests/editableui/editableuiview.js index 36fec6a8..bb551cec 100644 --- a/tests/editableui/editableuiview.js +++ b/tests/editableui/editableuiview.js @@ -121,6 +121,8 @@ describe( 'EditableUIView', () => { expect( editingViewRoot.hasClass( 'ck-blurred' ), 10 ).to.be.true; expect( secondEditingViewRoot.hasClass( 'ck-focused' ), 11 ).to.be.true; expect( secondEditingViewRoot.hasClass( 'ck-blurred' ), 12 ).to.be.false; + + secondEditableElement.remove(); } ); } ); } );