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

Commit

Permalink
Another fixes in error handling after review.
Browse files Browse the repository at this point in the history
  • Loading branch information
scofalik committed Jul 16, 2019
1 parent 0fa0f51 commit 04fa920
Show file tree
Hide file tree
Showing 4 changed files with 21 additions and 32 deletions.
22 changes: 14 additions & 8 deletions src/model/utils/insertcontent.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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 );
Expand Down Expand Up @@ -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' );
Expand Down
1 change: 1 addition & 0 deletions src/view/observer/selectionobserver.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
19 changes: 6 additions & 13 deletions src/view/view.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,6 @@
* @module engine/view/view
*/

/* globals console */

import Document from './document';
import DowncastWriter from './downcastwriter';
import Renderer from './renderer';
Expand All @@ -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';

/**
Expand Down Expand Up @@ -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.' );
}
}
}
Expand Down
11 changes: 0 additions & 11 deletions tests/view/view/view.js
Original file line number Diff line number Diff line change
Expand Up @@ -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', () => {
Expand Down

0 comments on commit 04fa920

Please sign in to comment.