浏览代码

Merge pull request #189 from ckeditor/t/180

Fix: The selected link should be highlighted using the class instead of a marker. Closes #180. Closes #176. Closes ckeditor/ckeditor5#888.
Szymon Cofalik 7 年之前
父节点
当前提交
dd1460fddd

+ 42 - 59
packages/ckeditor5-link/src/linkediting.js

@@ -9,9 +9,7 @@
 
 import Plugin from '@ckeditor/ckeditor5-core/src/plugin';
 import {
-	downcastAttributeToElement,
-	downcastMarkerToHighlight,
-	createViewElementFromHighlightDescriptor
+	downcastAttributeToElement
 } from '@ckeditor/ckeditor5-engine/src/conversion/downcast-converters';
 import { upcastElementToAttribute } from '@ckeditor/ckeditor5-engine/src/conversion/upcast-converters';
 import LinkCommand from './linkcommand';
@@ -20,8 +18,8 @@ import { createLinkElement } from './utils';
 import bindTwoStepCaretToAttribute from '@ckeditor/ckeditor5-engine/src/utils/bindtwostepcarettoattribute';
 import findLinkRange from './findlinkrange';
 import '../theme/link.css';
-import DocumentSelection from '@ckeditor/ckeditor5-engine/src/model/documentselection';
-import ModelSelection from '@ckeditor/ckeditor5-engine/src/model/selection';
+
+const HIGHLIGHT_CLASSES = [ 'ck', 'ck-link_selected' ];
 
 /**
  * The link engine feature.
@@ -69,74 +67,59 @@ export default class LinkEditing extends Plugin {
 	}
 
 	/**
-	 * Adds highlight over link which has selection inside, together with two-step caret movement indicates whenever
-	 * user is typing inside the link.
+	 * Adds a visual highlight style to a link in which the selection is anchored.
+	 * Together with two-step caret movement, they indicate that the user is typing inside the link.
+	 *
+	 * Highlight is turned on by adding `.ck .ck-link_selected` classes to the link in the view:
+	 *
+	 * * the classes are removed before conversion has started, as callbacks added with `'highest'` priority
+	 * to {@link module:engine/conversion/downcastdispatcher~DowncastDispatcher} events,
+	 * * the classes are added in the view post fixer, after other changes in the model tree were converted to the view.
+	 *
+	 * This way, adding and removing highlight does not interfere with conversion.
 	 *
 	 * @private
 	 */
 	_setupLinkHighlight() {
 		const editor = this.editor;
-		const model = this.editor.model;
-		const doc = model.document;
-		const highlightDescriptor = {
-			id: 'linkBoundaries',
-			classes: [
-				'ck',
-				'ck-link_selected'
-			],
-			priority: 1
-		};
-
-		// Convert linkBoundaries marker to view highlight.
-		editor.conversion.for( 'editingDowncast' )
-			.add( downcastMarkerToHighlight( {
-				model: 'linkBoundaries',
-				view: highlightDescriptor
-			} ) );
+		const view = editor.editing.view;
+		const highlightedLinks = new Set();
 
-		// Create marker over whole link when selection has "linkHref" attribute.
-		doc.registerPostFixer( writer => {
-			const selection = doc.selection;
-			const marker = model.markers.get( 'linkBoundaries' );
+		// Adding the class.
+		view.document.registerPostFixer( writer => {
+			const selection = editor.model.document.selection;
 
-			// Create marker over link when selection is inside or remove marker otherwise.
 			if ( selection.hasAttribute( 'linkHref' ) ) {
 				const modelRange = findLinkRange( selection.getFirstPosition(), selection.getAttribute( 'linkHref' ) );
-
-				if ( !marker || !marker.getRange().isEqual( modelRange ) ) {
-					writer.setMarker( 'linkBoundaries', modelRange );
-					return true;
+				const viewRange = editor.editing.mapper.toViewRange( modelRange );
+
+				// There might be multiple `a` elements in the `viewRange`, for example, when the `a` element is
+				// broken by a UIElement.
+				for ( const item of viewRange.getItems() ) {
+					if ( item.is( 'a' ) ) {
+						writer.addClass( HIGHLIGHT_CLASSES, item );
+						highlightedLinks.add( item );
+					}
 				}
-			} else if ( marker ) {
-				writer.removeMarker( 'linkBoundaries' );
-				return true;
 			}
-
-			return false;
 		} );
 
-		// Custom converter for selection's "linkHref" attribute - when collapsed selection has this attribute it is
-		// wrapped with <span> similar to that used by highlighting mechanism. This <span> will be merged together with
-		// highlight wrapper. This prevents link splitting When selection is at the beginning or at the end of the link.
-		// Without this converter:
-		//
-		//		<a href="url">{}</a><span class="ck-link_selected"><a href="url">foo</a></span>
-		//
-		// When converter is applied:
-		//
-		//		<span class="ck-link_selected"><a href="url">{}foo</a></span>
-		editor.editing.downcastDispatcher.on( 'attribute:linkHref', ( evt, data, conversionApi ) => {
-			const selection = data.item;
-
-			if ( !( selection instanceof DocumentSelection || selection instanceof ModelSelection ) || !selection.isCollapsed ) {
-				return;
+		// Removing the class.
+		editor.conversion.for( 'editingDowncast' ).add( dispatcher => {
+			// Make sure the highlight is removed on every possible event, before conversion is started.
+			dispatcher.on( 'insert', removeHighlight, { priority: 'highest' } );
+			dispatcher.on( 'remove', removeHighlight, { priority: 'highest' } );
+			dispatcher.on( 'attribute', removeHighlight, { priority: 'highest' } );
+			dispatcher.on( 'selection', removeHighlight, { priority: 'highest' } );
+
+			function removeHighlight() {
+				view.change( writer => {
+					for ( const item of highlightedLinks.values() ) {
+						writer.removeClass( HIGHLIGHT_CLASSES, item );
+						highlightedLinks.delete( item );
+					}
+				} );
 			}
-
-			const writer = conversionApi.writer;
-			const viewSelection = writer.document.selection;
-			const wrapper = createViewElementFromHighlightDescriptor( highlightDescriptor );
-
-			conversionApi.writer.wrap( viewSelection.getFirstRange(), wrapper );
 		} );
 	}
 }

+ 107 - 42
packages/ckeditor5-link/tests/linkediting.js

@@ -10,12 +10,15 @@ import UnlinkCommand from '../src/unlinkcommand';
 import VirtualTestEditor from '@ckeditor/ckeditor5-core/tests/_utils/virtualtesteditor';
 import Paragraph from '@ckeditor/ckeditor5-paragraph/src/paragraph';
 import Enter from '@ckeditor/ckeditor5-enter/src/enter';
+import ModelRange from '@ckeditor/ckeditor5-engine/src/model/range';
 import { getData as getModelData, setData as setModelData } from '@ckeditor/ckeditor5-engine/src/dev-utils/model';
 import { getData as getViewData } from '@ckeditor/ckeditor5-engine/src/dev-utils/view';
 import { isLinkElement } from '../src/utils';
 import { keyCodes } from '@ckeditor/ckeditor5-utils/src/keyboard';
-
-import { downcastAttributeToElement } from '@ckeditor/ckeditor5-engine/src/conversion/downcast-converters';
+import {
+	downcastMarkerToHighlight,
+	downcastAttributeToElement
+} from '@ckeditor/ckeditor5-engine/src/conversion/downcast-converters';
 
 /* global document */
 
@@ -146,27 +149,15 @@ describe( 'LinkEditing', () => {
 		} );
 	} );
 
-	describe( 'highlight link boundaries', () => {
-		it( 'should create marker in model when selection is inside a link', () => {
-			expect( model.markers.has( 'linkBoundaries' ) ).to.be.false;
-
-			setModelData( model,
-				'<paragraph>foo <$text linkHref="url">b{}ar</$text> baz</paragraph>'
-			);
-
-			expect( model.markers.has( 'linkBoundaries' ) ).to.be.true;
-			const marker = model.markers.get( 'linkBoundaries' );
-			expect( marker.getStart().path ).to.deep.equal( [ 0, 4 ] );
-			expect( marker.getEnd().path ).to.deep.equal( [ 0, 7 ] );
-		} );
-
-		it( 'should convert link boundaries marker to proper view', () => {
+	describe( 'link highlighting', () => {
+		it( 'should convert the highlight to a proper view classes', () => {
 			setModelData( model,
 				'<paragraph>foo <$text linkHref="url">b{}ar</$text> baz</paragraph>'
 			);
 
+			expect( model.document.selection.hasAttribute( 'linkHref' ) ).to.be.true;
 			expect( getViewData( view ) ).to.equal(
-				'<p>foo <span class="ck ck-link_selected"><a href="url">b{}ar</a></span> baz</p>'
+				'<p>foo <a class="ck ck-link_selected" href="url">b{}ar</a> baz</p>'
 			);
 		} );
 
@@ -182,13 +173,8 @@ describe( 'LinkEditing', () => {
 			} );
 
 			expect( model.document.selection.hasAttribute( 'linkHref' ) ).to.be.true;
-			expect( model.markers.has( 'linkBoundaries' ) ).to.be.true;
-			const marker = model.markers.get( 'linkBoundaries' );
-			expect( marker.getStart().path ).to.deep.equal( [ 0, 4 ] );
-			expect( marker.getEnd().path ).to.deep.equal( [ 0, 7 ] );
-
 			expect( getViewData( view ) ).to.equal(
-				'<p>foo <span class="ck ck-link_selected"><a href="url">{}bar</a></span> baz</p>'
+				'<p>foo <a class="ck ck-link_selected" href="url">{}bar</a> baz</p>'
 			);
 		} );
 
@@ -198,13 +184,8 @@ describe( 'LinkEditing', () => {
 			);
 
 			expect( model.document.selection.hasAttribute( 'linkHref' ) ).to.be.true;
-			expect( model.markers.has( 'linkBoundaries' ) ).to.be.true;
-			const marker = model.markers.get( 'linkBoundaries' );
-			expect( marker.getStart().path ).to.deep.equal( [ 0, 4 ] );
-			expect( marker.getEnd().path ).to.deep.equal( [ 0, 7 ] );
-
 			expect( getViewData( view ) ).to.equal(
-				'<p>foo <span class="ck ck-link_selected"><a href="url">bar{}</a></span> baz</p>'
+				'<p>foo <a class="ck ck-link_selected" href="url">bar{}</a> baz</p>'
 			);
 		} );
 
@@ -221,25 +202,19 @@ describe( 'LinkEditing', () => {
 			);
 
 			expect( model.document.selection.hasAttribute( 'linkHref' ) ).to.be.true;
-			expect( model.markers.has( 'linkBoundaries' ) ).to.be.true;
-			const marker = model.markers.get( 'linkBoundaries' );
-			expect( marker.getStart().path ).to.deep.equal( [ 1, 0 ] );
-			expect( marker.getEnd().path ).to.deep.equal( [ 1, 2 ] );
 		} );
 
-		it( 'should remove marker when selection is moved out from the link', () => {
+		it( 'should remove classes when selection is moved out from the link', () => {
 			setModelData( model,
 				'<paragraph>foo <$text linkHref="url">li{}nk</$text> baz</paragraph>'
 			);
 
 			expect( getViewData( view ) ).to.equal(
-				'<p>foo <span class="ck ck-link_selected"><a href="url">li{}nk</a></span> baz</p>'
+				'<p>foo <a class="ck ck-link_selected" href="url">li{}nk</a> baz</p>'
 			);
 
-			expect( model.markers.has( 'linkBoundaries' ) ).to.be.true;
 			model.change( writer => writer.setSelection( model.document.getRoot().getChild( 0 ), 0 ) );
 
-			expect( model.markers.has( 'linkBoundaries' ) ).to.be.false;
 			expect( getViewData( view ) ).to.equal(
 				'<p>{}foo <a href="url">link</a> baz</p>'
 			);
@@ -251,16 +226,106 @@ describe( 'LinkEditing', () => {
 			);
 
 			expect( getViewData( view ) ).to.equal(
-				'<p>foo <span class="ck ck-link_selected"><a href="url">li{}nk</a></span> baz</p>'
+				'<p>foo <a class="ck ck-link_selected" href="url">li{}nk</a> baz</p>'
 			);
 
-			expect( model.markers.has( 'linkBoundaries' ) ).to.be.true;
 			model.change( writer => writer.setSelection( model.document.getRoot().getChild( 0 ), 5 ) );
 
-			expect( model.markers.has( 'linkBoundaries' ) ).to.be.true;
 			expect( getViewData( view ) ).to.equal(
-				'<p>foo <span class="ck ck-link_selected"><a href="url">l{}ink</a></span> baz</p>'
+				'<p>foo <a class="ck ck-link_selected" href="url">l{}ink</a> baz</p>'
 			);
 		} );
+
+		describe( 'downcast conversion integration', () => {
+			it( 'works for the #insert event', () => {
+				setModelData( model,
+					'<paragraph>foo <$text linkHref="url">li{}nk</$text> baz</paragraph>'
+				);
+
+				model.change( writer => {
+					writer.insertText( 'FOO', { linkHref: 'url' }, model.document.selection.getFirstPosition() );
+				} );
+
+				expect( getViewData( view ) ).to.equal(
+					'<p>foo <a class="ck ck-link_selected" href="url">liFOO{}nk</a> baz</p>'
+				);
+			} );
+
+			it( 'works for the #remove event', () => {
+				setModelData( model,
+					'<paragraph>foo <$text linkHref="url">li{}nk</$text> baz</paragraph>'
+				);
+
+				model.change( writer => {
+					writer.remove( ModelRange.createFromParentsAndOffsets(
+						model.document.getRoot().getChild( 0 ), 0,
+						model.document.getRoot().getChild( 0 ), 5 )
+					);
+				} );
+
+				expect( getViewData( view ) ).to.equal(
+					'<p><a class="ck ck-link_selected" href="url">i{}nk</a> baz</p>'
+				);
+			} );
+
+			it( 'works for the #attribute event', () => {
+				setModelData( model,
+					'<paragraph>foo <$text linkHref="url">li{}nk</$text> baz</paragraph>'
+				);
+
+				model.change( writer => {
+					writer.setAttribute( 'linkHref', 'new-url', new ModelRange(
+						model.document.selection.getFirstPosition().getShiftedBy( -1 ),
+						model.document.selection.getFirstPosition().getShiftedBy( 1 ) )
+					);
+				} );
+
+				expect( getViewData( view ) ).to.equal(
+					'<p>foo <a href="url">l</a><a class="ck ck-link_selected" href="new-url">i{}n</a><a href="url">k</a> baz</p>'
+				);
+			} );
+
+			it( 'works for the #selection event', () => {
+				setModelData( model,
+					'<paragraph>foo <$text linkHref="url">li{}nk</$text> baz</paragraph>'
+				);
+
+				model.change( writer => {
+					writer.setSelection( new ModelRange(
+						model.document.selection.getFirstPosition().getShiftedBy( -1 ),
+						model.document.selection.getFirstPosition().getShiftedBy( 1 ) )
+					);
+				} );
+
+				expect( getViewData( view ) ).to.equal(
+					'<p>foo <a class="ck ck-link_selected" href="url">l{in}k</a> baz</p>'
+				);
+			} );
+
+			it( 'works for the #addMarker and #removeMarker events', () => {
+				downcastMarkerToHighlight( { model: 'fooMarker', view: {} } )( editor.editing.downcastDispatcher );
+
+				setModelData( model,
+					'<paragraph>foo <$text linkHref="url">li{}nk</$text> baz</paragraph>'
+				);
+
+				model.change( writer => {
+					writer.setMarker( 'fooMarker', ModelRange.createFromParentsAndOffsets(
+						model.document.getRoot().getChild( 0 ), 0,
+						model.document.getRoot().getChild( 0 ), 5 )
+					);
+				} );
+
+				expect( getViewData( view ) ).to.equal(
+					'<p><span>foo </span><a class="ck ck-link_selected" href="url"><span>l</span>i{}nk</a> baz</p>'
+				);
+
+				model.change( writer => writer.removeMarker( 'fooMarker' ) );
+
+				expect( getViewData( view ) ).to.equal(
+					'<p>foo <a class="ck ck-link_selected" href="url">li{}nk</a> baz</p>'
+				);
+			} );
+		} );
 	} );
 } );

+ 4 - 4
packages/ckeditor5-link/tests/linkui.js

@@ -250,11 +250,11 @@ describe( 'LinkUI', () => {
 				const spy = testUtils.sinon.stub( balloon, 'updatePosition' ).returns( {} );
 
 				expect( getViewData( view ) ).to.equal(
-					'<p><span class="ck ck-link_selected"><a href="url">f{}oo</a></span></p>'
+					'<p><a class="ck ck-link_selected" href="url">f{}oo</a></p>'
 				);
 
 				const root = viewDocument.getRoot();
-				const linkElement = root.getChild( 0 ).getChild( 0 ).getChild( 0 );
+				const linkElement = root.getChild( 0 ).getChild( 0 );
 				const text = linkElement.getChild( 0 );
 
 				// Move selection to foo[].
@@ -319,10 +319,10 @@ describe( 'LinkUI', () => {
 				const spyUpdate = testUtils.sinon.stub( balloon, 'updatePosition' ).returns( {} );
 				const spyHide = testUtils.sinon.spy( linkUIFeature, '_hideUI' );
 
-				expect( getViewData( view ) ).to.equal( '<p><span class="ck ck-link_selected"><a href="url">f{}oo</a></span></p>' );
+				expect( getViewData( view ) ).to.equal( '<p><a class="ck ck-link_selected" href="url">f{}oo</a></p>' );
 
 				const root = viewDocument.getRoot();
-				const text = root.getChild( 0 ).getChild( 0 ).getChild( 0 ).getChild( 0 );
+				const text = root.getChild( 0 ).getChild( 0 ).getChild( 0 );
 
 				// Move selection to f[o]o.
 				view.change( writer => {