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.
  • Loading branch information
@scofalik
scofalik committed Jan 17, 2025
1 parent d78ef09 commit 6716393
Show file tree
Hide file tree
Showing 2 changed files with 53 additions and 36 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
54 changes: 28 additions & 26 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 Down

0 comments on commit 6716393

Please sign in to comment.