From 24589f0e84c07ee6c3e3d9d6330d7e0b0d5147ad Mon Sep 17 00:00:00 2001 From: Szymon Cofalik Date: Tue, 3 Dec 2024 16:37:29 +0100 Subject: [PATCH 01/15] Other (engine): Introduced dynamic caching in `Mapper` in order to improve view-to-model mapping performance, and as a result improve editor load and data save performance. Unit tests WIP --- .../ckeditor5-engine/src/conversion/mapper.ts | 609 ++++++++++++++++-- .../src/view/documentfragment.ts | 13 +- packages/ckeditor5-engine/src/view/element.ts | 4 +- packages/ckeditor5-engine/src/view/node.ts | 15 +- .../tests/conversion/mapper.js | 324 ++++++++++ 5 files changed, 908 insertions(+), 57 deletions(-) diff --git a/packages/ckeditor5-engine/src/conversion/mapper.ts b/packages/ckeditor5-engine/src/conversion/mapper.ts index 026040ae5c6..39bfdcc19c5 100644 --- a/packages/ckeditor5-engine/src/conversion/mapper.ts +++ b/packages/ckeditor5-engine/src/conversion/mapper.ts @@ -13,10 +13,11 @@ import ModelRange from '../model/range.js'; import ViewPosition from '../view/position.js'; import ViewRange from '../view/range.js'; -import { CKEditorError, EmitterMixin } from '@ckeditor/ckeditor5-utils'; +import { CKEditorError, EmitterMixin, EventInfo } from '@ckeditor/ckeditor5-utils'; import type ViewDocumentFragment from '../view/documentfragment.js'; import type ViewElement from '../view/element.js'; +import type ViewText from '../view/text.js'; import type ModelElement from '../model/element.js'; import type ModelDocumentFragment from '../model/documentfragment.js'; import type ViewNode from '../view/node.js'; @@ -83,6 +84,11 @@ export default class Mapper extends /* #__PURE__ */ EmitterMixin() { */ private _unboundMarkerNames = new Set(); + /** + * Manages dynamic cache for the `Mapper` to improve the performance. + */ + private _cache: MapperCache = new MapperCache(); + /** * Creates an instance of the mapper. */ @@ -172,7 +178,12 @@ export default class Mapper extends /* #__PURE__ */ EmitterMixin() { if ( options.defer ) { this._deferredBindingRemovals.set( viewElement, viewElement.root ); } else { - this._viewToModelMapping.delete( viewElement ); + const wasFound = this._viewToModelMapping.delete( viewElement ); + + if ( wasFound ) { + // Stop tracking after the element is no longer mapped. + this._cache.stopTracking( viewElement ); + } if ( this._modelToViewMapping.get( modelElement ) == viewElement ) { this._modelToViewMapping.delete( modelElement ); @@ -197,7 +208,12 @@ export default class Mapper extends /* #__PURE__ */ EmitterMixin() { this._modelToViewMapping.delete( modelElement ); if ( this._viewToModelMapping.get( viewElement ) == modelElement ) { - this._viewToModelMapping.delete( viewElement ); + const wasFound = this._viewToModelMapping.delete( viewElement ); + + if ( wasFound ) { + // Stop tracking after the element is no longer mapped. + this._cache.stopTracking( viewElement ); + } } } @@ -426,6 +442,10 @@ export default class Mapper extends /* #__PURE__ */ EmitterMixin() { } /** + * **This method is deprecated and will be removed in one of the future CKEditor 5 releases.** + * + * **Using this method will turn off `Mapper` caching system and may degrade performance when operating on bigger documents.** + * * Registers a callback that evaluates the length in the model of a view element with the given name. * * The callback is fired with one argument, which is a view element instance. The callback is expected to return @@ -455,6 +475,7 @@ export default class Mapper extends /* #__PURE__ */ EmitterMixin() { * * @param viewElementName Name of view element for which callback is registered. * @param lengthCallback Function return a length of view element instance in model. + * @deprecated */ public registerViewToModelLength( viewElementName: string, @@ -469,7 +490,7 @@ export default class Mapper extends /* #__PURE__ */ EmitterMixin() { * * @param viewPosition Position for which a mapped ancestor should be found. */ - public findMappedViewAncestor( viewPosition: ViewPosition ): ViewElement { + public findMappedViewAncestor( viewPosition: ViewPosition ): ViewElement | ViewDocumentFragment { let parent: any = viewPosition.parent; while ( !this._viewToModelMapping.has( parent ) ) { @@ -503,7 +524,7 @@ export default class Mapper extends /* #__PURE__ */ EmitterMixin() { private _toModelOffset( viewParent: ViewElement, viewOffset: number, - viewBlock: ViewElement + viewBlock: ViewElement | ViewDocumentFragment ): number { if ( viewBlock != viewParent ) { // See example. @@ -584,64 +605,110 @@ export default class Mapper extends /* #__PURE__ */ EmitterMixin() { } /** - * Finds the position in the view node (or in its children) with the expected model offset. + * Finds the position in a view element or view document fragment node (or in its children) with the expected model offset. * - * Example: + * If the passed `viewContainer` is bound to model, `Mapper` will use caching mechanism to improve performance. * - * ``` - *

fobarbom

-> expected offset: 4 + * @param viewContainer Tree view element in which we are looking for the position. + * @param modelOffset Expected offset. + * @returns Found position. + */ + public findPositionIn( viewContainer: ViewElement | ViewDocumentFragment, modelOffset: number ): ViewPosition { + if ( modelOffset === 0 ) { + // Quickly return if asked for a position at the beginning of the container. No need to fire complex mechanisms to find it. + return this._moveViewPositionToTextNode( new ViewPosition( viewContainer, 0 ) ); + } + + // Use cache only if there are no custom view-to-model length callbacks and only for bound elements. + // View-to-model length callbacks are deprecated and should be removed in one of the following releases. + // Then it will be possible to simplify some logic inside `Mapper`. + // Note: we could consider requiring `viewContainer` to be a mapped item. + const useCache = this._viewToModelLengthCallbacks.size == 0 && this._viewToModelMapping.has( viewContainer ); + + if ( useCache ) { + const cacheItem = this._cache.get( viewContainer, modelOffset ); + + return this._findPositionStartingFrom( cacheItem.viewPosition, cacheItem.modelOffset, modelOffset, viewContainer, true ); + } else { + return this._findPositionStartingFrom( new ViewPosition( viewContainer, 0 ), 0, modelOffset, viewContainer, false ); + } + } + + /** + * Performs most of the logic for `Mapper#findPositionIn()`. * - * findPositionIn( p, 4 ): - *

|fobarbom

-> expected offset: 4, actual offset: 0 - *

fo|barbom

-> expected offset: 4, actual offset: 2 - *

fobar|bom

-> expected offset: 4, actual offset: 5 -> we are too far + * It allows to start looking for the requested model offset from a given starting position, to enable caching. Using the cache, + * you can set the starting point and skip all the calculations that were already previously done. * - * findPositionIn( b, 4 - ( 5 - 3 ) ): - *

fo|barbom

-> expected offset: 2, actual offset: 0 - *

fobar|bom

-> expected offset: 2, actual offset: 3 -> we are too far + * This method uses recursion to find positions inside deep structures. Example: * - * findPositionIn( bar, 2 - ( 3 - 3 ) ): - * We are in the text node so we can simple find the offset. - *

foba|rbom

-> expected offset: 2, actual offset: 2 -> position found * ``` + *

fobarbom

-> target offset: 4 + *

|fobarbom

-> target offset: 4, traversed offset: 0 + *

fo|barbom

-> target offset: 4, traversed offset: 2 + *

fobar|bom

-> target offset: 4, traversed offset: 5 -> we are too far, look recursively in . * - * @param viewParent Tree view element in which we are looking for the position. - * @param expectedOffset Expected offset. - * @returns Found position. + *

fo|barbom

-> target offset: 4, traversed offset: 2 + *

fobar|bom

-> target offset: 4, traversed offset: 5 -> we are too far, look inside "bar". + * + *

foba|rbom

-> target offset: 4, traversed offset: 2 -> position is inside text node at offset 4-2 = 2. + * ``` + * + * @param startViewPosition View position to start looking from. + * @param startModelOffset Model offset related to `startViewPosition`. + * @param targetModelOffset Target model offset to find. + * @param viewContainer Mapped ancestor of `startViewPosition`. `startModelOffset` is the offset inside a model element or model + * document fragment mapped to `viewContainer`. + * @param useCache Whether `Mapper` should cache positions while traversing the view tree looking for `expectedModelOffset`. + * @returns View position mapped to `targetModelOffset`. */ - public findPositionIn( viewParent: ViewNode | ViewDocumentFragment, expectedOffset: number ): ViewPosition { - // Last scanned view node. - let viewNode: ViewNode; - // Length of the last scanned view node. - let lastLength = 0; - - let modelOffset = 0; - let viewOffset = 0; + private _findPositionStartingFrom( + startViewPosition: ViewPosition, + startModelOffset: number, + targetModelOffset: number, + viewContainer: ViewElement | ViewDocumentFragment, + useCache: boolean + ): ViewPosition { + let viewParent = startViewPosition.parent as ViewElement | ViewText | ViewDocumentFragment; + let viewIndex = startViewPosition.offset; // In the text node it is simple: the offset in the model equals the offset in the text. if ( viewParent.is( '$text' ) ) { - return new ViewPosition( viewParent, expectedOffset ); + return new ViewPosition( viewParent, targetModelOffset - startModelOffset ); } - // In other cases we add lengths of child nodes to find the proper offset. + // Last scanned view node. + let viewNode: ViewNode; + // Total model offset of the view nodes that were visited so far. + let traversedModelOffset = startModelOffset; + // Model length of the last traversed view node. + let lastLength = 0; - // If it is smaller we add the length. - while ( modelOffset < expectedOffset ) { - viewNode = ( viewParent as ViewElement | ViewDocumentFragment ).getChild( viewOffset )!; + while ( traversedModelOffset < targetModelOffset ) { + viewNode = viewParent.getChild( viewIndex )!; lastLength = this.getModelLength( viewNode ); - modelOffset += lastLength; - viewOffset++; + traversedModelOffset += lastLength; + viewIndex++; + + if ( useCache ) { + this._cache.save( viewParent, viewIndex, viewContainer, traversedModelOffset ); + } } - // If it equals we found the position. - if ( modelOffset == expectedOffset ) { - return this._moveViewPositionToTextNode( new ViewPosition( viewParent, viewOffset ) ); + if ( traversedModelOffset == targetModelOffset ) { + // If it equals we found the position. + return this._moveViewPositionToTextNode( new ViewPosition( viewParent, viewIndex ) ); } - // If it is higher we need to enter last child. else { - // ( modelOffset - lastLength ) is the offset to the child we enter, - // so we subtract it from the expected offset to fine the offset in the child. - return this.findPositionIn( viewNode!, expectedOffset - ( modelOffset - lastLength ) ); + // If it is higher we overstepped with the last traversed view node. + // We need to "enter" it, and look for the view position / model offset inside the last visited view node. + return this._findPositionStartingFrom( + new ViewPosition( viewNode!, 0 ), + traversedModelOffset - lastLength, + targetModelOffset, + viewContainer, + useCache + ); } } @@ -675,6 +742,407 @@ export default class Mapper extends /* #__PURE__ */ EmitterMixin() { } } +/** + * Cache mechanism for {@link module:engine/conversion/mapper~Mapper Mapper}. + * + * `MapperCache` improves performance for model-to-view position mapping, which is the main `Mapper` task. Asking for a mapping is much + * more frequent than actually performing changes, and even if the change happens, we can still partially keep the cache. This makes + * caching a useful strategy for `Mapper`. + * + * `MapperCache` will store some data for view elements or view document fragments that are mapped by the `Mapper`. These view items + * are "tracked" by the `MapperCache`. For such view items, we will keep entries of model offsets inside their mapped counterpart. For + * the cached model offsets, we will keep a view position that is inside the tracked item. This allows us to either get the mapping + * instantly, or at least in less steps than when calculating it from the beginning. + * + * Important problem related to caching is invalidating the cache. The cache must be invalidated each time the tracked view item changes. + * Additionally, we should invalidate as small part of the cache as possible. Since all the logic is encapsulated inside `MapperCache`, + * the `MapperCache` listens to view items {@link module:engine/view/node~ViewNodeChangeEvent `change` event} and reacts to it. + * Then, it invalidates just the part of the cache that is "after" the changed part of the view. + * + * As mentioned, `MapperCache` currently is used only for model-to-view position mapping as it was much bigger problem than view-to-model + * mapping. However, it should be possible to use it also for view-to-model. + * + * The main assumptions regarding `MapperCache` are: + * + * * it is an internal tool, used by `Mapper`, transparent to the outside (no additional effort when developing a plugin or a converter), + * * it stores all the necessary data internally, which makes it easier to disable or debug, + * * it is optimized for initial downcast process (long insertions), which is crucial for editor init and data save, + * * it does not save all possible positions for memory considerations, although it is a possible improvement, which may have increase + * performance, as well as simplify some parts of the `MapperCache` logic. + * + * @internal + */ +export class MapperCache extends /* #__PURE__ */ EmitterMixin() { + /** + * For every view element or document fragment tracked by `MapperCache`, it holds currently cached data, or more precisely, + * model offset to view position mappings. See also {@link ~MappingCache MappingCache} and {@link ~CacheItem CacheItem}. + * + * If an item is tracked by `MapperCache` it has an entry in this structure, so this structure can be used to check which items + * are tracked by `MapperCache`. When an item is no longer tracked, it is removed from this structure. + * + * Although `MappingCache` and `CacheItem` structures allows for caching any model offsets and view positions, we only cache + * values for model offsets that are after a view node. So, in essence, positions inside text nodes are not cached. However, it takes + * from one to at most a few steps, to get from a cached position to a position that is inside a view text node. + * + * Additionally, only one item per `modelOffset` is cached. There can be several view nodes that "end" at the same `modelOffset`. + * In this case, we favour positions that are closer to the mapped item. For example + * + * * for model: `Some <$text bold=true italic=true>formatted text.`, + * * and view: `

Some formatted text.

`, + * + * for model offset `14` (after "d"), we store view position after `` element (i.e. view position: at `

`, offset `2`). + */ + private _cachedMapping = new WeakMap(); + + /** + * For a given view node, which position was cached, it holds an index to an item in the cache list of this view node tracked + * ancestor. The item pointed by index has view position and model offset that are after the given node. + * + * This allows for fast mapping view nodes to certain {@link ~MappingCache#cacheList `MappingCache#cacheList`} items and for faster + * cache invalidation. + * + * For example, consider view: `

Some bold text.

`, where `

` is a view element tracked by `MapperCache`. + * If all `

` children were visited by `MapperCache`, then `

` cache list would have four items, related to following model offsets: + * `0`, `5`, `9`, `15`. Then, view node `"Some "` would have index `1`, `` index `2`, and `" text." index `3`. + * + * Note that the value is always greater than `0`. The first item is always for model offset `0` (and view offset `0`), and obviously, + * there are no view nodes before this position. + * + * These values are not cleared manually at any point, but due to how the caching mechanism works, they don't need to, as there + * is no risk of an outdated value being used. Explanation below. + * + * Index for a node is set when there's no cache for the mapped ancestor, and we traverse the parent, and we go "over" the node. + * + * Index for a node is used when the cache is cleared for the mapped ancestor, to speed this process. The index is used to quickly + * access offset value by checking a value inside an array (cache list), at that index. + * + * If index for a node is outdated, we will end up checking incorrect index in cache list and this will lead to an error. But this + * cannot happen. Let's consider a node that is inside a mapped ancestor and for which we cached the index (e.g. 7) at some point. + * + * 1. If another node is put or removed after the node, this does not affect offsets or caching, and nothing happens. + * + * 2. If another node is put or removed before the node, we shrink the cache list array appropriately, so it includes only the nodes + * that are before the just inserted or removed node. E.g. if we inserted a node which is a fifth node in the mapped ancestor, + * cache list for this ancestor will now be only four items long. When we will try to use cached index 7, we will see that it no longer + * exists in the cache list which will tell us that this cached index is outdated. + * + * 3. If the node is moved to a different mapped ancestor, we still keep the cached index (as we never clear it manually). But as the + * node is inserted into that ancestor, as described, we clear the cache list for the ancestor. So again, when we will use the index, + * we will see that the cache list is shorter and the index is outdated. + * + * 4. The only way to expand cache list is to iterate over nodes when performing the mapping, if the cache is not available. In the + * same process, we will overwrite the cached index to a proper value. + */ + private _nodeToCacheListIndex = new WeakMap(); + + /** + * Saves cache for the given data. + * + * More precisely, for given `modelOffset` inside a model element mapped to `viewContainer`, it saves that mapped view position is + * in `viewParent` at offset `viewOffset`. + * + * `viewParent` is `viewContainer`, or is a non-mapped descendant of `viewContainer`. + * + * @param viewParent + * @param viewOffset + * @param viewContainer + * @param modelOffset + */ + public save( + viewParent: ViewElement | ViewDocumentFragment, + viewOffset: number, + viewContainer: ViewElement | ViewDocumentFragment, + modelOffset: number + ): void { + // Get current cache for the tracked ancestor. + const cache = this._cachedMapping.get( viewContainer )!; + // See if there is already a cache defined for `modelOffset`. + const cacheItem = cache.cacheMap.get( modelOffset ); + + if ( modelOffset <= cache.maxModelOffset && cacheItem ) { + // We have a valid cache, and we already cached this offset. Don't overwrite the cache. + // + // This assumes that `Mapper` works in a way that we first cache the parent and only then cache children, as we prefer position + // after the parent ("closer" to the tracked ancestor). It might be safer to check which position is preferred (newly saved or + // the one currently in cache) but it would require additional processing. For now, `Mapper#_findPositionIn()` and + // `Mapper#getModelLength()` are implemented so that parents are cached before their children. + // + // So, don't create new cache if one already exists. Instead, only save `_nodeToCacheListIndex` value for the related view node. + if ( viewOffset > 0 ) { + const viewChild = viewParent.getChild( viewOffset - 1 )!; + + // Figure out what index to save with `viewChild`. + // We have a `cacheItem` for the `modelOffset`, so we can get a `viewPosition` from there. Before that position, there + // must be a node. That node must have an index set. This will be the index we will want to use. + const index = this._nodeToCacheListIndex.get( cacheItem.viewPosition.nodeBefore! )!; + + this._nodeToCacheListIndex.set( viewChild, index ); + } + + return; + } + + const viewPosition = new ViewPosition( viewParent, viewOffset ); + const newCacheItem = { viewPosition, modelOffset }; + + // Extend the valid cache range. + cache.maxModelOffset = modelOffset > cache.maxModelOffset ? modelOffset : cache.maxModelOffset; + + // Save the new cache item to the `cacheMap`. + cache.cacheMap.set( modelOffset, newCacheItem ); + + // Save the new cache item to the `cacheList`. + let i = cache.cacheList.length - 1; + + // Mostly, we cache elements at the end of `cacheList` and the loop does not execute even once. But when we recursively visit nodes + // in `Mapper#_findPositionIn()`, then we will first cache the parent, and then it's children, and they will not be added at the + // end of `cacheList`. This is why we need to find correct index to insert them. + while ( i >= 0 && cache.cacheList[ i ].modelOffset > modelOffset ) { + i--; + } + + cache.cacheList.splice( i + 1, 0, newCacheItem ); + + if ( viewOffset > 0 ) { + const viewChild = viewParent.getChild( viewOffset - 1 )!; + // There was an idea to also cache `viewContainer` here but, it could lead to wrong results. If we wanted to cache + // `viewContainer`, we probably would need to clear `this._nodeToCacheListIndex` when cache is cleared. + // Also, there was no gain from caching this value, the results were almost the same (statistical error). + this._nodeToCacheListIndex.set( viewChild, i + 1 ); + } + } + + /** + * For given `modelOffset` inside a model element mapped to `viewContainer`, it returns the closest + * {@link ~CacheItem cache item (view position and related model offset)} to the requested model offset one. + * + * It can be the exact, requested mapping, or it can be mapping that is the closest starting point to look for the requested mapping. + * + * `viewContainer` must be a view element or document fragment that is mapped by the {@link ~Mapper Mapper}. + * + * If `viewContainer` is not yet tracked by the `MapperCache`, it will be automatically tracked after calling this method. + * + * @param viewContainer + * @param modelOffset + */ + public get( viewContainer: ViewElement | ViewDocumentFragment, modelOffset: number ): CacheItem { + const cache = this._cachedMapping.get( viewContainer ); + + if ( cache ) { + if ( modelOffset > cache.maxModelOffset ) { + return cache.cacheList[ cache.cacheList.length - 1 ]; + } else { + const cacheItem = cache.cacheMap.get( modelOffset ); + + if ( cacheItem ) { + return cacheItem; + } else { + return this._findInCacheList( cache.cacheList, modelOffset ); + } + } + } else { + return this.startTracking( viewContainer ); + } + } + + /** + * Starts tracking given `viewContainer`, which must be mapped to a model element or model document fragment. + * + * Note, that this method is automatically called by {@link ~MapperCache#get `MapperCache#get()`} and there is no need to call it + * manually. + * + * This method initializes the cache for `viewContainer` and adds callbacks for + * {@link module:engine/view/node~ViewNodeChangeEvent `change` event} fired by `viewContainer`. `MapperCache` listens to `change` event + * on the tracked elements to invalidate the stored cache. + * + * @param viewContainer + */ + public startTracking( viewContainer: ViewElement | ViewDocumentFragment ): CacheItem { + const viewPosition = new ViewPosition( viewContainer, 0 ); + const initialCacheItem = { viewPosition, modelOffset: 0 }; + const initialCache: MappingCache = { + maxModelOffset: 0, + cacheList: [ initialCacheItem ], + cacheMap: new Map( [ [ 0, initialCacheItem ] ] ) + }; + + this._cachedMapping.set( viewContainer, initialCache ); + + // Listen to changes in tracked view containers in order to invalidate the cache. + // + // Possible performance improvement. This event bubbles, so if there are multiple tracked (mapped) elements that are ancestors + // then this will be unnecessarily fired for each ancestor. This could be rewritten to listen only to roots and document fragments. + this.listenTo( viewContainer, 'change:children', ( evt, viewNode, data ) => { + this._invalidateCacheOnChildrenChange( viewNode as ViewElement | ViewDocumentFragment, ( data as any ).index as number ); + } ); + + this.listenTo( viewContainer, 'change:text', ( evt, viewNode ) => { + // Text node has changed. Clear all the cache starting from before this text node. + this._clearCacheBefore( viewNode as ViewText ); + } ); + + return initialCacheItem; + } + + /** + * Stops tracking given `viewContainer`. + * + * It removes the cached data and stops listening to {@link module:engine/view/node~ViewNodeChangeEvent `change` event} on the + * `viewContainer`. + * + * @param viewContainer + */ + public stopTracking( viewContainer: ViewElement | ViewDocumentFragment ): void { + this.stopListening( viewContainer ); + + this._cachedMapping.delete( viewContainer ); + } + + /** + * Invalidates cache after a change happens inside `viewParent` on given `index`. + * + * @param viewParent + * @param index + */ + private _invalidateCacheOnChildrenChange( viewParent: ViewElement | ViewDocumentFragment, index: number ) { + if ( index == 0 ) { + // Change at the beginning of the parent. + if ( this._cachedMapping.has( viewParent ) ) { + // If this is a tracked element, clear all cache. + this._clearCacheAll( viewParent ); + } else { + // If this is not a tracked element, remove cache starting from before this element. + this._clearCacheBefore( viewParent as ViewElement ); + } + } else { + // Change in the middle of the parent. Get a view node that's before the change. + const lastValidNode = viewParent.getChild( index - 1 )!; + + // Then, clear all cache starting from before this view node. + // + // Possible performance improvement. We could have had `_clearCacheAfter( lastValidNode )` instead. + // If the `lastValidNode` is the last unchanged node, then we could clear everything AFTER it, not before. + // However, with the current setup, it didn't work properly and the actual gain wasn't that big on the tested data. + // The problem was with following example:

FooXyzBar

. + // In this example we cache position after , i.e. view position `

` 2 is saved with model offset 6. + // Now, if we add some text in ``, we won't validate this cached item even though it gets outdated. + // So, if there's a need to have `_clearCacheAfter()`, we need to solve the above case first. + // + this._clearCacheBefore( lastValidNode ); + } + } + + /** + * Clears all the cache for given tracked `viewContainer`. + * + * @param viewContainer + */ + private _clearCacheAll( viewContainer: ViewElement | ViewDocumentFragment ) { + const cache = this._cachedMapping.get( viewContainer )!; + + if ( cache.maxModelOffset > 0 ) { + cache.maxModelOffset = 0; + cache.cacheList.length = 1; + cache.cacheMap.clear(); + cache.cacheMap.set( 0, cache.cacheList[ 0 ] ); + } + } + + /** + * Clears all the stored cache starting before given `viewNode`. The `viewNode` can be any node that is inside a tracked view element + * or view document fragment. + * + * @param viewNode + */ + private _clearCacheBefore( viewNode: ViewNode ): void { + // To quickly invalidate the cache, we base on the cache list index stored with the node. See docs for `this._nodeToCacheListIndex`. + const cacheListIndex = this._nodeToCacheListIndex.get( viewNode ); + + // If there is no index stored, it means that this `viewNode` has not been cached yet. + if ( cacheListIndex === undefined ) { + // If the node is not cached, maybe it's parent is. We will try to invalidate the cache starting from before the parent. + // Note, that there always must be a parent if we got here. + const viewParent = viewNode.parent as ViewElement; + + if ( !this._cachedMapping.has( viewParent ) ) { + // If the parent is a non-tracked element, try clearing the cache starting before it. + // This situation may happen e.g. if structure like `

Foo...` was stepped over in + // `Mapper#_findPositionIn()` and the children are not cached yet, but the `` element is. If something changes + // inside this structure, make sure to invalidate all the cache after ``. + return this._clearCacheBefore( viewParent ); + } else { + // If the parent is a tracked element, then it means there's no cache to clear (nothing after the element has been cached). + return; + } + } + + // Note: there was a consideration to save the `viewContainer` value together with `cacheListIndex` value. + // However, it is like it is on purpose. We want to find *current* mapped ancestor for the `viewNode`. + // This is an essential step to verify if the cache is still up-to-date. + // Actually, we could save `viewContainer` and compare it to current tracked ancestor to quickly invalidate. + // But this kinda happens with our flow and other assumptions around caching list index anyway. + let viewContainer = viewNode.parent!; + + while ( !this._cachedMapping.has( viewContainer ) ) { + viewContainer = viewContainer.parent!; + } + + this._clearCacheFromIndex( viewContainer, cacheListIndex ); + } + + /** + * Clears all the cache in the cache list related to given `viewContainer`, starting from `index` (inclusive). + * + * @param viewContainer + * @param index + */ + private _clearCacheFromIndex( viewContainer: ViewElement | ViewDocumentFragment, index: number ) { + // Cache is always available here because we initialize it just before adding a listener that fires `_clearCacheFromIndex()`. + const cache = this._cachedMapping.get( viewContainer )!; + const cacheItem = cache.cacheList[ index - 1 ]; + + if ( !cacheItem ) { + return; + } + + cache.maxModelOffset = cacheItem.modelOffset; + + const clearedItems = cache.cacheList.splice( index ); + + for ( const item of clearedItems ) { + cache.cacheMap.delete( item.modelOffset ); + } + } + + /** + * Finds a cache item in the given cache list, which `modelOffset` is closest (but smaller or equal) to given `offset`. + * + * Since `cacheList` is a sorted array, this uses binary search to retrieve the item quickly. + * + * @param cacheList + * @param offset + */ + private _findInCacheList( cacheList: Array, offset: number ): CacheItem { + let start = 0; + let end = cacheList.length - 1; + let index = ( end - start ) >> 1; + let item = cacheList[ index ]; + + while ( start < end ) { + if ( item.modelOffset < offset ) { + start = index + 1; + } else { + end = index - 1; + } + + index = start + ( ( end - start ) >> 1 ); + item = cacheList[ index ]; + } + + return item.modelOffset <= offset ? item : cacheList[ index - 1 ]; + } +} + /** * Fired for each model-to-view position mapping request. The purpose of this event is to enable custom model-to-view position * mapping. Callbacks added to this event take {@link module:engine/model/position~Position model position} and are expected to @@ -834,3 +1302,56 @@ export type MapperViewToModelPositionEventData = { */ viewPosition: ViewPosition; }; + +/** + * Holds cache data for model to view mappings for a view element or view document fragment. + * + * {@link ~MapperCache MapperCache} keeps a `MappingCache` item for each tracked view element or document fragment (which essentially means + * one `MappingCache` for each model element or a model document fragment). + */ +type MappingCache = { + /** + * Maximum model offset for which this item holds a valid cache. + * + * If there is a request for mapping for a model offset above this value, the view position needs to be calculated. + */ + maxModelOffset: number, + + /** + * List of cached {@link ~CacheItem cache items} for this mapped view/model element. The list is sorted by + * {@link ~CacheItem#modelOffset `CacheItem#modelOffset`} values. This makes it faster to search for the closest mapped model offset. + * + * For example, if the cache stores values for model offsets: `0`, `20` and `33`, and there is a mapping request for offset `26`, + * we can quickly find cached value for offset `20` and continue calculating offset from there. + * + * Note, that there is only one entry in `cacheList` for given `modelOffset` value. + */ + cacheList: Array, + + /** + * Map of cached {@link ~CacheItem cache items} for this mapped view/model element. The keys are `modelOffset`s + * (as defined in `CacheItem`) and values are the `CacheItem` objects. + * + * This is a different representation of `cacheList`, and it is used for different purposes. Since it is a map, it is very fast to + * retrieve data for exact `modelOffset`s which were already cached before. But a sorted list is better for finding the closest item + * if exact item is not found. Also, it is faster to clear "all entries after model offset X" for `cacheList`. + */ + cacheMap: Map +}; + +/** + * Informs that given `viewPosition` corresponds to given `modelOffset` (with the assumption that the model offset is in a model element + * mapped to the `viewPosition` parent or its ancestor). + * + * For example, for model `Some <$text bold=true>bold text` and view `

Some bold text

` + * and assuming `` and `

` are mapped, following `CacheItem`s are possible: + * + * * `viewPosition` = `

`, 1; `modelOffset` = 5 + * * `viewPosition` = `, 1; `modelOffset` = 9 + * * `viewPosition` = `"bold"`, 2; `modelOffset` = 7 + * * `viewPosition` = `" text"`, 0; `modelOffset` = 7 + */ +type CacheItem = { + viewPosition: ViewPosition, + modelOffset: number +}; diff --git a/packages/ckeditor5-engine/src/view/documentfragment.ts b/packages/ckeditor5-engine/src/view/documentfragment.ts index 2d599590e16..be4d811aeed 100644 --- a/packages/ckeditor5-engine/src/view/documentfragment.ts +++ b/packages/ckeditor5-engine/src/view/documentfragment.ts @@ -17,6 +17,7 @@ import type { default as Document, ChangeType } from './document.js'; import type Item from './item.js'; import type Node from './node.js'; +import { ViewNodeChangeEvent } from "./node.js"; /** * Document fragment. @@ -176,7 +177,7 @@ export default class DocumentFragment extends /* #__PURE__ */ EmitterMixin( Type * @returns Number of inserted nodes. */ public _insertChild( index: number, items: Item | string | Iterable ): number { - this._fireChange( 'children', this ); + this._fireChange( 'children', this, { index } ); let count = 0; const nodes = normalize( this.document, items ); @@ -206,7 +207,7 @@ export default class DocumentFragment extends /* #__PURE__ */ EmitterMixin( Type * @returns The array of removed nodes. */ public _removeChildren( index: number, howMany: number = 1 ): Array { - this._fireChange( 'children', this ); + this._fireChange( 'children', this, { index } ); for ( let i = index; i < index + howMany; i++ ) { ( this._children[ i ] as any ).parent = null; @@ -216,14 +217,14 @@ export default class DocumentFragment extends /* #__PURE__ */ EmitterMixin( Type } /** - * Fires `change` event with given type of the change. - * * @internal * @param type Type of the change. * @param node Changed node. + * @param data Additional data. + * @fires change */ - public _fireChange( type: ChangeType, node: Node | DocumentFragment ): void { - this.fire( 'change:' + type, node ); + public _fireChange( type: ChangeType, node: Node | DocumentFragment, data?: unknown ): void { + this.fire( `change:${ type }`, node, data ); } /** diff --git a/packages/ckeditor5-engine/src/view/element.ts b/packages/ckeditor5-engine/src/view/element.ts index 5571d9e0bbe..9d5a9eae14f 100644 --- a/packages/ckeditor5-engine/src/view/element.ts +++ b/packages/ckeditor5-engine/src/view/element.ts @@ -585,7 +585,7 @@ export default class Element extends Node { * @returns Number of inserted nodes. */ public _insertChild( index: number, items: Item | string | Iterable ): number { - this._fireChange( 'children', this ); + this._fireChange( 'children', this, { index } ); let count = 0; const nodes = normalize( this.document, items ); @@ -618,7 +618,7 @@ export default class Element extends Node { * @returns The array of removed nodes. */ public _removeChildren( index: number, howMany: number = 1 ): Array { - this._fireChange( 'children', this ); + this._fireChange( 'children', this, { index } ); for ( let i = index; i < index + howMany; i++ ) { ( this._children[ i ] as any ).parent = null; diff --git a/packages/ckeditor5-engine/src/view/node.ts b/packages/ckeditor5-engine/src/view/node.ts index 529522fb5dc..7af5dab7adf 100644 --- a/packages/ckeditor5-engine/src/view/node.ts +++ b/packages/ckeditor5-engine/src/view/node.ts @@ -257,13 +257,14 @@ export default abstract class Node extends /* #__PURE__ */ EmitterMixin( TypeChe * @internal * @param type Type of the change. * @param node Changed node. + * @param data Additional data. * @fires change */ - public _fireChange( type: ChangeType, node: Node ): void { - this.fire( `change:${ type }`, node ); + public _fireChange( type: ChangeType, node: Node, data?: unknown ): void { + this.fire( `change:${ type }`, node, data ); if ( this.parent ) { - this.parent._fireChange( type, node ); + this.parent._fireChange( type, node, data ); } } @@ -304,10 +305,14 @@ Node.prototype.is = function( type: string ): boolean { }; /** - * Fired when list of {@link module:engine/view/element~Element elements} children, attributes or data changes. + * Fired when list of {@link module:engine/view/element~Element elements} children, attributes or text changes. * * Change event is bubbled – it is fired on all ancestors. * + * All change events as the first parameter receive the node that has changed (the node for which children, attributes or text changed). + * + * If `change:children` event is fired, there is an additional second parameter, which is an object with additional data related to change. + * * @eventName ~Node#change * @eventName ~Node#change:children * @eventName ~Node#change:attributes @@ -315,5 +320,5 @@ Node.prototype.is = function( type: string ): boolean { */ export type ViewNodeChangeEvent = { name: 'change' | `change:${ ChangeType }`; - args: [ changedNode: Node ]; + args: [ changedNode: Node, data?: unknown ]; }; diff --git a/packages/ckeditor5-engine/tests/conversion/mapper.js b/packages/ckeditor5-engine/tests/conversion/mapper.js index f7a724686fb..3ca7b9f6293 100644 --- a/packages/ckeditor5-engine/tests/conversion/mapper.js +++ b/packages/ckeditor5-engine/tests/conversion/mapper.js @@ -4,6 +4,7 @@ */ import Mapper from '../../src/conversion/mapper.js'; +import { MapperCache } from '../../src/conversion/mapper.js'; import ModelElement from '../../src/model/element.js'; import ModelRootElement from '../../src/model/rootelement.js'; @@ -21,6 +22,7 @@ import ViewDocumentFragment from '../../src/view/documentfragment.js'; import { StylesProcessor } from '../../src/view/stylesmap.js'; import CKEditorError from '@ckeditor/ckeditor5-utils/src/ckeditorerror.js'; +import { ViewAttributeElement } from '../../src/index.js'; describe( 'Mapper', () => { let viewDocument; @@ -873,3 +875,325 @@ describe( 'Mapper', () => { } ); } ); } ); + +describe.only( 'MapperCache', () => { + let cache, viewContainer, viewDocument, viewTextAb, viewSpan, viewEm, viewTextCd, viewTextEf, viewTextGh; + + beforeEach( () => { + viewDocument = new ViewDocument( new StylesProcessor() ); + cache = new MapperCache(); + + //

abcdefgh

. + viewTextAb = new ViewText( viewDocument, 'ab' ); + viewTextCd = new ViewText( viewDocument, 'cd' ); + viewTextEf = new ViewText( viewDocument, 'ef' ); + viewEm = new ViewAttributeElement( viewDocument, 'em', null, [ viewTextEf ] ); + viewSpan = new ViewAttributeElement( viewDocument, 'span', null, [ viewTextCd, viewEm ] ); + viewTextGh = new ViewText( viewDocument, 'gh' ); + viewContainer = new ViewElement( viewDocument, 'p', null, [ viewTextAb, viewSpan, viewTextGh ] ); + } ); + + describe( 'get()', () => { + it( 'should start tracking and return position at the start of element if element was not tracked before', () => { + const { viewPosition, modelOffset } = cache.get( viewContainer, 6 ); + + expect( viewPosition.parent ).to.equal( viewContainer ); + expect( viewPosition.offset ).to.equal( 0 ); + expect( modelOffset ).to.equal( 0 ); + } ); + + it( 'should return previously saved position', () => { + cache.startTracking( viewContainer ); + cache.save( viewContainer, 2, viewContainer, 6 ); + + const { viewPosition, modelOffset } = cache.get( viewContainer, 6 ); + + expect( viewPosition.parent ).to.equal( viewContainer ); + expect( viewPosition.offset ).to.equal( 2 ); + expect( modelOffset ).to.equal( 6 ); + } ); + + it( 'should return previously saved position (deep)', () => { + cache.startTracking( viewContainer ); + cache.save( viewEm, 0, viewContainer, 4 ); + + const { viewPosition, modelOffset } = cache.get( viewContainer, 4 ); + + expect( viewPosition.parent ).to.equal( viewEm ); + expect( viewPosition.offset ).to.equal( 0 ); + expect( modelOffset ).to.equal( 4 ); + } ); + + it( 'should return closest saved position if exact position was not saved', () => { + cache.startTracking( viewContainer ); + cache.save( viewContainer, 2, viewContainer, 6 ); + + const { viewPosition, modelOffset } = cache.get( viewContainer, 8 ); + + expect( viewPosition.parent ).to.equal( viewContainer ); + expect( viewPosition.offset ).to.equal( 2 ); + expect( modelOffset ).to.equal( 6 ); + } ); + + it( 'should return closest saved position if exact position was not saved (deep)', () => { + cache.startTracking( viewContainer ); + cache.save( viewEm, 0, viewContainer, 4 ); + + const { viewPosition, modelOffset } = cache.get( viewContainer, 8 ); + + expect( viewPosition.parent ).to.equal( viewEm ); + expect( viewPosition.offset ).to.equal( 0 ); + expect( modelOffset ).to.equal( 4 ); + } ); + + it( 'should return closest saved position if exact position was not saved (multiple saved positions)', () => { + cache.startTracking( viewContainer ); + + // Save model offsets in random order to give extra spice and cover some code... + cache.save( viewContainer, 2, viewContainer, 6 ); + cache.save( viewContainer, 3, viewContainer, 8 ); + cache.save( viewSpan, 1, viewContainer, 4 ); + cache.save( viewContainer, 1, viewContainer, 2 ); + + check( 1, viewContainer, 0, 0 ); + check( 3, viewContainer, 1, 2 ); + check( 5, viewSpan, 1, 4 ); + check( 7, viewContainer, 2, 6 ); + check( 9, viewContainer, 3, 8 ); + } ); + } ); + + describe( 'save()', () => { + it( 'should not overwrite previously saved cache for given model offset', () => { + cache.startTracking( viewContainer ); + cache.save( viewContainer, 2, viewContainer, 6 ); + cache.save( viewSpan, 2, viewContainer, 6 ); + + const { viewPosition, modelOffset } = cache.get( viewContainer, 6 ); + + expect( viewPosition.parent ).to.equal( viewContainer ); + expect( viewPosition.offset ).to.equal( 2 ); + expect( modelOffset ).to.equal( 6 ); + } ); + } ); + + describe( 'stopTracking()', () => { + it( 'should remove all cache data', () => { + cache.startTracking( viewContainer ); + cache.save( viewContainer, 2, viewContainer, 6 ); + cache.stopTracking( viewContainer ); + + const { viewPosition, modelOffset } = cache.get( viewContainer, 2 ); + + expect( viewPosition.parent ).to.equal( viewContainer ); + expect( viewPosition.offset ).to.equal( 0 ); + expect( modelOffset ).to.equal( 0 ); + } ); + } ); + + describe( 'cache invalidation', () => { + it( 'should invalidate part of the existing cache when a node is inserted', () => { + cache.startTracking( viewContainer ); + + cache.save( viewContainer, 1, viewContainer, 2 ); + cache.save( viewContainer, 2, viewContainer, 6 ); + cache.save( viewSpan, 1, viewContainer, 4 ); + cache.save( viewContainer, 3, viewContainer, 8 ); + + //

abcdef^gh

->

abcdefgh

. + // This should invalidate cache starting from `` (yes, that's correct, we invalidate a bit more than necessary). + viewContainer._insertChild( 2, new ViewAttributeElement( viewDocument, 'strong' ) ); + + // Retained cached: + check( 2, viewContainer, 1, 2 ); + check( 3, viewContainer, 1, 2 ); + + // Later cache is cleared, the closest value is returned: + check( 6, viewContainer, 1, 2 ); + check( 8, viewContainer, 1, 2 ); + } ); + + it( 'should invalidate part of the existing cache when a node is inserted (deep)', () => { + cache.startTracking( viewContainer ); + + cache.save( viewContainer, 1, viewContainer, 2 ); + cache.save( viewSpan, 1, viewContainer, 4 ); + // Although we don't overwrite cache item when we save another cache for the same model offset, + // we store metadata with `viewEm`, which affects how cache is invalidated (it can be invalidated more precisely). + cache.save( viewEm, 1, viewContainer, 4 ); + cache.save( viewContainer, 2, viewContainer, 6 ); + cache.save( viewContainer, 3, viewContainer, 8 ); + + //

abcd^efgh

->

abcdefgh

. + // This should invalidate cache starting from ``. + viewEm._insertChild( 0, new ViewAttributeElement( viewDocument, 'strong' ) ); + + // Retained cached: + check( 2, viewContainer, 1, 2 ); + check( 3, viewContainer, 1, 2 ); + check( 4, viewSpan, 1, 4 ); + check( 5, viewSpan, 1, 4 ); + + // Later cache is cleared, the closest value is returned: + check( 6, viewSpan, 1, 4 ); + check( 8, viewSpan, 1, 4 ); + } ); + + it( 'should invalidate part of the existing cache when a node is inserted (deep #2 - direct parent not cached)', () => { + // This is a similar scenario as above, but this time, `` -- the direct parent of insertion -- is not cached. + cache.startTracking( viewContainer ); + + cache.save( viewContainer, 1, viewContainer, 2 ); + cache.save( viewSpan, 1, viewContainer, 4 ); + cache.save( viewContainer, 2, viewContainer, 6 ); + cache.save( viewContainer, 3, viewContainer, 8 ); + + //

abcd^efgh

->

abcdefgh

. + // This should invalidate cache starting from before `` (not before ``). + // That's because `` is not cached, so we invalidate starting from before its parent. + viewEm._insertChild( 0, new ViewAttributeElement( viewDocument, 'strong' ) ); + + // Retained cached: + check( 2, viewContainer, 1, 2 ); + check( 3, viewContainer, 1, 2 ); + + // Later cache is cleared, the closest value is returned: + check( 4, viewSpan, 1, 4 ); + check( 6, viewSpan, 1, 4 ); + } ); + + it( 'should invalidate part of the existing cache when a node is inserted (deep #3 - only top element cached)', () => { + // This is a similar scenario as above, but this time, `` -- the direct parent of insertion -- is not cached. + cache.startTracking( viewContainer ); + + cache.save( viewContainer, 1, viewContainer, 2 ); + cache.save( viewContainer, 2, viewContainer, 6 ); + cache.save( viewContainer, 3, viewContainer, 8 ); + + //

abcd^efgh

->

abcdefgh

. + // This should invalidate cache starting from before `` (not before ``). + // That's because `` is not cached, so we invalidate starting from before its parent. + viewEm._insertChild( 0, new ViewAttributeElement( viewDocument, 'strong' ) ); + + // Retained cached: + check( 2, viewContainer, 1, 2 ); + check( 3, viewContainer, 1, 2 ); + + // Later cache is cleared, the closest value is returned: + check( 4, viewContainer, 1, 2 ); + check( 6, viewContainer, 1, 2 ); + } ); + + it( 'nothing happens if change is after valid cache', () => { + // This is a similar scenario as above, but this time, `` -- the direct parent of insertion -- is not cached. + cache.startTracking( viewContainer ); + + cache.save( viewContainer, 1, viewContainer, 2 ); + cache.save( viewContainer, 2, viewContainer, 6 ); + + //

abcdefgh^

->

abcdefgh

. + // Only `` was cached so far. + viewContainer._insertChild( 3, new ViewAttributeElement( viewDocument, 'strong' ) ); + + // Retained cached: + check( 2, viewContainer, 1, 2 ); + check( 3, viewContainer, 1, 2 ); + check( 6, viewContainer, 2, 6 ); + check( 7, viewContainer, 2, 6 ); + + // No new cache added yet: + check( 8, viewContainer, 2, 6 ); + check( 9, viewContainer, 2, 6 ); + } ); + + it( 'should invalidate all cache if change is at the beginning of tracked element', () => { + cache.startTracking( viewContainer ); + + cache.save( viewContainer, 1, viewContainer, 2 ); + cache.save( viewSpan, 1, viewContainer, 4 ); + cache.save( viewContainer, 2, viewContainer, 6 ); + cache.save( viewContainer, 3, viewContainer, 8 ); + + //

^abcdefgh

->

abcdefgh

. + // This should invalidate all cache. + viewContainer._insertChild( 0, new ViewAttributeElement( viewDocument, 'strong' ) ); + + // Cache is cleared, the closest value is returned: + check( 1, viewContainer, 0, 0 ); + check( 2, viewContainer, 0, 0 ); + check( 8, viewContainer, 0, 0 ); + } ); + + it( 'should invalidate cache when text node changes', () => { + cache.startTracking( viewContainer ); + + cache.save( viewContainer, 1, viewContainer, 2 ); + cache.save( viewSpan, 1, viewContainer, 4 ); + cache.save( viewContainer, 2, viewContainer, 6 ); + cache.save( viewContainer, 3, viewContainer, 8 ); + + //

abcdefgh^

->

abcdefghi

. + // This should invalidate cache starting from before `"ghi"`. + viewTextGh._data = 'ghi'; + + // Retained cached: + check( 2, viewContainer, 1, 2 ); + check( 4, viewSpan, 1, 4 ); + check( 6, viewContainer, 2, 6 ); + + // Later cache is cleared, the closest value is returned: + check( 8, viewContainer, 2, 6 ); + } ); + + it( 'should invalidate cache when text node changes (deep)', () => { + cache.startTracking( viewContainer ); + + cache.save( viewContainer, 1, viewContainer, 2 ); + cache.save( viewSpan, 1, viewContainer, 4 ); + cache.save( viewEm, 1, viewContainer, 6 ); + cache.save( viewContainer, 2, viewContainer, 6 ); + cache.save( viewContainer, 3, viewContainer, 8 ); + + //

abcdef^gh

->

abcdefx

. + // This should invalidate cache starting from before `"efx"`. + viewTextEf._data = 'efx'; + + // Retained cached: + check( 2, viewContainer, 1, 2 ); + check( 4, viewSpan, 1, 4 ); + + // Later cache is cleared, the closest value is returned: + check( 6, viewSpan, 1, 4 ); + check( 8, viewSpan, 1, 4 ); + } ); + + it( 'should invalidate cache when text node changes (deep - text node not cached)', () => { + cache.startTracking( viewContainer ); + + cache.save( viewContainer, 1, viewContainer, 2 ); + cache.save( viewSpan, 1, viewContainer, 4 ); + cache.save( viewContainer, 2, viewContainer, 6 ); + cache.save( viewContainer, 3, viewContainer, 8 ); + + //

abcdef^gh^

->

abcdefx

. + // This should invalidate cache starting from before `"ghi"`. + viewTextEf._data = 'efx'; + + // Retained cached: + check( 2, viewContainer, 1, 2 ); + check( 4, viewSpan, 1, 4 ); + + // Later cache is cleared, the closest value is returned: + check( 6, viewSpan, 1, 4 ); + check( 8, viewSpan, 1, 4 ); + } ); + } ); + + function check( modelOffsetToCheck, expectedViewParent, expectedViewOffset, expectedModelOffset ) { + const { viewPosition, modelOffset } = cache.get( viewContainer, modelOffsetToCheck ); + + expect( viewPosition.parent ).to.equal( expectedViewParent ); + expect( viewPosition.offset ).to.equal( expectedViewOffset ); + expect( modelOffset ).to.equal( expectedModelOffset ); + } +} ); From ac6215e1f3a27a26065c2fef633bcb73e7f037fe Mon Sep 17 00:00:00 2001 From: Szymon Cofalik Date: Mon, 9 Dec 2024 11:27:59 +0100 Subject: [PATCH 02/15] Hoist cached positions to make sure that positions returned by `MapperCache` are always "top-most" possible positions for given model offset. Delete entires from `MapperCache#_nodeToCacheListIndex`. --- .../ckeditor5-engine/src/conversion/mapper.ts | 83 ++++++++++--------- 1 file changed, 46 insertions(+), 37 deletions(-) diff --git a/packages/ckeditor5-engine/src/conversion/mapper.ts b/packages/ckeditor5-engine/src/conversion/mapper.ts index 39bfdcc19c5..c3cc7666aa3 100644 --- a/packages/ckeditor5-engine/src/conversion/mapper.ts +++ b/packages/ckeditor5-engine/src/conversion/mapper.ts @@ -181,7 +181,7 @@ export default class Mapper extends /* #__PURE__ */ EmitterMixin() { const wasFound = this._viewToModelMapping.delete( viewElement ); if ( wasFound ) { - // Stop tracking after the element is no longer mapped. + // Stop tracking after the element is no longer mapped. We want to track all mapped elements and only mapped elements. this._cache.stopTracking( viewElement ); } @@ -211,7 +211,7 @@ export default class Mapper extends /* #__PURE__ */ EmitterMixin() { const wasFound = this._viewToModelMapping.delete( viewElement ); if ( wasFound ) { - // Stop tracking after the element is no longer mapped. + // Stop tracking after the element is no longer mapped. We want to track all mapped elements and only mapped elements. this._cache.stopTracking( viewElement ); } } @@ -795,43 +795,19 @@ export class MapperCache extends /* #__PURE__ */ EmitterMixin() { private _cachedMapping = new WeakMap(); /** - * For a given view node, which position was cached, it holds an index to an item in the cache list of this view node tracked - * ancestor. The item pointed by index has view position and model offset that are after the given node. + * When `MapperCache` {@link ~MapperCache#save saves} view position -> model offset mapping, a + * {@link ~CacheItem `CacheItem`} is inserted into certain {@link ~MappingCache#cacheList `MappingCache#cacheList`} at some index. + * Additionally, we store that index with the view node that is before the cached view position. * - * This allows for fast mapping view nodes to certain {@link ~MappingCache#cacheList `MappingCache#cacheList`} items and for faster - * cache invalidation. + * This allows to quickly get a {@link ~MappingCache#cacheList cache list} item related to certain view node, and hence, + * for fast cache invalidation. * * For example, consider view: `

Some bold text.

`, where `

` is a view element tracked by `MapperCache`. * If all `

` children were visited by `MapperCache`, then `

` cache list would have four items, related to following model offsets: * `0`, `5`, `9`, `15`. Then, view node `"Some "` would have index `1`, `` index `2`, and `" text." index `3`. * - * Note that the value is always greater than `0`. The first item is always for model offset `0` (and view offset `0`), and obviously, - * there are no view nodes before this position. - * - * These values are not cleared manually at any point, but due to how the caching mechanism works, they don't need to, as there - * is no risk of an outdated value being used. Explanation below. - * - * Index for a node is set when there's no cache for the mapped ancestor, and we traverse the parent, and we go "over" the node. - * - * Index for a node is used when the cache is cleared for the mapped ancestor, to speed this process. The index is used to quickly - * access offset value by checking a value inside an array (cache list), at that index. - * - * If index for a node is outdated, we will end up checking incorrect index in cache list and this will lead to an error. But this - * cannot happen. Let's consider a node that is inside a mapped ancestor and for which we cached the index (e.g. 7) at some point. - * - * 1. If another node is put or removed after the node, this does not affect offsets or caching, and nothing happens. - * - * 2. If another node is put or removed before the node, we shrink the cache list array appropriately, so it includes only the nodes - * that are before the just inserted or removed node. E.g. if we inserted a node which is a fifth node in the mapped ancestor, - * cache list for this ancestor will now be only four items long. When we will try to use cached index 7, we will see that it no longer - * exists in the cache list which will tell us that this cached index is outdated. - * - * 3. If the node is moved to a different mapped ancestor, we still keep the cached index (as we never clear it manually). But as the - * node is inserted into that ancestor, as described, we clear the cache list for the ancestor. So again, when we will use the index, - * we will see that the cache list is shorter and the index is outdated. - * - * 4. The only way to expand cache list is to iterate over nodes when performing the mapping, if the cache is not available. In the - * same process, we will overwrite the cached index to a proper value. + * Note that the index related with a node is always greater than `0`. The first item in cache list is always for model offset `0` + * (and view offset `0`), and it is not related to any node. */ private _nodeToCacheListIndex = new WeakMap(); @@ -927,22 +903,50 @@ export class MapperCache extends /* #__PURE__ */ EmitterMixin() { */ public get( viewContainer: ViewElement | ViewDocumentFragment, modelOffset: number ): CacheItem { const cache = this._cachedMapping.get( viewContainer ); + let result: CacheItem; if ( cache ) { if ( modelOffset > cache.maxModelOffset ) { - return cache.cacheList[ cache.cacheList.length - 1 ]; + result = cache.cacheList[ cache.cacheList.length - 1 ]; } else { const cacheItem = cache.cacheMap.get( modelOffset ); if ( cacheItem ) { - return cacheItem; + result = cacheItem; } else { - return this._findInCacheList( cache.cacheList, modelOffset ); + result = this._findInCacheList( cache.cacheList, modelOffset ); } } } else { - return this.startTracking( viewContainer ); + result = this.startTracking( viewContainer ); + } + + result.viewPosition = this._hoistViewPosition( result.viewPosition ); + + return result; + } + + /** + * Moves a view position to a preferred location. + * + * The view position is moved up from a non-tracked view element as long as it remains at the end of its current parent. Example: + * + * ``` + *

This is some formatted^ text.

->

This is some formatted^ text.

+ * ``` + * + * @param viewPosition + * @private + */ + private _hoistViewPosition( viewPosition: ViewPosition ): ViewPosition { + while ( viewPosition.parent.parent && !this._cachedMapping.has( viewPosition.parent as any ) && viewPosition.isAtEnd ) { + const parent = viewPosition.parent.parent; + const offset = parent.getChildIndex( viewPosition.parent ) + 1; + + viewPosition = new ViewPosition( parent, offset ); } + + return viewPosition; } /** @@ -1107,10 +1111,15 @@ export class MapperCache extends /* #__PURE__ */ EmitterMixin() { cache.maxModelOffset = cacheItem.modelOffset; + // Remove from cache all `CacheItem`s that are "after" the index to clear from. const clearedItems = cache.cacheList.splice( index ); + // For each removed item, make sure to also remove it from `cacheMap` and clear related entry in `_nodeToCacheListIndex`. for ( const item of clearedItems ) { cache.cacheMap.delete( item.modelOffset ); + + const viewNode = item.viewPosition.nodeBefore!; + this._nodeToCacheListIndex.delete( viewNode ); } } From b5c31ab8e412fd610884520f1aec2f139b411945 Mon Sep 17 00:00:00 2001 From: Szymon Cofalik Date: Mon, 9 Dec 2024 11:54:53 +0100 Subject: [PATCH 03/15] Better docs and other minor improvements after review. --- .../ckeditor5-engine/src/conversion/mapper.ts | 122 +++++++++--------- .../src/view/documentfragment.ts | 1 - 2 files changed, 63 insertions(+), 60 deletions(-) diff --git a/packages/ckeditor5-engine/src/conversion/mapper.ts b/packages/ckeditor5-engine/src/conversion/mapper.ts index c3cc7666aa3..d014cbc6c81 100644 --- a/packages/ckeditor5-engine/src/conversion/mapper.ts +++ b/packages/ckeditor5-engine/src/conversion/mapper.ts @@ -626,7 +626,7 @@ export default class Mapper extends /* #__PURE__ */ EmitterMixin() { const useCache = this._viewToModelLengthCallbacks.size == 0 && this._viewToModelMapping.has( viewContainer ); if ( useCache ) { - const cacheItem = this._cache.get( viewContainer, modelOffset ); + const cacheItem = this._cache.getClosest( viewContainer, modelOffset ); return this._findPositionStartingFrom( cacheItem.viewPosition, cacheItem.modelOffset, modelOffset, viewContainer, true ); } else { @@ -670,7 +670,7 @@ export default class Mapper extends /* #__PURE__ */ EmitterMixin() { useCache: boolean ): ViewPosition { let viewParent = startViewPosition.parent as ViewElement | ViewText | ViewDocumentFragment; - let viewIndex = startViewPosition.offset; + let viewOffset = startViewPosition.offset; // In the text node it is simple: the offset in the model equals the offset in the text. if ( viewParent.is( '$text' ) ) { @@ -685,19 +685,26 @@ export default class Mapper extends /* #__PURE__ */ EmitterMixin() { let lastLength = 0; while ( traversedModelOffset < targetModelOffset ) { - viewNode = viewParent.getChild( viewIndex )!; + viewNode = viewParent.getChild( viewOffset )!; lastLength = this.getModelLength( viewNode ); traversedModelOffset += lastLength; - viewIndex++; + viewOffset++; if ( useCache ) { - this._cache.save( viewParent, viewIndex, viewContainer, traversedModelOffset ); + // Note, that we cache the view position before and after a visited element here, so before we (possibly) "enter" it + // (see `else` below). + // + // Since `MapperCache#save` does not overwrite already cached model offsets, this way the cached position is set to + // a correct location, that is the closest to the mapped `viewContainer`. + // + // However, in some cases, we still need to "hoist" the cached position (see `MapperCache#_hoistViewPosition`). + this._cache.save( viewParent, viewOffset, viewContainer, traversedModelOffset ); } } if ( traversedModelOffset == targetModelOffset ) { // If it equals we found the position. - return this._moveViewPositionToTextNode( new ViewPosition( viewParent, viewIndex ) ); + return this._moveViewPositionToTextNode( new ViewPosition( viewParent, viewOffset ) ); } else { // If it is higher we overstepped with the last traversed view node. @@ -812,17 +819,12 @@ export class MapperCache extends /* #__PURE__ */ EmitterMixin() { private _nodeToCacheListIndex = new WeakMap(); /** - * Saves cache for the given data. + * Saves cache for given view position mapping <-> model offset mapping. * - * More precisely, for given `modelOffset` inside a model element mapped to `viewContainer`, it saves that mapped view position is - * in `viewParent` at offset `viewOffset`. - * - * `viewParent` is `viewContainer`, or is a non-mapped descendant of `viewContainer`. - * - * @param viewParent - * @param viewOffset - * @param viewContainer - * @param modelOffset + * @param viewParent View position parent. + * @param viewOffset View position offset. + * @param viewContainer Tracked view position ascendant (it may be the direct parent of the view position). + * @param modelOffset Model offset in the model element or document fragment which is mapped to `viewContainer`. */ public save( viewParent: ViewElement | ViewDocumentFragment, @@ -889,19 +891,19 @@ export class MapperCache extends /* #__PURE__ */ EmitterMixin() { } /** - * For given `modelOffset` inside a model element mapped to `viewContainer`, it returns the closest - * {@link ~CacheItem cache item (view position and related model offset)} to the requested model offset one. + * For given `modelOffset` inside a model element mapped to given `viewContainer`, it returns the closest saved + * {@link ~CacheItem cache item (view position and related model offset)} to the requested one. * - * It can be the exact, requested mapping, or it can be mapping that is the closest starting point to look for the requested mapping. + * It can be exactly the requested mapping, or it can be mapping that is the closest starting point to look for the requested mapping. * * `viewContainer` must be a view element or document fragment that is mapped by the {@link ~Mapper Mapper}. * * If `viewContainer` is not yet tracked by the `MapperCache`, it will be automatically tracked after calling this method. * - * @param viewContainer - * @param modelOffset + * @param viewContainer Tracked view element or document fragment, which cache will be used. + * @param modelOffset Model offset in a model element or document fragment, which is mapped to `viewContainer`. */ - public get( viewContainer: ViewElement | ViewDocumentFragment, modelOffset: number ): CacheItem { + public getClosest( viewContainer: ViewElement | ViewDocumentFragment, modelOffset: number ): CacheItem { const cache = this._cachedMapping.get( viewContainer ); let result: CacheItem; @@ -929,14 +931,28 @@ export class MapperCache extends /* #__PURE__ */ EmitterMixin() { /** * Moves a view position to a preferred location. * - * The view position is moved up from a non-tracked view element as long as it remains at the end of its current parent. Example: + * The view position is moved up from a non-tracked view element as long as it remains at the end of its current parent. + * + * See example below to understand when it is important: + * + * Starting state: + * + * ``` + *

This is some heavily formatted^ piece of text.

+ * ``` + * + * Then we remove " piece of " and invalidate some cache: * * ``` - *

This is some formatted^ text.

->

This is some formatted^ text.

+ *

This is some heavily formatted^ text.

* ``` * - * @param viewPosition - * @private + * Now, if we ask for model offset after letter "d" in "formatted", we should get a position in " text", but we will get in ``. + * For this scenario, we need to hoist the position. + * + * ``` + *

This is some heavily formatted^ text.

+ * ``` */ private _hoistViewPosition( viewPosition: ViewPosition ): ViewPosition { while ( viewPosition.parent.parent && !this._cachedMapping.has( viewPosition.parent as any ) && viewPosition.isAtEnd ) { @@ -958,8 +974,6 @@ export class MapperCache extends /* #__PURE__ */ EmitterMixin() { * This method initializes the cache for `viewContainer` and adds callbacks for * {@link module:engine/view/node~ViewNodeChangeEvent `change` event} fired by `viewContainer`. `MapperCache` listens to `change` event * on the tracked elements to invalidate the stored cache. - * - * @param viewContainer */ public startTracking( viewContainer: ViewElement | ViewDocumentFragment ): CacheItem { const viewPosition = new ViewPosition( viewContainer, 0 ); @@ -982,7 +996,7 @@ export class MapperCache extends /* #__PURE__ */ EmitterMixin() { this.listenTo( viewContainer, 'change:text', ( evt, viewNode ) => { // Text node has changed. Clear all the cache starting from before this text node. - this._clearCacheBefore( viewNode as ViewText ); + this._clearCacheStartingBefore( viewNode as ViewText ); } ); return initialCacheItem; @@ -993,8 +1007,6 @@ export class MapperCache extends /* #__PURE__ */ EmitterMixin() { * * It removes the cached data and stops listening to {@link module:engine/view/node~ViewNodeChangeEvent `change` event} on the * `viewContainer`. - * - * @param viewContainer */ public stopTracking( viewContainer: ViewElement | ViewDocumentFragment ): void { this.stopListening( viewContainer ); @@ -1004,9 +1016,6 @@ export class MapperCache extends /* #__PURE__ */ EmitterMixin() { /** * Invalidates cache after a change happens inside `viewParent` on given `index`. - * - * @param viewParent - * @param index */ private _invalidateCacheOnChildrenChange( viewParent: ViewElement | ViewDocumentFragment, index: number ) { if ( index == 0 ) { @@ -1016,7 +1025,7 @@ export class MapperCache extends /* #__PURE__ */ EmitterMixin() { this._clearCacheAll( viewParent ); } else { // If this is not a tracked element, remove cache starting from before this element. - this._clearCacheBefore( viewParent as ViewElement ); + this._clearCacheStartingBefore( viewParent as ViewElement ); } } else { // Change in the middle of the parent. Get a view node that's before the change. @@ -1032,14 +1041,12 @@ export class MapperCache extends /* #__PURE__ */ EmitterMixin() { // Now, if we add some text in ``, we won't validate this cached item even though it gets outdated. // So, if there's a need to have `_clearCacheAfter()`, we need to solve the above case first. // - this._clearCacheBefore( lastValidNode ); + this._clearCacheStartingBefore( lastValidNode ); } } /** * Clears all the cache for given tracked `viewContainer`. - * - * @param viewContainer */ private _clearCacheAll( viewContainer: ViewElement | ViewDocumentFragment ) { const cache = this._cachedMapping.get( viewContainer )!; @@ -1055,11 +1062,9 @@ export class MapperCache extends /* #__PURE__ */ EmitterMixin() { /** * Clears all the stored cache starting before given `viewNode`. The `viewNode` can be any node that is inside a tracked view element * or view document fragment. - * - * @param viewNode */ - private _clearCacheBefore( viewNode: ViewNode ): void { - // To quickly invalidate the cache, we base on the cache list index stored with the node. See docs for `this._nodeToCacheListIndex`. + private _clearCacheStartingBefore( viewNode: ViewNode ): void { + // To quickly invalidateclearCacheBefore the cache, we base on the cache list index stored with the node. See docs for `this._nodeToCacheListIndex`. const cacheListIndex = this._nodeToCacheListIndex.get( viewNode ); // If there is no index stored, it means that this `viewNode` has not been cached yet. @@ -1068,16 +1073,20 @@ export class MapperCache extends /* #__PURE__ */ EmitterMixin() { // Note, that there always must be a parent if we got here. const viewParent = viewNode.parent as ViewElement; + // If the parent is a non-tracked element, try clearing the cache starting before it. + // + // This situation may happen e.g. if structure like `

Foo...` was stepped over in + // `Mapper#_findPositionIn()` and the children are not cached yet, but the `` element is. If something changes + // inside this structure, make sure to invalidate all the cache after ``. + // + // If the parent is a tracked element, then it means there's no cache to clear (nothing after the element is cached). + // In this case, there's nothing to do. + // if ( !this._cachedMapping.has( viewParent ) ) { - // If the parent is a non-tracked element, try clearing the cache starting before it. - // This situation may happen e.g. if structure like `

Foo...` was stepped over in - // `Mapper#_findPositionIn()` and the children are not cached yet, but the `` element is. If something changes - // inside this structure, make sure to invalidate all the cache after ``. - return this._clearCacheBefore( viewParent ); - } else { - // If the parent is a tracked element, then it means there's no cache to clear (nothing after the element has been cached). - return; + this._clearCacheStartingBefore( viewParent ); } + + return; } // Note: there was a consideration to save the `viewContainer` value together with `cacheListIndex` value. @@ -1096,9 +1105,6 @@ export class MapperCache extends /* #__PURE__ */ EmitterMixin() { /** * Clears all the cache in the cache list related to given `viewContainer`, starting from `index` (inclusive). - * - * @param viewContainer - * @param index */ private _clearCacheFromIndex( viewContainer: ViewElement | ViewDocumentFragment, index: number ) { // Cache is always available here because we initialize it just before adding a listener that fires `_clearCacheFromIndex()`. @@ -1127,9 +1133,6 @@ export class MapperCache extends /* #__PURE__ */ EmitterMixin() { * Finds a cache item in the given cache list, which `modelOffset` is closest (but smaller or equal) to given `offset`. * * Since `cacheList` is a sorted array, this uses binary search to retrieve the item quickly. - * - * @param cacheList - * @param offset */ private _findInCacheList( cacheList: Array, offset: number ): CacheItem { let start = 0; @@ -1353,12 +1356,13 @@ type MappingCache = { * mapped to the `viewPosition` parent or its ancestor). * * For example, for model `Some <$text bold=true>bold text` and view `

Some bold text

` - * and assuming `` and `

` are mapped, following `CacheItem`s are possible: + * and assuming `` and `

` are mapped, following example `CacheItem`s are possible: * * * `viewPosition` = `

`, 1; `modelOffset` = 5 - * * `viewPosition` = `, 1; `modelOffset` = 9 * * `viewPosition` = `"bold"`, 2; `modelOffset` = 7 - * * `viewPosition` = `" text"`, 0; `modelOffset` = 7 + * * `viewPosition` = `, 1; `modelOffset` = 9 + * * `viewPosition` = `" text"`, 0; `modelOffset` = 9 + * * `viewPosition` = `

`, 2; `modelOffset` = 9 */ type CacheItem = { viewPosition: ViewPosition, diff --git a/packages/ckeditor5-engine/src/view/documentfragment.ts b/packages/ckeditor5-engine/src/view/documentfragment.ts index be4d811aeed..3dfd5b7b82f 100644 --- a/packages/ckeditor5-engine/src/view/documentfragment.ts +++ b/packages/ckeditor5-engine/src/view/documentfragment.ts @@ -17,7 +17,6 @@ import type { default as Document, ChangeType } from './document.js'; import type Item from './item.js'; import type Node from './node.js'; -import { ViewNodeChangeEvent } from "./node.js"; /** * Document fragment. From e0d2a00257c26612fb58684ddc0aaa09aee5604f Mon Sep 17 00:00:00 2001 From: Szymon Cofalik Date: Mon, 9 Dec 2024 12:02:25 +0100 Subject: [PATCH 04/15] Fix `MapperCache` tests. --- .../tests/conversion/mapper.js | 20 +++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/packages/ckeditor5-engine/tests/conversion/mapper.js b/packages/ckeditor5-engine/tests/conversion/mapper.js index 3ca7b9f6293..7ec2e1836cb 100644 --- a/packages/ckeditor5-engine/tests/conversion/mapper.js +++ b/packages/ckeditor5-engine/tests/conversion/mapper.js @@ -876,7 +876,7 @@ describe( 'Mapper', () => { } ); } ); -describe.only( 'MapperCache', () => { +describe( 'MapperCache', () => { let cache, viewContainer, viewDocument, viewTextAb, viewSpan, viewEm, viewTextCd, viewTextEf, viewTextGh; beforeEach( () => { @@ -893,9 +893,9 @@ describe.only( 'MapperCache', () => { viewContainer = new ViewElement( viewDocument, 'p', null, [ viewTextAb, viewSpan, viewTextGh ] ); } ); - describe( 'get()', () => { + describe( 'getClosest()', () => { it( 'should start tracking and return position at the start of element if element was not tracked before', () => { - const { viewPosition, modelOffset } = cache.get( viewContainer, 6 ); + const { viewPosition, modelOffset } = cache.getClosest( viewContainer, 6 ); expect( viewPosition.parent ).to.equal( viewContainer ); expect( viewPosition.offset ).to.equal( 0 ); @@ -906,7 +906,7 @@ describe.only( 'MapperCache', () => { cache.startTracking( viewContainer ); cache.save( viewContainer, 2, viewContainer, 6 ); - const { viewPosition, modelOffset } = cache.get( viewContainer, 6 ); + const { viewPosition, modelOffset } = cache.getClosest( viewContainer, 6 ); expect( viewPosition.parent ).to.equal( viewContainer ); expect( viewPosition.offset ).to.equal( 2 ); @@ -917,7 +917,7 @@ describe.only( 'MapperCache', () => { cache.startTracking( viewContainer ); cache.save( viewEm, 0, viewContainer, 4 ); - const { viewPosition, modelOffset } = cache.get( viewContainer, 4 ); + const { viewPosition, modelOffset } = cache.getClosest( viewContainer, 4 ); expect( viewPosition.parent ).to.equal( viewEm ); expect( viewPosition.offset ).to.equal( 0 ); @@ -928,7 +928,7 @@ describe.only( 'MapperCache', () => { cache.startTracking( viewContainer ); cache.save( viewContainer, 2, viewContainer, 6 ); - const { viewPosition, modelOffset } = cache.get( viewContainer, 8 ); + const { viewPosition, modelOffset } = cache.getClosest( viewContainer, 8 ); expect( viewPosition.parent ).to.equal( viewContainer ); expect( viewPosition.offset ).to.equal( 2 ); @@ -939,7 +939,7 @@ describe.only( 'MapperCache', () => { cache.startTracking( viewContainer ); cache.save( viewEm, 0, viewContainer, 4 ); - const { viewPosition, modelOffset } = cache.get( viewContainer, 8 ); + const { viewPosition, modelOffset } = cache.getClosest( viewContainer, 8 ); expect( viewPosition.parent ).to.equal( viewEm ); expect( viewPosition.offset ).to.equal( 0 ); @@ -969,7 +969,7 @@ describe.only( 'MapperCache', () => { cache.save( viewContainer, 2, viewContainer, 6 ); cache.save( viewSpan, 2, viewContainer, 6 ); - const { viewPosition, modelOffset } = cache.get( viewContainer, 6 ); + const { viewPosition, modelOffset } = cache.getClosest( viewContainer, 6 ); expect( viewPosition.parent ).to.equal( viewContainer ); expect( viewPosition.offset ).to.equal( 2 ); @@ -983,7 +983,7 @@ describe.only( 'MapperCache', () => { cache.save( viewContainer, 2, viewContainer, 6 ); cache.stopTracking( viewContainer ); - const { viewPosition, modelOffset } = cache.get( viewContainer, 2 ); + const { viewPosition, modelOffset } = cache.getClosest( viewContainer, 2 ); expect( viewPosition.parent ).to.equal( viewContainer ); expect( viewPosition.offset ).to.equal( 0 ); @@ -1190,7 +1190,7 @@ describe.only( 'MapperCache', () => { } ); function check( modelOffsetToCheck, expectedViewParent, expectedViewOffset, expectedModelOffset ) { - const { viewPosition, modelOffset } = cache.get( viewContainer, modelOffsetToCheck ); + const { viewPosition, modelOffset } = cache.getClosest( viewContainer, modelOffsetToCheck ); expect( viewPosition.parent ).to.equal( expectedViewParent ); expect( viewPosition.offset ).to.equal( expectedViewOffset ); From 34ec04ac9ec3906f3e613cf78015bd776bdcde2a Mon Sep 17 00:00:00 2001 From: Szymon Cofalik Date: Mon, 9 Dec 2024 12:02:57 +0100 Subject: [PATCH 05/15] Remove unnecessary condition in `MapperCache#save()`. --- packages/ckeditor5-engine/src/conversion/mapper.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/ckeditor5-engine/src/conversion/mapper.ts b/packages/ckeditor5-engine/src/conversion/mapper.ts index d014cbc6c81..e6f75ccc438 100644 --- a/packages/ckeditor5-engine/src/conversion/mapper.ts +++ b/packages/ckeditor5-engine/src/conversion/mapper.ts @@ -837,8 +837,8 @@ export class MapperCache extends /* #__PURE__ */ EmitterMixin() { // See if there is already a cache defined for `modelOffset`. const cacheItem = cache.cacheMap.get( modelOffset ); - if ( modelOffset <= cache.maxModelOffset && cacheItem ) { - // We have a valid cache, and we already cached this offset. Don't overwrite the cache. + if ( cacheItem ) { + // We already cached this offset. Don't overwrite the cache. // // This assumes that `Mapper` works in a way that we first cache the parent and only then cache children, as we prefer position // after the parent ("closer" to the tracked ancestor). It might be safer to check which position is preferred (newly saved or From 97ee894d27e37c8b00e2680ebeda6891f42888b1 Mon Sep 17 00:00:00 2001 From: Szymon Cofalik Date: Mon, 9 Dec 2024 12:41:03 +0100 Subject: [PATCH 06/15] Better typing for `change:children` event vs `change:text` and `change:attributes` events. --- .../ckeditor5-engine/src/conversion/mapper.ts | 10 ++--- .../src/view/documentfragment.ts | 2 +- packages/ckeditor5-engine/src/view/node.ts | 39 +++++++++++++------ 3 files changed, 34 insertions(+), 17 deletions(-) diff --git a/packages/ckeditor5-engine/src/conversion/mapper.ts b/packages/ckeditor5-engine/src/conversion/mapper.ts index e6f75ccc438..7bf5927e623 100644 --- a/packages/ckeditor5-engine/src/conversion/mapper.ts +++ b/packages/ckeditor5-engine/src/conversion/mapper.ts @@ -20,7 +20,7 @@ import type ViewElement from '../view/element.js'; import type ViewText from '../view/text.js'; import type ModelElement from '../model/element.js'; import type ModelDocumentFragment from '../model/documentfragment.js'; -import type ViewNode from '../view/node.js'; +import type { default as ViewNode, ViewNodeChangeChildrenEvent, ViewNodeChangeEvent } from '../view/node.js'; /** * Maps elements, positions and markers between the {@link module:engine/view/document~Document view} and @@ -990,13 +990,13 @@ export class MapperCache extends /* #__PURE__ */ EmitterMixin() { // // Possible performance improvement. This event bubbles, so if there are multiple tracked (mapped) elements that are ancestors // then this will be unnecessarily fired for each ancestor. This could be rewritten to listen only to roots and document fragments. - this.listenTo( viewContainer, 'change:children', ( evt, viewNode, data ) => { - this._invalidateCacheOnChildrenChange( viewNode as ViewElement | ViewDocumentFragment, ( data as any ).index as number ); + this.listenTo( viewContainer, 'change:children', ( evt, viewNode, data ) => { + this._invalidateCacheOnChildrenChange( viewNode, data.index ); } ); - this.listenTo( viewContainer, 'change:text', ( evt, viewNode ) => { + this.listenTo( viewContainer, 'change:text', ( evt, viewNode ) => { // Text node has changed. Clear all the cache starting from before this text node. - this._clearCacheStartingBefore( viewNode as ViewText ); + this._clearCacheStartingBefore( viewNode ); } ); return initialCacheItem; diff --git a/packages/ckeditor5-engine/src/view/documentfragment.ts b/packages/ckeditor5-engine/src/view/documentfragment.ts index 3dfd5b7b82f..ecac55ddbbc 100644 --- a/packages/ckeditor5-engine/src/view/documentfragment.ts +++ b/packages/ckeditor5-engine/src/view/documentfragment.ts @@ -222,7 +222,7 @@ export default class DocumentFragment extends /* #__PURE__ */ EmitterMixin( Type * @param data Additional data. * @fires change */ - public _fireChange( type: ChangeType, node: Node | DocumentFragment, data?: unknown ): void { + public _fireChange( type: ChangeType, node: Node | DocumentFragment, data?: { index: number } ): void { this.fire( `change:${ type }`, node, data ); } diff --git a/packages/ckeditor5-engine/src/view/node.ts b/packages/ckeditor5-engine/src/view/node.ts index 7af5dab7adf..0f617287290 100644 --- a/packages/ckeditor5-engine/src/view/node.ts +++ b/packages/ckeditor5-engine/src/view/node.ts @@ -260,8 +260,12 @@ export default abstract class Node extends /* #__PURE__ */ EmitterMixin( TypeChe * @param data Additional data. * @fires change */ - public _fireChange( type: ChangeType, node: Node, data?: unknown ): void { - this.fire( `change:${ type }`, node, data ); + public _fireChange( type: ChangeType, node: Node, data?: { index: number } ): void { + if ( type == 'children' ) { + this.fire( 'change:children', node, data! ); + } else { + this.fire( `change:${ type }`, node ); + } if ( this.parent ) { this.parent._fireChange( type, node, data ); @@ -305,20 +309,33 @@ Node.prototype.is = function( type: string ): boolean { }; /** - * Fired when list of {@link module:engine/view/element~Element elements} children, attributes or text changes. - * - * Change event is bubbled – it is fired on all ancestors. + * Fired when node attributes or text changes. * - * All change events as the first parameter receive the node that has changed (the node for which children, attributes or text changed). + * This event is bubbled – it is fired on all ancestors of the changed node. * - * If `change:children` event is fired, there is an additional second parameter, which is an object with additional data related to change. + * The first parameter and only parameter is the node that has changed (the node for which attributes or text changed). * * @eventName ~Node#change - * @eventName ~Node#change:children - * @eventName ~Node#change:attributes * @eventName ~Node#change:text + * @eventName ~Node#change:attributes */ export type ViewNodeChangeEvent = { - name: 'change' | `change:${ ChangeType }`; - args: [ changedNode: Node, data?: unknown ]; + name: 'change:text' | 'change:attributes'; + args: [ changedNode: Node ]; +}; + +/** + * Fired when the list of {@link module:engine/view/element~Element element's} children changes (i.e. a child is added to or removed from + * an element). If multiple children are added or removed, there is only one event. + * + * This event is bubbled – it is fired on all ancestors of the changed element. + * + * The first parameter is the element that has changed (the element for which children list changed). + * The second parameter is an object with `index` property, which informs on which index the change happened. + * + * @eventName ~Node#change:children + */ +export type ViewNodeChangeChildrenEvent = { + name: 'change:children'; + args: [ changedNode: Element | DocumentFragment, data: { index: number } ]; }; From 0b59c1180694f813ecbda6bea553976983846532 Mon Sep 17 00:00:00 2001 From: Szymon Cofalik Date: Mon, 9 Dec 2024 13:37:13 +0100 Subject: [PATCH 07/15] Reverted some of the changes related to view nodes `change` event types. Reverted some of the changes related to view nodes `change` event types. Reverted type change. --- .../ckeditor5-engine/src/conversion/mapper.ts | 8 ++-- packages/ckeditor5-engine/src/view/node.ts | 37 +++++-------------- 2 files changed, 14 insertions(+), 31 deletions(-) diff --git a/packages/ckeditor5-engine/src/conversion/mapper.ts b/packages/ckeditor5-engine/src/conversion/mapper.ts index 7bf5927e623..915c8b28449 100644 --- a/packages/ckeditor5-engine/src/conversion/mapper.ts +++ b/packages/ckeditor5-engine/src/conversion/mapper.ts @@ -20,7 +20,7 @@ import type ViewElement from '../view/element.js'; import type ViewText from '../view/text.js'; import type ModelElement from '../model/element.js'; import type ModelDocumentFragment from '../model/documentfragment.js'; -import type { default as ViewNode, ViewNodeChangeChildrenEvent, ViewNodeChangeEvent } from '../view/node.js'; +import type { default as ViewNode, ViewNodeChangeEvent } from '../view/node.js'; /** * Maps elements, positions and markers between the {@link module:engine/view/document~Document view} and @@ -490,7 +490,7 @@ export default class Mapper extends /* #__PURE__ */ EmitterMixin() { * * @param viewPosition Position for which a mapped ancestor should be found. */ - public findMappedViewAncestor( viewPosition: ViewPosition ): ViewElement | ViewDocumentFragment { + public findMappedViewAncestor( viewPosition: ViewPosition ): ViewElement { let parent: any = viewPosition.parent; while ( !this._viewToModelMapping.has( parent ) ) { @@ -990,8 +990,8 @@ export class MapperCache extends /* #__PURE__ */ EmitterMixin() { // // Possible performance improvement. This event bubbles, so if there are multiple tracked (mapped) elements that are ancestors // then this will be unnecessarily fired for each ancestor. This could be rewritten to listen only to roots and document fragments. - this.listenTo( viewContainer, 'change:children', ( evt, viewNode, data ) => { - this._invalidateCacheOnChildrenChange( viewNode, data.index ); + this.listenTo( viewContainer, 'change:children', ( evt, viewNode, data ) => { + this._invalidateCacheOnChildrenChange( viewNode as ViewElement | ViewDocumentFragment, data!.index ); } ); this.listenTo( viewContainer, 'change:text', ( evt, viewNode ) => { diff --git a/packages/ckeditor5-engine/src/view/node.ts b/packages/ckeditor5-engine/src/view/node.ts index 0f617287290..502d8275b08 100644 --- a/packages/ckeditor5-engine/src/view/node.ts +++ b/packages/ckeditor5-engine/src/view/node.ts @@ -261,11 +261,7 @@ export default abstract class Node extends /* #__PURE__ */ EmitterMixin( TypeChe * @fires change */ public _fireChange( type: ChangeType, node: Node, data?: { index: number } ): void { - if ( type == 'children' ) { - this.fire( 'change:children', node, data! ); - } else { - this.fire( `change:${ type }`, node ); - } + this.fire( `change:${ type }`, node, data ); if ( this.parent ) { this.parent._fireChange( type, node, data ); @@ -309,33 +305,20 @@ Node.prototype.is = function( type: string ): boolean { }; /** - * Fired when node attributes or text changes. + * Fired when list of {@link module:engine/view/element~Element elements} children, attributes or text changes. + * + * Change event is bubbled – it is fired on all ancestors. * - * This event is bubbled – it is fired on all ancestors of the changed node. + * All change events as the first parameter receive the node that has changed (the node for which children, attributes or text changed). * - * The first parameter and only parameter is the node that has changed (the node for which attributes or text changed). + * If `change:children` event is fired, there is an additional second parameter, which is an object with additional data related to change. * * @eventName ~Node#change - * @eventName ~Node#change:text + * @eventName ~Node#change:children * @eventName ~Node#change:attributes + * @eventName ~Node#change:text */ export type ViewNodeChangeEvent = { - name: 'change:text' | 'change:attributes'; - args: [ changedNode: Node ]; -}; - -/** - * Fired when the list of {@link module:engine/view/element~Element element's} children changes (i.e. a child is added to or removed from - * an element). If multiple children are added or removed, there is only one event. - * - * This event is bubbled – it is fired on all ancestors of the changed element. - * - * The first parameter is the element that has changed (the element for which children list changed). - * The second parameter is an object with `index` property, which informs on which index the change happened. - * - * @eventName ~Node#change:children - */ -export type ViewNodeChangeChildrenEvent = { - name: 'change:children'; - args: [ changedNode: Element | DocumentFragment, data: { index: number } ]; + name: 'change' | `change:${ ChangeType }`; + args: [ changedNode: Node, data?: { index: number } ]; }; From 9d28a7568f3d1f94e3f27c59c82f696cff63bdd5 Mon Sep 17 00:00:00 2001 From: Szymon Cofalik Date: Mon, 9 Dec 2024 14:55:54 +0100 Subject: [PATCH 08/15] Lint errors. --- .../ckeditor5-engine/src/conversion/mapper.ts | 17 +++++++++-------- .../ckeditor5-engine/tests/conversion/mapper.js | 3 +-- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/packages/ckeditor5-engine/src/conversion/mapper.ts b/packages/ckeditor5-engine/src/conversion/mapper.ts index 915c8b28449..5b482b762e9 100644 --- a/packages/ckeditor5-engine/src/conversion/mapper.ts +++ b/packages/ckeditor5-engine/src/conversion/mapper.ts @@ -13,7 +13,7 @@ import ModelRange from '../model/range.js'; import ViewPosition from '../view/position.js'; import ViewRange from '../view/range.js'; -import { CKEditorError, EmitterMixin, EventInfo } from '@ckeditor/ckeditor5-utils'; +import { CKEditorError, EmitterMixin } from '@ckeditor/ckeditor5-utils'; import type ViewDocumentFragment from '../view/documentfragment.js'; import type ViewElement from '../view/element.js'; @@ -669,7 +669,7 @@ export default class Mapper extends /* #__PURE__ */ EmitterMixin() { viewContainer: ViewElement | ViewDocumentFragment, useCache: boolean ): ViewPosition { - let viewParent = startViewPosition.parent as ViewElement | ViewText | ViewDocumentFragment; + const viewParent = startViewPosition.parent as ViewElement | ViewText | ViewDocumentFragment; let viewOffset = startViewPosition.offset; // In the text node it is simple: the offset in the model equals the offset in the text. @@ -1064,7 +1064,7 @@ export class MapperCache extends /* #__PURE__ */ EmitterMixin() { * or view document fragment. */ private _clearCacheStartingBefore( viewNode: ViewNode ): void { - // To quickly invalidateclearCacheBefore the cache, we base on the cache list index stored with the node. See docs for `this._nodeToCacheListIndex`. + // To quickly invalidate the cache, we base on the cache list index stored with the node. See docs for `this._nodeToCacheListIndex`. const cacheListIndex = this._nodeToCacheListIndex.get( viewNode ); // If there is no index stored, it means that this `viewNode` has not been cached yet. @@ -1322,12 +1322,13 @@ export type MapperViewToModelPositionEventData = { * one `MappingCache` for each model element or a model document fragment). */ type MappingCache = { + /** * Maximum model offset for which this item holds a valid cache. * * If there is a request for mapping for a model offset above this value, the view position needs to be calculated. */ - maxModelOffset: number, + maxModelOffset: number; /** * List of cached {@link ~CacheItem cache items} for this mapped view/model element. The list is sorted by @@ -1338,7 +1339,7 @@ type MappingCache = { * * Note, that there is only one entry in `cacheList` for given `modelOffset` value. */ - cacheList: Array, + cacheList: Array; /** * Map of cached {@link ~CacheItem cache items} for this mapped view/model element. The keys are `modelOffset`s @@ -1348,7 +1349,7 @@ type MappingCache = { * retrieve data for exact `modelOffset`s which were already cached before. But a sorted list is better for finding the closest item * if exact item is not found. Also, it is faster to clear "all entries after model offset X" for `cacheList`. */ - cacheMap: Map + cacheMap: Map; }; /** @@ -1365,6 +1366,6 @@ type MappingCache = { * * `viewPosition` = `

`, 2; `modelOffset` = 9 */ type CacheItem = { - viewPosition: ViewPosition, - modelOffset: number + viewPosition: ViewPosition; + modelOffset: number; }; diff --git a/packages/ckeditor5-engine/tests/conversion/mapper.js b/packages/ckeditor5-engine/tests/conversion/mapper.js index 7ec2e1836cb..774dfab516f 100644 --- a/packages/ckeditor5-engine/tests/conversion/mapper.js +++ b/packages/ckeditor5-engine/tests/conversion/mapper.js @@ -3,8 +3,7 @@ * For licensing, see LICENSE.md or https://ckeditor.com/legal/ckeditor-licensing-options */ -import Mapper from '../../src/conversion/mapper.js'; -import { MapperCache } from '../../src/conversion/mapper.js'; +import Mapper, { MapperCache } from '../../src/conversion/mapper.js'; import ModelElement from '../../src/model/element.js'; import ModelRootElement from '../../src/model/rootelement.js'; From e959c4c9250ce02ab294671638bf06ae933e8818 Mon Sep 17 00:00:00 2001 From: Szymon Cofalik Date: Mon, 9 Dec 2024 19:06:16 +0100 Subject: [PATCH 09/15] `MapperCache#_getClosest()` changed cached item value when it used `_hoistViewPosition()`. In some cases, we need to go "go up" in `Mapper#_findPositionStartingFrom()`. --- .../ckeditor5-engine/src/conversion/mapper.ts | 40 +++++++++++++++---- 1 file changed, 32 insertions(+), 8 deletions(-) diff --git a/packages/ckeditor5-engine/src/conversion/mapper.ts b/packages/ckeditor5-engine/src/conversion/mapper.ts index 5b482b762e9..0397c310a57 100644 --- a/packages/ckeditor5-engine/src/conversion/mapper.ts +++ b/packages/ckeditor5-engine/src/conversion/mapper.ts @@ -669,7 +669,7 @@ export default class Mapper extends /* #__PURE__ */ EmitterMixin() { viewContainer: ViewElement | ViewDocumentFragment, useCache: boolean ): ViewPosition { - const viewParent = startViewPosition.parent as ViewElement | ViewText | ViewDocumentFragment; + let viewParent = startViewPosition.parent as ViewElement | ViewText | ViewDocumentFragment; let viewOffset = startViewPosition.offset; // In the text node it is simple: the offset in the model equals the offset in the text. @@ -678,14 +678,36 @@ export default class Mapper extends /* #__PURE__ */ EmitterMixin() { } // Last scanned view node. - let viewNode: ViewNode; + let viewNode: ViewNode | undefined; // Total model offset of the view nodes that were visited so far. let traversedModelOffset = startModelOffset; // Model length of the last traversed view node. let lastLength = 0; while ( traversedModelOffset < targetModelOffset ) { - viewNode = viewParent.getChild( viewOffset )!; + viewNode = viewParent.getChild( viewOffset ); + + if ( !viewNode ) { + // If we still haven't reached the model offset, but we reached end of this `viewParent`, then we need to "leave" this + // element and "go up", looking further for the target model offset. This can happen when cached model offset is "deeper" + // but target model offset is "higher" in the view tree. + // + // Example: `

FooBar^BazXyz

` + // + // Consider `^` is last cached position, when the `targetModelOffset` is `12`. In such case, we need to "go up" from + // `` and continue traversing in `

`. + // + if ( viewParent == viewContainer ) { + // TODO + throw 'Reached end'; + } else { + viewOffset = viewParent.parent!.getChildIndex( viewParent as ViewElement ); + viewParent = viewParent.parent!; + + continue; + } + } + lastLength = this.getModelLength( viewNode ); traversedModelOffset += lastLength; viewOffset++; @@ -697,7 +719,7 @@ export default class Mapper extends /* #__PURE__ */ EmitterMixin() { // Since `MapperCache#save` does not overwrite already cached model offsets, this way the cached position is set to // a correct location, that is the closest to the mapped `viewContainer`. // - // However, in some cases, we still need to "hoist" the cached position (see `MapperCache#_hoistViewPosition`). + // However, in some cases, we still need to "hoist" the cached position (see `MapperCache#_hoistViewPosition()`). this._cache.save( viewParent, viewOffset, viewContainer, traversedModelOffset ); } } @@ -705,8 +727,7 @@ export default class Mapper extends /* #__PURE__ */ EmitterMixin() { if ( traversedModelOffset == targetModelOffset ) { // If it equals we found the position. return this._moveViewPositionToTextNode( new ViewPosition( viewParent, viewOffset ) ); - } - else { + } else { // If it is higher we overstepped with the last traversed view node. // We need to "enter" it, and look for the view position / model offset inside the last visited view node. return this._findPositionStartingFrom( @@ -923,9 +944,12 @@ export class MapperCache extends /* #__PURE__ */ EmitterMixin() { result = this.startTracking( viewContainer ); } - result.viewPosition = this._hoistViewPosition( result.viewPosition ); + const viewPosition = this._hoistViewPosition( result.viewPosition ); - return result; + return { + modelOffset: result.modelOffset, + viewPosition + }; } /** From 515192b51381563e414a4ea1e1042fe8eb28cf06 Mon Sep 17 00:00:00 2001 From: Szymon Cofalik Date: Fri, 13 Dec 2024 18:55:40 +0100 Subject: [PATCH 10/15] Added missing unit tests. Fixed one scenario involving half-valid cache. --- .../ckeditor5-engine/src/conversion/mapper.ts | 27 +++++- .../tests/conversion/mapper.js | 82 ++++++++++++++++++- 2 files changed, 105 insertions(+), 4 deletions(-) diff --git a/packages/ckeditor5-engine/src/conversion/mapper.ts b/packages/ckeditor5-engine/src/conversion/mapper.ts index 0397c310a57..95e03fd7d84 100644 --- a/packages/ckeditor5-engine/src/conversion/mapper.ts +++ b/packages/ckeditor5-engine/src/conversion/mapper.ts @@ -698,10 +698,15 @@ export default class Mapper extends /* #__PURE__ */ EmitterMixin() { // `` and continue traversing in `

`. // if ( viewParent == viewContainer ) { - // TODO - throw 'Reached end'; + /** + * A model position could not be mapped to the view because specified model offset was too big and could not be + * found inside the mapped view element or view document fragment. + * + * @error mapping-model-offset-not-found + */ + throw new CKEditorError( 'mapping-model-offset-not-found', this, { modelOffset: targetModelOffset, viewContainer } ); } else { - viewOffset = viewParent.parent!.getChildIndex( viewParent as ViewElement ); + viewOffset = viewParent.parent!.getChildIndex( viewParent as ViewElement ) + 1; viewParent = viewParent.parent!; continue; @@ -921,6 +926,22 @@ export class MapperCache extends /* #__PURE__ */ EmitterMixin() { * * If `viewContainer` is not yet tracked by the `MapperCache`, it will be automatically tracked after calling this method. * + * Note: this method will automatically "hoist" cached positions, i.e. it will return a position that is closest to the tracked element. + * + * For example, if `

` is tracked element, and `^` is cached position: + * + * ``` + *

This is some heavily formatted^ text.

+ * ``` + * + * If this position would be returned, instead, a position directly in `

` would be returned: + * + * ``` + *

This is some heavily formatted^ text.

+ * ``` + * + * Note, that `modelOffset` for both positions is the same. + * * @param viewContainer Tracked view element or document fragment, which cache will be used. * @param modelOffset Model offset in a model element or document fragment, which is mapped to `viewContainer`. */ diff --git a/packages/ckeditor5-engine/tests/conversion/mapper.js b/packages/ckeditor5-engine/tests/conversion/mapper.js index 774dfab516f..a36e704b317 100644 --- a/packages/ckeditor5-engine/tests/conversion/mapper.js +++ b/packages/ckeditor5-engine/tests/conversion/mapper.js @@ -468,6 +468,72 @@ describe( 'Mapper', () => { it( 'should transform modelP 9', () => createToViewTest( modelP, 9, viewTextB, 1 ) ); it( 'should transform modelP 10', () => createToViewTest( modelP, 10, viewTextM, 0 ) ); it( 'should transform modelP 11', () => createToViewTest( modelP, 11, viewP, 5 ) ); + + // Below tests a particular code execution path that can happen only if cache for given mapped element ends deep (in view) + // in that element while we request mapping that reaches somewhere further in this model element. + it( 'should transform modelDiv 2 starting from nested cache', () => { + // We need to create a different model and view for this sample as the one used in other tests cannot reproduce this. + // + // View structure is: + // + //

+ // ├─ a + // ├─ + // │ ├─ b + // │ ├─ + // │ │ └─ c + // │ ├─ + // │ │ └─ d + // │ └─ e + // └─ f + // + const modelP2 = new ModelElement( 'paragraph', {}, [ + new ModelText( 'a' ), + new ModelText( 'b', { b: true } ), + new ModelText( 'c', { b: true, u: true } ), + new ModelText( 'd', { b: true, i: true } ), + new ModelText( 'e', { b: true } ), + new ModelText( 'f' ) + ] ); + + const viewTextA = new ViewText( viewDocument, 'a' ); + const viewTextB = new ViewText( viewDocument, 'b' ); + const viewTextC = new ViewText( viewDocument, 'c' ); + const viewTextD = new ViewText( viewDocument, 'd' ); + const viewTextE = new ViewText( viewDocument, 'e' ); + const viewTextF = new ViewText( viewDocument, 'f' ); + + const viewU = new ViewElement( viewDocument, 'i', {}, [ viewTextC ] ); + const viewI = new ViewElement( viewDocument, 'i', {}, [ viewTextD ] ); + const viewB = new ViewElement( viewDocument, 'b', {}, [ viewTextB, viewU, viewI, viewTextE ] ); + + const viewP2 = new ViewElement( viewDocument, 'p', {}, [ viewTextA, viewB, viewTextF ] ); + + mapper.bindElements( modelP2, viewP2 ); + + // Since tests in this suite are artificial and the view and model is modified directly, the scenario is easier to simulate. + // + // First request some mappings to build whole cache. + // + mapper.toViewPosition( ModelPosition._createAt( modelP2, 1 ) ); + mapper.toViewPosition( ModelPosition._createAt( modelP2, 2 ) ); + mapper.toViewPosition( ModelPosition._createAt( modelP2, 3 ) ); + mapper.toViewPosition( ModelPosition._createAt( modelP2, 4 ) ); + mapper.toViewPosition( ModelPosition._createAt( modelP2, 5 ) ); + mapper.toViewPosition( ModelPosition._createAt( modelP2, 6 ) ); + // + // Then we will invalidate part of the cache by adding another letter to ``. + // + modelP2._removeChildren( 3, 1 ); + modelP2._insertChild( 3, new ModelText( 'dd', { b: true, i: true } ) ); + viewTextD._data = 'dd'; + // + // After invalidation the cache will end before ``: `

abc|ddef

` + // + // Then, again request mapping for model offset at the end of ``. + // This way, `Mapper` will start looking side `viewU` and will have to "traverse up" into `

` after reaching end of ``. + createToViewTest( modelP2, 7, viewTextF, 1 ); + } ); } ); describe( 'toModelRange', () => { @@ -492,6 +558,12 @@ describe( 'Mapper', () => { } ); } ); + it( 'should throw if model offset is to big and cannot be found in mapped view element', () => { + expect( () => { + mapper.findPositionIn( viewDiv, 5 ); + } ).to.throw( CKEditorError, 'mapping-model-offset-not-found' ); + } ); + function createToViewTest( modelElement, modelOffset, viewElement, viewOffset ) { const modelPosition = ModelPosition._createAt( modelElement, modelOffset ); const viewPosition = mapper.toViewPosition( modelPosition ); @@ -513,7 +585,7 @@ describe( 'Mapper', () => { viewTextX, viewTextFOO, viewTextZZ, viewTextLABEL, mapper; - before( () => { + beforeEach( () => { // Tree Model: // //

---> modelDiv @@ -960,6 +1032,14 @@ describe( 'MapperCache', () => { check( 7, viewContainer, 2, 6 ); check( 9, viewContainer, 3, 8 ); } ); + + it( 'should hoist returned position', () => { + cache.startTracking( viewContainer ); + + cache.save( viewEm, 1, viewContainer, 6 ); + + check( 6, viewContainer, 2, 6 ); + } ); } ); describe( 'save()', () => { From 74ca40a0245693d5cf733683ddc18cdbbceead64 Mon Sep 17 00:00:00 2001 From: Szymon Cofalik Date: Wed, 18 Dec 2024 16:05:00 +0100 Subject: [PATCH 11/15] Fix in API docs. --- packages/ckeditor5-engine/src/conversion/mapper.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/ckeditor5-engine/src/conversion/mapper.ts b/packages/ckeditor5-engine/src/conversion/mapper.ts index 95e03fd7d84..05270015c02 100644 --- a/packages/ckeditor5-engine/src/conversion/mapper.ts +++ b/packages/ckeditor5-engine/src/conversion/mapper.ts @@ -1013,7 +1013,7 @@ export class MapperCache extends /* #__PURE__ */ EmitterMixin() { /** * Starts tracking given `viewContainer`, which must be mapped to a model element or model document fragment. * - * Note, that this method is automatically called by {@link ~MapperCache#get `MapperCache#get()`} and there is no need to call it + * Note, that this method is automatically called by {@link ~MapperCache#get `MapperCache#getClosest()`} and there is no need to call it * manually. * * This method initializes the cache for `viewContainer` and adds callbacks for From ed6d11e759a0e09b33cdecf52697ccb00e6fe47f Mon Sep 17 00:00:00 2001 From: Szymon Cofalik Date: Wed, 18 Dec 2024 17:14:22 +0100 Subject: [PATCH 12/15] Changed `this.listenTo( x )` to `x.on()` to prevent memory leaks in `MapperCache`. --- .../ckeditor5-engine/src/conversion/mapper.ts | 47 ++++++++++++++----- 1 file changed, 36 insertions(+), 11 deletions(-) diff --git a/packages/ckeditor5-engine/src/conversion/mapper.ts b/packages/ckeditor5-engine/src/conversion/mapper.ts index 05270015c02..5d38a4f1412 100644 --- a/packages/ckeditor5-engine/src/conversion/mapper.ts +++ b/packages/ckeditor5-engine/src/conversion/mapper.ts @@ -13,7 +13,7 @@ import ModelRange from '../model/range.js'; import ViewPosition from '../view/position.js'; import ViewRange from '../view/range.js'; -import { CKEditorError, EmitterMixin } from '@ckeditor/ckeditor5-utils'; +import { CKEditorError, EmitterMixin, GetCallback } from '@ckeditor/ckeditor5-utils'; import type ViewDocumentFragment from '../view/documentfragment.js'; import type ViewElement from '../view/element.js'; @@ -844,6 +844,37 @@ export class MapperCache extends /* #__PURE__ */ EmitterMixin() { */ private _nodeToCacheListIndex = new WeakMap(); + /** + * Callback fired whenever there is a direct or indirect children change in tracked view element or tracked view document fragment. + * + * This is specified as a property to make it easier to set as an event callback and to later turn off that event. + */ + private _invalidateOnChildrenChangeCallback: GetCallback; + + /** + * Callback fired whenever a view text node directly or indirectly inside a tracked view element or tracked view document fragment + * changes its text data. + * + * This is specified as a property to make it easier to set as an event callback and to later turn off that event. + */ + private _invalidateOnTextChangeCallback: GetCallback; + + /** + * Creates an instance of mapper cache. + */ + constructor() { + super(); + + this._invalidateOnChildrenChangeCallback = ( evt, viewNode, data ) => { + this._clearCacheInsideParent( viewNode as ViewElement | ViewDocumentFragment, data!.index ); + }; + + this._invalidateOnTextChangeCallback = ( evt, viewNode ) => { + // Text node has changed. Clear all the cache starting from before this text node. + this._clearCacheStartingBefore( viewNode ); + }; + } + /** * Saves cache for given view position mapping <-> model offset mapping. * @@ -1035,14 +1066,8 @@ export class MapperCache extends /* #__PURE__ */ EmitterMixin() { // // Possible performance improvement. This event bubbles, so if there are multiple tracked (mapped) elements that are ancestors // then this will be unnecessarily fired for each ancestor. This could be rewritten to listen only to roots and document fragments. - this.listenTo( viewContainer, 'change:children', ( evt, viewNode, data ) => { - this._invalidateCacheOnChildrenChange( viewNode as ViewElement | ViewDocumentFragment, data!.index ); - } ); - - this.listenTo( viewContainer, 'change:text', ( evt, viewNode ) => { - // Text node has changed. Clear all the cache starting from before this text node. - this._clearCacheStartingBefore( viewNode ); - } ); + viewContainer.on( 'change:children', this._invalidateOnChildrenChangeCallback ); + viewContainer.on( 'change:text', this._invalidateOnTextChangeCallback ); return initialCacheItem; } @@ -1060,9 +1085,9 @@ export class MapperCache extends /* #__PURE__ */ EmitterMixin() { } /** - * Invalidates cache after a change happens inside `viewParent` on given `index`. + * Invalidates cache inside `viewParent`, starting from given `index` in that parent. */ - private _invalidateCacheOnChildrenChange( viewParent: ViewElement | ViewDocumentFragment, index: number ) { + private _clearCacheInsideParent( viewParent: ViewElement | ViewDocumentFragment, index: number ) { if ( index == 0 ) { // Change at the beginning of the parent. if ( this._cachedMapping.has( viewParent ) ) { From 6e5f0d43f6cbdbb86c86d4fe022394febbe4eb25 Mon Sep 17 00:00:00 2001 From: Szymon Cofalik Date: Wed, 18 Dec 2024 17:55:13 +0100 Subject: [PATCH 13/15] Fixed import. --- packages/ckeditor5-engine/src/conversion/mapper.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/ckeditor5-engine/src/conversion/mapper.ts b/packages/ckeditor5-engine/src/conversion/mapper.ts index 5d38a4f1412..41ced718aa4 100644 --- a/packages/ckeditor5-engine/src/conversion/mapper.ts +++ b/packages/ckeditor5-engine/src/conversion/mapper.ts @@ -13,7 +13,7 @@ import ModelRange from '../model/range.js'; import ViewPosition from '../view/position.js'; import ViewRange from '../view/range.js'; -import { CKEditorError, EmitterMixin, GetCallback } from '@ckeditor/ckeditor5-utils'; +import { CKEditorError, EmitterMixin, type GetCallback } from '@ckeditor/ckeditor5-utils'; import type ViewDocumentFragment from '../view/documentfragment.js'; import type ViewElement from '../view/element.js'; From 036baf09ff04ac24c2c1c24465bbd2f21abd9035 Mon Sep 17 00:00:00 2001 From: Szymon Cofalik Date: Thu, 19 Dec 2024 15:39:53 +0100 Subject: [PATCH 14/15] Use `.off` instead of `.stopListening`. Fixes in code style and API docs. --- .../ckeditor5-engine/src/conversion/mapper.ts | 33 +++++++------------ 1 file changed, 12 insertions(+), 21 deletions(-) diff --git a/packages/ckeditor5-engine/src/conversion/mapper.ts b/packages/ckeditor5-engine/src/conversion/mapper.ts index 41ced718aa4..9693e6a1375 100644 --- a/packages/ckeditor5-engine/src/conversion/mapper.ts +++ b/packages/ckeditor5-engine/src/conversion/mapper.ts @@ -849,7 +849,10 @@ export class MapperCache extends /* #__PURE__ */ EmitterMixin() { * * This is specified as a property to make it easier to set as an event callback and to later turn off that event. */ - private _invalidateOnChildrenChangeCallback: GetCallback; + private _invalidateOnChildrenChangeCallback: GetCallback = ( evt, viewNode, data ) => { + // View element or document fragment changed its children at `data.index`. Clear all cache starting from before that index. + this._clearCacheInsideParent( viewNode as ViewElement | ViewDocumentFragment, data!.index ); + }; /** * Callback fired whenever a view text node directly or indirectly inside a tracked view element or tracked view document fragment @@ -857,23 +860,10 @@ export class MapperCache extends /* #__PURE__ */ EmitterMixin() { * * This is specified as a property to make it easier to set as an event callback and to later turn off that event. */ - private _invalidateOnTextChangeCallback: GetCallback; - - /** - * Creates an instance of mapper cache. - */ - constructor() { - super(); - - this._invalidateOnChildrenChangeCallback = ( evt, viewNode, data ) => { - this._clearCacheInsideParent( viewNode as ViewElement | ViewDocumentFragment, data!.index ); - }; - - this._invalidateOnTextChangeCallback = ( evt, viewNode ) => { - // Text node has changed. Clear all the cache starting from before this text node. - this._clearCacheStartingBefore( viewNode ); - }; - } + private _invalidateOnTextChangeCallback: GetCallback = ( evt, viewNode ) => { + // Text node has changed. Clear all the cache starting from before this text node. + this._clearCacheStartingBefore( viewNode ); + }; /** * Saves cache for given view position mapping <-> model offset mapping. @@ -1044,8 +1034,8 @@ export class MapperCache extends /* #__PURE__ */ EmitterMixin() { /** * Starts tracking given `viewContainer`, which must be mapped to a model element or model document fragment. * - * Note, that this method is automatically called by {@link ~MapperCache#get `MapperCache#getClosest()`} and there is no need to call it - * manually. + * Note, that this method is automatically called by {@link ~MapperCache#getClosest() `MapperCache#getClosest()`} and there is no need + * to call it manually. * * This method initializes the cache for `viewContainer` and adds callbacks for * {@link module:engine/view/node~ViewNodeChangeEvent `change` event} fired by `viewContainer`. `MapperCache` listens to `change` event @@ -1079,7 +1069,8 @@ export class MapperCache extends /* #__PURE__ */ EmitterMixin() { * `viewContainer`. */ public stopTracking( viewContainer: ViewElement | ViewDocumentFragment ): void { - this.stopListening( viewContainer ); + viewContainer.off( 'change:children', this._invalidateOnChildrenChangeCallback ); + viewContainer.off( 'change:text', this._invalidateOnTextChangeCallback ); this._cachedMapping.delete( viewContainer ); } From acd1664af217b1b85e85d92a2d5b985cb42ea4ca Mon Sep 17 00:00:00 2001 From: Szymon Cofalik Date: Fri, 17 Jan 2025 13:56:10 +0100 Subject: [PATCH 15/15] Handle 0-model-length view elements placed at the beginning of the tracked view element. Added tests. Fixed too long test name. --- .../ckeditor5-engine/src/conversion/mapper.ts | 35 ++++-- .../tests/conversion/mapper.js | 118 +++++++++++++----- 2 files changed, 115 insertions(+), 38 deletions(-) diff --git a/packages/ckeditor5-engine/src/conversion/mapper.ts b/packages/ckeditor5-engine/src/conversion/mapper.ts index 9693e6a1375..53b1e13ee48 100644 --- a/packages/ckeditor5-engine/src/conversion/mapper.ts +++ b/packages/ckeditor5-engine/src/conversion/mapper.ts @@ -866,10 +866,11 @@ export class MapperCache extends /* #__PURE__ */ EmitterMixin() { }; /** - * Saves cache for given view position mapping <-> model offset mapping. + * Saves cache for given view position mapping <-> model offset mapping. The view position should be after a node (i.e. it cannot + * be the first position inside its parent, or in other words, `viewOffset` must be greater than `0`). * * @param viewParent View position parent. - * @param viewOffset View position offset. + * @param viewOffset View position offset. Must be greater than `0`. * @param viewContainer Tracked view position ascendant (it may be the direct parent of the view position). * @param modelOffset Model offset in the model element or document fragment which is mapped to `viewContainer`. */ @@ -893,16 +894,20 @@ export class MapperCache extends /* #__PURE__ */ EmitterMixin() { // `Mapper#getModelLength()` are implemented so that parents are cached before their children. // // So, don't create new cache if one already exists. Instead, only save `_nodeToCacheListIndex` value for the related view node. - if ( viewOffset > 0 ) { - const viewChild = viewParent.getChild( viewOffset - 1 )!; + const viewChild = viewParent.getChild( viewOffset - 1 )!; - // Figure out what index to save with `viewChild`. - // We have a `cacheItem` for the `modelOffset`, so we can get a `viewPosition` from there. Before that position, there - // must be a node. That node must have an index set. This will be the index we will want to use. - const index = this._nodeToCacheListIndex.get( cacheItem.viewPosition.nodeBefore! )!; + // Figure out what index to save with `viewChild`. + // We have a `cacheItem` for the `modelOffset`, so we can get a `viewPosition` from there. Before that view position, there + // must be a node. That node must have an index set. This will be the index we will want to use. + // Since we expect `viewOffset` to be greater than 0, then in almost all cases `modelOffset` will be greater than 0 as well. + // As a result, we can expect `cacheItem.viewPosition.nodeBefore` to be set. + // + // However, in an edge case, were the tracked element contains a 0-model-length view element as the first child (UI element or + // an empty attribute element), then `modelOffset` will be 0, and `cacheItem` will be the first cache item, which is before any + // view node. In such edge case, `cacheItem.viewPosition.nodeBefore` is undefined, and we manually set to `0`. + const index = cacheItem.viewPosition.nodeBefore ? this._nodeToCacheListIndex.get( cacheItem.viewPosition.nodeBefore )! : 0; - this._nodeToCacheListIndex.set( viewChild, index ); - } + this._nodeToCacheListIndex.set( viewChild, index ); return; } @@ -1168,6 +1173,16 @@ export class MapperCache extends /* #__PURE__ */ EmitterMixin() { * Clears all the cache in the cache list related to given `viewContainer`, starting from `index` (inclusive). */ private _clearCacheFromIndex( viewContainer: ViewElement | ViewDocumentFragment, index: number ) { + if ( index === 0 ) { + // Don't remove the first entry in the cache (this entry is always a mapping between view offset 0 <-> model offset 0, + // and it is a default value that is always expected to be in the cache list). + // + // The cache mechanism may ask to clear from index `0` in a case where a 0-model-length view element (UI element or empty + // attribute element) was at the beginning of tracked element. In such scenario, the view element is mapped through + // `nodeToCacheListIndex` to index `0`. + index = 1; + } + // Cache is always available here because we initialize it just before adding a listener that fires `_clearCacheFromIndex()`. const cache = this._cachedMapping.get( viewContainer )!; const cacheItem = cache.cacheList[ index - 1 ]; diff --git a/packages/ckeditor5-engine/tests/conversion/mapper.js b/packages/ckeditor5-engine/tests/conversion/mapper.js index a36e704b317..fe187bc0f8b 100644 --- a/packages/ckeditor5-engine/tests/conversion/mapper.js +++ b/packages/ckeditor5-engine/tests/conversion/mapper.js @@ -948,17 +948,20 @@ describe( 'Mapper', () => { } ); describe( 'MapperCache', () => { - let cache, viewContainer, viewDocument, viewTextAb, viewSpan, viewEm, viewTextCd, viewTextEf, viewTextGh; + let cache, viewContainer, viewDocument, viewTextAb, viewSpan, viewEm, viewB, viewTextCd, viewTextE, viewTextF, viewTextGh; beforeEach( () => { viewDocument = new ViewDocument( new StylesProcessor() ); cache = new MapperCache(); - //

abcdefgh

. + //

abcdefgh

. viewTextAb = new ViewText( viewDocument, 'ab' ); viewTextCd = new ViewText( viewDocument, 'cd' ); - viewTextEf = new ViewText( viewDocument, 'ef' ); - viewEm = new ViewAttributeElement( viewDocument, 'em', null, [ viewTextEf ] ); + viewTextE = new ViewText( viewDocument, 'e' ); + viewTextF = new ViewText( viewDocument, 'f' ); + + viewB = new ViewAttributeElement( viewDocument, 'b', null, [ viewTextE ] ); + viewEm = new ViewAttributeElement( viewDocument, 'em', null, [ viewB, viewTextF ] ); viewSpan = new ViewAttributeElement( viewDocument, 'span', null, [ viewTextCd, viewEm ] ); viewTextGh = new ViewText( viewDocument, 'gh' ); viewContainer = new ViewElement( viewDocument, 'p', null, [ viewTextAb, viewSpan, viewTextGh ] ); @@ -986,13 +989,13 @@ describe( 'MapperCache', () => { it( 'should return previously saved position (deep)', () => { cache.startTracking( viewContainer ); - cache.save( viewEm, 0, viewContainer, 4 ); + cache.save( viewEm, 1, viewContainer, 5 ); - const { viewPosition, modelOffset } = cache.getClosest( viewContainer, 4 ); + const { viewPosition, modelOffset } = cache.getClosest( viewContainer, 5 ); expect( viewPosition.parent ).to.equal( viewEm ); - expect( viewPosition.offset ).to.equal( 0 ); - expect( modelOffset ).to.equal( 4 ); + expect( viewPosition.offset ).to.equal( 1 ); + expect( modelOffset ).to.equal( 5 ); } ); it( 'should return closest saved position if exact position was not saved', () => { @@ -1008,13 +1011,13 @@ describe( 'MapperCache', () => { it( 'should return closest saved position if exact position was not saved (deep)', () => { cache.startTracking( viewContainer ); - cache.save( viewEm, 0, viewContainer, 4 ); + cache.save( viewEm, 1, viewContainer, 5 ); const { viewPosition, modelOffset } = cache.getClosest( viewContainer, 8 ); expect( viewPosition.parent ).to.equal( viewEm ); - expect( viewPosition.offset ).to.equal( 0 ); - expect( modelOffset ).to.equal( 4 ); + expect( viewPosition.offset ).to.equal( 1 ); + expect( modelOffset ).to.equal( 5 ); } ); it( 'should return closest saved position if exact position was not saved (multiple saved positions)', () => { @@ -1036,7 +1039,7 @@ describe( 'MapperCache', () => { it( 'should hoist returned position', () => { cache.startTracking( viewContainer ); - cache.save( viewEm, 1, viewContainer, 6 ); + cache.save( viewEm, 2, viewContainer, 6 ); check( 6, viewContainer, 2, 6 ); } ); @@ -1079,7 +1082,7 @@ describe( 'MapperCache', () => { cache.save( viewSpan, 1, viewContainer, 4 ); cache.save( viewContainer, 3, viewContainer, 8 ); - //

abcdef^gh

->

abcdefgh

. + //

abcdef^gh

->

abcdefgh

. // This should invalidate cache starting from `` (yes, that's correct, we invalidate a bit more than necessary). viewContainer._insertChild( 2, new ViewAttributeElement( viewDocument, 'strong' ) ); @@ -1103,7 +1106,7 @@ describe( 'MapperCache', () => { cache.save( viewContainer, 2, viewContainer, 6 ); cache.save( viewContainer, 3, viewContainer, 8 ); - //

abcd^efgh

->

abcdefgh

. + //

abcd^efgh

->

abcdefgh

. // This should invalidate cache starting from ``. viewEm._insertChild( 0, new ViewAttributeElement( viewDocument, 'strong' ) ); @@ -1127,7 +1130,7 @@ describe( 'MapperCache', () => { cache.save( viewContainer, 2, viewContainer, 6 ); cache.save( viewContainer, 3, viewContainer, 8 ); - //

abcd^efgh

->

abcdefgh

. + //

abcd^efgh

->

abcdefgh

. // This should invalidate cache starting from before `` (not before ``). // That's because `` is not cached, so we invalidate starting from before its parent. viewEm._insertChild( 0, new ViewAttributeElement( viewDocument, 'strong' ) ); @@ -1149,7 +1152,7 @@ describe( 'MapperCache', () => { cache.save( viewContainer, 2, viewContainer, 6 ); cache.save( viewContainer, 3, viewContainer, 8 ); - //

abcd^efgh

->

abcdefgh

. + //

abcd^efgh

->

abcdefgh

. // This should invalidate cache starting from before `` (not before ``). // That's because `` is not cached, so we invalidate starting from before its parent. viewEm._insertChild( 0, new ViewAttributeElement( viewDocument, 'strong' ) ); @@ -1164,13 +1167,12 @@ describe( 'MapperCache', () => { } ); it( 'nothing happens if change is after valid cache', () => { - // This is a similar scenario as above, but this time, `` -- the direct parent of insertion -- is not cached. cache.startTracking( viewContainer ); cache.save( viewContainer, 1, viewContainer, 2 ); cache.save( viewContainer, 2, viewContainer, 6 ); - //

abcdefgh^

->

abcdefgh

. + //

abcdefgh^

->

abcdefgh

. // Only `` was cached so far. viewContainer._insertChild( 3, new ViewAttributeElement( viewDocument, 'strong' ) ); @@ -1193,7 +1195,7 @@ describe( 'MapperCache', () => { cache.save( viewContainer, 2, viewContainer, 6 ); cache.save( viewContainer, 3, viewContainer, 8 ); - //

^abcdefgh

->

abcdefgh

. + //

^abcdefgh

->

abcdefgh

. // This should invalidate all cache. viewContainer._insertChild( 0, new ViewAttributeElement( viewDocument, 'strong' ) ); @@ -1211,7 +1213,7 @@ describe( 'MapperCache', () => { cache.save( viewContainer, 2, viewContainer, 6 ); cache.save( viewContainer, 3, viewContainer, 8 ); - //

abcdefgh^

->

abcdefghi

. + //

abcdefgh^

->

abcdefghi

. // This should invalidate cache starting from before `"ghi"`. viewTextGh._data = 'ghi'; @@ -1233,9 +1235,9 @@ describe( 'MapperCache', () => { cache.save( viewContainer, 2, viewContainer, 6 ); cache.save( viewContainer, 3, viewContainer, 8 ); - //

abcdef^gh

->

abcdefx

. - // This should invalidate cache starting from before `"efx"`. - viewTextEf._data = 'efx'; + //

abcde^fgh

->

abcdexf

. + // This should invalidate cache starting from before `"ex"`. + viewTextE._data = 'ex'; // Retained cached: check( 2, viewContainer, 1, 2 ); @@ -1254,9 +1256,9 @@ describe( 'MapperCache', () => { cache.save( viewContainer, 2, viewContainer, 6 ); cache.save( viewContainer, 3, viewContainer, 8 ); - //

abcdef^gh^

->

abcdefx

. - // This should invalidate cache starting from before `"ghi"`. - viewTextEf._data = 'efx'; + //

abcde^fgh

->

abcdexf

. + // This should invalidate cache starting from before `"ex"`. + viewTextE._data = 'ex'; // Retained cached: check( 2, viewContainer, 1, 2 ); @@ -1266,10 +1268,70 @@ describe( 'MapperCache', () => { check( 6, viewSpan, 1, 4 ); check( 8, viewSpan, 1, 4 ); } ); + + it( 'should invalidate cache if first child in tracked element has 0 model length (#1 - ui element + remove)', () => { + // This test checks a fix for an edge case scenario bug found in multi-level lists. + // Multi-level lists items always start from a UI element. Since it was a 0-model-length element it was not correctly saved + // to a cache, and later on, it prevented a validation to happen (the cache thought there is nothing cached). + const viewUIElement = new ViewUIElement( viewDocument, 'ui' ); + const viewTextX = new ViewText( viewDocument, 'x' ); + const viewB = new ViewAttributeElement( viewDocument, 'b', null, [ new ViewText( viewDocument, 'y' ) ] ); + //

xy

. + const viewContainer2 = new ViewElement( viewDocument, 'p', null, [ viewUIElement, viewTextX, viewB ] ); + + cache.startTracking( viewContainer2 ); + + cache.save( viewContainer2, 1, viewContainer2, 0 ); // Note that `viewUIElement` has 0 model length, hence `modelOffset` is 0. + cache.save( viewContainer2, 2, viewContainer2, 1 ); + cache.save( viewContainer2, 3, viewContainer2, 2 ); + + check( 0, viewContainer2, 0, 0, viewContainer2 ); // Still returns position `viewContainer2, 0`, not position after UI element. + check( 1, viewContainer2, 2, 1, viewContainer2 ); + check( 2, viewContainer2, 3, 2, viewContainer2 ); + check( 3, viewContainer2, 3, 2, viewContainer2 ); + + viewContainer2._removeChildren( 1, 1 ); + + // Should fully invalidate cached data. + check( 0, viewContainer2, 0, 0, viewContainer2 ); + check( 1, viewContainer2, 0, 0, viewContainer2 ); + check( 2, viewContainer2, 0, 0, viewContainer2 ); + check( 3, viewContainer2, 0, 0, viewContainer2 ); + } ); + + it( 'should invalidate cache if first child in tracked element has 0 model length (#2 - attribute element + typing)', () => { + // This test checks a similar scenario as above, although this scenario was working before the fix. + const viewUIElement = new ViewUIElement( viewDocument, 'ui' ); + const viewTextX = new ViewText( viewDocument, 'x' ); + const viewB = new ViewAttributeElement( viewDocument, 'b', null, [ new ViewText( viewDocument, 'y' ) ] ); + //

xy

. + const viewContainer2 = new ViewElement( viewDocument, 'p', null, [ viewUIElement, viewTextX, viewB ] ); + + cache.startTracking( viewContainer2 ); + + cache.save( viewContainer2, 1, viewContainer2, 0 ); // Note that `viewUIElement` has 0 model length, hence `modelOffset` is 0. + cache.save( viewContainer2, 2, viewContainer2, 1 ); + cache.save( viewContainer2, 3, viewContainer2, 2 ); + + check( 0, viewContainer2, 0, 0, viewContainer2 ); // Still returns position `viewContainer2, 0`, not position after UI element. + check( 1, viewContainer2, 2, 1, viewContainer2 ); + check( 2, viewContainer2, 3, 2, viewContainer2 ); + check( 3, viewContainer2, 3, 2, viewContainer2 ); + + viewTextX._data = 'xx'; + + // Should fully invalidate cached data. + check( 0, viewContainer2, 0, 0, viewContainer2 ); + check( 1, viewContainer2, 0, 0, viewContainer2 ); + check( 2, viewContainer2, 0, 0, viewContainer2 ); + check( 3, viewContainer2, 0, 0, viewContainer2 ); + } ); } ); - function check( modelOffsetToCheck, expectedViewParent, expectedViewOffset, expectedModelOffset ) { - const { viewPosition, modelOffset } = cache.getClosest( viewContainer, modelOffsetToCheck ); + function check( + modelOffsetToCheck, expectedViewParent, expectedViewOffset, expectedModelOffset, viewContainerToCheck = viewContainer + ) { + const { viewPosition, modelOffset } = cache.getClosest( viewContainerToCheck, modelOffsetToCheck ); expect( viewPosition.parent ).to.equal( expectedViewParent ); expect( viewPosition.offset ).to.equal( expectedViewOffset );