浏览代码

Fixed issues pointed out during code review.

Szymon Kupś 8 年之前
父节点
当前提交
e12ab76d2e

+ 1 - 1
packages/ckeditor5-image/src/image/imageengine.js

@@ -54,7 +54,7 @@ export default class ImageEngine extends Plugin {
 		// Build converter from model to view for editing pipeline.
 		buildModelConverter().for( editing.modelToView )
 			.fromElement( 'image' )
-			.toElement( () => toImageWidget( createImageViewElement(), editor.t ) );
+			.toElement( () => toImageWidget( createImageViewElement(), editor.t( 'image widget' ) ) );
 
 		createImageAttributeConverter( [ editing.modelToView, data.modelToView ], 'src' );
 		createImageAttributeConverter( [ editing.modelToView, data.modelToView ], 'alt' );

+ 10 - 12
packages/ckeditor5-image/src/image/utils.js

@@ -7,7 +7,7 @@
  * @module image/image/utils
  */
 
-import { widgetize, isWidget, setFakeSelectionLabel } from '../widget/utils';
+import { widgetize, isWidget } from '../widget/utils';
 import ModelElement from '@ckeditor/ckeditor5-engine/src/model/element';
 
 const imageSymbol = Symbol( 'isImage' );
@@ -15,29 +15,27 @@ const imageSymbol = Symbol( 'isImage' );
 /**
  * Converts given {@link module:engine/view/element~Element} to image widget:
  * * adds {@link module:engine/view/element~Element#setCustomProperty custom property} allowing to recognize image widget element,
- * * sets fake selection label function,
- * * calls {@link module:image/widget/utils~widgetize widgetize}.
+ * * calls {@link module:image/widget/utils~widgetize widgetize} function with proper element's label creator.
  *
  * @param {module:engine/view/element~Element} viewElement
- * @param {function} t Shorthand for {@link module:utils/locale~Locale#t}.
+ * @param {String} label Element's label. It will be concatenated with image's `alt` attribute if one is present.
  * @returns {module:engine/view/element~Element}
  */
-export function toImageWidget( viewElement, t ) {
+export function toImageWidget( viewElement, label ) {
 	viewElement.setCustomProperty( imageSymbol, true );
 
-	setFakeSelectionLabel( viewElement, () => {
-		let fakeSelectionLabel = t( 'image widget' );
+	return widgetize( viewElement, { label: labelCreator } );
+
+	function labelCreator() {
 		const imgElement = viewElement.getChild( 0 );
 		const altText = imgElement.getAttribute( 'alt' );
 
 		if ( altText ) {
-			fakeSelectionLabel = `${ altText } ${ fakeSelectionLabel }`;
+			label = `${ altText } ${ label }`;
 		}
 
-		return fakeSelectionLabel;
-	} );
-
-	return widgetize( viewElement );
+		return label;
+	}
 }
 
 /**

+ 19 - 11
packages/ckeditor5-image/src/widget/utils.js

@@ -8,7 +8,7 @@
  */
 
 const widgetSymbol = Symbol( 'isWidget' );
-const fakeSelectionLabelSymbol = Symbol( 'fakeSelectionLabel' );
+const labelSymbol = Symbol( 'label' );
 
 /**
  * CSS class added to each widget element.
@@ -42,40 +42,48 @@ export function isWidget( element ) {
  * * adds custom property allowing to recognize widget elements by using {@link ~isWidget}.
  *
  * @param {module:engine/view/element~Element} element
+ * @param {Object} [options]
+ * @param {String|Function} [options.label] Element's label provided to {@link ~setLabel} function. It can be passed as
+ * a plain string or a function returning a string.
  * @returns {module:engine/view/element~Element} Returns same element.
  */
-export function widgetize( element ) {
+export function widgetize( element, options ) {
+	options = options || {};
 	element.setAttribute( 'contenteditable', false );
 	element.getFillerOffset = getFillerOffset;
 	element.addClass( WIDGET_CLASS_NAME );
 	element.setCustomProperty( widgetSymbol, true );
 
+	if ( options.label ) {
+		setLabel( element, options.label );
+	}
+
 	return element;
 }
 
 /**
- * Sets fake selection label for given element.
+ * Sets label for given element.
  * It can be passed as a plain string or a function returning a string. Function will be called each time label is retrieved by
- * {module:image/widget/utils~getFakeSelectionLabel}.
+ * {@link ~getLabel}.
  *
  * @param {module:engine/view/element~Element} element
  * @param {String|Function} labelOrCreator
  */
-export function setFakeSelectionLabel( element, labelOrCreator ) {
-	element.setCustomProperty( fakeSelectionLabelSymbol, labelOrCreator );
+export function setLabel( element, labelOrCreator ) {
+	element.setCustomProperty( labelSymbol, labelOrCreator );
 }
 
 /**
- * Returns fake selection label for provided element.
+ * Returns label for provided element.
  *
  * @param {module:engine/view/element~Element} element
- * @return {String|undefined}
+ * @return {String}
  */
-export function getFakeSelectionLabel( element ) {
-	const labelCreator = element.getCustomProperty( fakeSelectionLabelSymbol );
+export function getLabel( element ) {
+	const labelCreator = element.getCustomProperty( labelSymbol );
 
 	if ( !labelCreator ) {
-		return undefined;
+		return '';
 	}
 
 	return typeof labelCreator == 'function' ? labelCreator() : labelCreator;

+ 2 - 2
packages/ckeditor5-image/src/widget/widgetengine.js

@@ -8,7 +8,7 @@
  */
 
 import Plugin from '@ckeditor/ckeditor5-core/src/plugin';
-import { WIDGET_SELECTED_CLASS_NAME, isWidget, getFakeSelectionLabel } from './utils';
+import { WIDGET_SELECTED_CLASS_NAME, isWidget, getLabel } from './utils';
 
 /**
  * The widget engine plugin.
@@ -42,7 +42,7 @@ export default class WidgetEngine extends Plugin {
 				return;
 			}
 
-			viewSelection.setFake( true, { label: getFakeSelectionLabel( selectedElement ) } );
+			viewSelection.setFake( true, { label: getLabel( selectedElement ) } );
 			selectedElement.addClass( WIDGET_SELECTED_CLASS_NAME );
 			previouslySelected = selectedElement;
 		}, { priority: 'low' } );

+ 6 - 6
packages/ckeditor5-image/tests/image/utils.js

@@ -6,7 +6,7 @@
 import ViewElement from '@ckeditor/ckeditor5-engine/src/view/element';
 import ModelElement from '@ckeditor/ckeditor5-engine/src/model/element';
 import { toImageWidget, isImageWidget, isImage } from '../../src/image/utils';
-import { isWidget, getFakeSelectionLabel } from '../../src/widget/utils';
+import { isWidget, getLabel } from '../../src/widget/utils';
 
 describe( 'image widget utils', () => {
 	let element, image;
@@ -14,7 +14,7 @@ describe( 'image widget utils', () => {
 	beforeEach( () => {
 		image = new ViewElement( 'img' );
 		element = new ViewElement( 'figure', null, image );
-		toImageWidget( element, ( t ) => t );
+		toImageWidget( element, 'image widget' );
 	} );
 
 	describe( 'toImageWidget()', () => {
@@ -22,13 +22,13 @@ describe( 'image widget utils', () => {
 			expect( isWidget( element ) ).to.be.true;
 		} );
 
-		it( 'should set fake selection label', () => {
-			expect( getFakeSelectionLabel( element ) ).to.equal( 'image widget' );
+		it( 'should set element\'s label', () => {
+			expect( getLabel( element ) ).to.equal( 'image widget' );
 		} );
 
-		it( 'should set fake selection label combined with alt attribute', () => {
+		it( 'should set element\'s label combined with alt attribute', () => {
 			image.setAttribute( 'alt', 'foo bar baz' );
-			expect( getFakeSelectionLabel( element ) ).to.equal( 'foo bar baz image widget' );
+			expect( getLabel( element ) ).to.equal( 'foo bar baz image widget' );
 		} );
 	} );
 

+ 24 - 10
packages/ckeditor5-image/tests/widget/utils.js

@@ -4,7 +4,7 @@
  */
 
 import ViewElement from '@ckeditor/ckeditor5-engine/src/view/element';
-import { widgetize, isWidget, WIDGET_CLASS_NAME, setFakeSelectionLabel, getFakeSelectionLabel } from '../../src/widget/utils';
+import { widgetize, isWidget, WIDGET_CLASS_NAME, setLabel, getLabel } from '../../src/widget/utils';
 
 describe( 'widget utils', () => {
 	let element;
@@ -27,6 +27,20 @@ describe( 'widget utils', () => {
 		it( 'should add proper CSS class', () => {
 			expect( element.hasClass( WIDGET_CLASS_NAME ) ).to.be.true;
 		} );
+
+		it( 'should add element\'s label if one is provided', () => {
+			element = new ViewElement( 'div' );
+			widgetize( element, { label: 'foo bar baz label' } );
+
+			expect( getLabel( element ) ).to.equal( 'foo bar baz label' );
+		} );
+
+		it( 'should add element\'s label if one is provided as function', () => {
+			element = new ViewElement( 'div' );
+			widgetize( element, { label: () => 'foo bar baz label' } );
+
+			expect( getLabel( element ) ).to.equal( 'foo bar baz label' );
+		} );
 	} );
 
 	describe( 'isWidget()', () => {
@@ -39,28 +53,28 @@ describe( 'widget utils', () => {
 		} );
 	} );
 
-	describe( 'fake selection label utils', () => {
-		it( 'should allow to set fake selection for element', () => {
+	describe( 'label utils', () => {
+		it( 'should allow to set label for element', () => {
 			const element = new ViewElement( 'p' );
-			setFakeSelectionLabel( element, 'foo bar baz' );
+			setLabel( element, 'foo bar baz' );
 
-			expect( getFakeSelectionLabel( element ) ).to.equal( 'foo bar baz' );
+			expect( getLabel( element ) ).to.equal( 'foo bar baz' );
 		} );
 
-		it( 'should return undefined for elements without fake selection label', () => {
+		it( 'should return empty string for elements without label', () => {
 			const element = new ViewElement( 'div' );
 
-			expect( getFakeSelectionLabel( element ) ).to.be.undefined;
+			expect( getLabel( element ) ).to.equal( '' );
 		} );
 
 		it( 'should allow to use a function as label creator', () => {
 			const element = new ViewElement( 'p' );
 			let caption = 'foo';
-			setFakeSelectionLabel( element, () => caption );
+			setLabel( element, () => caption );
 
-			expect( getFakeSelectionLabel( element ) ).to.equal( 'foo' );
+			expect( getLabel( element ) ).to.equal( 'foo' );
 			caption = 'bar';
-			expect( getFakeSelectionLabel( element ) ).to.equal( 'bar' );
+			expect( getLabel( element ) ).to.equal( 'bar' );
 		} );
 	} );
 } );

+ 4 - 5
packages/ckeditor5-image/tests/widget/widgetengine.js

@@ -10,7 +10,7 @@ import { setData as setModelData } from '@ckeditor/ckeditor5-engine/src/dev-util
 import { getData as getViewData } from '@ckeditor/ckeditor5-engine/src/dev-utils/view';
 import ViewContainer from '@ckeditor/ckeditor5-engine/src/view/containerelement';
 import ViewEditable from '@ckeditor/ckeditor5-engine/src/view/editableelement';
-import { widgetize, setFakeSelectionLabel } from '../../src/widget/utils';
+import { widgetize } from '../../src/widget/utils';
 
 describe( 'WidgetEngine', () => {
 	let editor, document, viewDocument;
@@ -32,8 +32,7 @@ describe( 'WidgetEngine', () => {
 				buildModelConverter().for( editor.editing.modelToView )
 					.fromElement( 'widget' )
 					.toElement( () => {
-						const element = widgetize( new ViewContainer( 'div' ) );
-						setFakeSelectionLabel( element, 'fake selection' );
+						const element = widgetize( new ViewContainer( 'div' ), { label: 'element label' } );
 
 						return element;
 					} );
@@ -57,10 +56,10 @@ describe( 'WidgetEngine', () => {
 		expect( viewDocument.selection.isFake ).to.be.true;
 	} );
 
-	it( 'should use element\'s fake selection label if one is provided', () => {
+	it( 'should use element\'s label to set fake selection if one is provided', () => {
 		setModelData( document, '[<widget>foo bar</widget>]' );
 
-		expect( viewDocument.selection.fakeSelectionLabel ).to.equal( 'fake selection' );
+		expect( viewDocument.selection.fakeSelectionLabel ).to.equal( 'element label' );
 	} );
 
 	it( 'fake selection should be empty if widget is not selected', () => {