Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fire addMarker event in data pipeline if marker is at element boundary #9897

Merged
merged 8 commits into from
Jun 29, 2021
21 changes: 16 additions & 5 deletions packages/ckeditor5-engine/src/controller/datacontroller.js
Original file line number Diff line number Diff line change
Expand Up @@ -251,6 +251,7 @@ export default class DataController {
// For document fragment, simply take the markers assigned to this document fragment.
// For model root, all markers in that root will be taken.
// For model element, we need to check which markers are intersecting with this element and relatively modify the markers' ranges.
// Collapsed markers at element boundary, although considered as not intersecting with the element, will also be returned.
const markers = modelElementOrFragment.is( 'documentFragment' ) ?
Array.from( modelElementOrFragment.markers ) :
_getMarkersRelativeToElement( modelElementOrFragment );
Expand Down Expand Up @@ -528,8 +529,9 @@ mix( DataController, ObservableMixin );

// Helper function for downcast conversion.
//
// Takes a document element (element that is added to a model document) and checks which markers are inside it
// and which markers are containing it. If the marker is intersecting with element, the intersection is returned.
// Takes a document element (element that is added to a model document) and checks which markers are inside it. If the marker is collapsed
// at element boundary, it is considered as contained inside the element and marker range is returned. Otherwise, if the marker is
// intersecting with the element, the intersection is returned.
function _getMarkersRelativeToElement( element ) {
const result = [];
const doc = element.root.document;
Expand All @@ -541,10 +543,19 @@ function _getMarkersRelativeToElement( element ) {
const elementRange = ModelRange._createIn( element );

for ( const marker of doc.model.markers ) {
const intersection = elementRange.getIntersection( marker.getRange() );
const markerRange = marker.getRange();

if ( intersection ) {
result.push( [ marker.name, intersection ] );
const isMarkerCollapsed = markerRange.isCollapsed;
const isMarkerAtElementBoundary = markerRange.start.isEqual( elementRange.start ) || markerRange.end.isEqual( elementRange.end );

if ( isMarkerCollapsed && isMarkerAtElementBoundary ) {
result.push( [ marker.name, markerRange ] );
} else {
const updatedMarkerRange = elementRange.getIntersection( markerRange );

if ( updatedMarkerRange ) {
result.push( [ marker.name, updatedMarkerRange ] );
}
}
}

Expand Down
96 changes: 94 additions & 2 deletions packages/ckeditor5-engine/tests/controller/datacontroller.js
Original file line number Diff line number Diff line change
Expand Up @@ -599,12 +599,13 @@ describe( 'DataController', () => {
describe( 'toView()', () => {
beforeEach( () => {
schema.register( 'paragraph', { inheritAllFrom: '$block' } );
schema.register( 'div' );
schema.register( 'div', { inheritAllFrom: '$block' } );

schema.extend( '$block', { allowIn: 'div' } );
schema.extend( 'div', { allowIn: '$root' } );
schema.extend( 'div', { allowIn: 'div' } );

downcastHelpers.elementToElement( { model: 'paragraph', view: 'p' } );
downcastHelpers.elementToElement( { model: 'div', view: 'div' } );
} );

it( 'should use #viewDocument as a parent for returned document fragments', () => {
Expand Down Expand Up @@ -691,6 +692,97 @@ describe( 'DataController', () => {
);
} );

// See https://github.com/ckeditor/ckeditor5/issues/8485.
it( 'should convert collapsed markers at element boundary', () => {
const modelElement = parseModel( '<div><paragraph>foo</paragraph></div>', schema );
const modelRoot = model.document.getRoot();

downcastHelpers.markerToData( { model: 'marker:a' } );
downcastHelpers.markerToData( { model: 'marker:b' } );

const modelParagraph = modelElement.getChild( 0 );

model.change( writer => {
writer.insert( modelElement, modelRoot, 0 );

const rangeAtStart = writer.createRange( writer.createPositionFromPath( modelParagraph, [ 0 ] ) );
const rangeAtEnd = writer.createRange( writer.createPositionFromPath( modelParagraph, [ 3 ] ) );

writer.addMarker( 'marker:a', { range: rangeAtStart, usingOperation: true } );
writer.addMarker( 'marker:b', { range: rangeAtEnd, usingOperation: true } );
} );

const viewElement = data.toView( modelParagraph );

expect( stringifyView( viewElement ) ).to.equal(
'<marker:a-start></marker:a-start><marker:a-end></marker:a-end>' +
'foo' +
'<marker:b-start></marker:b-start><marker:b-end></marker:b-end>'
);
} );

// See https://github.com/ckeditor/ckeditor5/issues/8485.
it( 'should convert collapsed markers at element boundary in a deeply nested element', () => {
const modelElement = parseModel( '<div><div><div><div><paragraph>foo</paragraph></div></div></div></div>', schema );
const modelRoot = model.document.getRoot();

downcastHelpers.markerToData( { model: 'marker:a' } );
downcastHelpers.markerToData( { model: 'marker:b' } );

const modelParagraph = modelElement.getChild( 0 ).getChild( 0 ).getChild( 0 ).getChild( 0 );

model.change( writer => {
writer.insert( modelElement, modelRoot, 0 );

const rangeAtStart = writer.createRange( writer.createPositionFromPath( modelParagraph, [ 0 ] ) );
const rangeAtEnd = writer.createRange( writer.createPositionFromPath( modelParagraph, [ 3 ] ) );

writer.addMarker( 'marker:a', { range: rangeAtStart, usingOperation: true } );
writer.addMarker( 'marker:b', { range: rangeAtEnd, usingOperation: true } );
} );

const viewElement = data.toView( modelElement );

expect( stringifyView( viewElement ) ).to.equal(
'<div><div><div><p>' +
'<marker:a-start></marker:a-start><marker:a-end></marker:a-end>' +
'foo' +
'<marker:b-start></marker:b-start><marker:b-end></marker:b-end>' +
'</p></div></div></div>'
);
} );

// See https://github.com/ckeditor/ckeditor5/issues/8485.
it( 'should skip collapsed markers at other element\'s boundaries', () => {
const modelElement = parseModel( '<div><paragraph>foo</paragraph><paragraph>bar</paragraph></div>', schema );
const modelRoot = model.document.getRoot();

downcastHelpers.markerToData( { model: 'marker:a' } );
downcastHelpers.markerToData( { model: 'marker:b' } );

const modelP1 = modelElement.getChild( 0 );
const modelP2 = modelElement.getChild( 1 );

model.change( writer => {
writer.insert( modelElement, modelRoot, 0 );

const rangeA = writer.createRange( writer.createPositionFromPath( modelP1, [ 0 ] ) );
const rangeB = writer.createRange( writer.createPositionFromPath( modelP2, [ 0 ] ) );

writer.addMarker( 'marker:a', { range: rangeA, usingOperation: true } );
writer.addMarker( 'marker:b', { range: rangeB, usingOperation: true } );
} );

const viewElementP1 = data.toView( modelP1 );
const viewElementP2 = data.toView( modelP2 );

// The `marker:b` should not be present as it belongs to other element.
expect( stringifyView( viewElementP1 ) ).to.equal( '<marker:a-start></marker:a-start><marker:a-end></marker:a-end>foo' );

// The `marker:a` should not be present as it belongs to other element.
expect( stringifyView( viewElementP2 ) ).to.equal( '<marker:b-start></marker:b-start><marker:b-end></marker:b-end>bar' );
} );

it( 'should keep view-model mapping', () => {
const modelDocumentFragment = parseModel( '<paragraph>foo</paragraph><paragraph>bar</paragraph>', schema );
const viewDocumentFragment = data.toView( modelDocumentFragment );
Expand Down