Skip to content

Commit

Permalink
Merge pull request #7546 from ckeditor/i/7444-2SCM-refactor-highlight
Browse files Browse the repository at this point in the history 
Other: Link's attribute element highlight is now `inlineHighlight()` - a public utility.
  • Loading branch information
@jodator
jodator authored Jul 21, 2020
2 parents dbee479 + 06c8992 commit fc59dc4
Show file tree
Hide file tree
Showing 10 changed files with 423 additions and 141 deletions.
2 changes: 1 addition & 1 deletion ...-engine/docs/framework/guides/deep-dive/conversion-preserving-custom-content.md
View file
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
View file
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
View file
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
View file
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
View file
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
25 changes: 16 additions & 9 deletions packages/ckeditor5-link/src/findlinkrange.js → ...r5-typing/src/utils/findattributerange.js
View file
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 ) {
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

0 comments on commit fc59dc4

Please sign in to comment.