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

Move attribute element highlight code from link to typing #7546

Merged
merged 23 commits into from
Jul 21, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
6a81a58
Add attribute assertion, to make test results more meaningful.
tomalec Jun 29, 2020
a6d3f01
Move the link highlight code to the -typing,
tomalec Jun 29, 2020
49276bc
Make findAttributeRange code less Link-specific.
tomalec Jun 29, 2020
802289e
Make highlight code less Link-specific.
tomalec Jun 29, 2020
c39cf62
Merge branch 'i/7444-2SCM-refactor' into i/7444-2SCM-refactor-highlight
tomalec Jul 1, 2020
64a32cc
Improve the usage of attribute assertion.
tomalec Jul 1, 2020
50dc07f
Use to.have.property assertion, to make test results more descriptive.
tomalec Jul 1, 2020
d03f0a8
Check the rendering in "should render … after splitting link" test.
tomalec Jul 1, 2020
f9b33f0
Merge branch 'master' into i/7444-2SCM-refactor-highlight
tomalec Jul 3, 2020
52a7bc7
Use attribute assertion for core test utils.
tomalec Jul 3, 2020
63c6485
Add tests for setupHighlight function,
tomalec Jul 5, 2020
780ea63
Replace enter command with lower-level split.
tomalec Jul 5, 2020
eb6dc86
Unify inlineHighlight function name,
tomalec Jul 20, 2020
ea2c463
Rephrase findAttributeRange API docs,
tomalec Jul 20, 2020
7bf18c0
Merge branch 'master' into i/7444-2SCM-refactor-highlight
tomalec Jul 20, 2020
ca94192
Merge branch 'master' into i/7444-2SCM-refactor-highlight
tomalec Jul 21, 2020
97f0f5a
Fix findLinkRange usage, missed after merge.
tomalec Jul 21, 2020
9da4517
Remove unused findLinkRange import, missed after merge.
tomalec Jul 21, 2020
2d964fa
Use sinon-chai and named spies in twostepcaretmovement.js.
tomalec Jul 21, 2020
bd3cc2e
Update findattributerange() docs.
jodator Jul 21, 2020
2d24eac
Merge branch 'master' into i/7444-2SCM-refactor-highlight
jodator Jul 21, 2020
b79115d
Fix sample in the preserving custom content guide.
jodator Jul 21, 2020
06c8992
Fix inlineHighlight() after is() API change.
jodator Jul 21, 2020
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
Original file line number Diff line number Diff line change
Expand Up @@ -429,7 +429,7 @@ function downcastCustomClassesToChild( viewElementName, modelElementName ) {
function findViewChild( viewElement, viewElementName, conversionApi ) {
const viewChildren = Array.from( conversionApi.writer.createRangeIn( viewElement ).getItems() );

return viewChildren.find( item => item.is( viewElementName ) );
return viewChildren.find( item => item.is( 'element', viewElementName ) );
}

/**
Expand Down
4 changes: 2 additions & 2 deletions packages/ckeditor5-link/src/linkcommand.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
*/

import Command from '@ckeditor/ckeditor5-core/src/command';
import findLinkRange from './findlinkrange';
import findAttributeRange from '@ckeditor/ckeditor5-typing/src/utils/findattributerange';
import toMap from '@ckeditor/ckeditor5-utils/src/tomap';
import Collection from '@ckeditor/ckeditor5-utils/src/collection';
import first from '@ckeditor/ckeditor5-utils/src/first';
Expand Down Expand Up @@ -171,7 +171,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( position, selection.getAttribute( 'linkHref' ), model );
const linkRange = findAttributeRange( position, 'linkHref', selection.getAttribute( 'linkHref' ), model );

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

Expand Down
70 changes: 5 additions & 65 deletions packages/ckeditor5-link/src/linkediting.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,12 +10,13 @@
import Plugin from '@ckeditor/ckeditor5-core/src/plugin';
import MouseObserver from '@ckeditor/ckeditor5-engine/src/view/observer/mouseobserver';
import TwoStepCaretMovement from '@ckeditor/ckeditor5-typing/src/twostepcaretmovement';
import inlineHighlight from '@ckeditor/ckeditor5-typing/src/utils/inlinehighlight';
import Input from '@ckeditor/ckeditor5-typing/src/input';
import Clipboard from '@ckeditor/ckeditor5-clipboard/src/clipboard';
import LinkCommand from './linkcommand';
import UnlinkCommand from './unlinkcommand';
import ManualDecorator from './utils/manualdecorator';
import findLinkRange from './findlinkrange';
import findAttributeRange from '@ckeditor/ckeditor5-typing/src/utils/findattributerange';
import { createLinkElement, ensureSafeUrl, getLocalizedDecorators, normalizeDecorators } from './utils';

import '../theme/link.css';
Expand Down Expand Up @@ -105,7 +106,7 @@ export default class LinkEditing extends Plugin {
twoStepCaretMovementPlugin.registerAttribute( 'linkHref' );

// Setup highlight over selected link.
this._setupLinkHighlight();
inlineHighlight( editor, 'linkHref', 'a', HIGHLIGHT_CLASS );

// Change the attributes of the selection in certain situations after the link was inserted into the document.
this._enableInsertContentSelectionAttributesFixer();
Expand Down Expand Up @@ -207,67 +208,6 @@ export default class LinkEditing extends Plugin {
} );
}

/**
* Adds a visual highlight style to a link in which the selection is anchored.
* Together with two-step caret movement, they indicate that the user is typing inside the link.
*
* Highlight is turned on by adding the `.ck-link_selected` class to the link in the view:
*
* * The class is removed before the conversion has started, as callbacks added with the `'highest'` priority
* to {@link module:engine/conversion/downcastdispatcher~DowncastDispatcher} events.
* * The class is added in the view post fixer, after other changes in the model tree were converted to the view.
*
* This way, adding and removing the highlight does not interfere with conversion.
*
* @private
*/
_setupLinkHighlight() {
const editor = this.editor;
const view = editor.editing.view;
const highlightedLinks = new Set();

// Adding the class.
view.document.registerPostFixer( writer => {
const selection = editor.model.document.selection;
let changed = false;

if ( selection.hasAttribute( '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
// broken by a UIElement.
for ( const item of viewRange.getItems() ) {
if ( item.is( 'element', 'a' ) && !item.hasClass( HIGHLIGHT_CLASS ) ) {
writer.addClass( HIGHLIGHT_CLASS, item );
highlightedLinks.add( item );
changed = true;
}
}
}

return changed;
} );

// Removing the class.
editor.conversion.for( 'editingDowncast' ).add( dispatcher => {
// Make sure the highlight is removed on every possible event, before conversion is started.
dispatcher.on( 'insert', removeHighlight, { priority: 'highest' } );
dispatcher.on( 'remove', removeHighlight, { priority: 'highest' } );
dispatcher.on( 'attribute', removeHighlight, { priority: 'highest' } );
dispatcher.on( 'selection', removeHighlight, { priority: 'highest' } );

function removeHighlight() {
view.change( writer => {
for ( const item of highlightedLinks.values() ) {
writer.removeClass( HIGHLIGHT_CLASS, item );
highlightedLinks.delete( item );
}
} );
}
} );
}

/**
* Starts listening to {@link module:engine/model/model~Model#event:insertContent} and corrects the model
* selection attributes if the selection is at the end of a link after inserting the content.
Expand Down Expand Up @@ -407,7 +347,7 @@ export default class LinkEditing extends Plugin {
}

const position = selection.getFirstPosition();
const linkRange = findLinkRange( position, selection.getAttribute( 'linkHref' ), editor.model );
const linkRange = findAttributeRange( position, 'linkHref', selection.getAttribute( 'linkHref' ), editor.model );

// ...check whether clicked start/end boundary of the link.
// If so, remove the `linkHref` attribute.
Expand Down Expand Up @@ -536,7 +476,7 @@ function shouldCopyAttributes( model ) {

// If nodes are not equal, maybe the link nodes has defined additional attributes inside.
// First, we need to find the entire link range.
const linkRange = findLinkRange( firstPosition, nodeAtFirstPosition.getAttribute( 'linkHref' ), model );
const linkRange = findAttributeRange( firstPosition, 'linkHref', nodeAtFirstPosition.getAttribute( 'linkHref' ), model );

// Then we can check whether selected range is inside the found link range. If so, attributes should be preserved.
return linkRange.containsRange( model.createRange( firstPosition, lastPosition ), true );
Expand Down
10 changes: 8 additions & 2 deletions packages/ckeditor5-link/src/unlinkcommand.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,8 @@
*/

import Command from '@ckeditor/ckeditor5-core/src/command';
import findAttributeRange from '@ckeditor/ckeditor5-typing/src/utils/findattributerange';
import first from '@ckeditor/ckeditor5-utils/src/first';
import findLinkRange from './findlinkrange';
import { isImageAllowed } from './utils';

/**
Expand Down Expand Up @@ -58,7 +58,13 @@ export default class UnlinkCommand extends Command {
model.change( writer => {
// Get ranges to unlink.
const rangesToUnlink = selection.isCollapsed ?
[ findLinkRange( selection.getFirstPosition(), selection.getAttribute( 'linkHref' ), model ) ] : selection.getRanges();
[ findAttributeRange(
selection.getFirstPosition(),
'linkHref',
selection.getAttribute( 'linkHref' ),
model
) ] :
selection.getRanges();

// Remove `linkHref` attribute from specified ranges.
for ( const range of rangesToUnlink ) {
Expand Down
46 changes: 26 additions & 20 deletions packages/ckeditor5-link/tests/linkediting.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,8 @@ import { getData as getViewData } from '@ckeditor/ckeditor5-engine/src/dev-utils
import { keyCodes } from '@ckeditor/ckeditor5-utils/src/keyboard';
import { isLinkElement } from '../src/utils';

import '@ckeditor/ckeditor5-core/tests/_utils/assertions/attribute';

/* global document */

describe( 'LinkEditing', () => {
Expand Down Expand Up @@ -89,7 +91,7 @@ describe( 'LinkEditing', () => {
setModelData( editor.model, '<paragraph>foo[]<$text linkHref="url">b</$text>ar</paragraph>' );

// The selection's gravity should read attributes from the left.
expect( selection.hasAttribute( 'linkHref' ), 'hasAttribute( \'linkHref\' )' ).to.be.false;
expect( selection ).not.to.have.attribute( 'linkHref' );

// So let's simulate the `keydown` event.
editor.editing.view.document.fire( 'keydown', {
Expand All @@ -100,8 +102,8 @@ describe( 'LinkEditing', () => {

expect( getModelData( model ) ).to.equal( '<paragraph>foo<$text linkHref="url">[]b</$text>ar</paragraph>' );
// Selection should get the attributes from the right.
expect( selection.hasAttribute( 'linkHref' ), 'hasAttribute( \'linkHref\' )' ).to.be.true;
expect( selection.getAttribute( 'linkHref' ), 'linkHref attribute' ).to.equal( 'url' );
expect( selection ).to.have.attribute( 'linkHref' );
expect( selection ).to.have.attribute( 'linkHref', 'url' );
} );

it( 'should be bound to the `linkHref` attribute (RTL)', async () => {
Expand All @@ -120,7 +122,7 @@ describe( 'LinkEditing', () => {
setModelData( editor.model, '<paragraph>foo[]<$text linkHref="url">b</$text>ar</paragraph>' );

// The selection's gravity should read attributes from the left.
expect( selection.hasAttribute( 'linkHref' ), 'hasAttribute( \'linkHref\' )' ).to.be.false;
expect( selection ).not.to.have.attribute( 'linkHref' );

// So let's simulate the `keydown` event.
editor.editing.view.document.fire( 'keydown', {
Expand All @@ -131,8 +133,8 @@ describe( 'LinkEditing', () => {

expect( getModelData( model ) ).to.equal( '<paragraph>foo<$text linkHref="url">[]b</$text>ar</paragraph>' );
// Selection should get the attributes from the right.
expect( selection.hasAttribute( 'linkHref' ), 'hasAttribute( \'linkHref\' )' ).to.be.true;
expect( selection.getAttribute( 'linkHref' ), 'linkHref attribute' ).to.equal( 'url' );
expect( selection ).to.have.attribute( 'linkHref' );
expect( selection ).to.have.attribute( 'linkHref', 'url' );

await editor.destroy();
} );
Expand Down Expand Up @@ -182,7 +184,7 @@ describe( 'LinkEditing', () => {
'</paragraph>'
);

expect( [ ...model.document.selection.getAttributeKeys() ] ).to.have.members( [ 'bold' ] );
expect( model.document.selection ).to.have.attribute( 'bold' );
} );

it( 'should not remove link atttributes when pasting in the middle of a link with the same URL', () => {
Expand All @@ -193,7 +195,7 @@ describe( 'LinkEditing', () => {
} );

expect( getModelData( model ) ).to.equal( '<paragraph><$text linkHref="ckeditor.com">foINSERTED[]o</$text></paragraph>' );
expect( [ ...model.document.selection.getAttributeKeys() ] ).to.have.members( [ 'linkHref' ] );
expect( model.document.selection ).to.have.attribute( 'linkHref' );
} );

it( 'should not remove link atttributes from the selection when pasting before a link when the gravity is overridden', () => {
Expand All @@ -205,7 +207,7 @@ describe( 'LinkEditing', () => {
domTarget: document.body
} );

expect( model.document.selection.isGravityOverridden ).to.be.true;
expect( model.document.selection ).to.have.property( 'isGravityOverridden', true );

model.change( writer => {
model.insertContent( writer.createText( 'INSERTED', { bold: true } ) );
Expand All @@ -219,8 +221,8 @@ describe( 'LinkEditing', () => {
'</paragraph>'
);

expect( model.document.selection.isGravityOverridden ).to.be.true;
expect( [ ...model.document.selection.getAttributeKeys() ] ).to.have.members( [ 'linkHref' ] );
expect( model.document.selection ).to.have.property( 'isGravityOverridden', true );
expect( model.document.selection ).to.have.attribute( 'linkHref' );
} );

it( 'should not remove link atttributes when pasting a link into another link (different URLs, no merge)', () => {
Expand All @@ -238,13 +240,13 @@ describe( 'LinkEditing', () => {
'</paragraph>'
);

expect( [ ...model.document.selection.getAttributeKeys() ] ).to.have.members( [ 'linkHref' ] );
expect( model.document.selection ).to.have.attribute( 'linkHref' );
} );

it( 'should not remove link atttributes when pasting before another link (different URLs, no merge)', () => {
setModelData( model, '<paragraph>[]<$text linkHref="ckeditor.com">foo</$text></paragraph>' );

expect( model.document.selection.isGravityOverridden ).to.be.false;
expect( model.document.selection ).to.have.property( 'isGravityOverridden', false );

model.change( writer => {
model.insertContent( writer.createText( 'INSERTED', { linkHref: 'http://INSERTED' } ) );
Expand All @@ -257,8 +259,8 @@ describe( 'LinkEditing', () => {
'</paragraph>'
);

expect( [ ...model.document.selection.getAttributeKeys() ] ).to.have.members( [ 'linkHref' ] );
expect( model.document.selection.getAttribute( 'linkHref' ) ).to.equal( 'http://INSERTED' );
expect( model.document.selection ).to.have.attribute( 'linkHref' );
expect( model.document.selection ).to.have.attribute( 'linkHref', 'http://INSERTED' );
} );
} );

Expand Down Expand Up @@ -375,7 +377,7 @@ describe( 'LinkEditing', () => {
'<paragraph>foo <$text linkHref="url">b{}ar</$text> baz</paragraph>'
);

expect( model.document.selection.hasAttribute( 'linkHref' ) ).to.be.true;
expect( model.document.selection ).to.have.attribute( 'linkHref' );
expect( getViewData( view ) ).to.equal(
'<p>foo <a class="ck-link_selected" href="url">b{}ar</a> baz</p>'
);
Expand All @@ -386,13 +388,13 @@ describe( 'LinkEditing', () => {
'<paragraph>foo {}<$text linkHref="url">bar</$text> baz</paragraph>'
);

expect( model.document.selection.hasAttribute( 'linkHref' ) ).to.be.false;
expect( model.document.selection ).to.not.have.attribute( 'linkHref' );

model.change( writer => {
writer.setSelectionAttribute( 'linkHref', 'url' );
} );

expect( model.document.selection.hasAttribute( 'linkHref' ) ).to.be.true;
expect( model.document.selection ).to.have.attribute( 'linkHref' );
expect( getViewData( view ) ).to.equal(
'<p>foo <a class="ck-link_selected" href="url">{}bar</a> baz</p>'
);
Expand All @@ -403,7 +405,7 @@ describe( 'LinkEditing', () => {
'<paragraph>foo <$text linkHref="url">bar</$text>{} baz</paragraph>'
);

expect( model.document.selection.hasAttribute( 'linkHref' ) ).to.be.true;
expect( model.document.selection ).to.have.attribute( 'linkHref' );
expect( getViewData( view ) ).to.equal(
'<p>foo <a class="ck-link_selected" href="url">bar{}</a> baz</p>'
);
Expand All @@ -421,7 +423,11 @@ describe( 'LinkEditing', () => {
'<paragraph><$text linkHref="url">[]nk</$text> baz</paragraph>'
);

expect( model.document.selection.hasAttribute( 'linkHref' ) ).to.be.true;
expect( model.document.selection ).to.have.attribute( 'linkHref' );
expect( getViewData( view ) ).to.equal(
'<p>foo <a href="url">li</a></p>' +
'<p><a class="ck-link_selected" href="url">{}nk</a> baz</p>'
);
} );

it( 'should remove classes when selection is moved out from the link', () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,38 +4,45 @@
*/

/**
* @module link/findlinkrange
* @module typing/utils/findattributerange
*/

/**
* Returns a range containing the entire link in which the given `position` is placed.
* Returns a model range that covers all consecutive nodes with the same `attributeName` and its `value`
* that intersect the given `position`.
*
* It can be used e.g. to get the entire range on which the `linkHref` attribute needs to be changed when having a
* selection inside a link.
*
* @param {module:engine/model/position~Position} position The start position.
* @param {String} value The `linkHref` attribute value.
* @param {String} attributeName The attribute name.
* @param {String} value The attribute value.
* @param {module:engine/model/model~Model} model The model instance.
* @returns {module:engine/model/range~Range} The link range.
*/
export default function findLinkRange( position, value, model ) {
return model.createRange( _findBound( position, value, true, model ), _findBound( position, value, false, model ) );
export default function findAttributeRange( position, attributeName, value, model ) {
jodator marked this conversation as resolved.
Show resolved Hide resolved
return model.createRange(
_findBound( position, attributeName, value, true, model ),
_findBound( position, attributeName, 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
// Walks forward or backward (depends on the `lookBack` flag), node by node, as long as they have the same attribute value
// and returns a position just before or after (depends on the `lookBack` flag) the last matched node.
//
// @param {module:engine/model/position~Position} position The start position.
// @param {String} value The `linkHref` attribute value.
// @param {String} attributeName The attribute name.
// @param {String} value The 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, model ) {
function _findBound( position, attributeName, 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 );

let lastNode = null;

while ( node && node.getAttribute( 'linkHref' ) == value ) {
while ( node && node.getAttribute( attributeName ) == value ) {
lastNode = node;
node = lookBack ? node.previousSibling : node.nextSibling;
}
Expand Down
Loading