diff --git a/packages/ckeditor5-engine/src/controller/editingcontroller.js b/packages/ckeditor5-engine/src/controller/editingcontroller.js index 06238ab4485..987500a03f6 100644 --- a/packages/ckeditor5-engine/src/controller/editingcontroller.js +++ b/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' } ); diff --git a/packages/ckeditor5-engine/tests/conversion/downcasthelpers.js b/packages/ckeditor5-engine/tests/conversion/downcasthelpers.js index 86cbbbe6bd5..a670adc4943 100644 --- a/packages/ckeditor5-engine/tests/conversion/downcasthelpers.js +++ b/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( '
f{}oobar
' ); } ); + it( 'should merge attribute elements from previous selection with overridden selection conversion', () => { + testSelection( + [ 3, 3 ], + 'foobar', + 'foo[]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( '
f{}oobar
' ); + } ); + it( 'should do nothing if the attribute element had been already removed', () => { testSelection( [ 3, 3 ], diff --git a/packages/ckeditor5-widget/src/widget.js b/packages/ckeditor5-widget/src/widget.js index c887431376e..e3958d6c582 100644 --- a/packages/ckeditor5-widget/src/widget.js +++ b/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: + // + // [] -> [] + // [] -> [] + // + // 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 - // - // [] -> [] - // - // 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: - // [] - 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 [] for ( const value of range ) { const node = value.item; diff --git a/packages/ckeditor5-widget/tests/widget.js b/packages/ckeditor5-widget/tests/widget.js index 935dd65c8d5..7fbc6d70d6d 100644 --- a/packages/ckeditor5-widget/tests/widget.js +++ b/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, '[]' ); + + 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( + '

' + + '' + + '[]' + + '

' + ); + + 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, 'foo[]bar' ); + + expect( spy.calledOnce ).to.be.true; + expect( getViewData( view ) ).to.equal( + '

' + + 'foo' + + '' + + 'bar{}' + + '

' + ); + } ); + it( 'should not apply fake view selection when an inline widget and some other content is surrounded by an attribute element', () => { setModelData( model, 'foo [<$text attr="foo">bar]' ); expect( getViewData( view ) ).to.equal( - '

foo ' + '{' + 'bar' + @@ -288,7 +346,6 @@ describe( 'Widget', () => { setModelData( model, 'foo [] bar' ); expect( getViewData( view ) ).to.equal( - '

foo ' + '{' + '

' + @@ -311,7 +368,6 @@ describe( 'Widget', () => { expect( viewDocument.selection.isFake ).to.be.false; expect( getViewData( view ) ).to.equal( - '

{foo

' + '
' + '' +