Merge pull request #9374 from ckeditor/i/9372

Fix (clipboard): Selection was stuck and unable to change in read-only mode. Closes #9372. Fix (clipboard): Nested editable should not be dragged. Closes #9370.
ckeditor · Apr 6, 2021 · 5735af2 · 5735af2
2 parents fa69320 + f46e02b
commit 5735af2
Show file tree

Hide file tree

Showing 2 changed files with 86 additions and 3 deletions.
diff --git a/packages/ckeditor5-clipboard/src/dragdrop.js b/packages/ckeditor5-clipboard/src/dragdrop.js
@@ -242,7 +242,7 @@ export default class DragDrop extends Plugin {
 			const selection = modelDocument.selection;
 
 			// Don't drag the editable element itself.
-			if ( data.target && data.target.is( 'rootElement' ) ) {
+			if ( data.target && data.target.is( 'editableElement' ) ) {
 				data.preventDefault();
 
 				return;
@@ -470,7 +470,9 @@ export default class DragDrop extends Plugin {
 			// If this was not a widget then we should check if we need to drag some text content.
 			// In Chrome set a 'draggable' attribute on closest editable to allow immediate dragging of the selected text range.
 			// In Firefox this is not needed. In Safari it makes the whole editable draggable (not just textual content).
-			if ( env.isBlink && !draggableElement && !viewDocument.selection.isCollapsed ) {
+			// Disabled in read-only mode because draggable="true" + contenteditable="false" results
+			// in not firing selectionchange event ever, which makes the selection stuck in read-only mode.
+			if ( env.isBlink && !editor.isReadOnly && !draggableElement && !viewDocument.selection.isCollapsed ) {
 				const selectedElement = viewDocument.selection.getSelectedElement();
 
 				if ( !selectedElement || !isWidget( selectedElement ) ) {

diff --git a/packages/ckeditor5-clipboard/tests/dragdrop.js b/packages/ckeditor5-clipboard/tests/dragdrop.js
@@ -769,6 +769,38 @@ describe( 'Drag and Drop', () => {
 				expect( spyClipboardOutput.notCalled ).to.be.true;
 			} );
 
+			it( 'should not start dragging if the editable would be dragged itself', () => {
+				setModelData( model, '<table><tableRow><tableCell><paragraph>[foo]bar</paragraph></tableCell></tableRow></table>' );
+
+				const dataTransferMock = createDataTransfer();
+				const spyClipboardOutput = sinon.spy();
+				const spyPreventDefault = sinon.spy();
+
+				viewDocument.on( 'clipboardOutput', ( event, data ) => spyClipboardOutput( data ) );
+
+				const modelElement = root.getNodeByPath( [ 0, 0, 0 ] );
+				const eventData = prepareEventData( model.createPositionAt( modelElement.getChild( 0 ), 3 ) );
+				eventData.target = mapper.toViewElement( modelElement );
+				eventData.domTarget = domConverter.mapViewToDom( eventData.target );
+
+				expect( eventData.target.is( 'editableElement', 'td' ) ).to.be.true;
+
+				viewDocument.fire( 'mousedown', {
+					...eventData
+				} );
+
+				viewDocument.fire( 'dragstart', {
+					...eventData,
+					dataTransfer: dataTransferMock,
+					preventDefault: spyPreventDefault,
+					stopPropagation: () => {
+					}
+				} );
+
+				expect( spyPreventDefault.called ).to.be.true;
+				expect( spyClipboardOutput.notCalled ).to.be.true;
+			} );
+
 			it( 'should mark allowed effect as "copy" if the editor is read-only', () => {
 				setModelData( model, '<paragraph>[foo]bar</paragraph>' );
 
@@ -781,7 +813,7 @@ describe( 'Drag and Drop', () => {
 
 				fireDragStart( dataTransferMock );
 
-				expect( viewDocument.getRoot().hasAttribute( 'draggable' ) ).to.be.true;
+				expect( viewDocument.getRoot().hasAttribute( 'draggable' ) ).to.be.false;
 				expect( dataTransferMock.effectAllowed ).to.equal( 'copy' );
 			} );
 
@@ -851,6 +883,55 @@ describe( 'Drag and Drop', () => {
 				);
 			} );
 
+			it( 'should start dragging by grabbing the widget selection handle (in read only mode)', () => {
+				setModelData( model,
+					'<paragraph>[]foobar</paragraph>' +
+					'<table><tableRow><tableCell><paragraph>abc</paragraph></tableCell></tableRow></table>'
+				);
+
+				editor.isReadOnly = true;
+
+				const dataTransferMock = createDataTransfer();
+				const spyClipboardOutput = sinon.spy();
+
+				viewDocument.on( 'clipboardOutput', ( event, data ) => spyClipboardOutput( data ) );
+
+				const domNode = view.getDomRoot().querySelector( '.ck-widget__selection-handle' );
+				const widgetViewElement = viewDocument.getRoot().getChild( 1 );
+				const selectionHandleElement = widgetViewElement.getChild( 0 );
+
+				expect( selectionHandleElement.hasClass( 'ck-widget__selection-handle' ) ).to.be.true;
+
+				const eventData = {
+					domTarget: domNode,
+					target: selectionHandleElement,
+					domEvent: {}
+				};
+
+				viewDocument.fire( 'mousedown', {
+					...eventData
+				} );
+
+				viewDocument.fire( 'dragstart', {
+					...eventData,
+					dataTransfer: dataTransferMock,
+					stopPropagation: () => {}
+				} );
+
+				expect( dataTransferMock.getData( 'text/html' ) ).to.equal(
+					'<figure class="table"><table><tbody><tr><td>abc</td></tr></tbody></table></figure>'
+				);
+
+				expect( widgetViewElement.getAttribute( 'draggable' ) ).to.equal( 'true' );
+
+				expect( spyClipboardOutput.called ).to.be.true;
+				expect( spyClipboardOutput.firstCall.firstArg.method ).to.equal( 'dragstart' );
+				expect( spyClipboardOutput.firstCall.firstArg.dataTransfer ).to.equal( dataTransferMock );
+				expect( stringifyView( spyClipboardOutput.firstCall.firstArg.content ) ).to.equal(
+					'<figure class="table"><table><tbody><tr><td>abc</td></tr></tbody></table></figure>'
+				);
+			} );
+
 			it( 'should start dragging by grabbing the widget element directly', () => {
 				setModelData( model,
 					'<paragraph>[]foobar</paragraph>' +