From 6a81a5871cab2be45bd7f2657e8df7a94e706802 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomek=20Wytr=C4=99bowicz?= Date: Mon, 29 Jun 2020 11:06:30 +0200 Subject: [PATCH 01/18] Add attribute assertion, to make test results more meaningful. --- packages/ckeditor5-link/tests/linkediting.js | 27 ++++++++++++++++---- 1 file changed, 22 insertions(+), 5 deletions(-) diff --git a/packages/ckeditor5-link/tests/linkediting.js b/packages/ckeditor5-link/tests/linkediting.js index 12b31e754ab..25e20d1abe5 100644 --- a/packages/ckeditor5-link/tests/linkediting.js +++ b/packages/ckeditor5-link/tests/linkediting.js @@ -18,7 +18,24 @@ import { keyCodes } from '@ckeditor/ckeditor5-utils/src/keyboard'; import Typing from '@ckeditor/ckeditor5-typing/src/typing'; import BoldEditing from '@ckeditor/ckeditor5-basic-styles/src/bold/boldediting'; -/* global document */ +/* globals document, chai */ + +chai.Assertion.addMethod( 'attribute', function attributeAssertion( type ) { + const obj = this._obj; + + // Check if it has the method at all. + new chai.Assertion( obj ).to.respondTo( 'hasAttribute' ); + + // Check if it has the attribute. + const hasAttribute = obj.hasAttribute( type ); + this.assert( + hasAttribute === true, + `expected #{this} to have '${ type }' attribute`, + `expected #{this} to not have the '${ type }' attribute`, + !chai.util.flag( this, 'negate' ), + hasAttribute + ); +} ); describe( 'LinkEditing', () => { let element, editor, model, view; @@ -371,7 +388,7 @@ describe( 'LinkEditing', () => { 'foo <$text linkHref="url">b{}ar baz' ); - expect( model.document.selection.hasAttribute( 'linkHref' ) ).to.be.true; + expect( model.document.selection ).to.have.an.attribute( 'linkHref' ); expect( getViewData( view ) ).to.equal( '

foo b{}ar baz

' ); @@ -388,7 +405,7 @@ describe( 'LinkEditing', () => { writer.setSelectionAttribute( 'linkHref', 'url' ); } ); - expect( model.document.selection.hasAttribute( 'linkHref' ) ).to.be.true; + expect( model.document.selection ).to.have.an.attribute( 'linkHref' ); expect( getViewData( view ) ).to.equal( '

foo {}bar baz

' ); @@ -399,7 +416,7 @@ describe( 'LinkEditing', () => { 'foo <$text linkHref="url">bar{} baz' ); - expect( model.document.selection.hasAttribute( 'linkHref' ) ).to.be.true; + expect( model.document.selection ).to.have.an.attribute( 'linkHref' ); expect( getViewData( view ) ).to.equal( '

foo bar{} baz

' ); @@ -417,7 +434,7 @@ describe( 'LinkEditing', () => { '<$text linkHref="url">[]nk baz' ); - expect( model.document.selection.hasAttribute( 'linkHref' ) ).to.be.true; + expect( model.document.selection ).to.have.an.attribute( 'linkHref' ); } ); it( 'should remove classes when selection is moved out from the link', () => { From a6d3f013692536909689b56fc73d453f3e00d4b2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomek=20Wytr=C4=99bowicz?= Date: Mon, 29 Jun 2020 16:01:00 +0200 Subject: [PATCH 02/18] Move the link highlight code to the -typing, rename findLinkRange to findAttributeRange, so it could be shared between attributes. --- packages/ckeditor5-link/src/linkcommand.js | 4 +- packages/ckeditor5-link/src/linkediting.js | 68 ++---------------- packages/ckeditor5-link/src/unlinkcommand.js | 4 +- .../src/utils/findattributerange.js} | 4 +- .../src/utils/inlinehighlight.js | 70 +++++++++++++++++++ .../tests/utils/findattributerange.js} | 24 +++---- 6 files changed, 92 insertions(+), 82 deletions(-) rename packages/{ckeditor5-link/src/findlinkrange.js => ckeditor5-typing/src/utils/findattributerange.js} (94%) create mode 100644 packages/ckeditor5-typing/src/utils/inlinehighlight.js rename packages/{ckeditor5-link/tests/findlinkrange.js => ckeditor5-typing/tests/utils/findattributerange.js} (86%) diff --git a/packages/ckeditor5-link/src/linkcommand.js b/packages/ckeditor5-link/src/linkcommand.js index 318dc7e803a..a7dfae84503 100644 --- a/packages/ckeditor5-link/src/linkcommand.js +++ b/packages/ckeditor5-link/src/linkcommand.js @@ -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'; @@ -160,7 +160,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, selection.getAttribute( 'linkHref' ), model ); writer.setAttribute( 'linkHref', href, linkRange ); diff --git a/packages/ckeditor5-link/src/linkediting.js b/packages/ckeditor5-link/src/linkediting.js index 9d8b633e59d..9eeff3db1c4 100644 --- a/packages/ckeditor5-link/src/linkediting.js +++ b/packages/ckeditor5-link/src/linkediting.js @@ -10,11 +10,12 @@ 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 setupLinkHighlight from '@ckeditor/ckeditor5-typing/src/utils/inlinehighlight'; import LinkCommand from './linkcommand'; import UnlinkCommand from './unlinkcommand'; import AutomaticDecorators from './utils/automaticdecorators'; 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'; @@ -103,7 +104,7 @@ export default class LinkEditing extends Plugin { twoStepCaretMovementPlugin.registerAttribute( 'linkHref' ); // Setup highlight over selected link. - this._setupLinkHighlight(); + setupLinkHighlight( editor, HIGHLIGHT_CLASS ); // Change the attributes of the selection in certain situations after the link was inserted into the document. this._enableInsertContentSelectionAttributesFixer(); @@ -199,67 +200,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( '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. @@ -399,7 +339,7 @@ export default class LinkEditing extends Plugin { } const position = selection.getFirstPosition(); - const linkRange = findLinkRange( position, selection.getAttribute( 'linkHref' ), editor.model ); + const linkRange = findAttributeRange( position, selection.getAttribute( 'linkHref' ), editor.model ); // ...check whether clicked start/end boundary of the link. // If so, remove the `linkHref` attribute. diff --git a/packages/ckeditor5-link/src/unlinkcommand.js b/packages/ckeditor5-link/src/unlinkcommand.js index fa6484d430e..3389aa7dfec 100644 --- a/packages/ckeditor5-link/src/unlinkcommand.js +++ b/packages/ckeditor5-link/src/unlinkcommand.js @@ -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 first from '@ckeditor/ckeditor5-utils/src/first'; /** @@ -57,7 +57,7 @@ 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(), selection.getAttribute( 'linkHref' ), model ) ] : selection.getRanges(); // Remove `linkHref` attribute from specified ranges. for ( const range of rangesToUnlink ) { diff --git a/packages/ckeditor5-link/src/findlinkrange.js b/packages/ckeditor5-typing/src/utils/findattributerange.js similarity index 94% rename from packages/ckeditor5-link/src/findlinkrange.js rename to packages/ckeditor5-typing/src/utils/findattributerange.js index dd1cc442ece..3ce2a52f0d4 100644 --- a/packages/ckeditor5-link/src/findlinkrange.js +++ b/packages/ckeditor5-typing/src/utils/findattributerange.js @@ -4,7 +4,7 @@ */ /** - * @module link/findlinkrange + * @module typing/utils/findattributerange */ /** @@ -17,7 +17,7 @@ * @param {String} value The `linkHref` attribute value. * @returns {module:engine/model/range~Range} The link range. */ -export default function findLinkRange( position, value, model ) { +export default function findAttributeRange( position, value, model ) { return model.createRange( _findBound( position, value, true, model ), _findBound( position, value, false, model ) ); } diff --git a/packages/ckeditor5-typing/src/utils/inlinehighlight.js b/packages/ckeditor5-typing/src/utils/inlinehighlight.js new file mode 100644 index 00000000000..78eba5f1c9d --- /dev/null +++ b/packages/ckeditor5-typing/src/utils/inlinehighlight.js @@ -0,0 +1,70 @@ +/** + * @license Copyright (c) 2003-2020, CKSource - Frederico Knabben. All rights reserved. + * For licensing, see LICENSE.md or https://ckeditor.com/legal/ckeditor-oss-license + */ + +import findAttributeRange from './findattributerange'; + +/** + * @module typing/utils/inlinehighlight + */ + +/** + * 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 given 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. + * + * @param {String} className The class name to apply in the view. + */ +export default function setupLinkHighlight( editor, className ) { + 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 = findAttributeRange( 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( 'a' ) && !item.hasClass( className ) ) { + writer.addClass( className, 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( className, item ); + highlightedLinks.delete( item ); + } + } ); + } + } ); +} diff --git a/packages/ckeditor5-link/tests/findlinkrange.js b/packages/ckeditor5-typing/tests/utils/findattributerange.js similarity index 86% rename from packages/ckeditor5-link/tests/findlinkrange.js rename to packages/ckeditor5-typing/tests/utils/findattributerange.js index ada64d33812..31a4cd61986 100644 --- a/packages/ckeditor5-link/tests/findlinkrange.js +++ b/packages/ckeditor5-typing/tests/utils/findattributerange.js @@ -3,12 +3,12 @@ * For licensing, see LICENSE.md or https://ckeditor.com/legal/ckeditor-oss-license */ -import findLinkRange from '../src/findlinkrange'; +import findAttributeRange from '../../src/utils/findattributerange'; import Model from '@ckeditor/ckeditor5-engine/src/model/model'; import Range from '@ckeditor/ckeditor5-engine/src/model/range'; import { setData } from '@ckeditor/ckeditor5-engine/src/dev-utils/model'; -describe( 'findLinkRange', () => { +describe( 'findAttributeRange', () => { let model, document, root; beforeEach( () => { @@ -23,7 +23,7 @@ describe( 'findLinkRange', () => { setData( model, '<$text linkHref="url">foobar' ); const startPosition = model.createPositionAt( root, [ 3 ] ); - const result = findLinkRange( startPosition, 'url', model ); + const result = findAttributeRange( startPosition, 'url', model ); expect( result ).to.instanceOf( Range ); expect( result.isEqual( model.createRange( model.createPositionAt( root, 0 ), model.createPositionAt( root, 6 ) ) ) ).to.true; @@ -33,7 +33,7 @@ describe( 'findLinkRange', () => { setData( model, 'abc <$text linkHref="url">foobar abc' ); const startPosition = model.createPositionAt( root, [ 7 ] ); - const result = findLinkRange( startPosition, 'url', model ); + const result = findAttributeRange( startPosition, 'url', model ); expect( result ).to.instanceOf( Range ); expect( result.isEqual( model.createRange( model.createPositionAt( root, 4 ), model.createPositionAt( root, 10 ) ) ) ).to.true; @@ -43,7 +43,7 @@ describe( 'findLinkRange', () => { setData( model, '<$text linkHref="url">foobar' ); const startPosition = model.createPositionAt( root, [ 0 ] ); - const result = findLinkRange( startPosition, 'url', model ); + const result = findAttributeRange( startPosition, 'url', model ); expect( result ).to.instanceOf( Range ); expect( result.isEqual( model.createRange( model.createPositionAt( root, 0 ), model.createPositionAt( root, 6 ) ) ) ).to.true; @@ -53,7 +53,7 @@ describe( 'findLinkRange', () => { setData( model, 'abc <$text linkHref="url">foobar abc' ); const startPosition = model.createPositionAt( root, [ 4 ] ); - const result = findLinkRange( startPosition, 'url', model ); + const result = findAttributeRange( startPosition, 'url', model ); expect( result ).to.instanceOf( Range ); expect( result.isEqual( model.createRange( model.createPositionAt( root, 4 ), model.createPositionAt( root, 10 ) ) ) ).to.true; @@ -63,7 +63,7 @@ describe( 'findLinkRange', () => { setData( model, '<$text linkHref="url">foobar' ); const startPosition = model.createPositionAt( root, [ 6 ] ); - const result = findLinkRange( startPosition, 'url', model ); + const result = findAttributeRange( startPosition, 'url', model ); expect( result ).to.instanceOf( Range ); expect( result.isEqual( model.createRange( model.createPositionAt( root, 0 ), model.createPositionAt( root, 6 ) ) ) ).to.true; @@ -73,7 +73,7 @@ describe( 'findLinkRange', () => { setData( model, 'abc <$text linkHref="url">foobar abc' ); const startPosition = model.createPositionAt( root, [ 10 ] ); - const result = findLinkRange( startPosition, 'url', model ); + const result = findAttributeRange( startPosition, 'url', model ); expect( result ).to.instanceOf( Range ); expect( result.isEqual( model.createRange( model.createPositionAt( root, 4 ), model.createPositionAt( root, 10 ) ) ) ).to.true; @@ -83,7 +83,7 @@ describe( 'findLinkRange', () => { setData( model, '<$text linkHref="other">abc<$text linkHref="url">foobar<$text linkHref="other">abc' ); const startPosition = model.createPositionAt( root, [ 6 ] ); - const result = findLinkRange( startPosition, 'url', model ); + const result = findAttributeRange( startPosition, 'url', model ); expect( result ).to.instanceOf( Range ); expect( result.isEqual( model.createRange( model.createPositionAt( root, 3 ), model.createPositionAt( root, 9 ) ) ) ).to.true; @@ -93,7 +93,7 @@ describe( 'findLinkRange', () => { setData( model, '<$text linkHref="other">abc<$text linkHref="url">foobar<$text linkHref="other">abc' ); const startPosition = model.createPositionAt( root, [ 3 ] ); - const result = findLinkRange( startPosition, 'url', model ); + const result = findAttributeRange( startPosition, 'url', model ); expect( result ).to.instanceOf( Range ); expect( result.isEqual( model.createRange( model.createPositionAt( root, 3 ), model.createPositionAt( root, 9 ) ) ) ).to.true; @@ -103,7 +103,7 @@ describe( 'findLinkRange', () => { setData( model, '<$text linkHref="other">abc<$text linkHref="url">foobar<$text linkHref="other">abc' ); const startPosition = model.createPositionAt( root, [ 9 ] ); - const result = findLinkRange( startPosition, 'url', model ); + const result = findAttributeRange( startPosition, 'url', model ); expect( result ).to.instanceOf( Range ); expect( result.isEqual( model.createRange( model.createPositionAt( root, 3 ), model.createPositionAt( root, 9 ) ) ) ).to.true; @@ -118,7 +118,7 @@ describe( 'findLinkRange', () => { ); const startPosition = model.createPositionAt( root.getNodeByPath( [ 1 ] ), 3 ); - const result = findLinkRange( startPosition, 'url', model ); + const result = findAttributeRange( startPosition, 'url', model ); expect( result ).to.instanceOf( Range ); const expectedRange = model.createRange( From 49276bc799dbe8cb167380efa82480554cebaa55 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomek=20Wytr=C4=99bowicz?= Date: Mon, 29 Jun 2020 16:24:39 +0200 Subject: [PATCH 03/18] Make findAttributeRange code less Link-specific. --- packages/ckeditor5-link/src/linkcommand.js | 2 +- packages/ckeditor5-link/src/linkediting.js | 2 +- packages/ckeditor5-link/src/unlinkcommand.js | 8 ++++++- .../src/utils/findattributerange.js | 22 ++++++++++++------- .../src/utils/inlinehighlight.js | 7 +++++- .../tests/utils/findattributerange.js | 20 ++++++++--------- 6 files changed, 39 insertions(+), 22 deletions(-) diff --git a/packages/ckeditor5-link/src/linkcommand.js b/packages/ckeditor5-link/src/linkcommand.js index a7dfae84503..3482bd45e71 100644 --- a/packages/ckeditor5-link/src/linkcommand.js +++ b/packages/ckeditor5-link/src/linkcommand.js @@ -160,7 +160,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 = findAttributeRange( position, selection.getAttribute( 'linkHref' ), model ); + const linkRange = findAttributeRange( position, 'linkHref', selection.getAttribute( 'linkHref' ), model ); writer.setAttribute( 'linkHref', href, linkRange ); diff --git a/packages/ckeditor5-link/src/linkediting.js b/packages/ckeditor5-link/src/linkediting.js index 9eeff3db1c4..94fb3929843 100644 --- a/packages/ckeditor5-link/src/linkediting.js +++ b/packages/ckeditor5-link/src/linkediting.js @@ -339,7 +339,7 @@ export default class LinkEditing extends Plugin { } const position = selection.getFirstPosition(); - const linkRange = findAttributeRange( 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. diff --git a/packages/ckeditor5-link/src/unlinkcommand.js b/packages/ckeditor5-link/src/unlinkcommand.js index 3389aa7dfec..db3ede817a9 100644 --- a/packages/ckeditor5-link/src/unlinkcommand.js +++ b/packages/ckeditor5-link/src/unlinkcommand.js @@ -57,7 +57,13 @@ export default class UnlinkCommand extends Command { model.change( writer => { // Get ranges to unlink. const rangesToUnlink = selection.isCollapsed ? - [ findAttributeRange( 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 ) { diff --git a/packages/ckeditor5-typing/src/utils/findattributerange.js b/packages/ckeditor5-typing/src/utils/findattributerange.js index 3ce2a52f0d4..b315ff30245 100644 --- a/packages/ckeditor5-typing/src/utils/findattributerange.js +++ b/packages/ckeditor5-typing/src/utils/findattributerange.js @@ -8,34 +8,40 @@ */ /** - * Returns a range containing the entire link in which the given `position` is placed. + * Returns a range containing the entire {@link module:engine/view/attributeelement~AttributeElement#childCount attribute element} + * in which the given `position` is placed. * * 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. * @returns {module:engine/model/range~Range} The link range. */ -export default function findAttributeRange( 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; } diff --git a/packages/ckeditor5-typing/src/utils/inlinehighlight.js b/packages/ckeditor5-typing/src/utils/inlinehighlight.js index 78eba5f1c9d..6f1db8462da 100644 --- a/packages/ckeditor5-typing/src/utils/inlinehighlight.js +++ b/packages/ckeditor5-typing/src/utils/inlinehighlight.js @@ -33,7 +33,12 @@ export default function setupLinkHighlight( editor, className ) { let changed = false; if ( selection.hasAttribute( 'linkHref' ) ) { - const modelRange = findAttributeRange( selection.getFirstPosition(), selection.getAttribute( 'linkHref' ), editor.model ); + const modelRange = findAttributeRange( + selection.getFirstPosition(), + 'linkHref', + 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 diff --git a/packages/ckeditor5-typing/tests/utils/findattributerange.js b/packages/ckeditor5-typing/tests/utils/findattributerange.js index 31a4cd61986..0bea97b36a4 100644 --- a/packages/ckeditor5-typing/tests/utils/findattributerange.js +++ b/packages/ckeditor5-typing/tests/utils/findattributerange.js @@ -23,7 +23,7 @@ describe( 'findAttributeRange', () => { setData( model, '<$text linkHref="url">foobar' ); const startPosition = model.createPositionAt( root, [ 3 ] ); - const result = findAttributeRange( startPosition, 'url', model ); + const result = findAttributeRange( startPosition, 'linkHref', 'url', model ); expect( result ).to.instanceOf( Range ); expect( result.isEqual( model.createRange( model.createPositionAt( root, 0 ), model.createPositionAt( root, 6 ) ) ) ).to.true; @@ -33,7 +33,7 @@ describe( 'findAttributeRange', () => { setData( model, 'abc <$text linkHref="url">foobar abc' ); const startPosition = model.createPositionAt( root, [ 7 ] ); - const result = findAttributeRange( startPosition, 'url', model ); + const result = findAttributeRange( startPosition, 'linkHref', 'url', model ); expect( result ).to.instanceOf( Range ); expect( result.isEqual( model.createRange( model.createPositionAt( root, 4 ), model.createPositionAt( root, 10 ) ) ) ).to.true; @@ -43,7 +43,7 @@ describe( 'findAttributeRange', () => { setData( model, '<$text linkHref="url">foobar' ); const startPosition = model.createPositionAt( root, [ 0 ] ); - const result = findAttributeRange( startPosition, 'url', model ); + const result = findAttributeRange( startPosition, 'linkHref', 'url', model ); expect( result ).to.instanceOf( Range ); expect( result.isEqual( model.createRange( model.createPositionAt( root, 0 ), model.createPositionAt( root, 6 ) ) ) ).to.true; @@ -53,7 +53,7 @@ describe( 'findAttributeRange', () => { setData( model, 'abc <$text linkHref="url">foobar abc' ); const startPosition = model.createPositionAt( root, [ 4 ] ); - const result = findAttributeRange( startPosition, 'url', model ); + const result = findAttributeRange( startPosition, 'linkHref', 'url', model ); expect( result ).to.instanceOf( Range ); expect( result.isEqual( model.createRange( model.createPositionAt( root, 4 ), model.createPositionAt( root, 10 ) ) ) ).to.true; @@ -63,7 +63,7 @@ describe( 'findAttributeRange', () => { setData( model, '<$text linkHref="url">foobar' ); const startPosition = model.createPositionAt( root, [ 6 ] ); - const result = findAttributeRange( startPosition, 'url', model ); + const result = findAttributeRange( startPosition, 'linkHref', 'url', model ); expect( result ).to.instanceOf( Range ); expect( result.isEqual( model.createRange( model.createPositionAt( root, 0 ), model.createPositionAt( root, 6 ) ) ) ).to.true; @@ -73,7 +73,7 @@ describe( 'findAttributeRange', () => { setData( model, 'abc <$text linkHref="url">foobar abc' ); const startPosition = model.createPositionAt( root, [ 10 ] ); - const result = findAttributeRange( startPosition, 'url', model ); + const result = findAttributeRange( startPosition, 'linkHref', 'url', model ); expect( result ).to.instanceOf( Range ); expect( result.isEqual( model.createRange( model.createPositionAt( root, 4 ), model.createPositionAt( root, 10 ) ) ) ).to.true; @@ -83,7 +83,7 @@ describe( 'findAttributeRange', () => { setData( model, '<$text linkHref="other">abc<$text linkHref="url">foobar<$text linkHref="other">abc' ); const startPosition = model.createPositionAt( root, [ 6 ] ); - const result = findAttributeRange( startPosition, 'url', model ); + const result = findAttributeRange( startPosition, 'linkHref', 'url', model ); expect( result ).to.instanceOf( Range ); expect( result.isEqual( model.createRange( model.createPositionAt( root, 3 ), model.createPositionAt( root, 9 ) ) ) ).to.true; @@ -93,7 +93,7 @@ describe( 'findAttributeRange', () => { setData( model, '<$text linkHref="other">abc<$text linkHref="url">foobar<$text linkHref="other">abc' ); const startPosition = model.createPositionAt( root, [ 3 ] ); - const result = findAttributeRange( startPosition, 'url', model ); + const result = findAttributeRange( startPosition, 'linkHref', 'url', model ); expect( result ).to.instanceOf( Range ); expect( result.isEqual( model.createRange( model.createPositionAt( root, 3 ), model.createPositionAt( root, 9 ) ) ) ).to.true; @@ -103,7 +103,7 @@ describe( 'findAttributeRange', () => { setData( model, '<$text linkHref="other">abc<$text linkHref="url">foobar<$text linkHref="other">abc' ); const startPosition = model.createPositionAt( root, [ 9 ] ); - const result = findAttributeRange( startPosition, 'url', model ); + const result = findAttributeRange( startPosition, 'linkHref', 'url', model ); expect( result ).to.instanceOf( Range ); expect( result.isEqual( model.createRange( model.createPositionAt( root, 3 ), model.createPositionAt( root, 9 ) ) ) ).to.true; @@ -118,7 +118,7 @@ describe( 'findAttributeRange', () => { ); const startPosition = model.createPositionAt( root.getNodeByPath( [ 1 ] ), 3 ); - const result = findAttributeRange( startPosition, 'url', model ); + const result = findAttributeRange( startPosition, 'linkHref', 'url', model ); expect( result ).to.instanceOf( Range ); const expectedRange = model.createRange( From 802289efd5c291e6a4f0b9365cc8590cff2f683c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomek=20Wytr=C4=99bowicz?= Date: Mon, 29 Jun 2020 16:33:34 +0200 Subject: [PATCH 04/18] Make highlight code less Link-specific. --- packages/ckeditor5-link/src/linkediting.js | 2 +- .../src/utils/inlinehighlight.js | 29 ++++++++++--------- 2 files changed, 17 insertions(+), 14 deletions(-) diff --git a/packages/ckeditor5-link/src/linkediting.js b/packages/ckeditor5-link/src/linkediting.js index 94fb3929843..b507457722b 100644 --- a/packages/ckeditor5-link/src/linkediting.js +++ b/packages/ckeditor5-link/src/linkediting.js @@ -104,7 +104,7 @@ export default class LinkEditing extends Plugin { twoStepCaretMovementPlugin.registerAttribute( 'linkHref' ); // Setup highlight over selected link. - setupLinkHighlight( editor, HIGHLIGHT_CLASS ); + setupLinkHighlight( editor, 'linkHref', 'a', HIGHLIGHT_CLASS ); // Change the attributes of the selection in certain situations after the link was inserted into the document. this._enableInsertContentSelectionAttributesFixer(); diff --git a/packages/ckeditor5-typing/src/utils/inlinehighlight.js b/packages/ckeditor5-typing/src/utils/inlinehighlight.js index 6f1db8462da..b92eb3b8460 100644 --- a/packages/ckeditor5-typing/src/utils/inlinehighlight.js +++ b/packages/ckeditor5-typing/src/utils/inlinehighlight.js @@ -10,10 +10,10 @@ import findAttributeRange from './findattributerange'; */ /** - * 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. + * Adds a visual highlight style to an attribute element in which the selection is anchored. + * Together with two-step caret movement, they indicate that the user is typing inside the element. * - * Highlight is turned on by adding the given class to the link in the view: + * Highlight is turned on by adding the given class to the attribute element 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. @@ -21,32 +21,35 @@ import findAttributeRange from './findattributerange'; * * This way, adding and removing the highlight does not interfere with conversion. * + * @param {module:core/editor/editor~Editor} editor The editor instance. + * @param {String} attributeName The attribute name to check. + * @param {String} tagName The tagName of a view item. * @param {String} className The class name to apply in the view. */ -export default function setupLinkHighlight( editor, className ) { +export default function setupLinkHighlight( editor, attributeName, tagName, className ) { const view = editor.editing.view; - const highlightedLinks = new Set(); + const highlightedElements = new Set(); // Adding the class. view.document.registerPostFixer( writer => { const selection = editor.model.document.selection; let changed = false; - if ( selection.hasAttribute( 'linkHref' ) ) { + if ( selection.hasAttribute( attributeName ) ) { const modelRange = findAttributeRange( selection.getFirstPosition(), - 'linkHref', - selection.getAttribute( 'linkHref' ), + attributeName, + selection.getAttribute( attributeName ), 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 + // There might be multiple view elements in the `viewRange`, for example, when the `a` element is // broken by a UIElement. for ( const item of viewRange.getItems() ) { - if ( item.is( 'a' ) && !item.hasClass( className ) ) { + if ( item.is( tagName ) && !item.hasClass( className ) ) { writer.addClass( className, item ); - highlightedLinks.add( item ); + highlightedElements.add( item ); changed = true; } } @@ -65,9 +68,9 @@ export default function setupLinkHighlight( editor, className ) { function removeHighlight() { view.change( writer => { - for ( const item of highlightedLinks.values() ) { + for ( const item of highlightedElements.values() ) { writer.removeClass( className, item ); - highlightedLinks.delete( item ); + highlightedElements.delete( item ); } } ); } From 64a32ccafef496da1c5df35efa9f8a0cc2a5976a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomek=20Wytr=C4=99bowicz?= Date: Wed, 1 Jul 2020 15:01:54 +0200 Subject: [PATCH 05/18] Improve the usage of attribute assertion. --- packages/ckeditor5-link/tests/linkediting.js | 99 ++++++++++++++------ 1 file changed, 69 insertions(+), 30 deletions(-) diff --git a/packages/ckeditor5-link/tests/linkediting.js b/packages/ckeditor5-link/tests/linkediting.js index 25e20d1abe5..f76dca274e2 100644 --- a/packages/ckeditor5-link/tests/linkediting.js +++ b/packages/ckeditor5-link/tests/linkediting.js @@ -20,21 +20,59 @@ import BoldEditing from '@ckeditor/ckeditor5-basic-styles/src/bold/boldediting'; /* globals document, chai */ -chai.Assertion.addMethod( 'attribute', function attributeAssertion( type ) { +/** + * Asserts that the target has an attribute with the given key name. + * See {@link module:engine/model/documentselection~DocumentSelection#hasAttribute hasAttribute}. + * + * expect( selection ).to.have.attribute( 'linkHref' ); + * + * When `value` is provided, .attribute also asserts that the attribute's value is equal to the given `value`. + * See {@link module:engine/model/documentselection~DocumentSelection#getAttribute getAttribute}. + * + * expect( selection ).to.have.attribute( 'linkHref', 'example.com' ); + * + * Negations works as well. + * + * @param {String} key Key of attribute to assert. + * @param {String} [value] Attribute value to assert. + * @param {String} [message] Additional message. + */ +chai.Assertion.addMethod( 'attribute', function attributeAssertion( key, value, message ) { + if ( message ) { + chai.util.flag( this, 'message', message ); + } + const obj = this._obj; - // Check if it has the method at all. - new chai.Assertion( obj ).to.respondTo( 'hasAttribute' ); - - // Check if it has the attribute. - const hasAttribute = obj.hasAttribute( type ); - this.assert( - hasAttribute === true, - `expected #{this} to have '${ type }' attribute`, - `expected #{this} to not have the '${ type }' attribute`, - !chai.util.flag( this, 'negate' ), - hasAttribute - ); + if ( arguments.length === 1 ) { + // Check if it has the method at all. + new chai.Assertion( obj ).to.respondTo( 'hasAttribute' ); + + // Check if it has the attribute. + const hasAttribute = obj.hasAttribute( key ); + this.assert( + hasAttribute === true, + `expected #{this} to have attribute '${ key }'`, + `expected #{this} to not have attribute '${ key }'`, + !chai.util.flag( this, 'negate' ), + hasAttribute + ); + } + + // If a value was given. + if ( arguments.length >= 2 ) { + // Check if it has the method at all. + new chai.Assertion( obj ).to.respondTo( 'getAttribute', message ); + + const attributeValue = obj.getAttribute( key ); + this.assert( + attributeValue === value, + `expected #{this} to have attribute '${ key }' of #{exp}, but got #{act}`, + `expected #{this} to not have attribute '${ key }' of #{exp}`, + attributeValue, + value + ); + } } ); describe( 'LinkEditing', () => { @@ -102,7 +140,7 @@ describe( 'LinkEditing', () => { setModelData( editor.model, 'foo[]<$text linkHref="url">bar' ); // 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', { @@ -113,8 +151,8 @@ describe( 'LinkEditing', () => { expect( getModelData( model ) ).to.equal( 'foo<$text linkHref="url">[]bar' ); // 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 () => { @@ -133,7 +171,7 @@ describe( 'LinkEditing', () => { setModelData( editor.model, 'foo[]<$text linkHref="url">bar' ); // 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', { @@ -144,8 +182,8 @@ describe( 'LinkEditing', () => { expect( getModelData( model ) ).to.equal( 'foo<$text linkHref="url">[]bar' ); // 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(); } ); @@ -195,7 +233,7 @@ describe( 'LinkEditing', () => { '' ); - 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', () => { @@ -206,7 +244,7 @@ describe( 'LinkEditing', () => { } ); expect( getModelData( model ) ).to.equal( '<$text linkHref="ckeditor.com">foINSERTED[]o' ); - 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', () => { @@ -233,7 +271,8 @@ describe( 'LinkEditing', () => { ); expect( model.document.selection.isGravityOverridden ).to.be.true; - 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 a link into another link (different URLs, no merge)', () => { @@ -251,7 +290,7 @@ describe( 'LinkEditing', () => { '' ); - 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)', () => { @@ -270,8 +309,8 @@ describe( 'LinkEditing', () => { '' ); - 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' ); } ); } ); @@ -388,7 +427,7 @@ describe( 'LinkEditing', () => { 'foo <$text linkHref="url">b{}ar baz' ); - expect( model.document.selection ).to.have.an.attribute( 'linkHref' ); + expect( model.document.selection ).to.have.attribute( 'linkHref' ); expect( getViewData( view ) ).to.equal( '

foo b{}ar baz

' ); @@ -399,13 +438,13 @@ describe( 'LinkEditing', () => { 'foo {}<$text linkHref="url">bar baz' ); - 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 ).to.have.an.attribute( 'linkHref' ); + expect( model.document.selection ).to.have.attribute( 'linkHref' ); expect( getViewData( view ) ).to.equal( '

foo {}bar baz

' ); @@ -416,7 +455,7 @@ describe( 'LinkEditing', () => { 'foo <$text linkHref="url">bar{} baz' ); - expect( model.document.selection ).to.have.an.attribute( 'linkHref' ); + expect( model.document.selection ).to.have.attribute( 'linkHref' ); expect( getViewData( view ) ).to.equal( '

foo bar{} baz

' ); @@ -434,7 +473,7 @@ describe( 'LinkEditing', () => { '<$text linkHref="url">[]nk baz' ); - expect( model.document.selection ).to.have.an.attribute( 'linkHref' ); + expect( model.document.selection ).to.have.attribute( 'linkHref' ); } ); it( 'should remove classes when selection is moved out from the link', () => { From 50dc07f5d42b33acaab84e84b04edf3666de1f9d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomek=20Wytr=C4=99bowicz?= Date: Wed, 1 Jul 2020 15:25:28 +0200 Subject: [PATCH 06/18] Use to.have.property assertion, to make test results more descriptive. --- packages/ckeditor5-link/tests/linkediting.js | 7 +++--- .../tests/twostepcaretmovement.js | 25 ++++++++++--------- 2 files changed, 16 insertions(+), 16 deletions(-) diff --git a/packages/ckeditor5-link/tests/linkediting.js b/packages/ckeditor5-link/tests/linkediting.js index f76dca274e2..02af9b14dbc 100644 --- a/packages/ckeditor5-link/tests/linkediting.js +++ b/packages/ckeditor5-link/tests/linkediting.js @@ -256,7 +256,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 } ) ); @@ -270,8 +270,7 @@ describe( 'LinkEditing', () => { '' ); - expect( model.document.selection.isGravityOverridden ).to.be.true; - + expect( model.document.selection ).to.have.property( 'isGravityOverridden', true ); expect( model.document.selection ).to.have.attribute( 'linkHref' ); } ); @@ -296,7 +295,7 @@ describe( 'LinkEditing', () => { it( 'should not remove link atttributes when pasting before another link (different URLs, no merge)', () => { setModelData( model, '[]<$text linkHref="ckeditor.com">foo' ); - 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' } ) ); diff --git a/packages/ckeditor5-typing/tests/twostepcaretmovement.js b/packages/ckeditor5-typing/tests/twostepcaretmovement.js index fbef655aec4..da4ebfa5a56 100644 --- a/packages/ckeditor5-typing/tests/twostepcaretmovement.js +++ b/packages/ckeditor5-typing/tests/twostepcaretmovement.js @@ -511,7 +511,7 @@ describe( 'TwoStepCaretMovement()', () => { } ); // <$text a="1">x<$text b="1">[] - expect( selection.isGravityOverridden ).to.be.true; + expect( selection ).to.have.property( 'isGravityOverridden', true ); expect( getSelectionAttributesArray( selection ) ).to.have.members( [ 'b' ] ); model.change( writer => { @@ -519,7 +519,7 @@ describe( 'TwoStepCaretMovement()', () => { } ); // <$text a="1">x<$text b="1">yz[] - expect( selection.isGravityOverridden ).to.be.false; + expect( selection ).to.have.property( 'isGravityOverridden', false ); expect( getSelectionAttributesArray( selection ) ).to.have.members( [ 'b' ] ); } ); @@ -539,7 +539,7 @@ describe( 'TwoStepCaretMovement()', () => { } ); // <$text b="1">[]<$text a="1">x - expect( selection.isGravityOverridden ).to.be.false; + expect( selection ).to.have.property( 'isGravityOverridden', false ); expect( getSelectionAttributesArray( selection ) ).to.have.members( [ 'b' ] ); model.change( writer => { @@ -547,7 +547,7 @@ describe( 'TwoStepCaretMovement()', () => { } ); // <$text b="1">yz[]<$text a="1">x - expect( selection.isGravityOverridden ).to.be.false; + expect( selection ).to.have.property( 'isGravityOverridden', false ); expect( getSelectionAttributesArray( selection ) ).to.have.members( [ 'b' ] ); } ); } ); @@ -646,13 +646,13 @@ describe( 'TwoStepCaretMovement()', () => { it( 'should not override gravity when selection is placed at the beginning of text', () => { setData( model, '<$text a="true">[]foo' ); - expect( selection.isGravityOverridden ).to.be.false; + expect( selection ).to.have.property( 'isGravityOverridden', false ); } ); it( 'should not override gravity when selection is placed at the end of text', () => { setData( model, '<$text a="true">foo[]' ); - expect( selection.isGravityOverridden ).to.be.false; + expect( selection ).to.have.property( 'isGravityOverridden', false ); } ); } ); @@ -693,7 +693,7 @@ describe( 'TwoStepCaretMovement()', () => { fireKeyDownEvent( { keyCode: keyCodes.arrowright } ); - expect( selection.isGravityOverridden ).to.be.false; + expect( selection ).to.have.property( 'isGravityOverridden', false ); } ); it( 'should do nothing when shift key is pressed', () => { @@ -704,7 +704,7 @@ describe( 'TwoStepCaretMovement()', () => { shiftKey: true } ); - expect( selection.isGravityOverridden ).to.be.false; + expect( selection ).to.have.property( 'isGravityOverridden', false ); } ); it( 'should do nothing when alt key is pressed', () => { @@ -715,7 +715,7 @@ describe( 'TwoStepCaretMovement()', () => { altKey: true } ); - expect( selection.isGravityOverridden ).to.be.false; + expect( selection ).to.have.property( 'isGravityOverridden', false ); } ); it( 'should do nothing when ctrl key is pressed', () => { @@ -726,7 +726,7 @@ describe( 'TwoStepCaretMovement()', () => { ctrlKey: true } ); - expect( selection.isGravityOverridden ).to.be.false; + expect( selection ).to.have.property( 'isGravityOverridden', false ); } ); it( 'should do nothing when the not a direct selection change but at the attribute boundary', () => { @@ -743,7 +743,7 @@ describe( 'TwoStepCaretMovement()', () => { writer.insertText( 'x', selection.getFirstPosition().getShiftedBy( -2 ) ); } ); - expect( selection.isGravityOverridden ).to.be.true; + expect( selection ).to.have.property( 'isGravityOverridden', true ); expect( getSelectionAttributesArray( selection ) ).to.have.members( [] ); } ); @@ -910,7 +910,8 @@ describe( 'TwoStepCaretMovement()', () => { expect( getSelectionAttributesArray( selection ) ).to.have.members( step.selectionAttributes, `#attributes ${ stepString }` ); - expect( selection.isGravityOverridden ).to.equal( step.isGravityOverridden, `#isGravityOverridden ${ stepString }` ); + expect( selection, `#isGravityOverridden ${ stepString }` ) + .to.have.property( 'isGravityOverridden', step.isGravityOverridden ); expect( preventDefaultSpy.callCount ).to.equal( step.preventDefault, `#preventDefault ${ stepString }` ); expect( evtStopSpy.callCount ).to.equal( step.evtStopCalled, `#evtStopCalled ${ stepString }` ); } From d03f0a8f90a756993233a5cf86983be1e83d9df6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomek=20Wytr=C4=99bowicz?= Date: Wed, 1 Jul 2020 15:30:20 +0200 Subject: [PATCH 07/18] =?UTF-8?q?Check=20the=20rendering=20in=20"should=20?= =?UTF-8?q?render=20=E2=80=A6=20after=20splitting=20link"=20test.?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- packages/ckeditor5-link/tests/linkediting.js | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/packages/ckeditor5-link/tests/linkediting.js b/packages/ckeditor5-link/tests/linkediting.js index 02af9b14dbc..2d7632e2593 100644 --- a/packages/ckeditor5-link/tests/linkediting.js +++ b/packages/ckeditor5-link/tests/linkediting.js @@ -473,6 +473,10 @@ describe( 'LinkEditing', () => { ); expect( model.document.selection ).to.have.attribute( 'linkHref' ); + expect( getViewData( view ) ).to.equal( + '

foo li

' + + '

{}nk baz

' + ); } ); it( 'should remove classes when selection is moved out from the link', () => { From 52a7bc75ffff5eeac45beefdfa01151235dc9342 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomek=20Wytr=C4=99bowicz?= Date: Fri, 3 Jul 2020 11:16:18 +0200 Subject: [PATCH 08/18] Use attribute assertion for core test utils. --- packages/ckeditor5-link/tests/linkediting.js | 57 +------------------- 1 file changed, 2 insertions(+), 55 deletions(-) diff --git a/packages/ckeditor5-link/tests/linkediting.js b/packages/ckeditor5-link/tests/linkediting.js index 2d7632e2593..27248e68586 100644 --- a/packages/ckeditor5-link/tests/linkediting.js +++ b/packages/ckeditor5-link/tests/linkediting.js @@ -18,62 +18,9 @@ import { keyCodes } from '@ckeditor/ckeditor5-utils/src/keyboard'; import Typing from '@ckeditor/ckeditor5-typing/src/typing'; import BoldEditing from '@ckeditor/ckeditor5-basic-styles/src/bold/boldediting'; -/* globals document, chai */ +import '@ckeditor/ckeditor5-core/tests/_utils/assertions/attribute'; -/** - * Asserts that the target has an attribute with the given key name. - * See {@link module:engine/model/documentselection~DocumentSelection#hasAttribute hasAttribute}. - * - * expect( selection ).to.have.attribute( 'linkHref' ); - * - * When `value` is provided, .attribute also asserts that the attribute's value is equal to the given `value`. - * See {@link module:engine/model/documentselection~DocumentSelection#getAttribute getAttribute}. - * - * expect( selection ).to.have.attribute( 'linkHref', 'example.com' ); - * - * Negations works as well. - * - * @param {String} key Key of attribute to assert. - * @param {String} [value] Attribute value to assert. - * @param {String} [message] Additional message. - */ -chai.Assertion.addMethod( 'attribute', function attributeAssertion( key, value, message ) { - if ( message ) { - chai.util.flag( this, 'message', message ); - } - - const obj = this._obj; - - if ( arguments.length === 1 ) { - // Check if it has the method at all. - new chai.Assertion( obj ).to.respondTo( 'hasAttribute' ); - - // Check if it has the attribute. - const hasAttribute = obj.hasAttribute( key ); - this.assert( - hasAttribute === true, - `expected #{this} to have attribute '${ key }'`, - `expected #{this} to not have attribute '${ key }'`, - !chai.util.flag( this, 'negate' ), - hasAttribute - ); - } - - // If a value was given. - if ( arguments.length >= 2 ) { - // Check if it has the method at all. - new chai.Assertion( obj ).to.respondTo( 'getAttribute', message ); - - const attributeValue = obj.getAttribute( key ); - this.assert( - attributeValue === value, - `expected #{this} to have attribute '${ key }' of #{exp}, but got #{act}`, - `expected #{this} to not have attribute '${ key }' of #{exp}`, - attributeValue, - value - ); - } -} ); +/* global document */ describe( 'LinkEditing', () => { let element, editor, model, view; From 63c6485a13ee34895bf3ecbc961666c8901a3807 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomek=20Wytr=C4=99bowicz?= Date: Sun, 5 Jul 2020 15:40:54 +0200 Subject: [PATCH 09/18] Add tests for setupHighlight function, copy them from linkediting, and adapt to no-plugin case. --- .../tests/utils/inlinehighlight.js | 232 ++++++++++++++++++ 1 file changed, 232 insertions(+) create mode 100644 packages/ckeditor5-typing/tests/utils/inlinehighlight.js diff --git a/packages/ckeditor5-typing/tests/utils/inlinehighlight.js b/packages/ckeditor5-typing/tests/utils/inlinehighlight.js new file mode 100644 index 00000000000..f76696b69d8 --- /dev/null +++ b/packages/ckeditor5-typing/tests/utils/inlinehighlight.js @@ -0,0 +1,232 @@ +/** + * @license Copyright (c) 2003-2020, CKSource - Frederico Knabben. All rights reserved. + * For licensing, see LICENSE.md or https://ckeditor.com/legal/ckeditor-oss-license + */ +import setupHighlight from '../../src/utils/inlinehighlight'; + +import ClassicTestEditor from '@ckeditor/ckeditor5-core/tests/_utils/classictesteditor'; +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 '@ckeditor/ckeditor5-core/tests/_utils/assertions/attribute'; + +/* global document */ + +describe( 'LinkEditing', () => { + let element, editor, model, view; + + beforeEach( async () => { + element = document.createElement( 'div' ); + document.body.appendChild( element ); + + editor = await ClassicTestEditor.create( element ); + + model = editor.model; + view = editor.editing.view; + + model.schema.register( 'paragraph', { inheritAllFrom: '$block' } ); + editor.conversion.elementToElement( { model: 'paragraph', view: 'p' } ); + + model.schema.extend( '$text', { allowAttributes: 'linkHref' } ); + + editor.conversion.for( 'editingDowncast' ) + .attributeToElement( { model: 'linkHref', view: ( href, writer ) => { + return writer.createAttributeElement( 'a', { href } ); + } } ); + + // Setup highlight over selected link. + setupHighlight( editor, 'linkHref', 'a', 'ck-link_selected' ); + } ); + + afterEach( async () => { + element.remove(); + + await editor.destroy(); + } ); + + describe( 'link highlighting', () => { + it( 'should convert the highlight to a proper view classes', () => { + setModelData( model, + 'foo <$text linkHref="url">b{}ar baz' + ); + + expect( model.document.selection ).to.have.attribute( 'linkHref' ); + expect( getViewData( view ) ).to.equal( + '

foo b{}ar baz

' + ); + } ); + + it( 'should work whenever selection has linkHref attribute - link start', () => { + setModelData( model, + 'foo {}<$text linkHref="url">bar baz' + ); + + expect( model.document.selection ).to.not.have.attribute( 'linkHref' ); + + model.change( writer => { + writer.setSelectionAttribute( 'linkHref', 'url' ); + } ); + + expect( model.document.selection ).to.have.attribute( 'linkHref' ); + expect( getViewData( view ) ).to.equal( + '

foo {}bar baz

' + ); + } ); + + it( 'should work whenever selection has linkHref attribute - link end', () => { + setModelData( model, + 'foo <$text linkHref="url">bar{} baz' + ); + + expect( model.document.selection ).to.have.attribute( 'linkHref' ); + expect( getViewData( view ) ).to.equal( + '

foo bar{} baz

' + ); + } ); + + it( 'should render highlight correctly after splitting the link', () => { + setModelData( model, + 'foo <$text linkHref="url">li{}nk baz' + ); + + editor.execute( 'enter' ); + + expect( getModelData( model ) ).to.equal( + 'foo <$text linkHref="url">li' + + '<$text linkHref="url">[]nk baz' + ); + + expect( model.document.selection ).to.have.attribute( 'linkHref' ); + expect( getViewData( view ) ).to.equal( + '

foo li

' + + '

{}nk baz

' + ); + } ); + + it( 'should remove classes when selection is moved out from the link', () => { + setModelData( model, + 'foo <$text linkHref="url">li{}nk baz' + ); + + expect( getViewData( view ) ).to.equal( + '

foo li{}nk baz

' + ); + + model.change( writer => writer.setSelection( model.document.getRoot().getChild( 0 ), 0 ) ); + + expect( getViewData( view ) ).to.equal( + '

{}foo link baz

' + ); + } ); + + it( 'should work correctly when selection is moved inside link', () => { + setModelData( model, + 'foo <$text linkHref="url">li{}nk baz' + ); + + expect( getViewData( view ) ).to.equal( + '

foo li{}nk baz

' + ); + + model.change( writer => writer.setSelection( model.document.getRoot().getChild( 0 ), 5 ) ); + + expect( getViewData( view ) ).to.equal( + '

foo l{}ink baz

' + ); + } ); + + describe( 'downcast conversion integration', () => { + it( 'works for the #insert event', () => { + setModelData( model, + 'foo <$text linkHref="url">li{}nk baz' + ); + + model.change( writer => { + writer.insertText( 'FOO', { linkHref: 'url' }, model.document.selection.getFirstPosition() ); + } ); + + expect( getViewData( view ) ).to.equal( + '

foo liFOO{}nk baz

' + ); + } ); + + it( 'works for the #remove event', () => { + setModelData( model, + 'foo <$text linkHref="url">li{}nk baz' + ); + + model.change( writer => { + 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( + '

i{}nk baz

' + ); + } ); + + it( 'works for the #attribute event', () => { + setModelData( model, + 'foo <$text linkHref="url">li{}nk baz' + ); + + model.change( writer => { + writer.setAttribute( 'linkHref', 'new-url', writer.createRange( + model.document.selection.getFirstPosition().getShiftedBy( -1 ), + model.document.selection.getFirstPosition().getShiftedBy( 1 ) ) + ); + } ); + + expect( getViewData( view ) ).to.equal( + '

foo li{}nk baz

' + ); + } ); + + it( 'works for the #selection event', () => { + setModelData( model, + 'foo <$text linkHref="url">li{}nk baz' + ); + + model.change( writer => { + writer.setSelection( writer.createRange( + model.document.selection.getFirstPosition().getShiftedBy( -1 ), + model.document.selection.getFirstPosition().getShiftedBy( 1 ) ) + ); + } ); + + expect( getViewData( view ) ).to.equal( + '

foo l{in}k baz

' + ); + } ); + + it( 'works for the addMarker and removeMarker events', () => { + editor.conversion.for( 'editingDowncast' ).markerToHighlight( { model: 'fooMarker', view: {} } ); + + setModelData( model, + 'foo <$text linkHref="url">li{}nk baz' + ); + + model.change( writer => { + 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 } ); + } ); + + expect( getViewData( view ) ).to.equal( + '

foo li{}nk baz

' + ); + + model.change( writer => writer.removeMarker( 'fooMarker' ) ); + + expect( getViewData( view ) ).to.equal( + '

foo li{}nk baz

' + ); + } ); + } ); + } ); +} ); From 780ea630e0a9f1d572d4b3a85dd7492b10b5e0a3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomek=20Wytr=C4=99bowicz?= Date: Sun, 5 Jul 2020 15:48:45 +0200 Subject: [PATCH 10/18] Replace enter command with lower-level split. --- packages/ckeditor5-typing/tests/utils/inlinehighlight.js | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/packages/ckeditor5-typing/tests/utils/inlinehighlight.js b/packages/ckeditor5-typing/tests/utils/inlinehighlight.js index f76696b69d8..cbbfd06fdeb 100644 --- a/packages/ckeditor5-typing/tests/utils/inlinehighlight.js +++ b/packages/ckeditor5-typing/tests/utils/inlinehighlight.js @@ -89,7 +89,12 @@ describe( 'LinkEditing', () => { 'foo <$text linkHref="url">li{}nk baz' ); - editor.execute( 'enter' ); + model.change( writer => { + const splitPos = model.document.selection.getFirstRange().start; + + writer.split( splitPos ); + writer.setSelection( splitPos.parent.nextSibling, 0 ); + } ); expect( getModelData( model ) ).to.equal( 'foo <$text linkHref="url">li' + From eb6dc862bae9ef149380b6ea154a840ee68c915c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomek=20Wytr=C4=99bowicz?= Date: Mon, 20 Jul 2020 11:19:30 +0200 Subject: [PATCH 11/18] Unify inlineHighlight function name, as suggested at https://github.com/ckeditor/ckeditor5/pull/7546#discussion_r454276759. --- packages/ckeditor5-link/src/linkediting.js | 4 ++-- packages/ckeditor5-typing/src/utils/inlinehighlight.js | 10 +++++++++- .../ckeditor5-typing/tests/utils/inlinehighlight.js | 4 ++-- 3 files changed, 13 insertions(+), 5 deletions(-) diff --git a/packages/ckeditor5-link/src/linkediting.js b/packages/ckeditor5-link/src/linkediting.js index b507457722b..8125dc5df49 100644 --- a/packages/ckeditor5-link/src/linkediting.js +++ b/packages/ckeditor5-link/src/linkediting.js @@ -10,7 +10,7 @@ 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 setupLinkHighlight from '@ckeditor/ckeditor5-typing/src/utils/inlinehighlight'; +import inlineHighlight from '@ckeditor/ckeditor5-typing/src/utils/inlinehighlight'; import LinkCommand from './linkcommand'; import UnlinkCommand from './unlinkcommand'; import AutomaticDecorators from './utils/automaticdecorators'; @@ -104,7 +104,7 @@ export default class LinkEditing extends Plugin { twoStepCaretMovementPlugin.registerAttribute( 'linkHref' ); // Setup highlight over selected link. - setupLinkHighlight( editor, 'linkHref', 'a', HIGHLIGHT_CLASS ); + 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(); diff --git a/packages/ckeditor5-typing/src/utils/inlinehighlight.js b/packages/ckeditor5-typing/src/utils/inlinehighlight.js index b92eb3b8460..cc512851d8a 100644 --- a/packages/ckeditor5-typing/src/utils/inlinehighlight.js +++ b/packages/ckeditor5-typing/src/utils/inlinehighlight.js @@ -21,12 +21,20 @@ import findAttributeRange from './findattributerange'; * * This way, adding and removing the highlight does not interfere with conversion. * + * Usage: + * + * import inlineHighlight from '@ckeditor/ckeditor5-typing/src/utils/inlinehighlight'; + * + * // Make `ck-link_selected` class be applied on an `a` element + * // whenever the corresponding `linkHref` attribute element is selected. + * inlineHighlight( editor, 'linkHref', 'a', 'ck-link_selected' ); + * * @param {module:core/editor/editor~Editor} editor The editor instance. * @param {String} attributeName The attribute name to check. * @param {String} tagName The tagName of a view item. * @param {String} className The class name to apply in the view. */ -export default function setupLinkHighlight( editor, attributeName, tagName, className ) { +export default function inlineHighlight( editor, attributeName, tagName, className ) { const view = editor.editing.view; const highlightedElements = new Set(); diff --git a/packages/ckeditor5-typing/tests/utils/inlinehighlight.js b/packages/ckeditor5-typing/tests/utils/inlinehighlight.js index cbbfd06fdeb..cfdcf91878a 100644 --- a/packages/ckeditor5-typing/tests/utils/inlinehighlight.js +++ b/packages/ckeditor5-typing/tests/utils/inlinehighlight.js @@ -2,7 +2,7 @@ * @license Copyright (c) 2003-2020, CKSource - Frederico Knabben. All rights reserved. * For licensing, see LICENSE.md or https://ckeditor.com/legal/ckeditor-oss-license */ -import setupHighlight from '../../src/utils/inlinehighlight'; +import inlineHighlight from '../../src/utils/inlinehighlight'; import ClassicTestEditor from '@ckeditor/ckeditor5-core/tests/_utils/classictesteditor'; import { getData as getModelData, setData as setModelData } from '@ckeditor/ckeditor5-engine/src/dev-utils/model'; @@ -35,7 +35,7 @@ describe( 'LinkEditing', () => { } } ); // Setup highlight over selected link. - setupHighlight( editor, 'linkHref', 'a', 'ck-link_selected' ); + inlineHighlight( editor, 'linkHref', 'a', 'ck-link_selected' ); } ); afterEach( async () => { From ea2c463b00d5dfb84120f653e5a7b09b2a7eac3f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomek=20Wytr=C4=99bowicz?= Date: Mon, 20 Jul 2020 11:29:00 +0200 Subject: [PATCH 12/18] Rephrase findAttributeRange API docs, to address https://github.com/ckeditor/ckeditor5/pull/7546#discussion_r454279790. --- packages/ckeditor5-typing/src/utils/findattributerange.js | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/packages/ckeditor5-typing/src/utils/findattributerange.js b/packages/ckeditor5-typing/src/utils/findattributerange.js index b315ff30245..1e3e6454892 100644 --- a/packages/ckeditor5-typing/src/utils/findattributerange.js +++ b/packages/ckeditor5-typing/src/utils/findattributerange.js @@ -8,8 +8,9 @@ */ /** - * Returns a range containing the entire {@link module:engine/view/attributeelement~AttributeElement#childCount attribute element} - * in which the given `position` is placed. + * Returns a model range that covers all consecutive + * {@link module:engine/view/attributeelement~AttributeElement attribute elements} + * of the given `attributeName` and `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. From 97f0f5a2e337ab291245ca71a93c53cdc8485f75 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomek=20Wytr=C4=99bowicz?= Date: Tue, 21 Jul 2020 12:10:38 +0200 Subject: [PATCH 13/18] Fix findLinkRange usage, missed after merge. Address https://github.com/ckeditor/ckeditor5/pull/7546#issuecomment-660961085. --- packages/ckeditor5-link/src/linkediting.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/ckeditor5-link/src/linkediting.js b/packages/ckeditor5-link/src/linkediting.js index c7f8d2909f8..d21020992fc 100644 --- a/packages/ckeditor5-link/src/linkediting.js +++ b/packages/ckeditor5-link/src/linkediting.js @@ -476,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 ); From 9da45174a2f847080f31f0ef80b6eb984755b3e1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomek=20Wytr=C4=99bowicz?= Date: Tue, 21 Jul 2020 12:12:07 +0200 Subject: [PATCH 14/18] Remove unused findLinkRange import, missed after merge. Address https://github.com/ckeditor/ckeditor5/pull/7546#issuecomment-660961085. --- packages/ckeditor5-link/src/unlinkcommand.js | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/ckeditor5-link/src/unlinkcommand.js b/packages/ckeditor5-link/src/unlinkcommand.js index 9dc05dd2471..e12fff2df8f 100644 --- a/packages/ckeditor5-link/src/unlinkcommand.js +++ b/packages/ckeditor5-link/src/unlinkcommand.js @@ -10,7 +10,6 @@ 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'; /** From 2d964fab89b91d930192286602b58ecf24763bf4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomek=20Wytr=C4=99bowicz?= Date: Tue, 21 Jul 2020 12:27:04 +0200 Subject: [PATCH 15/18] Use sinon-chai and named spies in twostepcaretmovement.js. --- .../tests/twostepcaretmovement.js | 35 +++++++++---------- 1 file changed, 17 insertions(+), 18 deletions(-) diff --git a/packages/ckeditor5-typing/tests/twostepcaretmovement.js b/packages/ckeditor5-typing/tests/twostepcaretmovement.js index da4ebfa5a56..c8f1abff9f2 100644 --- a/packages/ckeditor5-typing/tests/twostepcaretmovement.js +++ b/packages/ckeditor5-typing/tests/twostepcaretmovement.js @@ -31,8 +31,8 @@ describe( 'TwoStepCaretMovement()', () => { view = editor.editing.view; plugin = editor.plugins.get( TwoStepCaretMovement ); - preventDefaultSpy = sinon.spy(); - evtStopSpy = sinon.spy(); + preventDefaultSpy = sinon.spy().named( 'preventDefaultSpy' ); + evtStopSpy = sinon.spy().named( 'evtStopSpy' ); editor.model.schema.extend( '$text', { allowAttributes: [ 'a', 'b', 'c' ], @@ -657,9 +657,9 @@ describe( 'TwoStepCaretMovement()', () => { } ); it( 'should listen with the high+1 priority on view.document#keydown', () => { - const highestPrioritySpy = sinon.spy(); - const highPrioritySpy = sinon.spy(); - const normalPrioritySpy = sinon.spy(); + const highestPrioritySpy = sinon.spy().named( 'highestPrioritySpy' ); + const highPrioritySpy = sinon.spy().named( 'highPrioritySpy' ); + const normalPrioritySpy = sinon.spy().named( 'normalPrioritySpy' ); setData( model, '<$text c="true">foo[]<$text a="true" b="true">bar' ); @@ -672,12 +672,11 @@ describe( 'TwoStepCaretMovement()', () => { preventDefault: preventDefaultSpy } ); - sinon.assert.callOrder( - highestPrioritySpy, - preventDefaultSpy ); + expect( highestPrioritySpy ).to.be.calledOnce; + expect( preventDefaultSpy ).to.be.calledImmediatelyAfter( highestPrioritySpy ); - sinon.assert.notCalled( highPrioritySpy ); - sinon.assert.notCalled( normalPrioritySpy ); + expect( highPrioritySpy ).not.to.be.called; + expect( normalPrioritySpy ).not.to.be.called; } ); it( 'should do nothing when key other then arrow left and right is pressed', () => { @@ -758,8 +757,8 @@ describe( 'TwoStepCaretMovement()', () => { keyCode: keyCodes.arrowright } ); - sinon.assert.calledOnce( forwardStub ); - sinon.assert.notCalled( backwardStub ); + expect( forwardStub ).to.be.calledOnce; + expect( backwardStub ).not.to.be.called; setData( model, '<$text>foo<$text a="true">[]bar' ); @@ -767,8 +766,8 @@ describe( 'TwoStepCaretMovement()', () => { keyCode: keyCodes.arrowleft } ); - sinon.assert.calledOnce( backwardStub ); - sinon.assert.calledOnce( forwardStub ); + expect( forwardStub ).to.be.calledOnce; + expect( backwardStub ).to.be.calledOnce; } ); it( 'should use the opposite helper methods (RTL content direction)', () => { @@ -811,8 +810,8 @@ describe( 'TwoStepCaretMovement()', () => { keyCode: keyCodes.arrowleft } ); - sinon.assert.calledOnce( forwardStub ); - sinon.assert.notCalled( backwardStub ); + expect( forwardStub ).to.be.calledOnce; + expect( backwardStub ).not.to.be.called; setData( model, '<$text>foo<$text a="true">[]bar' ); @@ -820,8 +819,8 @@ describe( 'TwoStepCaretMovement()', () => { keyCode: keyCodes.arrowright } ); - sinon.assert.calledOnce( backwardStub ); - sinon.assert.calledOnce( forwardStub ); + expect( forwardStub ).to.be.calledOnce; + expect( backwardStub ).to.be.calledOnce; return newEditor.destroy(); } ); From bd3cc2e185d88b1b9e16f7c39543a37c1d42eed2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Tue, 21 Jul 2020 14:47:39 +0200 Subject: [PATCH 16/18] Update findattributerange() docs. --- packages/ckeditor5-typing/src/utils/findattributerange.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/ckeditor5-typing/src/utils/findattributerange.js b/packages/ckeditor5-typing/src/utils/findattributerange.js index 1e3e6454892..ca7ffe50562 100644 --- a/packages/ckeditor5-typing/src/utils/findattributerange.js +++ b/packages/ckeditor5-typing/src/utils/findattributerange.js @@ -8,9 +8,8 @@ */ /** - * Returns a model range that covers all consecutive - * {@link module:engine/view/attributeelement~AttributeElement attribute elements} - * of the given `attributeName` and `value` that intersect the given `position`. + * 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. @@ -18,6 +17,7 @@ * @param {module:engine/model/position~Position} position The start position. * @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 findAttributeRange( position, attributeName, value, model ) { From b79115d1b43a963d2b059f6e9828d298b66476a3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Tue, 21 Jul 2020 15:15:06 +0200 Subject: [PATCH 17/18] Fix sample in the preserving custom content guide. --- .../guides/deep-dive/conversion-preserving-custom-content.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/ckeditor5-engine/docs/framework/guides/deep-dive/conversion-preserving-custom-content.md b/packages/ckeditor5-engine/docs/framework/guides/deep-dive/conversion-preserving-custom-content.md index 13d5c4fea9f..062ed1f4cac 100644 --- a/packages/ckeditor5-engine/docs/framework/guides/deep-dive/conversion-preserving-custom-content.md +++ b/packages/ckeditor5-engine/docs/framework/guides/deep-dive/conversion-preserving-custom-content.md @@ -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 ) ); } /** From 06c8992d9670582b572fa8713170f5a323979e5d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Tue, 21 Jul 2020 15:15:44 +0200 Subject: [PATCH 18/18] Fix inlineHighlight() after is() API change. --- packages/ckeditor5-typing/src/utils/inlinehighlight.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/ckeditor5-typing/src/utils/inlinehighlight.js b/packages/ckeditor5-typing/src/utils/inlinehighlight.js index cc512851d8a..81151054e97 100644 --- a/packages/ckeditor5-typing/src/utils/inlinehighlight.js +++ b/packages/ckeditor5-typing/src/utils/inlinehighlight.js @@ -55,7 +55,7 @@ export default function inlineHighlight( editor, attributeName, tagName, classNa // There might be multiple view elements in the `viewRange`, for example, when the `a` element is // broken by a UIElement. for ( const item of viewRange.getItems() ) { - if ( item.is( tagName ) && !item.hasClass( className ) ) { + if ( item.is( 'element', tagName ) && !item.hasClass( className ) ) { writer.addClass( className, item ); highlightedElements.add( item ); changed = true;