Skip to content
This repository has been archived by the owner on Jun 26, 2020. It is now read-only.

T/1002: Introduce Position#getCommonAncestor( otherPosition ) and Range#getCommonAncestor() #1005

Merged
merged 10 commits into from
Jul 11, 2017
21 changes: 21 additions & 0 deletions src/model/documentfragment.js
Original file line number Diff line number Diff line change
Expand Up @@ -171,6 +171,27 @@ export default class DocumentFragment {
return [];
}

/**
* Returns a descendant node by its path relative to this element.
*
* // <this>a<b>c</b></this>
* this.getNodeByPath( [ 0 ] ); // -> "a"
* this.getNodeByPath( [ 1 ] ); // -> <b>
* this.getNodeByPath( [ 1, 0 ] ); // -> "c"
*
* @param {Array.<Number>} relativePath Path of the node to find, relative to this element.
* @returns {module:engine/model/node~Node|module:engine/model/documentfragment~DocumentFragment}
*/
getNodeByPath( relativePath ) {
let node = this; // eslint-disable-line consistent-this

for ( const index of relativePath ) {
node = node.getChild( index );
}

return node;
}

/**
* Converts offset "position" to index "position".
*
Expand Down
22 changes: 22 additions & 0 deletions src/model/position.js
Original file line number Diff line number Diff line change
Expand Up @@ -318,6 +318,28 @@ export default class Position {
return this.path.slice( 0, diffAt );
}

/**
* Returns an {@link module:engine/model/element~Element} or {@link module:engine/model/documentfragment~DocumentFragment}
* which is a common ancestor for both positions. The {@link #root roots} of these two positions must be identical.
*
* @param {module:engine/model/position~Position} position The second position.
* @returns {module:engine/model/element~Element|module:engine/model/documentfragment~DocumentFragment|null}
*/
getCommonAncestor( position ) {
if ( this.root !== position.root ) {
return null;
}

const node = this.root.getNodeByPath( this.getCommonPath( position ) );

// Although paths can indicate a text node, text node is not an ancestor of a position.
if ( node.is( 'text' ) ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is something I expected from this method's model implementation. But I wonder what other think. Is "text node" a position's ancestor?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, @scofalik and @pjasiun confirm that a text node is not an ancestor of position.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's add a comment here saying that "Although paths can indicate a text node, text node is not an ancestor of a position."

return node.parent;
}

return node;
}

/**
* Returns a new instance of `Position`, that has same {@link #parent parent} but it's offset
* is shifted by `shift` value (can be a negative value).
Expand Down
10 changes: 10 additions & 0 deletions src/model/range.js
Original file line number Diff line number Diff line change
Expand Up @@ -445,6 +445,16 @@ export default class Range {
return ranges;
}

/**
* Returns an {@link module:engine/model/element~Element} or {@link module:engine/model/documentfragment~DocumentFragment}
* which is a common ancestor for both positions.
*
* @returns {module:engine/model/element~Element|module:engine/model/documentfragment~DocumentFragment|null}
*/
getCommonAncestor() {
return this.start.getCommonAncestor( this.end );
}

/**
* Returns a range that is a result of transforming this range by a change in the model document.
*
Expand Down
20 changes: 20 additions & 0 deletions src/view/position.js
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,26 @@ export default class Position {
}
}

/**
* Returns a {@link module:engine/view/node~Node} or {@link module:engine/view/documentfragment~DocumentFragment}
* which is a common ancestor for both positions.
*
* @param {module:engine/view/position~Position} position
* @returns {module:engine/view/node~Node|module:engine/view/documentfragment~DocumentFragment|null}
*/
getCommonAncestor( position ) {
const ancestorsA = this.getAncestors();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getAncestors() accept options.includeNode so will this work in such a case: <p>x{y}z</p>? For such positions the text node should be returned. And for such <p>[x]</p> the element.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The second issue – you didn't pass options.parentFirst too so how can you iterate from i==0 (leafs)? What if one ancestors array is longer than the other?

const ancestorsB = position.getAncestors();

let i = 0;

while ( ancestorsA[ i ] == ancestorsB[ i ] && ancestorsA[ i ] ) {
i++;
}

return i === 0 ? null : ancestorsA[ i - 1 ];
}

/**
* Checks whether this position equals given position.
*
Expand Down
10 changes: 10 additions & 0 deletions src/view/range.js
Original file line number Diff line number Diff line change
Expand Up @@ -304,6 +304,16 @@ export default class Range {
return new TreeWalker( options );
}

/**
* Returns a {@link module:engine/view/node~Node} or {@link module:engine/view/documentfragment~DocumentFragment}
* which is a common ancestor for both positions.
*
* @returns {module:engine/view/node~Node|module:engine/view/documentfragment~DocumentFragment|null}
*/
getCommonAncestor() {
return this.start.getCommonAncestor( this.end );
}

/**
* Returns an iterator that iterates over all {@link module:engine/view/item~Item view items} that are in this range and returns
* them.
Expand Down
21 changes: 21 additions & 0 deletions tests/model/documentfragment.js
Original file line number Diff line number Diff line change
Expand Up @@ -300,4 +300,25 @@ describe( 'DocumentFragment', () => {
expect( deserialized.getChild( 1 ).parent ).to.equal( deserialized );
} );
} );

describe( 'getNodeByPath', () => {
it( 'should return the whole document fragment if path is empty', () => {
const frag = new DocumentFragment();

expect( frag.getNodeByPath( [] ) ).to.equal( frag );
} );

it( 'should return a descendant of this node', () => {
const image = new Element( 'image' );
const element = new Element( 'elem', [], [
new Element( 'elem', [], [
new Text( 'foo' ),
image
] )
] );
const frag = new DocumentFragment( element );

expect( frag.getNodeByPath( [ 0, 0, 1 ] ) ).to.equal( image );
} );
} );
} );
43 changes: 43 additions & 0 deletions tests/model/position.js
Original file line number Diff line number Diff line change
Expand Up @@ -844,4 +844,47 @@ describe( 'Position', () => {
).to.throw( CKEditorError, /model-position-fromjson-no-root/ );
} );
} );

describe( 'getCommonAncestor()', () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All tests use the same length of paths.

it( 'returns null when roots of the position are not the same', () => {
const pos1 = new Position( root, [ 0 ] );
const pos2 = new Position( otherRoot, [ 1, 1 ] );

test( pos1, pos2, null );
} );

it( 'for two the same positions returns the parent element', () => {
const fPosition = new Position( root, [ 1, 0, 0 ] );
const otherPosition = new Position( root, [ 1, 0, 0 ] );

test( fPosition, otherPosition, li1 );
} );

it( 'for two positions in the same element returns the element', () => {
const fPosition = new Position( root, [ 1, 0, 0 ] );
const zPosition = new Position( root, [ 1, 0, 2 ] );

test( fPosition, zPosition, li1 );
} );

it( 'works when one positions is nested deeper than the other', () => {
const zPosition = new Position( root, [ 1, 0, 2 ] );
const liPosition = new Position( root, [ 1, 1 ] );

test( liPosition, zPosition, ul );
} );

it( 'works fine with positions hooked in `DocumentFragment`', () => {
const docFrag = new DocumentFragment( [ p, ul ] );
const zPosition = new Position( docFrag, [ 1, 0, 2 ] );
const afterLiPosition = new Position( docFrag, [ 1, 2 ] );

test( zPosition, afterLiPosition, ul );
} );

function test( positionA, positionB, lca ) {
expect( positionA.getCommonAncestor( positionB ) ).to.equal( lca );
expect( positionB.getCommonAncestor( positionA ) ).to.equal( lca );
}
} );
} );
6 changes: 6 additions & 0 deletions tests/model/range.js
Original file line number Diff line number Diff line change
Expand Up @@ -1223,6 +1223,12 @@ describe( 'Range', () => {
} );
} );

describe( 'getCommonAncestor()', () => {
it( 'should return common ancestor for positions from Range', () => {
expect( range.getCommonAncestor() ).to.equal( root );
} );
} );

function mapNodesToNames( nodes ) {
return nodes.map( node => {
return ( node instanceof Element ) ? 'E:' + node.name : 'T:' + node.data;
Expand Down
65 changes: 65 additions & 0 deletions tests/view/position.js
Original file line number Diff line number Diff line change
Expand Up @@ -519,4 +519,69 @@ describe( 'Position', () => {
document.destroy();
} );
} );

describe( 'getCommonAncestor()', () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here you use positions on the same depts as well.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's just a single test with positions in text nodes and it uses a 0 offset. Please add more, with better offsets (ones which have a chance to break something).

let div, section, article, p, ul, li1, li2, foz, bar, lipsum;

// |- div
// |- ul
// | |- li
// | | |- f
// | | |- o
// | | |- z
// | |- li
// | |- b
// | |- a
// | |- r
// |- section
// |- article
// |- p
// |- l
// |- i
// |- p
// |- s
// |- u
// |- m

beforeEach( () => {
foz = new Text( 'foz' );
bar = new Text( 'bar' );
li1 = new Element( 'li', null, foz );
li2 = new Element( 'li', null, bar );
ul = new Element( 'ul', null, [ li1, li2 ] );

lipsum = new Text( 'lipsum' );
p = new Element( 'p', null, lipsum );
article = new Element( 'article', null, p );
section = new Element( 'section', null, article );

div = new Element( 'div', null, [ ul, section ] );
} );

it( 'for two the same positions returns the parent element', () => {
const fPosition = new Position( li1, 0 );
const otherPosition = Position.createFromPosition( fPosition );

test( fPosition, otherPosition, li1 );
} );

it( 'for two positions in the same element returns the element', () => {
const fPosition = new Position( li1, 0 );
const zPosition = new Position( li1, 2 );

test( fPosition, zPosition, li1 );
} );

it( 'works when one positions is nested deeper than the other', () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"when one position"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or rather "one of the positions"

const zPosition = new Position( li1, 2 );
const iPosition = Position.createAt( lipsum, 'start' );

test( iPosition, zPosition, div );
} );

function test( positionA, positionB, lca ) {
expect( positionA.getCommonAncestor( positionB ) ).to.equal( lca );
expect( positionB.getCommonAncestor( positionA ) ).to.equal( lca );
}
} );
} );
16 changes: 16 additions & 0 deletions tests/view/range.js
Original file line number Diff line number Diff line change
Expand Up @@ -692,4 +692,20 @@ describe( 'Range', () => {
} );
} );
} );

describe( 'getCommonAncestor()', () => {
it( 'should return common ancestor for positions from Range', () => {
const foz = new Text( 'foz' );
const bar = new Text( 'bar' );

const li1 = new Element( 'li', null, foz );
const li2 = new Element( 'li', null, bar );

const ul = new Element( 'ul', null, [ li1, li2 ] );

const range = new Range( new Position( li1, 0 ), new Position( li2, 2 ) );

expect( range.getCommonAncestor() ).to.equal( ul );
} );
} );
} );