Bladeren bron

Changed logic of link manual decorator's defaultValue.

Kuba Niegowski 5 jaren geleden
bovenliggende
commit
a74d4a0bd7

+ 1 - 1
packages/ckeditor5-link/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.
  */

+ 20 - 29
packages/ckeditor5-link/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 );
 	}
 }

+ 0 - 37
packages/ckeditor5-link/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' } );
-				} );
-			}
 		} );
 	}
 

+ 1 - 1
packages/ckeditor5-link/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' );
 

+ 11 - 11
packages/ckeditor5-link/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 );

+ 2 - 2
packages/ckeditor5-link/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.

+ 2 - 26
packages/ckeditor5-link/tests/linkcommand.js

@@ -332,14 +332,6 @@ describe( 'LinkCommand', () => {
 
 				expect( getData( model ) ).to.equal( 'foo[<$text linkHref="url">url</$text>]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[]rl</$text>bar' );
-
-				command.execute( 'url', { linkIsFoo: false, linkIsBar: false, linkIsSth: false } );
-
-				expect( getData( model ) ).to.equal( 'foo[<$text linkHref="url" linkIsSth="false">url</$text>]bar' );
-			} );
 		} );
 
 		describe( 'range selection', () => {
@@ -368,14 +360,6 @@ describe( 'LinkCommand', () => {
 
 				expect( getData( model ) ).to.equal( 'foo[<$text linkHref="url">url</$text>]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</$text>]bar' );
-
-				command.execute( 'url', { linkIsFoo: false, linkIsBar: false, linkIsSth: false } );
-
-				expect( getData( model ) ).to.equal( 'foo[<$text linkHref="url" linkIsSth="false">url</$text>]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</$text>]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</$text>]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;
 			} );
 		} );
 	} );

+ 0 - 40
packages/ckeditor5-link/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: '<p><a href="url" target="_blank" rel="noopener noreferrer" download="file">Foo</a>' +
-							'<a href="example.com" download="file">Bar</a></p>',
-						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(
-							'<paragraph>' +
-							'<$text linkHref="url" linkIsDownloadable="true" linkIsExternal="true">Foo</$text>' +
-							'<$text linkHref="example.com" linkIsDownloadable="true" linkIsExternal="false">Bar</$text>' +
-							'</paragraph>'
-						);
-					} );
-			} );
-
 			it( 'should not upcast partial and incorrect attributes', () => {
 				return VirtualTestEditor
 					.create( {

+ 105 - 11
packages/ckeditor5-link/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' ) );
 				} );
 		} );
 

+ 1 - 1
packages/ckeditor5-link/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', () => {