Skip to content

Commit

Permalink
Handle 0-model-length view elements placed at the beginning of the tr…
Browse files Browse the repository at this point in the history 
…acked view element.

Added tests.

Fixed too long test name.
  • Loading branch information
@scofalik
scofalik committed Jan 28, 2025
1 parent 036baf0 commit acd1664
Show file tree
Hide file tree
Showing 2 changed files with 115 additions and 38 deletions.
35 changes: 25 additions & 10 deletions packages/ckeditor5-engine/src/conversion/mapper.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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`.
*/
Expand All @@ -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;
}
Expand Down Expand Up @@ -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 ];
Expand Down
118 changes: 90 additions & 28 deletions packages/ckeditor5-engine/tests/conversion/mapper.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();

// <p>ab<span>cd<em>ef</em></span>gh</p>.
// <p>ab<span>cd<em><b>e</b>f</em></span>gh</p>.
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 ] );
Expand Down Expand Up @@ -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', () => {
Expand All @@ -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)', () => {
Expand All @@ -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 );
} );
Expand Down Expand Up @@ -1079,7 +1082,7 @@ describe( 'MapperCache', () => {
cache.save( viewSpan, 1, viewContainer, 4 );
cache.save( viewContainer, 3, viewContainer, 8 );

// <p>ab<span>cd<em>ef</em></span>^gh</p> -> <p>ab<span>cd<em>ef</em></span><strong></strong>gh</p>.
// <p>ab<span>cd<em><b>e</b>f</em></span>^gh</p> -> <p>ab<span>cd<em><b>e</b>f</em></span><strong></strong>gh</p>.
// This should invalidate cache starting from `<span>` (yes, that's correct, we invalidate a bit more than necessary).
viewContainer._insertChild( 2, new ViewAttributeElement( viewDocument, 'strong' ) );

Expand All @@ -1103,7 +1106,7 @@ describe( 'MapperCache', () => {
cache.save( viewContainer, 2, viewContainer, 6 );
cache.save( viewContainer, 3, viewContainer, 8 );

// <p>ab<span>cd<em>^ef</em></span>gh</p> -> <p>ab<span>cd<em><strong></strong>ef</em></span>gh</p>.
// <p>ab<span>cd<em>^<b>e</b>f</em></span>gh</p> -> <p>ab<span>cd<em><strong></strong><b>e</b>f</em></span>gh</p>.
// This should invalidate cache starting from `<em>`.
viewEm._insertChild( 0, new ViewAttributeElement( viewDocument, 'strong' ) );

Expand All @@ -1127,7 +1130,7 @@ describe( 'MapperCache', () => {
cache.save( viewContainer, 2, viewContainer, 6 );
cache.save( viewContainer, 3, viewContainer, 8 );

// <p>ab<span>cd<em>^ef</em></span>gh</p> -> <p>ab<span>cd<em><strong></strong>ef</em></span>gh</p>.
// <p>ab<span>cd<em>^<b>e</b>f</em></span>gh</p> -> <p>ab<span>cd<em><strong></strong><b>e</b>f</em></span>gh</p>.
// This should invalidate cache starting from before `<span>` (not before `<em>`).
// That's because `<em>` is not cached, so we invalidate starting from before its parent.
viewEm._insertChild( 0, new ViewAttributeElement( viewDocument, 'strong' ) );
Expand All @@ -1149,7 +1152,7 @@ describe( 'MapperCache', () => {
cache.save( viewContainer, 2, viewContainer, 6 );
cache.save( viewContainer, 3, viewContainer, 8 );

// <p>ab<span>cd<em>^ef</em></span>gh</p> -> <p>ab<span>cd<em><strong></strong>ef</em></span>gh</p>.
// <p>ab<span>cd<em>^<b>e</b>f</em></span>gh</p> -> <p>ab<span>cd<em><strong></strong><b>e</b>f</em></span>gh</p>.
// This should invalidate cache starting from before `<span>` (not before `<em>`).
// That's because `<em>` is not cached, so we invalidate starting from before its parent.
viewEm._insertChild( 0, new ViewAttributeElement( viewDocument, 'strong' ) );
Expand All @@ -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, `<em>` -- the direct parent of insertion -- is not cached.
cache.startTracking( viewContainer );

cache.save( viewContainer, 1, viewContainer, 2 );
cache.save( viewContainer, 2, viewContainer, 6 );

// <p>ab<span>cd<em>ef</em></span>gh^</p> -> <p>ab<span>cd<em><strong></strong>ef</em></span>gh</p>.
// <p>ab<span>cd<em>ef</em></span>gh^</p> -> <p>ab<span>cd<em>ef</em></span>gh<strong></strong></p>.
// Only `<span>` was cached so far.
viewContainer._insertChild( 3, new ViewAttributeElement( viewDocument, 'strong' ) );

Expand All @@ -1193,7 +1195,7 @@ describe( 'MapperCache', () => {
cache.save( viewContainer, 2, viewContainer, 6 );
cache.save( viewContainer, 3, viewContainer, 8 );

// <p>^ab<span>cd<em>ef</em></span>gh</p> -> <p><strong></strong>ab<span>cd<em>ef</em></span>gh</p>.
// <p>^ab<span>cd<em><b>e</b>f</em></span>gh</p> -> <p><strong></strong>ab<span>cd<em><b>e</b>f</em></span>gh</p>.
// This should invalidate all cache.
viewContainer._insertChild( 0, new ViewAttributeElement( viewDocument, 'strong' ) );

Expand All @@ -1211,7 +1213,7 @@ describe( 'MapperCache', () => {
cache.save( viewContainer, 2, viewContainer, 6 );
cache.save( viewContainer, 3, viewContainer, 8 );

// <p>ab<span>cd<em>ef</em></span>gh^</p> -> <p><strong></strong>ab<span>cd<em>ef</em></span>ghi</p>.
// <p>ab<span>cd<em><b>e</b>f</em></span>gh^</p> -> <p><strong></strong>ab<span>cd<em><b>e</b>f</em></span>ghi</p>.
// This should invalidate cache starting from before `"ghi"`.
viewTextGh._data = 'ghi';

Expand All @@ -1233,9 +1235,9 @@ describe( 'MapperCache', () => {
cache.save( viewContainer, 2, viewContainer, 6 );
cache.save( viewContainer, 3, viewContainer, 8 );

// <p>ab<span>cd<em>ef^</em></span>gh</p> -> <p><strong></strong>ab<span>cd<em>efx</em></span></p>.
// This should invalidate cache starting from before `"efx"`.
viewTextEf._data = 'efx';
// <p>ab<span>cd<em><b>e^</b>f</em></span>gh</p> -> <p><strong></strong>ab<span>cd<em><b>ex</b>f</em></span></p>.
// This should invalidate cache starting from before `"ex"`.
viewTextE._data = 'ex';

// Retained cached:
check( 2, viewContainer, 1, 2 );
Expand All @@ -1254,9 +1256,9 @@ describe( 'MapperCache', () => {
cache.save( viewContainer, 2, viewContainer, 6 );
cache.save( viewContainer, 3, viewContainer, 8 );

// <p>ab<span>cd<em>ef^</em></span>gh^</p> -> <p><strong></strong>ab<span>cd<em>efx</em></span></p>.
// This should invalidate cache starting from before `"ghi"`.
viewTextEf._data = 'efx';
// <p>ab<span>cd<em><b>e^</b>f</em></span>gh</p> -> <p><strong></strong>ab<span>cd<em><b>ex</b>f</em></span></p>.
// This should invalidate cache starting from before `"ex"`.
viewTextE._data = 'ex';

// Retained cached:
check( 2, viewContainer, 1, 2 );
Expand All @@ -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' ) ] );
// <p><ui />x<b>y</b></p>.
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' ) ] );
// <p><ui />x<b>y</b></p>.
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 );
Expand Down

0 comments on commit acd1664

Please sign in to comment.