diff --git a/packages/ckeditor5-engine/src/model/position.js b/packages/ckeditor5-engine/src/model/position.js index da3e0a0131e..795d6d12e20 100644 --- a/packages/ckeditor5-engine/src/model/position.js +++ b/packages/ckeditor5-engine/src/model/position.js @@ -218,7 +218,7 @@ export default class Position { * @type {module:engine/model/text~Text|null} */ get textNode() { - return getTextNode( this, this.parent ); + return getTextNodeAtPosition( this, this.parent ); } /** @@ -228,16 +228,10 @@ export default class Position { * @type {module:engine/model/node~Node|null} */ get nodeAfter() { - // Cache parent and reuse for performance reasons. This also means that we cannot use the #index property. - // See #6579. + // Cache the parent and reuse for performance reasons. See #6579 and #6582. const parent = this.parent; - const textNode = getTextNode( this, parent ); - if ( textNode !== null ) { - return null; - } - - return parent.getChild( parent.offsetToIndex( this.offset ) ); + return getNodeAfterPosition( this, parent, getTextNodeAtPosition( this, parent ) ); } /** @@ -247,16 +241,10 @@ export default class Position { * @type {module:engine/model/node~Node|null} */ get nodeBefore() { - // Cache parent and reuse for performance reasons. This also means that we cannot use the #index property. - // See #6579. + // Cache the parent and reuse for performance reasons. See #6579 and #6582. const parent = this.parent; - const textNode = getTextNode( this, parent ); - if ( textNode !== null ) { - return null; - } - - return parent.getChild( parent.offsetToIndex( this.offset ) - 1 ); + return getNodeBeforePosition( this, parent, getTextNodeAtPosition( this, parent ) ); } /** @@ -1078,10 +1066,28 @@ export default class Position { * @typedef {String} module:engine/model/position~PositionStickiness */ -// Helper function used to inline text node access by using a cached parent. -// Reduces the access to the Position#parent property 3 times (in total, when taken into account what #nodeAfter and #nodeBefore do). -// See #6579. -function getTextNode( position, positionParent ) { +/** + * Returns a text node at the given position. + * + * This is a helper function optimized to reuse the position parent instance for performance reasons. + * + * Normally, you should use {@link module:engine/model/position~Position#textNode `Position#textNode`}. + * If you start hitting performance issues with {@link module:engine/model/position~Position#parent `Position#parent`} + * check if your algorithm does not access it multiple times (which can happen directly or indirectly via other position properties). + * + * See https://github.com/ckeditor/ckeditor5/issues/6579. + * + * See also: + * + * * {@link module:engine/model/position~getNodeAfterPosition} + * * {@link module:engine/model/position~getNodeBeforePosition} + * + * @param {module:engine/model/position~Position} position + * @param {module:engine/model/element~Element|module:engine/model/documentfragment~DocumentFragment} positionParent The parent of the + * given position. + * @returns {module:engine/model/text~Text|null} + */ +export function getTextNodeAtPosition( position, positionParent ) { const node = positionParent.getChild( positionParent.offsetToIndex( position.offset ) ); if ( node && node.is( 'text' ) && node.startOffset < position.offset ) { @@ -1090,3 +1096,60 @@ function getTextNode( position, positionParent ) { return null; } + +/** + * Returns the node after the given position. + * + * This is a helper function optimized to reuse the position parent instance and the calculation of the text node at the + * specific position for performance reasons. + * + * Normally, you should use {@link module:engine/model/position~Position#nodeAfter `Position#nodeAfter`}. + * If you start hitting performance issues with {@link module:engine/model/position~Position#parent `Position#parent`} and/or + * {@link module:engine/model/position~Position#textNode `Position#textNode`} + * check if your algorithm does not access those properties multiple times + * (which can happen directly or indirectly via other position properties). + * + * See https://github.com/ckeditor/ckeditor5/issues/6579 and https://github.com/ckeditor/ckeditor5/issues/6582. + * + * See also: + * + * * {@link module:engine/model/position~getTextNodeAtPosition} + * * {@link module:engine/model/position~getNodeBeforePosition} + * + * @param {module:engine/model/position~Position} position + * @param {module:engine/model/element~Element|module:engine/model/documentfragment~DocumentFragment} positionParent The parent of the + * given position. + * @param {module:engine/model/text~Text|null} textNode Text node at the given position. + * @returns {module:engine/model/node~Node|null} + */ +export function getNodeAfterPosition( position, positionParent, textNode ) { + if ( textNode !== null ) { + return null; + } + + return positionParent.getChild( positionParent.offsetToIndex( position.offset ) ); +} + +/** + * Returns the node before the given position. + * + * Refer to {@link module:engine/model/position~getNodeBeforePosition} for documentation on when to use this util method. + * + * See also: + * + * * {@link module:engine/model/position~getTextNodeAtPosition} + * * {@link module:engine/model/position~getNodeAfterPosition} + * + * @param {module:engine/model/position~Position} position + * @param {module:engine/model/element~Element|module:engine/model/documentfragment~DocumentFragment} positionParent The parent of the + * given position. + * @param {module:engine/model/text~Text|null} textNode Text node at the given position. + * @returns {module:engine/model/node~Node|null} + */ +export function getNodeBeforePosition( position, positionParent, textNode ) { + if ( textNode !== null ) { + return null; + } + + return positionParent.getChild( positionParent.offsetToIndex( position.offset ) - 1 ); +} diff --git a/packages/ckeditor5-engine/src/model/treewalker.js b/packages/ckeditor5-engine/src/model/treewalker.js index f70da7700ad..bcf0b52884b 100644 --- a/packages/ckeditor5-engine/src/model/treewalker.js +++ b/packages/ckeditor5-engine/src/model/treewalker.js @@ -10,7 +10,12 @@ import Text from './text'; import TextProxy from './textproxy'; import Element from './element'; -import Position from './position'; +import { + default as Position, + getTextNodeAtPosition, + getNodeAfterPosition, + getNodeBeforePosition +} from './position'; import CKEditorError from '@ckeditor/ckeditor5-utils/src/ckeditorerror'; /** @@ -225,7 +230,11 @@ export default class TreeWalker { return { done: true }; } - const node = position.textNode ? position.textNode : position.nodeAfter; + // Get node just after the current position. + // Use a highly optimized version instead of checking the text node first and then getting the node after. See #6582. + const positionParent = position.parent; + const textNodeAtPosition = getTextNodeAtPosition( position, positionParent ); + const node = textNodeAtPosition ? textNodeAtPosition : getNodeAfterPosition( position, positionParent, textNodeAtPosition ); if ( node instanceof Element ) { if ( !this.shallow ) { @@ -299,8 +308,11 @@ export default class TreeWalker { return { done: true }; } - // Get node just before current position - const node = position.textNode ? position.textNode : position.nodeBefore; + // Get node just before the current position. + // Use a highly optimized version instead of checking the text node first and then getting the node before. See #6582. + const positionParent = position.parent; + const textNodeAtPosition = getTextNodeAtPosition( position, positionParent ); + const node = textNodeAtPosition ? textNodeAtPosition : getNodeBeforePosition( position, positionParent, textNodeAtPosition ); if ( node instanceof Element ) { position.offset--; diff --git a/packages/ckeditor5-engine/tests/model/position.js b/packages/ckeditor5-engine/tests/model/position.js index 043221db508..2c6d3b6e68e 100644 --- a/packages/ckeditor5-engine/tests/model/position.js +++ b/packages/ckeditor5-engine/tests/model/position.js @@ -8,7 +8,12 @@ import DocumentFragment from '../../src/model/documentfragment'; import Element from '../../src/model/element'; import Text from '../../src/model/text'; import TextProxy from '../../src/model/textproxy'; -import Position from '../../src/model/position'; +import { + default as Position, + getTextNodeAtPosition, + getNodeAfterPosition, + getNodeBeforePosition +} from '../../src/model/position'; import Range from '../../src/model/range'; import MarkerOperation from '../../src/model/operation/markeroperation'; import AttributeOperation from '../../src/model/operation/attributeoperation'; @@ -485,10 +490,12 @@ describe( 'Position', () => { } ); } ); - it( 'should have proper parent path', () => { - const position = new Position( root, [ 1, 2, 3 ] ); + describe( 'getParentPath()', () => { + it( 'should have proper parent path', () => { + const position = new Position( root, [ 1, 2, 3 ] ); - expect( position.getParentPath() ).to.deep.equal( [ 1, 2 ] ); + expect( position.getParentPath() ).to.deep.equal( [ 1, 2 ] ); + } ); } ); describe( 'isBefore()', () => { @@ -1275,4 +1282,39 @@ describe( 'Position', () => { expect( positionB.getCommonAncestor( positionA ) ).to.equal( lca ); } } ); + + describe( 'getTextNodeAtPosition() util', () => { + it( 'returns a text node at the given position', () => { + const position = new Position( root, [ 1, 0, 1 ] ); + const positionParent = position.parent; + + expect( getTextNodeAtPosition( position, positionParent ) ).to.equal( foz ); + } ); + + // This util is covered with tests by Position#textNode tests. + } ); + + describe( 'getNodeAfterPosition() util', () => { + it( 'returns a node after the position', () => { + const position = new Position( root, [ 1, 0 ] ); + const positionParent = position.parent; + const textNode = getTextNodeAtPosition( position, positionParent ); + + expect( getNodeAfterPosition( position, positionParent, textNode ) ).to.equal( li1 ); + } ); + + // This util is covered with tests by Position#nodeAfter tests. + } ); + + describe( 'getNodeBeforePosition() util', () => { + it( 'returns a node before the position', () => { + const position = new Position( root, [ 1, 1 ] ); + const positionParent = position.parent; + const textNode = getTextNodeAtPosition( position, positionParent ); + + expect( getNodeBeforePosition( position, positionParent, textNode ) ).to.equal( li1 ); + } ); + + // This util is covered with tests by Position#nodeBefore tests. + } ); } );