#9580: Safeguarded the way widget plugin sets the fake selection

packages/ckeditor5-engine/src/controller/editingcontroller.js

@@ -104,7 +104,7 @@ export default class EditingController {
 		this.downcastDispatcher.on( 'remove', remove(), { priority: 'low' } );
 
 		// Attach default model selection converters.
-		this.downcastDispatcher.on( 'selection', clearAttributes(), { priority: 'low' } );
+		this.downcastDispatcher.on( 'selection', clearAttributes(), { priority: 'high' } );
 		this.downcastDispatcher.on( 'selection', convertRangeSelection(), { priority: 'low' } );
 		this.downcastDispatcher.on( 'selection', convertCollapsedSelection(), { priority: 'low' } );
 

packages/ckeditor5-engine/tests/conversion/downcasthelpers.js

@@ -3458,7 +3458,7 @@ describe( 'downcast selection converters', () => {
 		downcastHelpers.markerToHighlight( { model: 'marker', view: { classes: 'marker' }, converterPriority: 1 } );
 
 		// Default selection converters.
-		dispatcher.on( 'selection', clearAttributes(), { priority: 'low' } );
+		dispatcher.on( 'selection', clearAttributes(), { priority: 'high' } );
 		dispatcher.on( 'selection', convertRangeSelection(), { priority: 'low' } );
 		dispatcher.on( 'selection', convertCollapsedSelection(), { priority: 'low' } );
 	} );
@@ -3854,6 +3854,50 @@ describe( 'downcast selection converters', () => {
 				expect( viewString ).to.equal( '<div>f{}oobar</div>' );
 			} );
 
+			it( 'should merge attribute elements from previous selection with overridden selection conversion', () => {
+				testSelection(
+					[ 3, 3 ],
+					'foobar',
+					'foo<strong>[]</strong>bar',
+					{ bold: 'true' }
+				);
+
+				const spy = sinon.spy();
+
+				dispatcher.on( 'selection', ( evt, data, conversionApi ) => {
+					const selection = data.selection;
+
+					if ( !conversionApi.consumable.consume( selection, 'selection' ) ) {
+						return;
+					}
+
+					const viewRanges = [];
+
+					for ( const range of selection.getRanges() ) {
+						viewRanges.push( conversionApi.mapper.toViewRange( range ) );
+					}
+
+					conversionApi.writer.setSelection( viewRanges, { backward: selection.isBackward } );
+
+					spy();
+				} );
+
+				view.change( writer => {
+					const modelRange = model.createRange( model.createPositionAt( modelRoot, 1 ), model.createPositionAt( modelRoot, 1 ) );
+					model.change( writer => {
+						writer.setSelection( modelRange );
+					} );
+
+					dispatcher.convertSelection( modelDoc.selection, model.markers, writer );
+				} );
+
+				expect( spy.calledOnce ).to.be.true;
+				expect( viewSelection.rangeCount ).to.equal( 1 );
+
+				const viewString = stringifyView( viewRoot, viewSelection, { showType: false } );
+				expect( viewString ).to.equal( '<div>f{}oobar</div>' );
+			} );
+
 			it( 'should do nothing if the attribute element had been already removed', () => {
 				testSelection(
 					[ 3, 3 ],

packages/ckeditor5-widget/src/widget.js

@@ -52,7 +52,8 @@ export default class Widget extends Plugin {
 	 * @inheritDoc
 	 */
 	init() {
-		const view = this.editor.editing.view;
+		const editor = this.editor;
+		const view = editor.editing.view;
 		const viewDocument = view.document;
 
 		/**
@@ -64,42 +65,69 @@ export default class Widget extends Plugin {
 		this._previouslySelected = new Set();
 
 		// Model to view selection converter.
-		// Converts selection placed over widget element to fake selection
+		// Converts selection placed over widget element to fake selection.
+		//
+		// By default, the selection is downcasted by the engine to surround the attribute element, even though its only
+		// child is an inline widget. A similar thing also happens when a collapsed marker is rendered as a UI element
+		// next to an inline widget: the view selection contains both the widget and the marker.
+		//
+		// This prevents creating a correct fake selection when this inline widget is selected. Normalize the selection
+		// in these cases based on the model:
+		//
+		//		[<attributeElement><inlineWidget /></attributeElement>] -> <attributeElement>[<inlineWidget />]</attributeElement>
+		//		[<uiElement></uiElement><inlineWidget />] -> <uiElement></uiElement>[<inlineWidget />]
+		//
+		// Thanks to this:
+		//
+		// * fake selection can be set correctly,
+		// * any logic depending on (View)Selection#getSelectedElement() also works OK.
+		//
+		// See https://github.com/ckeditor/ckeditor5/issues/9524.
+		this.editor.editing.downcastDispatcher.on( 'selection', ( evt, data, conversionApi ) => {
+			const viewWriter = conversionApi.writer;
+			const modelSelection = data.selection;
+
+			// The collapsed selection can't contain any widget.
+			if ( modelSelection.isCollapsed ) {
+				return;
+			}
+
+			const selectedModelElement = modelSelection.getSelectedElement();
+
+			if ( !selectedModelElement ) {
+				return;
+			}
+
+			const selectedViewElement = editor.editing.mapper.toViewElement( selectedModelElement );
+
+			if ( !isWidget( selectedViewElement ) ) {
+				return;
+			}
+
+			if ( !conversionApi.consumable.consume( modelSelection, 'selection' ) ) {
+				return;
+			}
+
+			viewWriter.setSelection( viewWriter.createRangeOn( selectedViewElement ), {
+				fake: true,
+				label: getLabel( selectedViewElement )
+			} );
+		} );
+
+		// Mark all widgets inside the selection with the css class.
+		// This handler is registered at the 'low' priority so it's triggered after the real selection conversion.
 		this.editor.editing.downcastDispatcher.on( 'selection', ( evt, data, conversionApi ) => {
 			// Remove selected class from previously selected widgets.
 			this._clearPreviouslySelectedWidgets( conversionApi.writer );
 
 			const viewWriter = conversionApi.writer;
 			const viewSelection = viewWriter.document.selection;
-			let selectedElement = viewSelection.getSelectedElement();
-			let lastMarked = null;
 
-			// By default, the selection is downcasted by the engine to surround the attribute element, even though its only
-			// child is an inline widget. This prevents creating a correct fake selection when this inline widget is selected.
-			// Normalize the selection in this case
-			//
-			//		[<attributeElement><inlineWidget /></attributeElement>] -> <attributeElement>[<inlineWidget />]</attributeElement>
-			//
-			// Thanks to this:
-			//
-			// * fake selection can be set correctly,
-			// * any logic depending on (View)Selection#getSelectedElement() also works OK.
-			//
-			// See https://github.com/ckeditor/ckeditor5/issues/9524.
-			if ( selectedElement ) {
-				// Trim the range first because the selection could be on a couple of nested attributes enclosing the widget:
-				// [<attributeElementA><attributeElementB><inlineWidget /></attributeElementB></attributeElementA>]
-				selectedElement = viewWriter.createRangeOn( selectedElement ).getTrimmed().getContainedElement();
-
-				if ( selectedElement && isWidget( selectedElement ) ) {
-					viewWriter.setSelection( viewWriter.createRangeOn( selectedElement ), {
-						fake: true,
-						label: getLabel( selectedElement )
-					} );
-				}
-			}
+			let lastMarked = null;
 
 			for ( const range of viewSelection.getRanges() ) {
+				// Note: There could be multiple selected widgets in a range but no fake selection.
+				// All of them must be marked as selected, for instance [<widget></widget><widget></widget>]
 				for ( const value of range ) {
 					const node = value.item;
 

packages/ckeditor5-widget/tests/widget.js

@@ -269,11 +269,69 @@ describe( 'Widget', () => {
 		expect( viewDocument.selection.isFake ).to.be.true;
 	} );
 
+	it( 'should apply fake view selection when the model selection surrounds the inline widget and an UI element', () => {
+		setModelData( model, '<paragraph>[]<inline-widget></inline-widget></paragraph>' );
+
+		editor.conversion.for( 'editingDowncast' ).markerToElement( {
+			model: 'testMarker',
+			view: ( data, { writer } ) => writer.createUIElement( 'span', { class: 'ui' } )
+		} );
+
+		model.change( writer => {
+			writer.addMarker( 'testMarker', {
+				range: writer.createRange( writer.createPositionAt( model.document.getRoot().getChild( 0 ), 0 ) ),
+				usingOperation: true
+			} );
+
+			writer.setSelection( model.document.getRoot().getChild( 0 ), 'in' );
+		} );
+
+		expect( getViewData( view ) ).to.equal(
+			'<p>' +
+				'<span class="ui"></span>' +
+				'[<span class="ck-widget ck-widget_selected" contenteditable="false"></span>]' +
+			'</p>'
+		);
+
+		expect( viewDocument.selection.isFake ).to.be.true;
+	} );
+
+	it( 'should allow overriding the selection downcast', () => {
+		const spy = sinon.spy();
+
+		editor.conversion.for( 'editingDowncast' ).add(
+			dispatcher => dispatcher.on( 'selection', ( evt, data, conversionApi ) => {
+				const selection = data.selection;
+
+				if ( !conversionApi.consumable.consume( selection, 'selection' ) ) {
+					return;
+				}
+
+				const position = model.createPositionAt( selection.getFirstPosition().findAncestor( 'paragraph' ), 'end' );
+				const viewPosition = conversionApi.mapper.toViewPosition( position );
+
+				conversionApi.writer.setSelection( viewPosition );
+
+				spy();
+			}, { priority: 'high' } )
+		);
+
+		setModelData( model, '<paragraph>foo[<inline-widget></inline-widget>]bar</paragraph>' );
+
+		expect( spy.calledOnce ).to.be.true;
+		expect( getViewData( view ) ).to.equal(
+			'<p>' +
+				'foo' +
+				'<span class="ck-widget" contenteditable="false"></span>' +
+				'bar{}' +
+			'</p>'
+		);
+	} );
+
 	it( 'should not apply fake view selection when an inline widget and some other content is surrounded by an attribute element', () => {
 		setModelData( model, '<paragraph>foo [<inline-widget attr="foo"></inline-widget><$text attr="foo">bar]</$text></paragraph>' );
 
 		expect( getViewData( view ) ).to.equal(
-
 			'<p>foo ' +
 				'{<attr value="foo">' +
 					'<span class="ck-widget ck-widget_selected" contenteditable="false"></span>bar' +
@@ -288,7 +346,6 @@ describe( 'Widget', () => {
 		setModelData( model, '<paragraph>foo [<inline attr="foo"></inline>] bar</paragraph>' );
 
 		expect( getViewData( view ) ).to.equal(
-
 			'<p>foo ' +
 				'{<attr value="foo">' +
 					'<figure></figure>' +
@@ -311,7 +368,6 @@ describe( 'Widget', () => {
 
 		expect( viewDocument.selection.isFake ).to.be.false;
 		expect( getViewData( view ) ).to.equal(
-
 			'<p>{foo</p>' +
 			'<div class="ck-widget ck-widget_selected" contenteditable="false">' +
 				'<b></b>' +