From a58265d1c76c1e2345772861b05770280af13297 Mon Sep 17 00:00:00 2001 From: Maciej Bukowski Date: Wed, 10 Jul 2019 14:38:41 +0200 Subject: [PATCH 1/7] Removed usage of logger. --- src/conversion/downcasthelpers.js | 22 +++++++++------- src/model/documentselection.js | 8 +----- src/model/operation/transform.js | 17 ++++++------- src/model/utils/insertcontent.js | 18 ++++++------- src/view/observer/selectionobserver.js | 15 ++++------- src/view/view.js | 9 ++++--- tests/conversion/downcasthelpers.js | 13 +++++----- tests/model/documentselection.js | 32 ++++++------------------ tests/model/operation/transform.js | 14 ++++------- tests/view/observer/selectionobserver.js | 32 ++++++++++-------------- tests/view/view/view.js | 29 ++++++++++----------- 11 files changed, 86 insertions(+), 123 deletions(-) diff --git a/src/conversion/downcasthelpers.js b/src/conversion/downcasthelpers.js index 68856db23..d77e6c23e 100644 --- a/src/conversion/downcasthelpers.js +++ b/src/conversion/downcasthelpers.js @@ -3,6 +3,14 @@ * For licensing, see LICENSE.md or https://ckeditor.com/legal/ckeditor-oss-license */ +/** + * Contains downcast (model-to-view) converters for {@link module:engine/conversion/downcastdispatcher~DowncastDispatcher}. + * + * @module engine/conversion/downcasthelpers + */ + +/* globals console */ + import ModelRange from '../model/range'; import ModelSelection from '../model/selection'; import ModelElement from '../model/element'; @@ -11,14 +19,8 @@ import ViewAttributeElement from '../view/attributeelement'; import DocumentSelection from '../model/documentselection'; import ConversionHelpers from './conversionhelpers'; -import log from '@ckeditor/ckeditor5-utils/src/log'; import { cloneDeep } from 'lodash-es'; - -/** - * Contains downcast (model-to-view) converters for {@link module:engine/conversion/downcastdispatcher~DowncastDispatcher}. - * - * @module engine/conversion/downcasthelpers - */ +import { attachLinkToDocumentation } from '@ckeditor/ckeditor5-utils/src/ckeditorerror'; /** * Downcast conversion helper functions. @@ -824,8 +826,10 @@ function changeAttribute( attributeCreator ) { * * @error conversion-attribute-to-attribute-on-text */ - log.warn( 'conversion-attribute-to-attribute-on-text: ' + - 'Trying to convert text node\'s attribute with attribute-to-attribute converter.' ); + console.warn( attachLinkToDocumentation( + 'conversion-attribute-to-attribute-on-text: ' + + 'Trying to convert text node\'s attribute with attribute-to-attribute converter.' + ) ); return; } diff --git a/src/model/documentselection.js b/src/model/documentselection.js index 4cbddca10..6cd82e7dd 100644 --- a/src/model/documentselection.js +++ b/src/model/documentselection.js @@ -17,7 +17,6 @@ import TextProxy from './textproxy'; import toMap from '@ckeditor/ckeditor5-utils/src/tomap'; import Collection from '@ckeditor/ckeditor5-utils/src/collection'; import CKEditorError from '@ckeditor/ckeditor5-utils/src/ckeditorerror'; -import log from '@ckeditor/ckeditor5-utils/src/log'; import uid from '@ckeditor/ckeditor5-utils/src/uid'; const storePrefix = 'selection:'; @@ -804,12 +803,7 @@ class LiveSelection extends Selection { this._checkRange( range ); if ( range.root == this._document.graveyard ) { - /** - * Trying to add a Range that is in the graveyard root. Range rejected. - * - * @warning model-selection-range-in-graveyard - */ - log.warn( 'model-selection-range-in-graveyard: Trying to add a Range that is in the graveyard root. Range rejected.' ); + // @if CK_DEBUG // console.warn( 'Trying to add a Range that is in the graveyard root. Range rejected.' ); return; } diff --git a/src/model/operation/transform.js b/src/model/operation/transform.js index 55da75f64..695c6f4fb 100644 --- a/src/model/operation/transform.js +++ b/src/model/operation/transform.js @@ -16,7 +16,6 @@ import Range from '../range'; import Position from '../position'; import compareArrays from '@ckeditor/ckeditor5-utils/src/comparearrays'; -import log from '@ckeditor/ckeditor5-utils/src/log'; const transformations = new Map(); @@ -102,14 +101,14 @@ export function transform( a, b, context = {} ) { return transformationFunction( a, b, context ); } catch ( e ) { - log.error( 'Error during operation transformation!', e.message ); - log.error( 'Transformed operation', a ); - log.error( 'Operation transformed by', b ); - log.error( 'context.aIsStrong', context.aIsStrong ); - log.error( 'context.aWasUndone', context.aWasUndone ); - log.error( 'context.bWasUndone', context.bWasUndone ); - log.error( 'context.abRelation', context.abRelation ); - log.error( 'context.baRelation', context.baRelation ); + // @if CK_DEBUG // console.error( 'Error during operation transformation!', e.message ); + // @if CK_DEBUG // console.error( 'Transformed operation', a ); + // @if CK_DEBUG // console.error( 'Operation transformed by', b ); + // @if CK_DEBUG // console.error( 'context.aIsStrong', context.aIsStrong ); + // @if CK_DEBUG // console.error( 'context.aWasUndone', context.aWasUndone ); + // @if CK_DEBUG // console.error( 'context.bWasUndone', context.bWasUndone ); + // @if CK_DEBUG // console.error( 'context.abRelation', context.abRelation ); + // @if CK_DEBUG // console.error( 'context.baRelation', context.baRelation ); throw e; } diff --git a/src/model/utils/insertcontent.js b/src/model/utils/insertcontent.js index cc81970d4..53c56a7c8 100644 --- a/src/model/utils/insertcontent.js +++ b/src/model/utils/insertcontent.js @@ -7,13 +7,15 @@ * @module engine/model/utils/insertcontent */ +/* globals console */ + import Position from '../position'; import LivePosition from '../liveposition'; import Element from '../element'; import Range from '../range'; -import log from '@ckeditor/ckeditor5-utils/src/log'; import DocumentSelection from '../documentselection'; import Selection from '../selection'; +import { attachLinkToDocumentation } from '@ckeditor/ckeditor5-utils/src/ckeditorerror'; /** * Inserts content into the editor (specified selection) as one would expect the paste @@ -85,13 +87,7 @@ export default function insertContent( model, content, selectable, placeOrOffset } else { // We are not testing else because it's a safe check for unpredictable edge cases: // an insertion without proper range to select. - - /** - * Cannot determine a proper selection range after insertion. - * - * @warning insertcontent-no-range - */ - log.warn( 'insertcontent-no-range: Cannot determine a proper selection range after insertion.' ); + // @if CK_DEBUG // console.warn( 'insertcontent-no-range: Cannot determine a proper selection range after insertion.' ); } const affectedRange = insertion.getAffectedRange() || model.createRange( insertionPosition ); @@ -322,8 +318,8 @@ class Insertion { if ( !this.schema.checkChild( this.position, node ) ) { // Algorithm's correctness check. We should never end up here but it's good to know that we did. // Note that it would often be a silent issue if we insert node in a place where it's not allowed. - log.error( - 'insertcontent-wrong-position: The node cannot be inserted on the given position.', + console.error( + attachLinkToDocumentation( 'insertcontent-wrong-position: The node cannot be inserted on the given position.' ), { node, position: this.position } ); @@ -442,7 +438,7 @@ class Insertion { // Algorithm's correctness check. We should never end up here but it's good to know that we did. // At this point the insertion position should be after the node we'll merge. If it isn't, // it should need to be secured as in the left merge case. - log.error( 'insertcontent-wrong-position-on-merge: The insertion position should equal the merge position' ); + // @if CK_DEBUG // console.error( 'The insertion position should equal the merge position' ); } // Move the position to the previous node, so it isn't moved to the graveyard on merge. diff --git a/src/view/observer/selectionobserver.js b/src/view/observer/selectionobserver.js index 31b48c7bf..e9604a570 100644 --- a/src/view/observer/selectionobserver.js +++ b/src/view/observer/selectionobserver.js @@ -11,7 +11,6 @@ import Observer from './observer'; import MutationObserver from './mutationobserver'; -import log from '@ckeditor/ckeditor5-utils/src/log'; import { debounce } from 'lodash-es'; /** @@ -150,15 +149,11 @@ export default class SelectionObserver extends Observer { // This counter is reset each second. 60 selection changes in 1 second is enough high number // to be very difficult (impossible) to achieve using just keyboard keys (during normal editor use). if ( ++this._loopbackCounter > 60 ) { - /** - * Selection change observer detected an infinite rendering loop. - * Most probably you try to put the selection in the position which is not allowed - * by the browser and browser fixes it automatically what causes `selectionchange` event on - * which a loopback through a model tries to re-render the wrong selection and again. - * - * @error selectionchange-infinite-loop - */ - log.warn( 'selectionchange-infinite-loop: Selection change observer detected an infinite rendering loop.' ); + // Selection change observer detected an infinite rendering loop. + // Most probably you try to put the selection in the position which is not allowed + // by the browser and browser fixes it automatically what causes `selectionchange` event on + // which a loopback through a model tries to re-render the wrong selection and again. + // @if CK_DEBUG // console.warn( 'Selection change observer detected an infinite rendering loop.' ); return; } diff --git a/src/view/view.js b/src/view/view.js index c2abf2a47..95a7ded6a 100644 --- a/src/view/view.js +++ b/src/view/view.js @@ -7,6 +7,8 @@ * @module engine/view/view */ +/* globals console */ + import Document from './document'; import DowncastWriter from './downcastwriter'; import Renderer from './renderer'; @@ -24,12 +26,11 @@ import CompositionObserver from './observer/compositionobserver'; import InputObserver from './observer/inputobserver'; import ObservableMixin from '@ckeditor/ckeditor5-utils/src/observablemixin'; -import log from '@ckeditor/ckeditor5-utils/src/log'; import mix from '@ckeditor/ckeditor5-utils/src/mix'; import { scrollViewportToShowTarget } from '@ckeditor/ckeditor5-utils/src/dom/scroll'; import { injectUiElementHandling } from './uielement'; import { injectQuirksHandling } from './filler'; -import CKEditorError from '@ckeditor/ckeditor5-utils/src/ckeditorerror'; +import CKEditorError, { attachLinkToDocumentation } from '@ckeditor/ckeditor5-utils/src/ckeditorerror'; import env from '@ckeditor/ckeditor5-utils/src/env'; /** @@ -405,7 +406,9 @@ export default class View { * * @error view-focus-no-selection */ - log.warn( 'view-focus-no-selection: There is no selection in any editable to focus.' ); + console.warn( attachLinkToDocumentation( + 'view-focus-no-selection: There is no selection in any editable to focus.' + ) ); } } } diff --git a/tests/conversion/downcasthelpers.js b/tests/conversion/downcasthelpers.js index 1687144e6..9a4a04985 100644 --- a/tests/conversion/downcasthelpers.js +++ b/tests/conversion/downcasthelpers.js @@ -3,6 +3,8 @@ * For licensing, see LICENSE.md or https://ckeditor.com/legal/ckeditor-oss-license */ +/* globals console */ + import EditingController from '../../src/controller/editingcontroller'; import Model from '../../src/model/model'; @@ -15,7 +17,6 @@ import ViewContainerElement from '../../src/view/containerelement'; import ViewUIElement from '../../src/view/uielement'; import ViewText from '../../src/view/text'; -import log from '@ckeditor/ckeditor5-utils/src/log'; import testUtils from '@ckeditor/ckeditor5-core/tests/_utils/utils'; import DowncastHelpers, { @@ -614,7 +615,7 @@ describe( 'DowncastHelpers', () => { // #1587 it( 'config.view and config.model as strings in generic conversion (elements only)', () => { - const logStub = testUtils.sinon.stub( log, 'warn' ); + const consoleWarnStub = testUtils.sinon.stub( console, 'warn' ); downcastHelpers.elementToElement( { model: 'paragraph', view: 'p' } ); @@ -626,7 +627,7 @@ describe( 'DowncastHelpers', () => { } ); expectResult( '

' ); - expect( logStub.callCount ).to.equal( 0 ); + expect( consoleWarnStub.callCount ).to.equal( 0 ); model.change( writer => { writer.removeAttribute( 'test', modelRoot.getChild( 1 ) ); @@ -637,7 +638,7 @@ describe( 'DowncastHelpers', () => { // #1587 it( 'config.view and config.model as strings in generic conversion (elements + text)', () => { - const logStub = testUtils.sinon.stub( log, 'warn' ); + const consoleWarnStub = testUtils.sinon.stub( console, 'warn' ); downcastHelpers.elementToElement( { model: 'paragraph', view: 'p' } ); @@ -652,8 +653,8 @@ describe( 'DowncastHelpers', () => { } ); expectResult( '

Foo

Bar

' ); - expect( logStub.callCount ).to.equal( 2 ); - expect( logStub.alwaysCalledWithMatch( 'conversion-attribute-to-attribute-on-text' ) ).to.true; + expect( consoleWarnStub.callCount ).to.equal( 2 ); + expect( consoleWarnStub.alwaysCalledWithMatch( 'conversion-attribute-to-attribute-on-text' ) ).to.true; model.change( writer => { writer.removeAttribute( 'test', modelRoot.getChild( 1 ) ); diff --git a/tests/model/documentselection.js b/tests/model/documentselection.js index b25cd66a8..b73cb4912 100644 --- a/tests/model/documentselection.js +++ b/tests/model/documentselection.js @@ -17,15 +17,12 @@ import AttributeOperation from '../../src/model/operation/attributeoperation'; import SplitOperation from '../../src/model/operation/splitoperation'; import Collection from '@ckeditor/ckeditor5-utils/src/collection'; import count from '@ckeditor/ckeditor5-utils/src/count'; -import testUtils from '@ckeditor/ckeditor5-core/tests/_utils/utils'; import { setData, getData } from '../../src/dev-utils/model'; import { expectToThrowCKEditorError } from '@ckeditor/ckeditor5-utils/tests/_utils/utils'; describe( 'DocumentSelection', () => { let model, doc, root, selection, liveRange, range; - testUtils.createSinonSandbox(); - const fooStoreAttrKey = DocumentSelection._getStoreAttributeKey( 'foo' ); const abcStoreAttrKey = DocumentSelection._getStoreAttributeKey( 'abc' ); @@ -54,6 +51,7 @@ describe( 'DocumentSelection', () => { } ); afterEach( () => { + sinon.restore(); model.destroy(); liveRange.detach(); } ); @@ -457,9 +455,6 @@ describe( 'DocumentSelection', () => { expect( ranges[ 0 ].detach.called ).to.be.true; expect( ranges[ 1 ].detach.called ).to.be.true; - - ranges[ 0 ].detach.restore(); - ranges[ 1 ].detach.restore(); } ); } ); @@ -512,7 +507,7 @@ describe( 'DocumentSelection', () => { it( 'detaches all existing ranges', () => { selection._setTo( [ range, liveRange ] ); - const spy = testUtils.sinon.spy( LiveRange.prototype, 'detach' ); + const spy = sinon.spy( LiveRange.prototype, 'detach' ); selection._setTo( root, 0 ); expect( spy.calledTwice ).to.be.true; @@ -534,7 +529,7 @@ describe( 'DocumentSelection', () => { const startPos = Position._createAt( root, 1 ); const endPos = Position._createAt( root, 2 ); const newEndPos = Position._createAt( root, 4 ); - const spy = testUtils.sinon.spy( LiveRange.prototype, 'detach' ); + const spy = sinon.spy( LiveRange.prototype, 'detach' ); selection._setTo( new Range( startPos, endPos ) ); @@ -569,11 +564,6 @@ describe( 'DocumentSelection', () => { selection._setTo( null ); } ); - afterEach( () => { - ranges[ 0 ].detach.restore(); - ranges[ 1 ].detach.restore(); - } ); - it( 'should remove all stored ranges (and reset to default range)', () => { expect( Array.from( selection.getRanges() ).length ).to.equal( 1 ); expect( selection.anchor.isEqual( new Position( root, [ 0, 0 ] ) ) ).to.be.true; @@ -601,19 +591,13 @@ describe( 'DocumentSelection', () => { }, /model-selection-set-ranges-not-range/, model ); } ); - it( 'should log a warning when trying to set selection to the graveyard', () => { - // eslint-disable-next-line no-undef - const warnStub = sinon.stub( console, 'warn' ); - - const range = new Range( new Position( model.document.graveyard, [ 0 ] ) ); - selection._setTo( range ); - - expect( warnStub.calledOnce ).to.equal( true ); - expect( warnStub.getCall( 0 ).args[ 0 ] ).to.match( /model-selection-range-in-graveyard/ ); + it( 'should not do nothing when trying to set selection to the graveyard', () => { + expect( () => { + const range = new Range( new Position( model.document.graveyard, [ 0 ] ) ); + selection._setTo( range ); + } ).to.not.throw(); expect( selection._ranges ).to.deep.equal( [] ); - - warnStub.restore(); } ); it( 'should detach removed ranges', () => { diff --git a/tests/model/operation/transform.js b/tests/model/operation/transform.js index 118593598..8a309b94e 100644 --- a/tests/model/operation/transform.js +++ b/tests/model/operation/transform.js @@ -19,8 +19,6 @@ import MoveOperation from '../../../src/model/operation/moveoperation'; import RenameOperation from '../../../src/model/operation/renameoperation'; import NoOperation from '../../../src/model/operation/nooperation'; -import log from '@ckeditor/ckeditor5-utils/src/log'; - describe( 'transform', () => { let model, doc, root, op, nodeA, nodeB, expected; @@ -33,6 +31,10 @@ describe( 'transform', () => { nodeB = new Node(); } ); + afterEach( () => { + sinon.restore(); + } ); + function expectOperation( op, params ) { for ( const i in params ) { if ( params.hasOwnProperty( i ) ) { @@ -58,9 +60,7 @@ describe( 'transform', () => { aIsStrong: true }; - it( 'error logging', () => { - const spy = sinon.stub( log, 'error' ); - + it( 'should throw an error when one of operations is invalid', () => { const nodeA = new Node(); const nodeB = new Node(); @@ -81,10 +81,6 @@ describe( 'transform', () => { baRelation: null } ); } ).to.throw(); - - sinon.assert.called( spy ); - - log.error.restore(); } ); describe( 'InsertOperation', () => { diff --git a/tests/view/observer/selectionobserver.js b/tests/view/observer/selectionobserver.js index ae8654e32..d60ee858d 100644 --- a/tests/view/observer/selectionobserver.js +++ b/tests/view/observer/selectionobserver.js @@ -3,24 +3,20 @@ * For licensing, see LICENSE.md or https://ckeditor.com/legal/ckeditor-oss-license */ -/* globals setTimeout, document */ +/* globals setTimeout, document, console */ import ViewRange from '../../../src/view/range'; -import testUtils from '@ckeditor/ckeditor5-core/tests/_utils/utils'; import DocumentSelection from '../../../src/view/documentselection'; import ViewSelection from '../../../src/view/selection'; import View from '../../../src/view/view'; import SelectionObserver from '../../../src/view/observer/selectionobserver'; import FocusObserver from '../../../src/view/observer/focusobserver'; -import log from '@ckeditor/ckeditor5-utils/src/log'; import createViewRoot from '../_utils/createroot'; import { parse } from '../../../src/dev-utils/view'; describe( 'SelectionObserver', () => { let view, viewDocument, viewRoot, selectionObserver, domRoot, domMain, domDocument; - testUtils.createSinonSandbox(); - beforeEach( done => { domDocument = document; domRoot = domDocument.createElement( 'div' ); @@ -56,6 +52,7 @@ describe( 'SelectionObserver', () => { } ); afterEach( () => { + sinon.restore(); domRoot.parentElement.removeChild( domRoot ); view.destroy(); @@ -162,7 +159,7 @@ describe( 'SelectionObserver', () => { changeDomSelection(); } ); - it( 'should warn and not enter infinite loop', () => { + it( 'should not enter infinite loop', () => { let counter = 70; const viewFoo = viewDocument.getRoot().getChild( 0 ).getChild( 0 ); @@ -170,17 +167,14 @@ describe( 'SelectionObserver', () => { writer.setSelection( viewFoo, 0 ); } ); - return new Promise( ( resolve, reject ) => { - testUtils.sinon.stub( log, 'warn' ).callsFake( msg => { - expect( msg ).to.match( /^selectionchange-infinite-loop/ ); + const selectionChangeSpy = sinon.spy(); - resolve(); - } ); + viewDocument.on( 'selectionChange', selectionChangeSpy ); + return new Promise( resolve => { viewDocument.on( 'selectionChangeDone', () => { - if ( !counter ) { - reject( new Error( 'Infinite loop warning was not logged.' ) ); - } + expect( selectionChangeSpy.callCount ).to.equal( 60 ); + resolve(); } ); while ( counter > 0 ) { @@ -193,10 +187,10 @@ describe( 'SelectionObserver', () => { it( 'should not be treated as an infinite loop if selection is changed only few times', done => { const viewFoo = viewDocument.getRoot().getChild( 0 ).getChild( 0 ); viewDocument.selection._setTo( ViewRange._createFromParentsAndOffsets( viewFoo, 0, viewFoo, 0 ) ); - const spy = testUtils.sinon.spy( log, 'warn' ); + const consoleWarnSpy = sinon.spy( console, 'warn' ); viewDocument.on( 'selectionChangeDone', () => { - expect( spy.called ).to.be.false; + expect( consoleWarnSpy.called ).to.be.false; done(); } ); @@ -206,10 +200,10 @@ describe( 'SelectionObserver', () => { } ); it( 'should not be treated as an infinite loop if changes are not often', () => { - const clock = testUtils.sinon.useFakeTimers( { + const clock = sinon.useFakeTimers( { toFake: [ 'setInterval', 'clearInterval' ] } ); - const stub = testUtils.sinon.stub( log, 'warn' ); + const consoleWarnStub = sinon.stub( console, 'warn' ); // We need to recreate SelectionObserver, so it will use mocked setInterval. selectionObserver.disable(); @@ -220,7 +214,7 @@ describe( 'SelectionObserver', () => { return doChanges() .then( doChanges ) .then( () => { - sinon.assert.notCalled( stub ); + sinon.assert.notCalled( consoleWarnStub ); clock.restore(); } ); diff --git a/tests/view/view/view.js b/tests/view/view/view.js index 9b410ad50..66de89bb6 100644 --- a/tests/view/view/view.js +++ b/tests/view/view/view.js @@ -3,7 +3,7 @@ * For licensing, see LICENSE.md or https://ckeditor.com/legal/ckeditor-oss-license */ -/* globals document */ +/* globals document, console */ import View from '../../../src/view/view'; import Observer from '../../../src/view/observer/observer'; @@ -19,13 +19,11 @@ import ViewElement from '../../../src/view/element'; import ViewPosition from '../../../src/view/position'; import ViewSelection from '../../../src/view/selection'; import { isBlockFiller, BR_FILLER } from '../../../src/view/filler'; -import log from '@ckeditor/ckeditor5-utils/src/log'; import count from '@ckeditor/ckeditor5-utils/src/count'; import global from '@ckeditor/ckeditor5-utils/src/dom/global'; import createViewRoot from '../_utils/createroot'; import createElement from '@ckeditor/ckeditor5-utils/src/dom/createelement'; -import testUtils from '@ckeditor/ckeditor5-core/tests/_utils/utils'; import { expectToThrowCKEditorError } from '@ckeditor/ckeditor5-utils/tests/_utils/utils'; import env from '@ckeditor/ckeditor5-utils/src/env'; @@ -33,8 +31,6 @@ describe( 'view', () => { const DEFAULT_OBSERVERS_COUNT = 6; let domRoot, view, viewDocument, ObserverMock, instantiated, enabled, ObserverMockGlobalCount; - testUtils.createSinonSandbox(); - beforeEach( () => { domRoot = createElement( document, 'div', { id: 'editor', @@ -75,6 +71,7 @@ describe( 'view', () => { } ); afterEach( () => { + sinon.restore(); domRoot.remove(); view.destroy(); } ); @@ -366,11 +363,11 @@ describe( 'view', () => { describe( 'scrollToTheSelection()', () => { beforeEach( () => { // Silence the Rect warnings. - testUtils.sinon.stub( log, 'warn' ); + sinon.stub( console, 'warn' ); } ); it( 'does nothing when there are no ranges in the selection', () => { - const stub = testUtils.sinon.stub( global.window, 'scrollTo' ); + const stub = sinon.stub( global.window, 'scrollTo' ); view.scrollToTheSelection(); sinon.assert.notCalled( stub ); @@ -378,7 +375,7 @@ describe( 'view', () => { it( 'scrolls to the first range in selection with an offset', () => { const root = createViewRoot( viewDocument, 'div', 'main' ); - const stub = testUtils.sinon.stub( global.window, 'scrollTo' ); + const stub = sinon.stub( global.window, 'scrollTo' ); const range = ViewRange._createIn( root ); view.attachDomRoot( domRoot ); @@ -449,8 +446,8 @@ describe( 'view', () => { } ); it( 'should focus editable with selection', () => { - const converterFocusSpy = testUtils.sinon.spy( view.domConverter, 'focus' ); - const renderSpy = testUtils.sinon.spy( view, 'forceRender' ); + const converterFocusSpy = sinon.spy( view.domConverter, 'focus' ); + const renderSpy = sinon.spy( view, 'forceRender' ); view.focus(); @@ -466,8 +463,8 @@ describe( 'view', () => { } ); it( 'should not focus if document is already focused', () => { - const converterFocusSpy = testUtils.sinon.spy( view.domConverter, 'focus' ); - const renderSpy = testUtils.sinon.spy( view, 'forceRender' ); + const converterFocusSpy = sinon.spy( view.domConverter, 'focus' ); + const renderSpy = sinon.spy( view, 'forceRender' ); viewDocument.isFocused = true; view.focus(); @@ -476,15 +473,15 @@ describe( 'view', () => { expect( renderSpy.called ).to.be.false; } ); - it( 'should log warning when no selection', () => { - const logSpy = testUtils.sinon.stub( log, 'warn' ); + it( 'should log warning when there is no selection', () => { + const consoleWarnSpy = sinon.stub( console, 'warn' ); view.change( writer => { writer.setSelection( null ); } ); view.focus(); - expect( logSpy.calledOnce ).to.be.true; - expect( logSpy.args[ 0 ][ 0 ] ).to.match( /^view-focus-no-selection/ ); + expect( consoleWarnSpy.calledOnce ).to.be.true; + expect( consoleWarnSpy.args[ 0 ][ 0 ] ).to.match( /^view-focus-no-selection/ ); } ); } ); From e75fcb09a8334c813aabdcf59d3268128ff216ee Mon Sep 17 00:00:00 2001 From: Maciej Bukowski Date: Wed, 10 Jul 2019 15:49:12 +0200 Subject: [PATCH 2/7] Fixed codestyle. --- tests/conversion/conversion.js | 2 +- tests/conversion/downcastdispatcher.js | 4 ++-- tests/model/document.js | 4 ++-- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/tests/conversion/conversion.js b/tests/conversion/conversion.js index 48912a436..e17ce29b2 100644 --- a/tests/conversion/conversion.js +++ b/tests/conversion/conversion.js @@ -81,7 +81,7 @@ describe( 'Conversion', () => { it( 'should be chainable', () => { const helpers = conversion.for( 'upcast' ); - const addResult = helpers.add( () => { } ); + const addResult = helpers.add( () => {} ); expect( addResult ).to.equal( helpers ); } ); diff --git a/tests/conversion/downcastdispatcher.js b/tests/conversion/downcastdispatcher.js index a2c7054f0..890aacffb 100644 --- a/tests/conversion/downcastdispatcher.js +++ b/tests/conversion/downcastdispatcher.js @@ -392,8 +392,8 @@ describe( 'DowncastDispatcher', () => { const viewFigure = new ViewContainerElement( 'figure', null, viewCaption ); // Create custom highlight handler mock. - viewFigure._setCustomProperty( 'addHighlight', () => { } ); - viewFigure._setCustomProperty( 'removeHighlight', () => { } ); + viewFigure._setCustomProperty( 'addHighlight', () => {} ); + viewFigure._setCustomProperty( 'removeHighlight', () => {} ); // Create mapper mock. dispatcher.conversionApi.mapper = { diff --git a/tests/model/document.js b/tests/model/document.js index e7c79d71b..61cb699b2 100644 --- a/tests/model/document.js +++ b/tests/model/document.js @@ -44,7 +44,7 @@ describe( 'Document', () => { baseVersion: 0, isDocumentOperation: true, _execute: sinon.stub().returns( data ), - _validate: () => { } + _validate: () => {} }; batch = new Batch(); @@ -82,7 +82,7 @@ describe( 'Document', () => { const operation = { baseVersion: 1, isDocumentOperation: true, - _execute: () => { } + _execute: () => {} }; expectToThrowCKEditorError( () => { From 0fa0f518a5ca1465240d9c90e21c1a6736fe8af9 Mon Sep 17 00:00:00 2001 From: Maciej Bukowski Date: Fri, 12 Jul 2019 12:30:20 +0200 Subject: [PATCH 3/7] Fixed errors/warnings. --- src/conversion/downcasthelpers.js | 13 ++++++------- src/model/utils/insertcontent.js | 10 ++++++++-- tests/conversion/downcasthelpers.js | 27 +++++++++------------------ 3 files changed, 23 insertions(+), 27 deletions(-) diff --git a/src/conversion/downcasthelpers.js b/src/conversion/downcasthelpers.js index d77e6c23e..98d035433 100644 --- a/src/conversion/downcasthelpers.js +++ b/src/conversion/downcasthelpers.js @@ -9,7 +9,7 @@ * @module engine/conversion/downcasthelpers */ -/* globals console */ +/* globals */ import ModelRange from '../model/range'; import ModelSelection from '../model/selection'; @@ -20,7 +20,7 @@ import DocumentSelection from '../model/documentselection'; import ConversionHelpers from './conversionhelpers'; import { cloneDeep } from 'lodash-es'; -import { attachLinkToDocumentation } from '@ckeditor/ckeditor5-utils/src/ckeditorerror'; +import CKEditorError from '@ckeditor/ckeditor5-utils/src/ckeditorerror'; /** * Downcast conversion helper functions. @@ -826,12 +826,11 @@ function changeAttribute( attributeCreator ) { * * @error conversion-attribute-to-attribute-on-text */ - console.warn( attachLinkToDocumentation( + throw new CKEditorError( 'conversion-attribute-to-attribute-on-text: ' + - 'Trying to convert text node\'s attribute with attribute-to-attribute converter.' - ) ); - - return; + 'Trying to convert text node\'s attribute with attribute-to-attribute converter.', + [ data, conversionApi ] + ); } // First remove the old attribute if there was one. diff --git a/src/model/utils/insertcontent.js b/src/model/utils/insertcontent.js index 53c56a7c8..65beaf232 100644 --- a/src/model/utils/insertcontent.js +++ b/src/model/utils/insertcontent.js @@ -15,7 +15,7 @@ import Element from '../element'; import Range from '../range'; import DocumentSelection from '../documentselection'; import Selection from '../selection'; -import { attachLinkToDocumentation } from '@ckeditor/ckeditor5-utils/src/ckeditorerror'; +import CKEditorError, { attachLinkToDocumentation } from '@ckeditor/ckeditor5-utils/src/ckeditorerror'; /** * Inserts content into the editor (specified selection) as one would expect the paste @@ -438,7 +438,13 @@ class Insertion { // Algorithm's correctness check. We should never end up here but it's good to know that we did. // At this point the insertion position should be after the node we'll merge. If it isn't, // it should need to be secured as in the left merge case. - // @if CK_DEBUG // console.error( 'The insertion position should equal the merge position' ); + /** + * An internal error occured during merging insertion content with siblings. + * The insertion position should equal to the merge position. + * + * @error insertcontent-invalid-insertion-position + */ + throw new CKEditorError( 'insertcontent-invalid-insertion-position', this ); } // Move the position to the previous node, so it isn't moved to the graveyard on merge. diff --git a/tests/conversion/downcasthelpers.js b/tests/conversion/downcasthelpers.js index 9a4a04985..97dc763ab 100644 --- a/tests/conversion/downcasthelpers.js +++ b/tests/conversion/downcasthelpers.js @@ -33,6 +33,7 @@ import { stringify as stringifyView } from '../../src/dev-utils/view'; import View from '../../src/view/view'; import createViewRoot from '../view/_utils/createroot'; import { setData as setModelData } from '../../src/dev-utils/model'; +import { expectToThrowCKEditorError } from '@ckeditor/ckeditor5-utils/tests/_utils/utils'; describe( 'DowncastHelpers', () => { let model, modelRoot, viewRoot, downcastHelpers, controller; @@ -638,29 +639,19 @@ describe( 'DowncastHelpers', () => { // #1587 it( 'config.view and config.model as strings in generic conversion (elements + text)', () => { - const consoleWarnStub = testUtils.sinon.stub( console, 'warn' ); - downcastHelpers.elementToElement( { model: 'paragraph', view: 'p' } ); downcastHelpers.attributeToAttribute( { model: 'test', view: 'test' } ); - model.change( writer => { - writer.insertElement( 'paragraph', modelRoot, 0 ); - writer.insertElement( 'paragraph', { test: '1' }, modelRoot, 1 ); - - writer.insertText( 'Foo', { test: '2' }, modelRoot.getChild( 0 ), 0 ); - writer.insertText( 'Bar', { test: '3' }, modelRoot.getChild( 1 ), 0 ); - } ); - - expectResult( '

Foo

Bar

' ); - expect( consoleWarnStub.callCount ).to.equal( 2 ); - expect( consoleWarnStub.alwaysCalledWithMatch( 'conversion-attribute-to-attribute-on-text' ) ).to.true; - - model.change( writer => { - writer.removeAttribute( 'test', modelRoot.getChild( 1 ) ); - } ); + expectToThrowCKEditorError( () => { + model.change( writer => { + writer.insertElement( 'paragraph', modelRoot, 0 ); + writer.insertElement( 'paragraph', { test: '1' }, modelRoot, 1 ); - expectResult( '

Foo

Bar

' ); + writer.insertText( 'Foo', { test: '2' }, modelRoot.getChild( 0 ), 0 ); + writer.insertText( 'Bar', { test: '3' }, modelRoot.getChild( 1 ), 0 ); + } ); + }, /^conversion-attribute-to-attribute-on-text/ ); } ); it( 'should convert attribute insert/change/remove on a model node', () => { From 04fa92058a83d70c7384713cab9f462d2f1ae90f Mon Sep 17 00:00:00 2001 From: Szymon Cofalik Date: Tue, 16 Jul 2019 17:09:00 +0200 Subject: [PATCH 4/7] Another fixes in error handling after review. --- src/model/utils/insertcontent.js | 22 ++++++++++++++-------- src/view/observer/selectionobserver.js | 1 + src/view/view.js | 19 ++++++------------- tests/view/view/view.js | 11 ----------- 4 files changed, 21 insertions(+), 32 deletions(-) diff --git a/src/model/utils/insertcontent.js b/src/model/utils/insertcontent.js index 65beaf232..05aaaf6cb 100644 --- a/src/model/utils/insertcontent.js +++ b/src/model/utils/insertcontent.js @@ -7,15 +7,13 @@ * @module engine/model/utils/insertcontent */ -/* globals console */ - import Position from '../position'; import LivePosition from '../liveposition'; import Element from '../element'; import Range from '../range'; import DocumentSelection from '../documentselection'; import Selection from '../selection'; -import CKEditorError, { attachLinkToDocumentation } from '@ckeditor/ckeditor5-utils/src/ckeditorerror'; +import CKEditorError from '@ckeditor/ckeditor5-utils/src/ckeditorerror'; /** * Inserts content into the editor (specified selection) as one would expect the paste @@ -87,7 +85,8 @@ export default function insertContent( model, content, selectable, placeOrOffset } else { // We are not testing else because it's a safe check for unpredictable edge cases: // an insertion without proper range to select. - // @if CK_DEBUG // console.warn( 'insertcontent-no-range: Cannot determine a proper selection range after insertion.' ); + // + // @if CK_DEBUG // console.warn( 'Cannot determine a proper selection range after insertion.' ); } const affectedRange = insertion.getAffectedRange() || model.createRange( insertionPosition ); @@ -318,12 +317,19 @@ class Insertion { if ( !this.schema.checkChild( this.position, node ) ) { // Algorithm's correctness check. We should never end up here but it's good to know that we did. // Note that it would often be a silent issue if we insert node in a place where it's not allowed. - console.error( - attachLinkToDocumentation( 'insertcontent-wrong-position: The node cannot be inserted on the given position.' ), + + /** + * Given node cannot be inserted on the given position. + * + * @error insertcontent-wrong-position + * @param {module:engine/model/node~Node} node Node to insert. + * @param {module:engine/model/position~Position} position Position to insert the node at. + */ + throw new CKEditorError( + 'insertcontent-wrong-position: Given node cannot be inserted on the given position.', + this, { node, position: this.position } ); - - return; } const livePos = LivePosition.fromPosition( this.position, 'toNext' ); diff --git a/src/view/observer/selectionobserver.js b/src/view/observer/selectionobserver.js index e9604a570..3709da937 100644 --- a/src/view/observer/selectionobserver.js +++ b/src/view/observer/selectionobserver.js @@ -153,6 +153,7 @@ export default class SelectionObserver extends Observer { // Most probably you try to put the selection in the position which is not allowed // by the browser and browser fixes it automatically what causes `selectionchange` event on // which a loopback through a model tries to re-render the wrong selection and again. + // // @if CK_DEBUG // console.warn( 'Selection change observer detected an infinite rendering loop.' ); return; diff --git a/src/view/view.js b/src/view/view.js index 95a7ded6a..6101f5c41 100644 --- a/src/view/view.js +++ b/src/view/view.js @@ -7,8 +7,6 @@ * @module engine/view/view */ -/* globals console */ - import Document from './document'; import DowncastWriter from './downcastwriter'; import Renderer from './renderer'; @@ -30,7 +28,7 @@ import mix from '@ckeditor/ckeditor5-utils/src/mix'; import { scrollViewportToShowTarget } from '@ckeditor/ckeditor5-utils/src/dom/scroll'; import { injectUiElementHandling } from './uielement'; import { injectQuirksHandling } from './filler'; -import CKEditorError, { attachLinkToDocumentation } from '@ckeditor/ckeditor5-utils/src/ckeditorerror'; +import CKEditorError from '@ckeditor/ckeditor5-utils/src/ckeditorerror'; import env from '@ckeditor/ckeditor5-utils/src/env'; /** @@ -399,16 +397,11 @@ export default class View { this.domConverter.focus( editable ); this.forceRender(); } else { - /** - * Before focusing view document, selection should be placed inside one of the view's editables. - * Normally its selection will be converted from model document (which have default selection), but - * when using view document on its own, we need to manually place selection before focusing it. - * - * @error view-focus-no-selection - */ - console.warn( attachLinkToDocumentation( - 'view-focus-no-selection: There is no selection in any editable to focus.' - ) ); + // Before focusing view document, selection should be placed inside one of the view's editables. + // Normally its selection will be converted from model document (which have default selection), but + // when using view document on its own, we need to manually place selection before focusing it. + // + // @if CK_DEBUG // console.warn( 'There is no selection in any editable to focus.' ); } } } diff --git a/tests/view/view/view.js b/tests/view/view/view.js index 66de89bb6..b9647dfba 100644 --- a/tests/view/view/view.js +++ b/tests/view/view/view.js @@ -472,17 +472,6 @@ describe( 'view', () => { expect( converterFocusSpy.called ).to.be.false; expect( renderSpy.called ).to.be.false; } ); - - it( 'should log warning when there is no selection', () => { - const consoleWarnSpy = sinon.stub( console, 'warn' ); - view.change( writer => { - writer.setSelection( null ); - } ); - - view.focus(); - expect( consoleWarnSpy.calledOnce ).to.be.true; - expect( consoleWarnSpy.args[ 0 ][ 0 ] ).to.match( /^view-focus-no-selection/ ); - } ); } ); describe( 'isFocused', () => { From 279edd2394a58297771f21a5bbaf4b2c1f3d767a Mon Sep 17 00:00:00 2001 From: Szymon Cofalik Date: Wed, 17 Jul 2019 16:49:07 +0200 Subject: [PATCH 5/7] Tests: Re-added and changed test that was earlier removed which caused CC drop. --- tests/view/view/view.js | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/tests/view/view/view.js b/tests/view/view/view.js index b9647dfba..d8ccd3aa3 100644 --- a/tests/view/view/view.js +++ b/tests/view/view/view.js @@ -472,6 +472,16 @@ describe( 'view', () => { expect( converterFocusSpy.called ).to.be.false; expect( renderSpy.called ).to.be.false; } ); + + it( 'should not crash when there is no selection', () => { + expect( () => { + view.change( writer => { + writer.setSelection( null ); + } ); + + view.focus(); + } ).not.to.throw(); + } ); } ); describe( 'isFocused', () => { From 677777ba0fc3d01f652a68885cee00048f2aad7f Mon Sep 17 00:00:00 2001 From: Szymon Cofalik Date: Wed, 17 Jul 2019 16:52:10 +0200 Subject: [PATCH 6/7] Update src/conversion/downcasthelpers.js Co-Authored-By: Piotr Jasiun --- src/conversion/downcasthelpers.js | 1 - 1 file changed, 1 deletion(-) diff --git a/src/conversion/downcasthelpers.js b/src/conversion/downcasthelpers.js index 4147e8c0e..e9a68529e 100644 --- a/src/conversion/downcasthelpers.js +++ b/src/conversion/downcasthelpers.js @@ -9,7 +9,6 @@ * @module engine/conversion/downcasthelpers */ -/* globals */ import ModelRange from '../model/range'; import ModelSelection from '../model/selection'; From f0278d262253e0e9bdbeb2af535d0553a499ffa8 Mon Sep 17 00:00:00 2001 From: Szymon Cofalik Date: Wed, 17 Jul 2019 16:52:32 +0200 Subject: [PATCH 7/7] Update src/conversion/downcasthelpers.js --- src/conversion/downcasthelpers.js | 1 - 1 file changed, 1 deletion(-) diff --git a/src/conversion/downcasthelpers.js b/src/conversion/downcasthelpers.js index e9a68529e..3343d8e31 100644 --- a/src/conversion/downcasthelpers.js +++ b/src/conversion/downcasthelpers.js @@ -9,7 +9,6 @@ * @module engine/conversion/downcasthelpers */ - import ModelRange from '../model/range'; import ModelSelection from '../model/selection'; import ModelElement from '../model/element';