Merge pull request #58 from ckeditor/t/57

Fix: Selection converter will mark only the topmost widget in case of selecting a widget with another widget nested inside it. Closes #57.
ckeditor · Nov 19, 2018 · a78efec · a78efec
2 parents 49afa6e + cf06c9f
commit a78efec
Show file tree

Hide file tree

Showing 7 changed files with 275 additions and 6 deletions.
diff --git a/src/utils.js b/src/utils.js
@@ -31,13 +31,17 @@ export const WIDGET_CLASS_NAME = 'ck-widget';
 export const WIDGET_SELECTED_CLASS_NAME = 'ck-widget_selected';
 
 /**
- * Returns `true` if given {@link module:engine/view/element~Element} is a widget.
+ * Returns `true` if given {@link module:engine/view/node~Node} is an {@link module:engine/view/element~Element} and a widget.
  *
- * @param {module:engine/view/element~Element} element
+ * @param {module:engine/view/node~Node} node
  * @returns {Boolean}
  */
-export function isWidget( element ) {
-	return !!element.getCustomProperty( widgetSymbol );
+export function isWidget( node ) {
+	if ( !node.is( 'element' ) ) {
+		return false;
+	}
+
+	return !!node.getCustomProperty( widgetSymbol );
 }
 
 /**
@@ -86,7 +90,7 @@ export function isWidget( element ) {
  * @returns {module:engine/view/element~Element} Returns the same element.
  */
 export function toWidget( element, writer, options = {} ) {
-	// The selection on Edge behaves better when the whole editor contents is in a single contentedible element.
+	// The selection on Edge behaves better when the whole editor contents is in a single contenteditable element.
 	// https://github.com/ckeditor/ckeditor5/issues/1079
 	if ( !env.isEdge ) {
 		writer.setAttribute( 'contenteditable', 'false', element );

diff --git a/src/widget.js b/src/widget.js
@@ -63,14 +63,18 @@ export default class Widget extends Plugin {
 			const viewWriter = conversionApi.writer;
 			const viewSelection = viewWriter.document.selection;
 			const selectedElement = viewSelection.getSelectedElement();
+			let lastMarked = null;
 
 			for ( const range of viewSelection.getRanges() ) {
 				for ( const value of range ) {
 					const node = value.item;
 
-					if ( node.is( 'element' ) && isWidget( node ) ) {
+					// Do not mark nested widgets in selected one. See: #57.
+					if ( isWidget( node ) && !isChild( node, lastMarked ) ) {
 						viewWriter.addClass( WIDGET_SELECTED_CLASS_NAME, node );
+
 						this._previouslySelected.add( node );
+						lastMarked = node;
 
 						// Check if widget is a single element selected.
 						if ( node == selectedElement ) {
@@ -420,3 +424,16 @@ function isInsideNestedEditable( element ) {
 
 	return false;
 }
+
+// Checks whether the specified `element` is a child of the `parent` element.
+//
+// @param {module:engine/view/element~Element} element An element to check.
+// @param {module:engine/view/element~Element|null} parent A parent for the element.
+// @returns {Boolean}
+function isChild( element, parent ) {
+	if ( !parent ) {
+		return false;
+	}
+
+	return Array.from( element.getAncestors() ).includes( parent );
+}
diff --git a/tests/manual/nested-widgets.html b/tests/manual/nested-widgets.html
@@ -0,0 +1,23 @@
+<head>
+	<style>
+		body {
+			max-width: 800px;
+			margin: 20px auto;
+		}
+
+		.ck-content .widget {
+			background: rgba( 0, 0, 0, 0.1 );
+			padding: 20px !important;
+			min-height: 50px;
+		}
+	</style>
+</head>
+<div id="editor">
+	<h2>Heading 1</h2>
+	<p>Paragraph</p>
+	<div class="widget">
+		<div class="widget"></div>
+		<div class="widget"></div>
+	</div>
+	<p>Paragraph</p>
+</div>
diff --git a/tests/manual/nested-widgets.js b/tests/manual/nested-widgets.js
@@ -0,0 +1,78 @@
+/**
+ * @license Copyright (c) 2003-2018, CKSource - Frederico Knabben. All rights reserved.
+ * For licensing, see LICENSE.md.
+ */
+
+/* globals console, window, document, setInterval */
+
+import Widget from '../../src/widget';
+import { toWidget } from '../../src/utils';
+import ClassicEditor from '@ckeditor/ckeditor5-editor-classic/src/classiceditor';
+import ArticlePluginSet from '@ckeditor/ckeditor5-core/tests/_utils/articlepluginset';
+import { downcastElementToElement } from '@ckeditor/ckeditor5-engine/src/conversion/downcast-converters';
+import { upcastElementToElement } from '@ckeditor/ckeditor5-engine/src/conversion/upcast-converters';
+import { getData as getModelData } from '@ckeditor/ckeditor5-engine/src/dev-utils/model';
+import { getData as getViewData } from '@ckeditor/ckeditor5-engine/src/dev-utils/view';
+
+function MyPlugin( editor ) {
+	editor.model.schema.register( 'div', {
+		allowIn: [ '$root', 'div' ],
+		isObject: true
+	} );
+
+	editor.model.schema.extend( '$text', {
+		allowIn: 'div'
+	} );
+
+	editor.conversion.for( 'downcast' ).add( downcastElementToElement( {
+		model: 'div',
+		view: ( modelElement, writer ) => {
+			return toWidget( writer.createContainerElement( 'div', { class: 'widget' } ), writer );
+		}
+	} ) );
+
+	editor.conversion.for( 'upcast' ).add( upcastElementToElement( {
+		model: 'div',
+		view: 'div'
+	} ) );
+}
+
+ClassicEditor
+	.create( document.querySelector( '#editor' ), {
+		plugins: [ ArticlePluginSet, Widget, MyPlugin ],
+		toolbar: [
+			'heading',
+			'|',
+			'bold',
+			'italic',
+			'link',
+			'bulletedList',
+			'numberedList',
+			'blockQuote',
+			'insertTable',
+			'mediaEmbed',
+			'undo',
+			'redo'
+		],
+		image: {
+			toolbar: [ 'imageStyle:full', 'imageStyle:side', '|', 'imageTextAlternative' ]
+		},
+		table: {
+			contentToolbar: [
+				'tableColumn',
+				'tableRow',
+				'mergeTableCells'
+			]
+		}
+	} )
+	.then( editor => {
+		window.editor = editor;
+
+		setInterval( () => {
+			console.log( getModelData( editor.model ) );
+			console.log( getViewData( editor.editing.view ) );
+		}, 3000 );
+	} )
+	.catch( err => {
+		console.error( err.stack );
+	} );
diff --git a/tests/manual/nested-widgets.md b/tests/manual/nested-widgets.md
@@ -0,0 +1,3 @@
+# Nested widgets
+
+* When selecting the most top-outer widget, nested widgets should not be selected.
diff --git a/tests/utils.js b/tests/utils.js
@@ -6,6 +6,7 @@
 /* global document */
 
 import DowncastWriter from '@ckeditor/ckeditor5-engine/src/view/downcastwriter';
+import Text from '@ckeditor/ckeditor5-engine/src/view/text';
 import ViewElement from '@ckeditor/ckeditor5-engine/src/view/element';
 import ViewEditableElement from '@ckeditor/ckeditor5-engine/src/view/editableelement';
 import ViewDocument from '@ckeditor/ckeditor5-engine/src/view/document';
@@ -143,6 +144,10 @@ describe( 'widget utils', () => {
 		it( 'should return false for non-widgetized elements', () => {
 			expect( isWidget( new ViewElement( 'p' ) ) ).to.be.false;
 		} );
+
+		it( 'should return false for text node', () => {
+			expect( isWidget( new Text( 'p' ) ) ).to.be.false;
+		} );
 	} );
 
 	describe( 'label utils', () => {

diff --git a/tests/widget.js b/tests/widget.js
@@ -1177,11 +1177,19 @@ describe( 'Widget', () => {
 
 					model.schema.register( 'widget', {
 						inheritAllFrom: '$block',
+						allowIn: 'widget',
 						isObject: true
 					} );
 					model.schema.register( 'paragraph', {
 						inheritAllFrom: '$block'
 					} );
+					model.schema.register( 'nested', {
+						allowIn: 'widget',
+						isLimit: true
+					} );
+					model.schema.extend( '$text', {
+						allowIn: 'nested'
+					} );
 
 					editor.conversion.for( 'downcast' )
 						.add( downcastElementToElement( { model: 'paragraph', view: 'p' } ) )
@@ -1192,6 +1200,10 @@ describe( 'Widget', () => {
 
 								return toWidget( widget, viewWriter, { hasSelectionHandler: true } );
 							}
+						} ) )
+						.add( downcastElementToElement( {
+							model: 'nested',
+							view: ( modelItem, viewWriter ) => viewWriter.createEditableElement( 'figcaption', { contenteditable: true } )
 						} ) );
 				} );
 		} );
@@ -1210,5 +1222,132 @@ describe( 'Widget', () => {
 
 			expect( getModelData( model ) ).to.equal( '<paragraph>bar</paragraph>[<widget></widget>]<paragraph>foo</paragraph>' );
 		} );
+
+		it( 'should select the most top-outer widget if widgets are nested', () => {
+			setModelData( model, '<widget><widget></widget><widget></widget></widget>' );
+
+			// The top-outer widget.
+			const viewWidgetSelectionHandler = viewDocument.getRoot().getChild( 0 );
+
+			const domEventDataMock = {
+				target: viewWidgetSelectionHandler,
+				preventDefault: sinon.spy()
+			};
+
+			viewDocument.fire( 'mousedown', domEventDataMock );
+
+			expect( getViewData( view ) ).to.equal(
+				'[<div class="ck-widget ck-widget_selectable ck-widget_selected" contenteditable="false">' +
+					'<div class="ck-widget ck-widget_selectable" contenteditable="false">' +
+						'<div class="ck ck-widget__selection-handler"></div>' +
+					'</div>' +
+					'<div class="ck-widget ck-widget_selectable" contenteditable="false">' +
+						'<div class="ck ck-widget__selection-handler"></div>' +
+					'</div>' +
+					'<div class="ck ck-widget__selection-handler"></div>' +
+				'</div>]'
+			);
+		} );
+
+		it( 'should select a proper widget if they are nested and multiplied', () => {
+			setModelData( model,
+				'<widget></widget>' +
+				'<widget>' +
+					'<widget></widget>' +
+					'<widget></widget>' +
+				'</widget>' +
+				'<widget></widget>'
+			);
+
+			const viewWidgetSelectionHandler = viewDocument.getRoot().getChild( 1 );
+
+			const domEventDataMock = {
+				target: viewWidgetSelectionHandler,
+				preventDefault: sinon.spy()
+			};
+
+			viewDocument.fire( 'mousedown', domEventDataMock );
+
+			expect( getViewData( view ) ).to.equal(
+				'<div class="ck-widget ck-widget_selectable" contenteditable="false">' +
+					'<div class="ck ck-widget__selection-handler"></div>' +
+				'</div>' +
+				'[<div class="ck-widget ck-widget_selectable ck-widget_selected" contenteditable="false">' +
+					'<div class="ck-widget ck-widget_selectable" contenteditable="false">' +
+						'<div class="ck ck-widget__selection-handler"></div>' +
+					'</div>' +
+					'<div class="ck-widget ck-widget_selectable" contenteditable="false">' +
+						'<div class="ck ck-widget__selection-handler"></div>' +
+					'</div>' +
+					'<div class="ck ck-widget__selection-handler"></div>' +
+				'</div>]' +
+				'<div class="ck-widget ck-widget_selectable" contenteditable="false">' +
+					'<div class="ck ck-widget__selection-handler"></div>' +
+				'</div>'
+			);
+		} );
+
+		it( 'works fine with a widget that contains more children', () => {
+			setModelData( model,
+				'<widget>' +
+					'<nested>foo bar</nested>' +
+					'<widget></widget>' +
+				'</widget>'
+			);
+
+			const viewWidgetSelectionHandler = viewDocument.getRoot().getChild( 0 );
+
+			const domEventDataMock = {
+				target: viewWidgetSelectionHandler,
+				preventDefault: sinon.spy()
+			};
+
+			viewDocument.fire( 'mousedown', domEventDataMock );
+
+			expect( getViewData( view ) ).to.equal(
+				'[<div class="ck-widget ck-widget_selectable ck-widget_selected" contenteditable="false">' +
+					'<figcaption contenteditable="true">foo bar</figcaption>' +
+					'<div class="ck-widget ck-widget_selectable" contenteditable="false">' +
+						'<div class="ck ck-widget__selection-handler"></div>' +
+					'</div>' +
+					'<div class="ck ck-widget__selection-handler"></div>' +
+				'</div>]'
+			);
+		} );
+
+		it( 'should select a proper widget for more complex structures', () => {
+			setModelData( model,
+				'<widget>' +
+					'<widget></widget>' +
+					'<widget>' +
+						'<widget></widget>' +
+					'</widget>' +
+				'</widget>'
+			);
+
+			const viewWidgetSelectionHandler = viewDocument.getRoot().getChild( 0 ).getChild( 1 );
+
+			const domEventDataMock = {
+				target: viewWidgetSelectionHandler,
+				preventDefault: sinon.spy()
+			};
+
+			viewDocument.fire( 'mousedown', domEventDataMock );
+
+			expect( getViewData( view ) ).to.equal(
+				'<div class="ck-widget ck-widget_selectable" contenteditable="false">' +
+					'<div class="ck-widget ck-widget_selectable" contenteditable="false">' +
+						'<div class="ck ck-widget__selection-handler"></div>' +
+					'</div>' +
+					'[<div class="ck-widget ck-widget_selectable ck-widget_selected" contenteditable="false">' +
+						'<div class="ck-widget ck-widget_selectable" contenteditable="false">' +
+							'<div class="ck ck-widget__selection-handler"></div>' +
+						'</div>' +
+						'<div class="ck ck-widget__selection-handler"></div>' +
+					'</div>]' +
+					'<div class="ck ck-widget__selection-handler"></div>' +
+				'</div>'
+			);
+		} );
 	} );
 } );
Original file line number	Diff line number	Diff line change
		@@ -0,0 +1,3 @@
		# Nested widgets

		* When selecting the most top-outer widget, nested widgets should not be selected.