From 3c88b6d59ab2df9daf47c09359c830cac43ae351 Mon Sep 17 00:00:00 2001 From: Kuba Niegowski Date: Thu, 2 Apr 2020 18:36:48 +0200 Subject: [PATCH 1/4] Added defaultValue to ManualDecorators. --- src/link.js | 1 + src/linkcommand.js | 49 ++++++++++-------- src/utils/manualdecorator.js | 10 +++- tests/linkcommand.js | 91 ++++++++++++++++++++++++++++------ tests/utils/manualdecorator.js | 17 +++++++ 5 files changed, 132 insertions(+), 36 deletions(-) diff --git a/src/link.js b/src/link.js index 7570e82..87b235f 100644 --- a/src/link.js +++ b/src/link.js @@ -215,4 +215,5 @@ export default class Link extends Plugin { * @property {Object} attributes Key-value pairs used as link attributes added to the output during the * {@glink framework/guides/architecture/editing-engine#conversion downcasting}. * Attributes should follow the {@link module:engine/view/elementdefinition~ElementDefinition} syntax. + * @property {Boolean} [defaultValue=false] Controls whether the decorator is "on" by default. */ diff --git a/src/linkcommand.js b/src/linkcommand.js index ad17f4e..c5f6078 100644 --- a/src/linkcommand.js +++ b/src/linkcommand.js @@ -46,7 +46,7 @@ export default class LinkCommand extends Command { */ restoreManualDecoratorStates() { for ( const manualDecorator of this.manualDecorators ) { - manualDecorator.value = this._getDecoratorStateFromModel( manualDecorator.id ); + manualDecorator.value = this._getDecoratorStateFromModel( manualDecorator ); } } @@ -60,7 +60,7 @@ export default class LinkCommand extends Command { this.value = doc.selection.getAttribute( 'linkHref' ); for ( const manualDecorator of this.manualDecorators ) { - manualDecorator.value = this._getDecoratorStateFromModel( manualDecorator.id ); + manualDecorator.value = this._getDecoratorStateFromModel( manualDecorator ); } this.isEnabled = model.schema.checkAttributeInSelection( doc.selection, 'linkHref' ); @@ -132,14 +132,18 @@ export default class LinkCommand extends Command { const model = this.editor.model; const selection = model.document.selection; // Stores information about manual decorators to turn them on/off when command is applied. - const truthyManualDecorators = []; - const falsyManualDecorators = []; + const setManualDecorators = []; + const removeManualDecorators = []; for ( const name in manualDecoratorIds ) { - if ( manualDecoratorIds[ name ] ) { - truthyManualDecorators.push( name ); + const manualDecorator = this.manualDecorators.get( name ); + const value = manualDecoratorIds[ name ]; + + // We need to set an attribute for the disabled decorator if it's enabled by default (`defaultValue`). + if ( value || value != manualDecorator.defaultValue ) { + setManualDecorators.push( { name, value } ); } else { - falsyManualDecorators.push( name ); + removeManualDecorators.push( name ); } } @@ -155,12 +159,12 @@ export default class LinkCommand extends Command { writer.setAttribute( 'linkHref', href, linkRange ); - truthyManualDecorators.forEach( item => { - writer.setAttribute( item, true, linkRange ); + setManualDecorators.forEach( ( { name, value } ) => { + writer.setAttribute( name, value, linkRange ); } ); - falsyManualDecorators.forEach( item => { - writer.removeAttribute( item, linkRange ); + removeManualDecorators.forEach( name => { + writer.removeAttribute( name, linkRange ); } ); // Create new range wrapping changed link. @@ -174,8 +178,8 @@ export default class LinkCommand extends Command { attributes.set( 'linkHref', href ); - truthyManualDecorators.forEach( item => { - attributes.set( item, true ); + setManualDecorators.forEach( ( { name, value } ) => { + attributes.set( name, value ); } ); const node = writer.createText( href, attributes ); @@ -193,12 +197,12 @@ export default class LinkCommand extends Command { for ( const range of ranges ) { writer.setAttribute( 'linkHref', href, range ); - truthyManualDecorators.forEach( item => { - writer.setAttribute( item, true, range ); + setManualDecorators.forEach( ( { name, value } ) => { + writer.setAttribute( name, value, range ); } ); - falsyManualDecorators.forEach( item => { - writer.removeAttribute( item, range ); + removeManualDecorators.forEach( name => { + writer.removeAttribute( name, range ); } ); } } @@ -209,11 +213,16 @@ export default class LinkCommand extends Command { * Provides information whether a decorator with a given name is present in the currently processed selection. * * @private - * @param {String} decoratorName The name of the manual decorator used in the model + * @param {module:link/utils~ManualDecorator} decorator The manual decorator used in the model. * @returns {Boolean} The information whether a given decorator is currently present in the selection. */ - _getDecoratorStateFromModel( decoratorName ) { + _getDecoratorStateFromModel( decorator ) { const doc = this.editor.model.document; - return doc.selection.getAttribute( decoratorName ) || false; + + if ( doc.selection.hasAttribute( decorator.id ) ) { + return doc.selection.getAttribute( decorator.id ); + } + + return decorator.defaultValue; } } diff --git a/src/utils/manualdecorator.js b/src/utils/manualdecorator.js index aba63ef..037af63 100644 --- a/src/utils/manualdecorator.js +++ b/src/utils/manualdecorator.js @@ -27,8 +27,9 @@ export default class ManualDecorator { * @param {String} config.label The label used in the user interface to toggle the manual decorator. * @param {Object} config.attributes A set of attributes added to output data when the decorator is active for a specific link. * Attributes should keep the format of attributes defined in {@link module:engine/view/elementdefinition~ElementDefinition}. + * @param {Boolean} [config.defaultValue=false] Controls whether the decorator is "on" by default. */ - constructor( { id, label, attributes } ) { + constructor( { id, label, attributes, defaultValue } ) { /** * An ID of a manual decorator which is the name of the attribute in the model, for example: 'linkManualDecorator0'. * @@ -44,6 +45,13 @@ export default class ManualDecorator { */ this.set( 'value' ); + /** + * The default value of manual decorator. + * + * @type {Boolean} + */ + this.defaultValue = defaultValue || false; + /** * The label used in the user interface to toggle the manual decorator. * diff --git a/tests/linkcommand.js b/tests/linkcommand.js index 86426d6..b911f88 100644 --- a/tests/linkcommand.js +++ b/tests/linkcommand.js @@ -284,10 +284,18 @@ describe( 'LinkCommand', () => { target: '_blank' } } ) ); + command.manualDecorators.add( new ManualDecorator( { + id: 'linkIsSth', + label: 'Sth', + attributes: { + class: 'sth' + }, + defaultValue: true + } ) ); model.schema.extend( '$text', { allowIn: '$root', - allowAttributes: [ 'linkHref', 'linkIsFoo', 'linkIsBar' ] + allowAttributes: [ 'linkHref', 'linkIsFoo', 'linkIsBar', 'linkIsSth' ] } ); model.schema.register( 'p', { inheritAllFrom: '$block' } ); @@ -302,19 +310,19 @@ describe( 'LinkCommand', () => { it( 'should insert additional attributes to link when it is created', () => { setData( model, 'foo[]bar' ); - command.execute( 'url', { linkIsFoo: true, linkIsBar: true } ); + command.execute( 'url', { linkIsFoo: true, linkIsBar: true, linkIsSth: true } ); expect( getData( model ) ).to - .equal( 'foo[<$text linkHref="url" linkIsBar="true" linkIsFoo="true">url]bar' ); + .equal( 'foo[<$text linkHref="url" linkIsBar="true" linkIsFoo="true" linkIsSth="true">url]bar' ); } ); it( 'should add additional attributes to link when link is modified', () => { setData( model, 'f<$text linkHref="url">o[]obar' ); - command.execute( 'url', { linkIsFoo: true, linkIsBar: true } ); + command.execute( 'url', { linkIsFoo: true, linkIsBar: true, linkIsSth: true } ); expect( getData( model ) ).to - .equal( 'f[<$text linkHref="url" linkIsBar="true" linkIsFoo="true">ooba]r' ); + .equal( 'f[<$text linkHref="url" linkIsBar="true" linkIsFoo="true" linkIsSth="true">ooba]r' ); } ); it( 'should remove additional attributes to link if those are falsy', () => { @@ -324,25 +332,33 @@ describe( 'LinkCommand', () => { expect( getData( model ) ).to.equal( 'foo[<$text linkHref="url">url]bar' ); } ); + + it( 'should add additional attributes to link if those are falsy but decorator\'s defaultValue is set to true', () => { + setData( model, 'foo<$text linkHref="url" linkIsBar="true" linkIsFoo="true">u[]rlbar' ); + + command.execute( 'url', { linkIsFoo: false, linkIsBar: false, linkIsSth: false } ); + + expect( getData( model ) ).to.equal( 'foo[<$text linkHref="url" linkIsSth="false">url]bar' ); + } ); } ); describe( 'range selection', () => { it( 'should insert additional attributes to link when it is created', () => { setData( model, 'f[ooba]r' ); - command.execute( 'url', { linkIsFoo: true, linkIsBar: true } ); + command.execute( 'url', { linkIsFoo: true, linkIsBar: true, linkIsSth: true } ); expect( getData( model ) ).to - .equal( 'f[<$text linkHref="url" linkIsBar="true" linkIsFoo="true">ooba]r' ); + .equal( 'f[<$text linkHref="url" linkIsBar="true" linkIsFoo="true" linkIsSth="true">ooba]r' ); } ); it( 'should add additional attributes to link when link is modified', () => { setData( model, 'f[<$text linkHref="foo">ooba]r' ); - command.execute( 'url', { linkIsFoo: true, linkIsBar: true } ); + command.execute( 'url', { linkIsFoo: true, linkIsBar: true, linkIsSth: true } ); expect( getData( model ) ).to - .equal( 'f[<$text linkHref="url" linkIsBar="true" linkIsFoo="true">ooba]r' ); + .equal( 'f[<$text linkHref="url" linkIsBar="true" linkIsFoo="true" linkIsSth="true">ooba]r' ); } ); it( 'should remove additional attributes to link if those are falsy', () => { @@ -352,29 +368,66 @@ describe( 'LinkCommand', () => { expect( getData( model ) ).to.equal( 'foo[<$text linkHref="url">url]bar' ); } ); + + it( 'should add additional attributes to link if those are falsy but decorator\'s defaultValue is set to true', () => { + setData( model, 'foo[<$text linkHref="url" linkIsBar="true" linkIsFoo="true">url]bar' ); + + command.execute( 'url', { linkIsFoo: false, linkIsBar: false, linkIsSth: false } ); + + expect( getData( model ) ).to.equal( 'foo[<$text linkHref="url" linkIsSth="false">url]bar' ); + } ); } ); describe( 'restoreManualDecoratorStates()', () => { it( 'synchronize values with current model state', () => { - setData( model, 'foo<$text linkHref="url" linkIsBar="true" linkIsFoo="true">u[]rlbar' ); + setData( model, 'foo<$text linkHref="url" linkIsBar="true" linkIsFoo="true" linkIsSth="true">u[]rlbar' ); expect( decoratorStates( command.manualDecorators ) ).to.deep.equal( { linkIsFoo: true, - linkIsBar: true + linkIsBar: true, + linkIsSth: true } ); command.manualDecorators.first.value = false; expect( decoratorStates( command.manualDecorators ) ).to.deep.equal( { linkIsFoo: false, - linkIsBar: true + linkIsBar: true, + linkIsSth: true + } ); + + command.restoreManualDecoratorStates(); + + expect( decoratorStates( command.manualDecorators ) ).to.deep.equal( { + linkIsFoo: true, + linkIsBar: true, + linkIsSth: true + } ); + } ); + + it( 'synchronize values with current model state when the decorator that is "on" default is "off"', () => { + setData( model, 'foo<$text linkHref="url" linkIsBar="true" linkIsFoo="true" linkIsSth="false">u[]rlbar' ); + + expect( decoratorStates( command.manualDecorators ) ).to.deep.equal( { + linkIsFoo: true, + linkIsBar: true, + linkIsSth: false + } ); + + command.manualDecorators.last.value = true; + + expect( decoratorStates( command.manualDecorators ) ).to.deep.equal( { + linkIsFoo: true, + linkIsBar: true, + linkIsSth: true } ); command.restoreManualDecoratorStates(); expect( decoratorStates( command.manualDecorators ) ).to.deep.equal( { linkIsFoo: true, - linkIsBar: true + linkIsBar: true, + linkIsSth: false } ); } ); } ); @@ -383,8 +436,16 @@ describe( 'LinkCommand', () => { it( 'obtain current values from the model', () => { setData( model, 'foo[<$text linkHref="url" linkIsBar="true">url]bar' ); - expect( command._getDecoratorStateFromModel( 'linkIsFoo' ) ).to.be.false; - expect( command._getDecoratorStateFromModel( 'linkIsBar' ) ).to.be.true; + expect( command._getDecoratorStateFromModel( command.manualDecorators.get( 'linkIsFoo' ) ) ).to.be.false; + expect( command._getDecoratorStateFromModel( command.manualDecorators.get( 'linkIsBar' ) ) ).to.be.true; + } ); + + it( 'fallbacks to defaultValue if there is no attribute in model', () => { + setData( model, 'foo[<$text linkHref="url">url]bar' ); + + expect( command._getDecoratorStateFromModel( command.manualDecorators.get( 'linkIsFoo' ) ) ).to.be.false; + expect( command._getDecoratorStateFromModel( command.manualDecorators.get( 'linkIsBar' ) ) ).to.be.false; + expect( command._getDecoratorStateFromModel( command.manualDecorators.get( 'linkIsSth' ) ) ).to.be.true; } ); } ); } ); diff --git a/tests/utils/manualdecorator.js b/tests/utils/manualdecorator.js index de986aa..9a82d98 100644 --- a/tests/utils/manualdecorator.js +++ b/tests/utils/manualdecorator.js @@ -24,6 +24,23 @@ describe( 'Manual Decorator', () => { expect( manualDecorator.id ).to.equal( 'foo' ); expect( manualDecorator.label ).to.equal( 'bar' ); expect( manualDecorator.attributes ).to.deep.equal( { one: 'two' } ); + expect( manualDecorator.defaultValue ).to.deep.equal( false ); + } ); + + it( 'constructor with defaultValue', () => { + manualDecorator = new ManualDecorator( { + id: 'foo', + label: 'bar', + attributes: { + one: 'two' + }, + defaultValue: true + } ); + + expect( manualDecorator.id ).to.equal( 'foo' ); + expect( manualDecorator.label ).to.equal( 'bar' ); + expect( manualDecorator.attributes ).to.deep.equal( { one: 'two' } ); + expect( manualDecorator.defaultValue ).to.deep.equal( true ); } ); it( '#value is observable', () => { From 2068a0f14b56a598c0a41e1f03e3136e6e32f634 Mon Sep 17 00:00:00 2001 From: Kuba Niegowski Date: Mon, 6 Apr 2020 18:51:20 +0200 Subject: [PATCH 2/4] Added upcast conversion for link manual decorators with default values. --- src/linkediting.js | 37 +++++++++++++++++++++++++++++++++++++ tests/linkediting.js | 40 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 77 insertions(+) diff --git a/src/linkediting.js b/src/linkediting.js index 93ea0ad..c897069 100644 --- a/src/linkediting.js +++ b/src/linkediting.js @@ -15,6 +15,8 @@ import AutomaticDecorators from './utils/automaticdecorators'; import ManualDecorator from './utils/manualdecorator'; import bindTwoStepCaretToAttribute from '@ckeditor/ckeditor5-engine/src/utils/bindtwostepcarettoattribute'; import findLinkRange from './findlinkrange'; +import Matcher from '@ckeditor/ckeditor5-engine/src/view/matcher'; + import '../theme/link.css'; const HIGHLIGHT_CLASS = 'ck-link_selected'; @@ -187,6 +189,41 @@ export default class LinkEditing extends Plugin { key: decorator.id } } ); + + // For a decorator that is enabled by default we must check if it was applied to reflect that in the model. + if ( decorator.defaultValue ) { + editor.conversion.for( 'upcast' ).add( dispatcher => { + const matcher = new Matcher( { + name: 'a', + attributes: { + href: true + } + } ); + + dispatcher.on( 'element', ( evt, data, conversionApi ) => { + const matcherResult = matcher.match( data.viewItem ); + + if ( !matcherResult ) { + return; + } + + // We must check if all decorator values are set on view element. + const decoratorValue = Object.entries( decorator.attributes ).every( ( [ key, value ] ) => { + return matcherResult.element.getAttribute( key ) == value; + } ); + + // The decorator is applied, model attribute was already up-casted. + if ( decoratorValue ) { + return; + } + + // Set attribute on each item in range according to Schema. + for ( const node of Array.from( data.modelRange.getItems() ) ) { + conversionApi.writer.setAttribute( decorator.id, false, node ); + } + }, { priority: 'low' } ); + } ); + } } ); } diff --git a/tests/linkediting.js b/tests/linkediting.js index 3a27b9b..fcbd3e3 100644 --- a/tests/linkediting.js +++ b/tests/linkediting.js @@ -664,6 +664,46 @@ describe( 'LinkEditing', () => { } ); } ); + it( 'should upcast attributes from initial data for a manual decorator enabled by default', () => { + return VirtualTestEditor + .create( { + initialData: '

Foo' + + 'Bar

', + plugins: [ Paragraph, LinkEditing, Enter ], + link: { + decorators: { + isExternal: { + mode: 'manual', + label: 'Open in a new window', + attributes: { + target: '_blank', + rel: 'noopener noreferrer' + }, + defaultValue: true + }, + isDownloadable: { + mode: 'manual', + label: 'Downloadable', + attributes: { + download: 'file' + } + } + } + } + } ) + .then( newEditor => { + editor = newEditor; + model = editor.model; + + expect( getModelData( model, { withoutSelection: true } ) ).to.equal( + '' + + '<$text linkHref="url" linkIsDownloadable="true" linkIsExternal="true">Foo' + + '<$text linkHref="example.com" linkIsDownloadable="true" linkIsExternal="false">Bar' + + '' + ); + } ); + } ); + it( 'should not upcast partial and incorrect attributes', () => { return VirtualTestEditor .create( { From 5ea7fac9afac2a58a648e6f1d22f3e1c7c441db6 Mon Sep 17 00:00:00 2001 From: Kuba Niegowski Date: Tue, 7 Apr 2020 20:09:05 +0200 Subject: [PATCH 3/4] Changed logic of link manual decorator's defaultValue. --- src/link.js | 2 +- src/linkcommand.js | 49 ++++++-------- src/linkediting.js | 37 ----------- src/linkui.js | 2 +- src/ui/linkformview.js | 22 +++---- src/utils/manualdecorator.js | 4 +- tests/linkcommand.js | 28 +------- tests/linkediting.js | 40 ------------ tests/ui/linkformview.js | 116 +++++++++++++++++++++++++++++---- tests/utils/manualdecorator.js | 2 +- 10 files changed, 143 insertions(+), 159 deletions(-) diff --git a/src/link.js b/src/link.js index 87b235f..9c1fa77 100644 --- a/src/link.js +++ b/src/link.js @@ -215,5 +215,5 @@ export default class Link extends Plugin { * @property {Object} attributes Key-value pairs used as link attributes added to the output during the * {@glink framework/guides/architecture/editing-engine#conversion downcasting}. * Attributes should follow the {@link module:engine/view/elementdefinition~ElementDefinition} syntax. - * @property {Boolean} [defaultValue=false] Controls whether the decorator is "on" by default. + * @property {Boolean} [defaultValue] Controls whether the decorator is "on" by default. */ diff --git a/src/linkcommand.js b/src/linkcommand.js index c5f6078..4720256 100644 --- a/src/linkcommand.js +++ b/src/linkcommand.js @@ -46,7 +46,7 @@ export default class LinkCommand extends Command { */ restoreManualDecoratorStates() { for ( const manualDecorator of this.manualDecorators ) { - manualDecorator.value = this._getDecoratorStateFromModel( manualDecorator ); + manualDecorator.value = this._getDecoratorStateFromModel( manualDecorator.id ); } } @@ -60,7 +60,7 @@ export default class LinkCommand extends Command { this.value = doc.selection.getAttribute( 'linkHref' ); for ( const manualDecorator of this.manualDecorators ) { - manualDecorator.value = this._getDecoratorStateFromModel( manualDecorator ); + manualDecorator.value = this._getDecoratorStateFromModel( manualDecorator.id ); } this.isEnabled = model.schema.checkAttributeInSelection( doc.selection, 'linkHref' ); @@ -132,18 +132,14 @@ export default class LinkCommand extends Command { const model = this.editor.model; const selection = model.document.selection; // Stores information about manual decorators to turn them on/off when command is applied. - const setManualDecorators = []; - const removeManualDecorators = []; + const truthyManualDecorators = []; + const falsyManualDecorators = []; for ( const name in manualDecoratorIds ) { - const manualDecorator = this.manualDecorators.get( name ); - const value = manualDecoratorIds[ name ]; - - // We need to set an attribute for the disabled decorator if it's enabled by default (`defaultValue`). - if ( value || value != manualDecorator.defaultValue ) { - setManualDecorators.push( { name, value } ); + if ( manualDecoratorIds[ name ] ) { + truthyManualDecorators.push( name ); } else { - removeManualDecorators.push( name ); + falsyManualDecorators.push( name ); } } @@ -159,12 +155,12 @@ export default class LinkCommand extends Command { writer.setAttribute( 'linkHref', href, linkRange ); - setManualDecorators.forEach( ( { name, value } ) => { - writer.setAttribute( name, value, linkRange ); + truthyManualDecorators.forEach( item => { + writer.setAttribute( item, true, linkRange ); } ); - removeManualDecorators.forEach( name => { - writer.removeAttribute( name, linkRange ); + falsyManualDecorators.forEach( item => { + writer.removeAttribute( item, linkRange ); } ); // Create new range wrapping changed link. @@ -178,8 +174,8 @@ export default class LinkCommand extends Command { attributes.set( 'linkHref', href ); - setManualDecorators.forEach( ( { name, value } ) => { - attributes.set( name, value ); + truthyManualDecorators.forEach( item => { + attributes.set( item, true ); } ); const node = writer.createText( href, attributes ); @@ -197,12 +193,12 @@ export default class LinkCommand extends Command { for ( const range of ranges ) { writer.setAttribute( 'linkHref', href, range ); - setManualDecorators.forEach( ( { name, value } ) => { - writer.setAttribute( name, value, range ); + truthyManualDecorators.forEach( item => { + writer.setAttribute( item, true, range ); } ); - removeManualDecorators.forEach( name => { - writer.removeAttribute( name, range ); + falsyManualDecorators.forEach( item => { + writer.removeAttribute( item, range ); } ); } } @@ -213,16 +209,11 @@ export default class LinkCommand extends Command { * Provides information whether a decorator with a given name is present in the currently processed selection. * * @private - * @param {module:link/utils~ManualDecorator} decorator The manual decorator used in the model. + * @param {String} decoratorName The name of the manual decorator used in the model * @returns {Boolean} The information whether a given decorator is currently present in the selection. */ - _getDecoratorStateFromModel( decorator ) { + _getDecoratorStateFromModel( decoratorName ) { const doc = this.editor.model.document; - - if ( doc.selection.hasAttribute( decorator.id ) ) { - return doc.selection.getAttribute( decorator.id ); - } - - return decorator.defaultValue; + return doc.selection.getAttribute( decoratorName ); } } diff --git a/src/linkediting.js b/src/linkediting.js index c897069..93ea0ad 100644 --- a/src/linkediting.js +++ b/src/linkediting.js @@ -15,8 +15,6 @@ import AutomaticDecorators from './utils/automaticdecorators'; import ManualDecorator from './utils/manualdecorator'; import bindTwoStepCaretToAttribute from '@ckeditor/ckeditor5-engine/src/utils/bindtwostepcarettoattribute'; import findLinkRange from './findlinkrange'; -import Matcher from '@ckeditor/ckeditor5-engine/src/view/matcher'; - import '../theme/link.css'; const HIGHLIGHT_CLASS = 'ck-link_selected'; @@ -189,41 +187,6 @@ export default class LinkEditing extends Plugin { key: decorator.id } } ); - - // For a decorator that is enabled by default we must check if it was applied to reflect that in the model. - if ( decorator.defaultValue ) { - editor.conversion.for( 'upcast' ).add( dispatcher => { - const matcher = new Matcher( { - name: 'a', - attributes: { - href: true - } - } ); - - dispatcher.on( 'element', ( evt, data, conversionApi ) => { - const matcherResult = matcher.match( data.viewItem ); - - if ( !matcherResult ) { - return; - } - - // We must check if all decorator values are set on view element. - const decoratorValue = Object.entries( decorator.attributes ).every( ( [ key, value ] ) => { - return matcherResult.element.getAttribute( key ) == value; - } ); - - // The decorator is applied, model attribute was already up-casted. - if ( decoratorValue ) { - return; - } - - // Set attribute on each item in range according to Schema. - for ( const node of Array.from( data.modelRange.getItems() ) ) { - conversionApi.writer.setAttribute( decorator.id, false, node ); - } - }, { priority: 'low' } ); - } ); - } } ); } diff --git a/src/linkui.js b/src/linkui.js index 49a2dc2..629bfe7 100644 --- a/src/linkui.js +++ b/src/linkui.js @@ -144,7 +144,7 @@ export default class LinkUI extends Plugin { const editor = this.editor; const linkCommand = editor.commands.get( 'link' ); - const formView = new LinkFormView( editor.locale, linkCommand.manualDecorators ); + const formView = new LinkFormView( editor.locale, linkCommand ); formView.urlInputView.fieldView.bind( 'value' ).to( linkCommand, 'value' ); diff --git a/src/ui/linkformview.js b/src/ui/linkformview.js index 653bafb..2b050ee 100644 --- a/src/ui/linkformview.js +++ b/src/ui/linkformview.js @@ -39,10 +39,9 @@ export default class LinkFormView extends View { * Also see {@link #render}. * * @param {module:utils/locale~Locale} [locale] The localization services instance. - * @param {module:utils/collection~Collection} [manualDecorators] Reference to manual decorators in - * {@link module:link/linkcommand~LinkCommand#manualDecorators}. + * @param {module:link/linkcommand~LinkCommand} linkCommand Reference to {@link module:link/linkcommand~LinkCommand}. */ - constructor( locale, manualDecorators = [] ) { + constructor( locale, linkCommand ) { super( locale ); const t = locale.t; @@ -94,7 +93,7 @@ export default class LinkFormView extends View { * @readonly * @type {module:ui/viewcollection~ViewCollection} */ - this._manualDecoratorSwitches = this._createManualDecoratorSwitches( manualDecorators ); + this._manualDecoratorSwitches = this._createManualDecoratorSwitches( linkCommand ); /** * A collection of child views in the form. @@ -102,7 +101,7 @@ export default class LinkFormView extends View { * @readonly * @type {module:ui/viewcollection~ViewCollection} */ - this.children = this._createFormChildren( manualDecorators ); + this.children = this._createFormChildren( linkCommand.manualDecorators ); /** * A collection of views that can be focused in the form. @@ -135,7 +134,7 @@ export default class LinkFormView extends View { const classList = [ 'ck', 'ck-link-form' ]; - if ( manualDecorators.length ) { + if ( linkCommand.manualDecorators.length ) { classList.push( 'ck-link-form_layout-vertical' ); } @@ -258,14 +257,13 @@ export default class LinkFormView extends View { * made based on {@link module:link/linkcommand~LinkCommand#manualDecorators}. * * @private - * @param {module:utils/collection~Collection} manualDecorators A reference to the - * collection of manual decorators stored in the link command. + * @param {module:link/linkcommand~LinkCommand} linkCommand A reference to the link command. * @returns {module:ui/viewcollection~ViewCollection} of switch buttons. */ - _createManualDecoratorSwitches( manualDecorators ) { + _createManualDecoratorSwitches( linkCommand ) { const switches = this.createCollection(); - for ( const manualDecorator of manualDecorators ) { + for ( const manualDecorator of linkCommand.manualDecorators ) { const switchButton = new SwitchButtonView( this.locale ); switchButton.set( { @@ -274,7 +272,9 @@ export default class LinkFormView extends View { withText: true } ); - switchButton.bind( 'isOn' ).to( manualDecorator, 'value' ); + switchButton.bind( 'isOn' ).toMany( [ manualDecorator, linkCommand ], 'value', ( decoratorValue, commandValue ) => { + return commandValue === undefined && decoratorValue === undefined ? manualDecorator.defaultValue : decoratorValue; + } ); switchButton.on( 'execute', () => { manualDecorator.set( 'value', !switchButton.isOn ); diff --git a/src/utils/manualdecorator.js b/src/utils/manualdecorator.js index 037af63..ca23f90 100644 --- a/src/utils/manualdecorator.js +++ b/src/utils/manualdecorator.js @@ -27,7 +27,7 @@ export default class ManualDecorator { * @param {String} config.label The label used in the user interface to toggle the manual decorator. * @param {Object} config.attributes A set of attributes added to output data when the decorator is active for a specific link. * Attributes should keep the format of attributes defined in {@link module:engine/view/elementdefinition~ElementDefinition}. - * @param {Boolean} [config.defaultValue=false] Controls whether the decorator is "on" by default. + * @param {Boolean} [config.defaultValue] Controls whether the decorator is "on" by default. */ constructor( { id, label, attributes, defaultValue } ) { /** @@ -50,7 +50,7 @@ export default class ManualDecorator { * * @type {Boolean} */ - this.defaultValue = defaultValue || false; + this.defaultValue = defaultValue; /** * The label used in the user interface to toggle the manual decorator. diff --git a/tests/linkcommand.js b/tests/linkcommand.js index b911f88..b24fd78 100644 --- a/tests/linkcommand.js +++ b/tests/linkcommand.js @@ -332,14 +332,6 @@ describe( 'LinkCommand', () => { expect( getData( model ) ).to.equal( 'foo[<$text linkHref="url">url]bar' ); } ); - - it( 'should add additional attributes to link if those are falsy but decorator\'s defaultValue is set to true', () => { - setData( model, 'foo<$text linkHref="url" linkIsBar="true" linkIsFoo="true">u[]rlbar' ); - - command.execute( 'url', { linkIsFoo: false, linkIsBar: false, linkIsSth: false } ); - - expect( getData( model ) ).to.equal( 'foo[<$text linkHref="url" linkIsSth="false">url]bar' ); - } ); } ); describe( 'range selection', () => { @@ -368,14 +360,6 @@ describe( 'LinkCommand', () => { expect( getData( model ) ).to.equal( 'foo[<$text linkHref="url">url]bar' ); } ); - - it( 'should add additional attributes to link if those are falsy but decorator\'s defaultValue is set to true', () => { - setData( model, 'foo[<$text linkHref="url" linkIsBar="true" linkIsFoo="true">url]bar' ); - - command.execute( 'url', { linkIsFoo: false, linkIsBar: false, linkIsSth: false } ); - - expect( getData( model ) ).to.equal( 'foo[<$text linkHref="url" linkIsSth="false">url]bar' ); - } ); } ); describe( 'restoreManualDecoratorStates()', () => { @@ -436,16 +420,8 @@ describe( 'LinkCommand', () => { it( 'obtain current values from the model', () => { setData( model, 'foo[<$text linkHref="url" linkIsBar="true">url]bar' ); - expect( command._getDecoratorStateFromModel( command.manualDecorators.get( 'linkIsFoo' ) ) ).to.be.false; - expect( command._getDecoratorStateFromModel( command.manualDecorators.get( 'linkIsBar' ) ) ).to.be.true; - } ); - - it( 'fallbacks to defaultValue if there is no attribute in model', () => { - setData( model, 'foo[<$text linkHref="url">url]bar' ); - - expect( command._getDecoratorStateFromModel( command.manualDecorators.get( 'linkIsFoo' ) ) ).to.be.false; - expect( command._getDecoratorStateFromModel( command.manualDecorators.get( 'linkIsBar' ) ) ).to.be.false; - expect( command._getDecoratorStateFromModel( command.manualDecorators.get( 'linkIsSth' ) ) ).to.be.true; + expect( command._getDecoratorStateFromModel( 'linkIsFoo' ) ).to.be.undefined; + expect( command._getDecoratorStateFromModel( 'linkIsBar' ) ).to.be.true; } ); } ); } ); diff --git a/tests/linkediting.js b/tests/linkediting.js index fcbd3e3..3a27b9b 100644 --- a/tests/linkediting.js +++ b/tests/linkediting.js @@ -664,46 +664,6 @@ describe( 'LinkEditing', () => { } ); } ); - it( 'should upcast attributes from initial data for a manual decorator enabled by default', () => { - return VirtualTestEditor - .create( { - initialData: '

Foo' + - 'Bar

', - plugins: [ Paragraph, LinkEditing, Enter ], - link: { - decorators: { - isExternal: { - mode: 'manual', - label: 'Open in a new window', - attributes: { - target: '_blank', - rel: 'noopener noreferrer' - }, - defaultValue: true - }, - isDownloadable: { - mode: 'manual', - label: 'Downloadable', - attributes: { - download: 'file' - } - } - } - } - } ) - .then( newEditor => { - editor = newEditor; - model = editor.model; - - expect( getModelData( model, { withoutSelection: true } ) ).to.equal( - '' + - '<$text linkHref="url" linkIsDownloadable="true" linkIsExternal="true">Foo' + - '<$text linkHref="example.com" linkIsDownloadable="true" linkIsExternal="false">Bar' + - '' - ); - } ); - } ); - it( 'should not upcast partial and incorrect attributes', () => { return VirtualTestEditor .create( { diff --git a/tests/ui/linkformview.js b/tests/ui/linkformview.js index 205f5cf..402274d 100644 --- a/tests/ui/linkformview.js +++ b/tests/ui/linkformview.js @@ -18,6 +18,8 @@ import Collection from '@ckeditor/ckeditor5-utils/src/collection'; import { add as addTranslations, _clear as clearTranslations } from '@ckeditor/ckeditor5-utils/src/translation-service'; import ClassicTestEditor from '@ckeditor/ckeditor5-core/tests/_utils/classictesteditor'; import Link from '../../src/link'; +import ObservableMixin from '@ckeditor/ckeditor5-utils/src/observablemixin'; +import mix from '@ckeditor/ckeditor5-utils/src/mix'; describe( 'LinkFormView', () => { let view; @@ -25,7 +27,7 @@ describe( 'LinkFormView', () => { testUtils.createSinonSandbox(); beforeEach( () => { - view = new LinkFormView( { t: val => val } ); + view = new LinkFormView( { t: val => val }, { manualDecorators: [] } ); view.render(); } ); @@ -109,7 +111,7 @@ describe( 'LinkFormView', () => { it( 'should register child views\' #element in #focusTracker', () => { const spy = testUtils.sinon.spy( FocusTracker.prototype, 'add' ); - view = new LinkFormView( { t: () => {} } ); + view = new LinkFormView( { t: () => {} }, { manualDecorators: [] } ); view.render(); sinon.assert.calledWithExactly( spy.getCall( 0 ), view.urlInputView.element ); @@ -118,7 +120,7 @@ describe( 'LinkFormView', () => { } ); it( 'starts listening for #keystrokes coming from #element', () => { - view = new LinkFormView( { t: () => {} } ); + view = new LinkFormView( { t: () => {} }, { manualDecorators: [] } ); const spy = sinon.spy( view.keystrokes, 'listenTo' ); @@ -193,7 +195,7 @@ describe( 'LinkFormView', () => { } ); describe( 'manual decorators', () => { - let view, collection; + let view, collection, linkCommand; beforeEach( () => { collection = new Collection(); collection.add( new ManualDecorator( { @@ -208,7 +210,8 @@ describe( 'LinkFormView', () => { label: 'Download', attributes: { download: 'download' - } + }, + defaultValue: true } ) ); collection.add( new ManualDecorator( { id: 'decorator3', @@ -220,7 +223,17 @@ describe( 'LinkFormView', () => { } } ) ); - view = new LinkFormView( { t: val => val }, collection ); + class LinkCommandMock { + constructor( manualDecorators ) { + this.manualDecorators = manualDecorators; + this.set( 'value' ); + } + } + mix( LinkCommandMock, ObservableMixin ); + + linkCommand = new LinkCommandMock( collection ); + + view = new LinkFormView( { t: val => val }, linkCommand ); view.render(); } ); @@ -234,15 +247,18 @@ describe( 'LinkFormView', () => { expect( view._manualDecoratorSwitches.get( 0 ) ).to.deep.include( { name: 'decorator1', - label: 'Foo' + label: 'Foo', + isOn: undefined } ); expect( view._manualDecoratorSwitches.get( 1 ) ).to.deep.include( { name: 'decorator2', - label: 'Download' + label: 'Download', + isOn: true } ); expect( view._manualDecoratorSwitches.get( 2 ) ).to.deep.include( { name: 'decorator3', - label: 'Multi' + label: 'Multi', + isOn: undefined } ); } ); @@ -264,11 +280,29 @@ describe( 'LinkFormView', () => { expect( viewItem.isOn ).to.be.false; } ); + it( 'reacts on switch button changes for the decorator with defaultValue', () => { + const modelItem = collection.get( 1 ); + const viewItem = view._manualDecoratorSwitches.get( 1 ); + + expect( modelItem.value ).to.be.undefined; + expect( viewItem.isOn ).to.be.true; + + viewItem.element.dispatchEvent( new Event( 'click' ) ); + + expect( modelItem.value ).to.be.false; + expect( viewItem.isOn ).to.be.false; + + viewItem.element.dispatchEvent( new Event( 'click' ) ); + + expect( modelItem.value ).to.be.true; + expect( viewItem.isOn ).to.be.true; + } ); + describe( 'getDecoratorSwitchesState()', () => { it( 'should provide object with decorators states', () => { expect( view.getDecoratorSwitchesState() ).to.deep.equal( { decorator1: undefined, - decorator2: undefined, + decorator2: true, decorator3: undefined } ); @@ -280,9 +314,69 @@ describe( 'LinkFormView', () => { expect( view.getDecoratorSwitchesState() ).to.deep.equal( { decorator1: true, + decorator2: false, + decorator3: false + } ); + } ); + + it( 'should use decorator default value if command and decorator values are not set', () => { + expect( view.getDecoratorSwitchesState() ).to.deep.equal( { + decorator1: undefined, decorator2: true, + decorator3: undefined + } ); + } ); + + it( 'should use a decorator value if decorator value is set', () => { + for ( const decorator of collection ) { + decorator.value = true; + } + + expect( view.getDecoratorSwitchesState() ).to.deep.equal( { + decorator1: true, + decorator2: true, + decorator3: true + } ); + + for ( const decorator of collection ) { + decorator.value = false; + } + + expect( view.getDecoratorSwitchesState() ).to.deep.equal( { + decorator1: false, + decorator2: false, + decorator3: false + } ); + } ); + + it( 'should use a decorator value if link command value is set', () => { + linkCommand.value = ''; + + expect( view.getDecoratorSwitchesState() ).to.deep.equal( { + decorator1: undefined, + decorator2: undefined, + decorator3: undefined + } ); + + for ( const decorator of collection ) { + decorator.value = false; + } + + expect( view.getDecoratorSwitchesState() ).to.deep.equal( { + decorator1: false, + decorator2: false, decorator3: false } ); + + for ( const decorator of collection ) { + decorator.value = true; + } + + expect( view.getDecoratorSwitchesState() ).to.deep.equal( { + decorator1: true, + decorator2: true, + decorator3: true + } ); } ); } ); } ); @@ -322,7 +416,7 @@ describe( 'LinkFormView', () => { } ) .then( newEditor => { editor = newEditor; - linkFormView = new LinkFormView( editor.locale, editor.commands.get( 'link' ).manualDecorators ); + linkFormView = new LinkFormView( editor.locale, editor.commands.get( 'link' ) ); } ); } ); diff --git a/tests/utils/manualdecorator.js b/tests/utils/manualdecorator.js index 9a82d98..348159c 100644 --- a/tests/utils/manualdecorator.js +++ b/tests/utils/manualdecorator.js @@ -24,7 +24,7 @@ describe( 'Manual Decorator', () => { expect( manualDecorator.id ).to.equal( 'foo' ); expect( manualDecorator.label ).to.equal( 'bar' ); expect( manualDecorator.attributes ).to.deep.equal( { one: 'two' } ); - expect( manualDecorator.defaultValue ).to.deep.equal( false ); + expect( manualDecorator.defaultValue ).to.deep.equal( undefined ); } ); it( 'constructor with defaultValue', () => { From b2dc424adfbe0d8af8e3d088b39d51187d041452 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotrek=20Koszuli=C5=84ski?= Date: Tue, 7 Apr 2020 21:21:23 +0200 Subject: [PATCH 4/4] Added more documentation and fixed the pointless interface doc. --- docs/features/link.md | 11 ++++++++++- src/link.js | 10 ++++++++-- 2 files changed, 18 insertions(+), 3 deletions(-) diff --git a/docs/features/link.md b/docs/features/link.md index 75789d0..5b1f70d 100644 --- a/docs/features/link.md +++ b/docs/features/link.md @@ -132,7 +132,7 @@ ClassicEditor // ... link: { decorators: { - addTargetToExternalLinks: { + openInNewTab: { mode: 'manual', label: 'Open in a new tab', attributes: { @@ -195,6 +195,15 @@ ClassicEditor attributes: { download: 'file' } + }, + openInNewTab: { + mode: 'manual', + label: 'Open in a new tab', + defaultValue: true, // This option will be selected by default. + attributes: { + target: '_blank', + rel: 'noopener noreferrer' + } } } } diff --git a/src/link.js b/src/link.js index 9c1fa77..2fa90aa 100644 --- a/src/link.js +++ b/src/link.js @@ -147,8 +147,13 @@ export default class Link extends Plugin { */ /** - * Represents a link decorator definition ({@link module:link/link~LinkDecoratorManualDefinition `'manual'`} - * or {@link module:link/link~LinkDecoratorAutomaticDefinition `'automatic'`}). + * A link decorator definition. Two types implement this defition: + * + * * {@link module:link/link~LinkDecoratorManualDefinition} + * * {@link module:link/link~LinkDecoratorAutomaticDefinition} + * + * Refer to their document for more information about available options or to the + * {@glink features/link#custom-link-attributes-decorators link feature guide} for general information. * * @interface LinkDecoratorDefinition */ @@ -203,6 +208,7 @@ export default class Link extends Plugin { * { * mode: 'manual', * label: 'Open in a new tab', + * defaultValue: true, * attributes: { * target: '_blank', * rel: 'noopener noreferrer'