Procházet zdrojové kódy

Link selection attributes should be cleared after inserting a link via insertContent() for better UX.

Aleksander Nowodzinski před 5 roky
rodič
revize
18fbee4434

+ 97 - 0
packages/ckeditor5-link/src/linkediting.js

@@ -101,6 +101,9 @@ export default class LinkEditing extends Plugin {
 
 		// Setup highlight over selected link.
 		this._setupLinkHighlight();
+
+		// Change the attributes of the selection in certain situations after the link was inserted into the document.
+		this._enableInsertContentSelectionAttributesFixer();
 	}
 
 	/**
@@ -250,4 +253,98 @@ export default class LinkEditing extends Plugin {
 			}
 		} );
 	}
+
+	/**
+	 * Starts listening to {@link module:engine/model/model~Model#event:insertContent} and corrects the model
+	 * selection attributes if the selection is at the end of a link after inserting the content.
+	 *
+	 * The purpose of this action is to improve the overall UX because the user is no longer "trapped" by the
+	 * `linkHref` attribute of the selection and they can type a "clean" (`linkHref`–less) text right away.
+	 *
+	 * See https://github.com/ckeditor/ckeditor5/issues/6053.
+	 *
+	 * @private
+	 */
+	_enableInsertContentSelectionAttributesFixer() {
+		const editor = this.editor;
+		const model = editor.model;
+		const selection = model.document.selection;
+
+		model.on( 'insertContent', () => {
+			const nodeBefore = selection.anchor.nodeBefore;
+			const nodeAfter = selection.anchor.nodeAfter;
+
+			// NOTE: ↰ and ↱ represent the gravity of the selection.
+
+			// The only truly valid case is:
+			//
+			//		                                 ↰
+			//		...<$text linkHref="foo">INSERTED[]</$text>
+			//
+			// If the selection is not "trapped" by the `linkHref` attribute after inserting, there's nothing
+			// to fix there.
+			if ( !selection.hasAttribute( 'linkHref' ) ) {
+				return;
+			}
+
+			// Filter out the following case where a link with the same href (e.g. <a href="foo">INSERTED</a>) is inserted
+			// in the middle of an existing link:
+			//
+			// Before insertion:
+			//		                       ↰
+			//		<$text linkHref="foo">l[]ink</$text>
+			//
+			// Expected after insertion:
+			//		                               ↰
+			//		<$text linkHref="foo">lINSERTED[]ink</$text>
+			//
+			if ( !nodeBefore ) {
+				return;
+			}
+
+			// Filter out the following case where the selection has the "linkHref" attribute because the
+			// gravity is overridden and some text with another attribute (e.g. <b>INSERTED</b>) is inserted:
+			//
+			// Before insertion:
+			//
+			//		                       ↱
+			//		<$text linkHref="foo">[]link</$text>
+			//
+			// Expected after insertion:
+			//
+			//		                                                          ↱
+			//		<$text bold="true">INSERTED</$text><$text linkHref="foo">[]link</$text>
+			//
+			if ( !nodeBefore.hasAttribute( 'linkHref' ) ) {
+				return;
+			}
+
+			// Filter out the following case where a link is a inserted in the middle (or before) another link
+			// (different URLs, so they will not merge). In this (let's say weird) case, we can leave the selection
+			// attributes as they are because the user will end up writing in one link or another anyway.
+			//
+			// Before insertion:
+			//
+			//		                       ↰
+			//		<$text linkHref="foo">l[]ink</$text>
+			//
+			// Expected after insertion:
+			//
+			//		                                                             ↰
+			//		<$text linkHref="foo">l</$text><$text linkHref="bar">INSERTED[]</$text><$text linkHref="foo">ink</$text>
+			//
+			if ( nodeAfter && nodeAfter.hasAttribute( 'linkHref' ) ) {
+				return;
+			}
+
+			// Make the selection free of link-related model attributes.
+			// All link-related model attributes start with "link". That includes not only "linkHref"
+			// but also all decorator attributes (they have dynamic names).
+			model.change( writer => {
+				[ ...model.document.selection.getAttributeKeys() ]
+					.filter( name => name.startsWith( 'link' ) )
+					.forEach( name => writer.removeSelectionAttribute( name ) );
+			} );
+		}, { priority: 'low' } );
+	}
 }

+ 239 - 84
packages/ckeditor5-link/tests/linkediting.js

@@ -7,7 +7,7 @@ import LinkEditing from '../src/linkediting';
 import LinkCommand from '../src/linkcommand';
 import UnlinkCommand from '../src/unlinkcommand';
 
-import VirtualTestEditor from '@ckeditor/ckeditor5-core/tests/_utils/virtualtesteditor';
+import ClassicTestEditor from '@ckeditor/ckeditor5-core/tests/_utils/classictesteditor';
 import Paragraph from '@ckeditor/ckeditor5-paragraph/src/paragraph';
 import Enter from '@ckeditor/ckeditor5-enter/src/enter';
 import Plugin from '@ckeditor/ckeditor5-core/src/plugin';
@@ -19,22 +19,43 @@ import { keyCodes } from '@ckeditor/ckeditor5-utils/src/keyboard';
 /* global document */
 
 describe( 'LinkEditing', () => {
-	let editor, model, view;
-
-	beforeEach( () => {
-		return VirtualTestEditor
-			.create( {
-				plugins: [ Paragraph, LinkEditing, Enter ]
-			} )
-			.then( newEditor => {
-				editor = newEditor;
-				model = editor.model;
-				view = editor.editing.view;
-			} );
+	let element, editor, model, view;
+
+	beforeEach( async () => {
+		element = document.createElement( 'div' );
+		document.body.appendChild( element );
+
+		editor = await ClassicTestEditor.create( element, {
+			plugins: [ Paragraph, LinkEditing, Enter ],
+			link: {
+				decorators: {
+					isExternal: {
+						mode: 'manual',
+						label: 'Open in a new window',
+						attributes: {
+							target: '_blank',
+							rel: 'noopener noreferrer'
+						}
+					}
+				}
+			}
+		} );
+
+		editor.model.schema.extend( '$text', { allowAttributes: 'bold' } );
+
+		editor.conversion.attributeToElement( {
+			model: 'bold',
+			view: 'b'
+		} );
+
+		model = editor.model;
+		view = editor.editing.view;
 	} );
 
-	afterEach( () => {
-		editor.destroy();
+	afterEach( async () => {
+		element.remove();
+
+		await editor.destroy();
 	} );
 
 	it( 'should have pluginName', () => {
@@ -73,8 +94,8 @@ describe( 'LinkEditing', () => {
 		} );
 
 		it( 'should be bound to th `linkHref` attribute (RTL)', () => {
-			return VirtualTestEditor
-				.create( {
+			return ClassicTestEditor
+				.create( element, {
 					plugins: [ Paragraph, LinkEditing, Enter ],
 					language: {
 						content: 'ar'
@@ -104,6 +125,130 @@ describe( 'LinkEditing', () => {
 		} );
 	} );
 
+	// https://github.com/ckeditor/ckeditor5/issues/6053
+	describe( 'selection attribute management on paste', () => {
+		it( 'should remove link atttributes when pasting a link', () => {
+			setModelData( model, '<paragraph>foo[]</paragraph>' );
+
+			model.change( writer => {
+				model.insertContent( writer.createText( 'INSERTED', { linkHref: 'ckeditor.com' } ) );
+			} );
+
+			expect( getModelData( model ) ).to.equal( '<paragraph>foo<$text linkHref="ckeditor.com">INSERTED</$text>[]</paragraph>' );
+
+			expect( [ ...model.document.selection.getAttributeKeys() ] ).to.be.empty;
+		} );
+
+		it( 'should remove all atttributes starting with "link" (e.g. decorator attributes) when pasting a link', () => {
+			setModelData( model, '<paragraph>foo[]</paragraph>' );
+
+			model.change( writer => {
+				model.insertContent( writer.createText( 'INSERTED', { linkHref: 'ckeditor.com', linkIsExternal: true } ) );
+			} );
+
+			expect( getModelData( model ) ).to.equal(
+				'<paragraph>' +
+					'foo<$text linkHref="ckeditor.com" linkIsExternal="true">INSERTED</$text>[]' +
+				'</paragraph>'
+			);
+
+			expect( [ ...model.document.selection.getAttributeKeys() ] ).to.be.empty;
+		} );
+
+		it( 'should not remove link atttributes when pasting a non-link content', () => {
+			setModelData( model, '<paragraph><$text linkHref="ckeditor.com">foo[]</$text></paragraph>' );
+
+			model.change( writer => {
+				model.insertContent( writer.createText( 'INSERTED', { bold: 'true' } ) );
+			} );
+
+			expect( getModelData( model ) ).to.equal(
+				'<paragraph>' +
+					'<$text linkHref="ckeditor.com">foo</$text>' +
+					'<$text bold="true">INSERTED[]</$text>' +
+				'</paragraph>'
+			);
+
+			expect( [ ...model.document.selection.getAttributeKeys() ] ).to.have.members( [ 'bold' ] );
+		} );
+
+		it( 'should not remove link atttributes when pasting in the middle of a link with the same URL', () => {
+			setModelData( model, '<paragraph><$text linkHref="ckeditor.com">fo[]o</$text></paragraph>' );
+
+			model.change( writer => {
+				model.insertContent( writer.createText( 'INSERTED', { linkHref: 'ckeditor.com' } ) );
+			} );
+
+			expect( getModelData( model ) ).to.equal( '<paragraph><$text linkHref="ckeditor.com">foINSERTED[]o</$text></paragraph>' );
+			expect( [ ...model.document.selection.getAttributeKeys() ] ).to.have.members( [ 'linkHref' ] );
+		} );
+
+		it( 'should not remove link atttributes from the selection when pasting before a link when the gravity is overridden', () => {
+			setModelData( model, '<paragraph>foo[]<$text linkHref="ckeditor.com">bar</$text></paragraph>' );
+
+			view.document.fire( 'keydown', {
+				keyCode: keyCodes.arrowright,
+				preventDefault: () => {},
+				domTarget: document.body
+			} );
+
+			expect( model.document.selection.isGravityOverridden ).to.be.true;
+
+			model.change( writer => {
+				model.insertContent( writer.createText( 'INSERTED', { bold: true } ) );
+			} );
+
+			expect( getModelData( model ) ).to.equal(
+				'<paragraph>' +
+					'foo' +
+					'<$text bold="true">INSERTED</$text>' +
+					'<$text linkHref="ckeditor.com">[]bar</$text>' +
+				'</paragraph>'
+			);
+
+			expect( model.document.selection.isGravityOverridden ).to.be.true;
+			expect( [ ...model.document.selection.getAttributeKeys() ] ).to.have.members( [ 'linkHref' ] );
+		} );
+
+		it( 'should not remove link atttributes when pasting a link into another link (different URLs, no merge)', () => {
+			setModelData( model, '<paragraph><$text linkHref="ckeditor.com">f[]oo</$text></paragraph>' );
+
+			model.change( writer => {
+				model.insertContent( writer.createText( 'INSERTED', { linkHref: 'http://INSERTED' } ) );
+			} );
+
+			expect( getModelData( model ) ).to.equal(
+				'<paragraph>' +
+					'<$text linkHref="ckeditor.com">f</$text>' +
+					'<$text linkHref="http://INSERTED">INSERTED[]</$text>' +
+					'<$text linkHref="ckeditor.com">oo</$text>' +
+				'</paragraph>'
+			);
+
+			expect( [ ...model.document.selection.getAttributeKeys() ] ).to.have.members( [ 'linkHref' ] );
+		} );
+
+		it( 'should not remove link atttributes when pasting before another link (different URLs, no merge)', () => {
+			setModelData( model, '<paragraph>[]<$text linkHref="ckeditor.com">foo</$text></paragraph>' );
+
+			expect( model.document.selection.isGravityOverridden ).to.be.false;
+
+			model.change( writer => {
+				model.insertContent( writer.createText( 'INSERTED', { linkHref: 'http://INSERTED' } ) );
+			} );
+
+			expect( getModelData( model ) ).to.equal(
+				'<paragraph>' +
+					'<$text linkHref="http://INSERTED">INSERTED[]</$text>' +
+					'<$text linkHref="ckeditor.com">foo</$text>' +
+				'</paragraph>'
+			);
+
+			expect( [ ...model.document.selection.getAttributeKeys() ] ).to.have.members( [ 'linkHref' ] );
+			expect( model.document.selection.getAttribute( 'linkHref' ) ).to.equal( 'http://INSERTED' );
+		} );
+	} );
+
 	describe( 'command', () => {
 		it( 'should register link command', () => {
 			const command = editor.commands.get( 'link' );
@@ -434,23 +579,21 @@ describe( 'LinkEditing', () => {
 
 			describe( 'for link.addTargetToExternalLinks = false', () => {
 				let editor, model;
-				beforeEach( () => {
-					return VirtualTestEditor
-						.create( {
-							plugins: [ Paragraph, LinkEditing, Enter ],
-							link: {
-								addTargetToExternalLinks: true
-							}
-						} )
-						.then( newEditor => {
-							editor = newEditor;
-							model = editor.model;
-							view = editor.editing.view;
-						} );
+
+				beforeEach( async () => {
+					editor = await ClassicTestEditor.create( element, {
+						plugins: [ Paragraph, LinkEditing, Enter ],
+						link: {
+							addTargetToExternalLinks: true
+						}
+					} );
+
+					model = editor.model;
+					view = editor.editing.view;
 				} );
 
-				afterEach( () => {
-					editor.destroy();
+				afterEach( async () => {
+					await editor.destroy();
 				} );
 
 				it( 'link.addTargetToExternalLinks is set as true value', () => {
@@ -510,43 +653,41 @@ describe( 'LinkEditing', () => {
 					}
 				];
 
-				beforeEach( () => {
-					editor.destroy();
-					return VirtualTestEditor
-						.create( {
-							plugins: [ Paragraph, LinkEditing, Enter ],
-							link: {
-								addTargetToExternalLinks: false,
-								decorators: {
-									isExternal: {
-										mode: 'automatic',
-										callback: url => url.startsWith( 'http' ),
-										attributes: {
-											target: '_blank'
-										}
-									},
-									isDownloadable: {
-										mode: 'automatic',
-										callback: url => url.includes( 'download' ),
-										attributes: {
-											download: 'download'
-										}
-									},
-									isMail: {
-										mode: 'automatic',
-										callback: url => url.startsWith( 'mailto:' ),
-										attributes: {
-											class: 'mail-url'
-										}
+				beforeEach( async () => {
+					await editor.destroy();
+
+					editor = await ClassicTestEditor.create( element, {
+						plugins: [ Paragraph, LinkEditing, Enter ],
+						link: {
+							addTargetToExternalLinks: false,
+							decorators: {
+								isExternal: {
+									mode: 'automatic',
+									callback: url => url.startsWith( 'http' ),
+									attributes: {
+										target: '_blank'
+									}
+								},
+								isDownloadable: {
+									mode: 'automatic',
+									callback: url => url.includes( 'download' ),
+									attributes: {
+										download: 'download'
+									}
+								},
+								isMail: {
+									mode: 'automatic',
+									callback: url => url.startsWith( 'mailto:' ),
+									attributes: {
+										class: 'mail-url'
 									}
 								}
 							}
-						} )
-						.then( newEditor => {
-							editor = newEditor;
-							model = editor.model;
-							view = editor.editing.view;
-						} );
+						}
+					} );
+
+					model = editor.model;
+					view = editor.editing.view;
 				} );
 
 				testLinks.forEach( link => {
@@ -574,7 +715,7 @@ describe( 'LinkEditing', () => {
 		} );
 
 		describe( 'custom linkHref converter', () => {
-			beforeEach( () => {
+			beforeEach( async () => {
 				class CustomLinks extends Plugin {
 					init() {
 						const editor = this.editor;
@@ -599,19 +740,18 @@ describe( 'LinkEditing', () => {
 						} );
 					}
 				}
-				editor.destroy();
-				return VirtualTestEditor
-					.create( {
-						plugins: [ Paragraph, LinkEditing, Enter, CustomLinks ],
-						link: {
-							addTargetToExternalLinks: true
-						}
-					} )
-					.then( newEditor => {
-						editor = newEditor;
-						model = editor.model;
-						view = editor.editing.view;
-					} );
+
+				await editor.destroy();
+
+				editor = await ClassicTestEditor.create( element, {
+					plugins: [ Paragraph, LinkEditing, Enter, CustomLinks ],
+					link: {
+						addTargetToExternalLinks: true
+					}
+				} );
+
+				model = editor.model;
+				view = editor.editing.view;
 			} );
 
 			it( 'has possibility to override default one', () => {
@@ -625,9 +765,20 @@ describe( 'LinkEditing', () => {
 		} );
 
 		describe( 'upcast converter', () => {
+			let element, editor;
+
+			beforeEach( () => {
+				element = document.createElement( 'div' );
+				document.body.appendChild( element );
+			} );
+
+			afterEach( () => {
+				element.remove();
+			} );
+
 			it( 'should upcast attributes from initial data', () => {
-				return VirtualTestEditor
-					.create( {
+				return ClassicTestEditor
+					.create( element, {
 						initialData: '<p><a href="url" target="_blank" rel="noopener noreferrer" download="file">Foo</a>' +
 							'<a href="example.com" download="file">Bar</a></p>',
 						plugins: [ Paragraph, LinkEditing, Enter ],
@@ -661,12 +812,14 @@ describe( 'LinkEditing', () => {
 								'<$text linkHref="example.com" linkIsDownloadable="true">Bar</$text>' +
 							'</paragraph>'
 						);
+
+						return editor.destroy();
 					} );
 			} );
 
 			it( 'should not upcast partial and incorrect attributes', () => {
-				return VirtualTestEditor
-					.create( {
+				return ClassicTestEditor
+					.create( element, {
 						initialData: '<p><a href="url" target="_blank" download="something">Foo</a>' +
 							'<a href="example.com" download="test">Bar</a></p>',
 						plugins: [ Paragraph, LinkEditing, Enter ],
@@ -700,6 +853,8 @@ describe( 'LinkEditing', () => {
 								'<$text linkHref="example.com">Bar</$text>' +
 							'</paragraph>'
 						);
+
+						return editor.destroy();
 					} );
 			} );
 		} );

+ 4 - 0
packages/ckeditor5-link/tests/manual/tickets/6053/1.html

@@ -0,0 +1,4 @@
+<div id="editor">
+	<p>Some <b>bold</b> text <a href="https://ckeditor.com" download="download">CKEditor decorated</a> and <a href="https://cksource.com">CKSource</a>.</p>
+	<p>An "empty" paragraph to paste links:</p>
+</div>

+ 30 - 0
packages/ckeditor5-link/tests/manual/tickets/6053/1.js

@@ -0,0 +1,30 @@
+/**
+ * @license Copyright (c) 2003-2020, CKSource - Frederico Knabben. All rights reserved.
+ * For licensing, see LICENSE.md or https://ckeditor.com/legal/ckeditor-oss-license
+ */
+
+/* globals console:false, document, window */
+
+import ClassicEditor from '@ckeditor/ckeditor5-editor-classic/src/classiceditor';
+import ArticlePluginSet from '@ckeditor/ckeditor5-core/tests/_utils/articlepluginset';
+
+ClassicEditor
+	.create( document.querySelector( '#editor' ), {
+		plugins: [ ArticlePluginSet ],
+		toolbar: [ 'undo', 'redo', 'link' ],
+		link: {
+			decorators: {
+				isDownloadable: {
+					mode: 'manual',
+					label: 'Downloadable',
+					attributes: {
+						download: 'download'
+					}
+				}
+			}
+		}
+	} )
+	.then( newEditor => {
+		window.editor = newEditor;
+	} )
+	.catch( err => console.error( err.stack ) );

+ 18 - 0
packages/ckeditor5-link/tests/manual/tickets/6053/1.md

@@ -0,0 +1,18 @@
+# Issue [#6053](https://github.com/ckeditor/ckeditor5/issues/6053) manual test.
+
+## The link selection attributes should cleared in certain situations after the link was pasted
+
+1. Copy a part of a link in the content.
+2. Paste it a paragraph.
+
+**Expected**: The link should be pasted but you should be able to type unlinked text right away.
+
+1. Try copying and pasting links in different places:
+	1. In the middle of another link.
+	2. At the beginning/end of another link
+	3. Using links with different URLs.
+	4. When the selection gravity is overridden at the boundary of an existing link (use arrow left and right keys).
+
+**Expected**: If a pasted link is not followed by another link, you should always be able to type unlinked text right away after pasting. There should be no selection attributes starting with "link".
+
+**Tip**: If not sure, check the Model/Selection tab in the inspector and look for selection attributes.