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

T/ckeditor5 engine/1555 #206

Merged
merged 4 commits into from
Nov 1, 2018
Merged

T/ckeditor5 engine/1555 #206

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4766717
Remove Position, Range, Selection imports from engine/model.
jodator Sep 26, 2018
da3c42e
Remove Position, Range, Selection imports from view/model.
jodator Oct 15, 2018
6422424
Merge branch 'master' into t/ckeditor5-engine/1555
jodator Oct 29, 2018
84853b8
Review writerSetSelection() API calls with single position.
jodator Oct 29, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 4 additions & 7 deletions src/findlinkrange.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,6 @@
* @module link/findlinkrange
*/

import Range from '@ckeditor/ckeditor5-engine/src/model/range';
import Position from '@ckeditor/ckeditor5-engine/src/model/position';

/**
* Returns a range containing the entire link in which the given `position` is placed.
*
Expand All @@ -20,8 +17,8 @@ import Position from '@ckeditor/ckeditor5-engine/src/model/position';
* @param {String} value The `linkHref` attribute value.
* @returns {module:engine/model/range~Range} The link range.
*/
export default function findLinkRange( position, value ) {
return new Range( _findBound( position, value, true ), _findBound( position, value, false ) );
export default function findLinkRange( position, value, model ) {
return model.createRange( _findBound( position, value, true, model ), _findBound( position, value, false, model ) );
}

// Walks forward or backward (depends on the `lookBack` flag), node by node, as long as they have the same `linkHref` attribute value
Expand All @@ -31,7 +28,7 @@ export default function findLinkRange( position, value ) {
// @param {String} value The `linkHref` attribute value.
// @param {Boolean} lookBack Whether the walk direction is forward (`false`) or backward (`true`).
// @returns {module:engine/model/position~Position} The position just before the last matched node.
function _findBound( position, value, lookBack ) {
function _findBound( position, value, lookBack, model ) {
// Get node before or after position (depends on `lookBack` flag).
// When position is inside text node then start searching from text node.
let node = position.textNode || ( lookBack ? position.nodeBefore : position.nodeAfter );
Expand All @@ -43,5 +40,5 @@ function _findBound( position, value, lookBack ) {
node = lookBack ? node.previousSibling : node.nextSibling;
}

return lastNode ? Position.createAt( lastNode, lookBack ? 'before' : 'after' ) : position;
return lastNode ? model.createPositionAt( lastNode, lookBack ? 'before' : 'after' ) : position;
}
5 changes: 2 additions & 3 deletions src/linkcommand.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@
*/

import Command from '@ckeditor/ckeditor5-core/src/command';
import Range from '@ckeditor/ckeditor5-engine/src/model/range';
import findLinkRange from './findlinkrange';
import toMap from '@ckeditor/ckeditor5-utils/src/tomap';

Expand Down Expand Up @@ -65,7 +64,7 @@ export default class LinkCommand extends Command {
// When selection is inside text with `linkHref` attribute.
if ( selection.hasAttribute( 'linkHref' ) ) {
// Then update `linkHref` value.
const linkRange = findLinkRange( selection.getFirstPosition(), selection.getAttribute( 'linkHref' ) );
const linkRange = findLinkRange( selection.getFirstPosition(), selection.getAttribute( 'linkHref' ), model );

writer.setAttribute( 'linkHref', href, linkRange );

Expand All @@ -85,7 +84,7 @@ export default class LinkCommand extends Command {
writer.insert( node, position );

// Create new range wrapping created node.
writer.setSelection( Range.createOn( node ) );
writer.setSelection( writer.createRangeOn( node ) );
}
} else {
// If selection has non-collapsed ranges, we change attribute on nodes inside those ranges
Expand Down
6 changes: 2 additions & 4 deletions src/linkediting.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,7 @@
*/

import Plugin from '@ckeditor/ckeditor5-core/src/plugin';
import {
downcastAttributeToElement
} from '@ckeditor/ckeditor5-engine/src/conversion/downcast-converters';
import { downcastAttributeToElement } from '@ckeditor/ckeditor5-engine/src/conversion/downcast-converters';
import { upcastElementToAttribute } from '@ckeditor/ckeditor5-engine/src/conversion/upcast-converters';
import LinkCommand from './linkcommand';
import UnlinkCommand from './unlinkcommand';
Expand Down Expand Up @@ -96,7 +94,7 @@ export default class LinkEditing extends Plugin {
const selection = editor.model.document.selection;

if ( selection.hasAttribute( 'linkHref' ) ) {
const modelRange = findLinkRange( selection.getFirstPosition(), selection.getAttribute( 'linkHref' ) );
const modelRange = findLinkRange( selection.getFirstPosition(), selection.getAttribute( 'linkHref' ), editor.model );
const viewRange = editor.editing.mapper.toViewRange( modelRange );

// There might be multiple `a` elements in the `viewRange`, for example, when the `a` element is
Expand Down
6 changes: 3 additions & 3 deletions src/linkui.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@

import Plugin from '@ckeditor/ckeditor5-core/src/plugin';
import ClickObserver from '@ckeditor/ckeditor5-engine/src/view/observer/clickobserver';
import Range from '@ckeditor/ckeditor5-engine/src/view/range';
import { isLinkElement } from './utils';
import ContextualBalloon from '@ckeditor/ckeditor5-ui/src/panel/balloon/contextualballoon';

Expand Down Expand Up @@ -515,7 +514,8 @@ export default class LinkUI extends Plugin {
* @returns {module:engine/view/attributeelement~AttributeElement|null}
*/
_getSelectedLinkElement() {
const selection = this.editor.editing.view.document.selection;
const view = this.editor.editing.view;
const selection = view.document.selection;

if ( selection.isCollapsed ) {
return findLinkElementAncestor( selection.getFirstPosition() );
Expand All @@ -531,7 +531,7 @@ export default class LinkUI extends Plugin {
}

// Check if the link element is fully selected.
if ( Range.createIn( startLink ).getTrimmed().isEqual( range ) ) {
if ( view.createRangeIn( startLink ).getTrimmed().isEqual( range ) ) {
return startLink;
} else {
return null;
Expand Down
2 changes: 1 addition & 1 deletion src/unlinkcommand.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ export default class UnlinkCommand extends Command {
model.change( writer => {
// Get ranges to unlink.
const rangesToUnlink = selection.isCollapsed ?
[ findLinkRange( selection.getFirstPosition(), selection.getAttribute( 'linkHref' ) ) ] : selection.getRanges();
[ findLinkRange( selection.getFirstPosition(), selection.getAttribute( 'linkHref' ), model ) ] : selection.getRanges();
Copy link

Choose a reason for hiding this comment

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

It should be simplified...


// Remove `linkHref` attribute from specified ranges.
for ( const range of rangesToUnlink ) {
Expand Down
65 changes: 34 additions & 31 deletions tests/findlinkrange.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@
import findLinkRange from '../src/findlinkrange';
import Model from '@ckeditor/ckeditor5-engine/src/model/model';
import Range from '@ckeditor/ckeditor5-engine/src/model/range';
import Position from '@ckeditor/ckeditor5-engine/src/model/position';
import { setData } from '@ckeditor/ckeditor5-engine/src/dev-utils/model';

describe( 'findLinkRange', () => {
Expand All @@ -23,91 +22,91 @@ describe( 'findLinkRange', () => {
it( 'should find link range searching from the center of the link #1', () => {
setData( model, '<$text linkHref="url">foobar</$text>' );

const startPosition = new Position( root, [ 3 ] );
const result = findLinkRange( startPosition, 'url' );
const startPosition = model.createPositionAt( root, [ 3 ] );
const result = findLinkRange( startPosition, 'url', model );

expect( result ).to.instanceOf( Range );
expect( result.isEqual( Range.createFromParentsAndOffsets( root, 0, root, 6 ) ) ).to.true;
expect( result.isEqual( model.createRange( model.createPositionAt( root, 0 ), model.createPositionAt( root, 6 ) ) ) ).to.true;
} );

it( 'should find link range searching from the center of the link #2', () => {
setData( model, 'abc <$text linkHref="url">foobar</$text> abc' );

const startPosition = new Position( root, [ 7 ] );
const result = findLinkRange( startPosition, 'url' );
const startPosition = model.createPositionAt( root, [ 7 ] );
const result = findLinkRange( startPosition, 'url', model );

expect( result ).to.instanceOf( Range );
expect( result.isEqual( Range.createFromParentsAndOffsets( root, 4, root, 10 ) ) ).to.true;
expect( result.isEqual( model.createRange( model.createPositionAt( root, 4 ), model.createPositionAt( root, 10 ) ) ) ).to.true;
} );

it( 'should find link range searching from the beginning of the link #1', () => {
setData( model, '<$text linkHref="url">foobar</$text>' );

const startPosition = new Position( root, [ 0 ] );
const result = findLinkRange( startPosition, 'url' );
const startPosition = model.createPositionAt( root, [ 0 ] );
const result = findLinkRange( startPosition, 'url', model );

expect( result ).to.instanceOf( Range );
expect( result.isEqual( Range.createFromParentsAndOffsets( root, 0, root, 6 ) ) ).to.true;
expect( result.isEqual( model.createRange( model.createPositionAt( root, 0 ), model.createPositionAt( root, 6 ) ) ) ).to.true;
} );

it( 'should find link range searching from the beginning of the link #2', () => {
setData( model, 'abc <$text linkHref="url">foobar</$text> abc' );

const startPosition = new Position( root, [ 4 ] );
const result = findLinkRange( startPosition, 'url' );
const startPosition = model.createPositionAt( root, [ 4 ] );
const result = findLinkRange( startPosition, 'url', model );

expect( result ).to.instanceOf( Range );
expect( result.isEqual( Range.createFromParentsAndOffsets( root, 4, root, 10 ) ) ).to.true;
expect( result.isEqual( model.createRange( model.createPositionAt( root, 4 ), model.createPositionAt( root, 10 ) ) ) ).to.true;
} );

it( 'should find link range searching from the end of the link #1', () => {
setData( model, '<$text linkHref="url">foobar</$text>' );

const startPosition = new Position( root, [ 6 ] );
const result = findLinkRange( startPosition, 'url' );
const startPosition = model.createPositionAt( root, [ 6 ] );
const result = findLinkRange( startPosition, 'url', model );

expect( result ).to.instanceOf( Range );
expect( result.isEqual( Range.createFromParentsAndOffsets( root, 0, root, 6 ) ) ).to.true;
expect( result.isEqual( model.createRange( model.createPositionAt( root, 0 ), model.createPositionAt( root, 6 ) ) ) ).to.true;
} );

it( 'should find link range searching from the end of the link #2', () => {
setData( model, 'abc <$text linkHref="url">foobar</$text> abc' );

const startPosition = new Position( root, [ 10 ] );
const result = findLinkRange( startPosition, 'url' );
const startPosition = model.createPositionAt( root, [ 10 ] );
const result = findLinkRange( startPosition, 'url', model );

expect( result ).to.instanceOf( Range );
expect( result.isEqual( Range.createFromParentsAndOffsets( root, 4, root, 10 ) ) ).to.true;
expect( result.isEqual( model.createRange( model.createPositionAt( root, 4 ), model.createPositionAt( root, 10 ) ) ) ).to.true;
} );

it( 'should find link range when link stick to other link searching from the center of the link', () => {
setData( model, '<$text linkHref="other">abc</$text><$text linkHref="url">foobar</$text><$text linkHref="other">abc</$text>' );

const startPosition = new Position( root, [ 6 ] );
const result = findLinkRange( startPosition, 'url' );
const startPosition = model.createPositionAt( root, [ 6 ] );
const result = findLinkRange( startPosition, 'url', model );

expect( result ).to.instanceOf( Range );
expect( result.isEqual( Range.createFromParentsAndOffsets( root, 3, root, 9 ) ) ).to.true;
expect( result.isEqual( model.createRange( model.createPositionAt( root, 3 ), model.createPositionAt( root, 9 ) ) ) ).to.true;
} );

it( 'should find link range when link stick to other link searching from the beginning of the link', () => {
setData( model, '<$text linkHref="other">abc</$text><$text linkHref="url">foobar</$text><$text linkHref="other">abc</$text>' );

const startPosition = new Position( root, [ 3 ] );
const result = findLinkRange( startPosition, 'url' );
const startPosition = model.createPositionAt( root, [ 3 ] );
const result = findLinkRange( startPosition, 'url', model );

expect( result ).to.instanceOf( Range );
expect( result.isEqual( Range.createFromParentsAndOffsets( root, 3, root, 9 ) ) ).to.true;
expect( result.isEqual( model.createRange( model.createPositionAt( root, 3 ), model.createPositionAt( root, 9 ) ) ) ).to.true;
} );

it( 'should find link range when link stick to other link searching from the end of the link', () => {
setData( model, '<$text linkHref="other">abc</$text><$text linkHref="url">foobar</$text><$text linkHref="other">abc</$text>' );

const startPosition = new Position( root, [ 9 ] );
const result = findLinkRange( startPosition, 'url' );
const startPosition = model.createPositionAt( root, [ 9 ] );
const result = findLinkRange( startPosition, 'url', model );

expect( result ).to.instanceOf( Range );
expect( result.isEqual( Range.createFromParentsAndOffsets( root, 3, root, 9 ) ) ).to.true;
expect( result.isEqual( model.createRange( model.createPositionAt( root, 3 ), model.createPositionAt( root, 9 ) ) ) ).to.true;
} );

it( 'should find link range only inside current parent', () => {
Expand All @@ -118,10 +117,14 @@ describe( 'findLinkRange', () => {
'<p><$text linkHref="url">foobar</$text></p>'
);

const startPosition = new Position( root, [ 1, 3 ] );
const result = findLinkRange( startPosition, 'url' );
const startPosition = model.createPositionAt( root.getNodeByPath( [ 1 ] ), 3 );
const result = findLinkRange( startPosition, 'url', model );

expect( result ).to.instanceOf( Range );
expect( result.isEqual( new Range( new Position( root, [ 1, 0 ] ), new Position( root, [ 1, 6 ] ) ) ) ).to.true;
const expectedRange = model.createRange(
model.createPositionAt( root.getNodeByPath( [ 1 ] ), 0 ),
model.createPositionAt( root.getNodeByPath( [ 1 ] ), 6 )
);
expect( result.isEqual( expectedRange ) ).to.true;
} );
} );
19 changes: 9 additions & 10 deletions tests/linkediting.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ import UnlinkCommand from '../src/unlinkcommand';
import VirtualTestEditor from '@ckeditor/ckeditor5-core/tests/_utils/virtualtesteditor';
import Paragraph from '@ckeditor/ckeditor5-paragraph/src/paragraph';
import Enter from '@ckeditor/ckeditor5-enter/src/enter';
import ModelRange from '@ckeditor/ckeditor5-engine/src/model/range';
import { getData as getModelData, setData as setModelData } from '@ckeditor/ckeditor5-engine/src/dev-utils/model';
import { getData as getViewData } from '@ckeditor/ckeditor5-engine/src/dev-utils/view';
import { isLinkElement } from '../src/utils';
Expand Down Expand Up @@ -283,10 +282,10 @@ describe( 'LinkEditing', () => {
);

model.change( writer => {
writer.remove( ModelRange.createFromParentsAndOffsets(
model.document.getRoot().getChild( 0 ), 0,
model.document.getRoot().getChild( 0 ), 5 )
);
writer.remove( writer.createRange(
writer.createPositionAt( model.document.getRoot().getChild( 0 ), 0 ),
writer.createPositionAt( model.document.getRoot().getChild( 0 ), 5 )
) );
} );

expect( getViewData( view ) ).to.equal(
Expand All @@ -300,7 +299,7 @@ describe( 'LinkEditing', () => {
);

model.change( writer => {
writer.setAttribute( 'linkHref', 'new-url', new ModelRange(
writer.setAttribute( 'linkHref', 'new-url', writer.createRange(
model.document.selection.getFirstPosition().getShiftedBy( -1 ),
model.document.selection.getFirstPosition().getShiftedBy( 1 ) )
);
Expand All @@ -317,7 +316,7 @@ describe( 'LinkEditing', () => {
);

model.change( writer => {
writer.setSelection( new ModelRange(
writer.setSelection( writer.createRange(
model.document.selection.getFirstPosition().getShiftedBy( -1 ),
model.document.selection.getFirstPosition().getShiftedBy( 1 ) )
);
Expand All @@ -336,9 +335,9 @@ describe( 'LinkEditing', () => {
);

model.change( writer => {
const range = ModelRange.createFromParentsAndOffsets(
model.document.getRoot().getChild( 0 ), 0,
model.document.getRoot().getChild( 0 ), 5
const range = writer.createRange(
writer.createPositionAt( model.document.getRoot().getChild( 0 ), 0 ),
writer.createPositionAt( model.document.getRoot().getChild( 0 ), 5 )
);

writer.addMarker( 'fooMarker', { range, usingOperation: true } );
Expand Down
17 changes: 11 additions & 6 deletions tests/linkui.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ import LinkActionsView from '../src/ui/linkactionsview';
import ContextualBalloon from '@ckeditor/ckeditor5-ui/src/panel/balloon/contextualballoon';
import ButtonView from '@ckeditor/ckeditor5-ui/src/button/buttonview';

import Range from '@ckeditor/ckeditor5-engine/src/view/range';
import ClickObserver from '@ckeditor/ckeditor5-engine/src/view/observer/clickobserver';

describe( 'LinkUI', () => {
Expand Down Expand Up @@ -291,7 +290,7 @@ describe( 'LinkUI', () => {

// Move selection to foo[].
view.change( writer => {
writer.setSelection( Range.createFromParentsAndOffsets( text, 3, text, 3 ), true );
writer.setSelection( text, 3, true );
} );

sinon.assert.calledOnce( spy );
Expand All @@ -311,7 +310,7 @@ describe( 'LinkUI', () => {
const text = root.getChild( 0 ).getChild( 0 );

view.change( writer => {
writer.setSelection( Range.createFromParentsAndOffsets( text, 3, text, 3 ), true );
writer.setSelection( text, 3, true );
} );

sinon.assert.calledOnce( spy );
Expand All @@ -334,7 +333,7 @@ describe( 'LinkUI', () => {

// Move selection to b[]ar.
view.change( writer => {
writer.setSelection( Range.createFromParentsAndOffsets( text, 1, text, 1 ), true );
writer.setSelection( text, 1, true );
} );

sinon.assert.calledOnce( spyHide );
Expand All @@ -357,7 +356,10 @@ describe( 'LinkUI', () => {

// Move selection to f[o]o.
view.change( writer => {
writer.setSelection( Range.createFromParentsAndOffsets( text, 1, text, 2 ), true );
writer.setSelection( writer.createRange(
writer.createPositionAt( text, 1 ),
writer.createPositionAt( text, 2 )
), true );
} );

sinon.assert.calledOnce( spyHide );
Expand All @@ -378,7 +380,10 @@ describe( 'LinkUI', () => {

// Move selection to f[o]o.
view.change( writer => {
writer.setSelection( Range.createFromParentsAndOffsets( text, 1, text, 2 ), true );
writer.setSelection( writer.createRange(
writer.createPositionAt( text, 1 ),
writer.createPositionAt( text, 2 )
), true );
} );

sinon.assert.calledOnce( spyHide );
Expand Down
Loading