Skip to content
This repository was archived by the owner on Jun 26, 2020. It is now read-only.

Commit c75c4ca

Browse files
scofalikscofalik
authored
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.
2 parents fb42a7f + 50d91ab commit c75c4ca

File tree

3 files changed

+153
-105
lines changed

3 files changed

+153
-105
lines changed

‎src/linkediting.js‎

Lines changed: 42 additions & 59 deletions
Original file line numberDiff line numberDiff line change
@@ -9,9 +9,7 @@
99

1010
import Plugin from '@ckeditor/ckeditor5-core/src/plugin';
1111
import {
12-
downcastAttributeToElement,
13-
downcastMarkerToHighlight,
14-
createViewElementFromHighlightDescriptor
12+
downcastAttributeToElement
1513
} from '@ckeditor/ckeditor5-engine/src/conversion/downcast-converters';
1614
import { upcastElementToAttribute } from '@ckeditor/ckeditor5-engine/src/conversion/upcast-converters';
1715
import LinkCommand from './linkcommand';
@@ -20,8 +18,8 @@ import { createLinkElement } from './utils';
2018
import bindTwoStepCaretToAttribute from '@ckeditor/ckeditor5-engine/src/utils/bindtwostepcarettoattribute';
2119
import findLinkRange from './findlinkrange';
2220
import '../theme/link.css';
23-
import DocumentSelection from '@ckeditor/ckeditor5-engine/src/model/documentselection';
24-
import ModelSelection from '@ckeditor/ckeditor5-engine/src/model/selection';
21+
22+
const HIGHLIGHT_CLASSES = [ 'ck', 'ck-link_selected' ];
2523

2624
/**
2725
* The link engine feature.
@@ -69,74 +67,59 @@ export default class LinkEditing extends Plugin {
6967
}
7068

7169
/**
72-
* Adds highlight over link which has selection inside, together with two-step caret movement indicates whenever
73-
* user is typing inside the link.
70+
* Adds a visual highlight style to a link in which the selection is anchored.
71+
* Together with two-step caret movement, they indicate that the user is typing inside the link.
72+
*
73+
* Highlight is turned on by adding `.ck .ck-link_selected` classes to the link in the view:
74+
*
75+
* * the classes are removed before conversion has started, as callbacks added with `'highest'` priority
76+
* to {@link module:engine/conversion/downcastdispatcher~DowncastDispatcher} events,
77+
* * the classes are added in the view post fixer, after other changes in the model tree were converted to the view.
78+
*
79+
* This way, adding and removing highlight does not interfere with conversion.
7480
*
7581
* @private
7682
*/
7783
_setupLinkHighlight() {
7884
const editor = this.editor;
79-
const model = this.editor.model;
80-
const doc = model.document;
81-
const highlightDescriptor = {
82-
id: 'linkBoundaries',
83-
classes: [
84-
'ck',
85-
'ck-link_selected'
86-
],
87-
priority: 1
88-
};
89-
90-
// Convert linkBoundaries marker to view highlight.
91-
editor.conversion.for( 'editingDowncast' )
92-
.add( downcastMarkerToHighlight( {
93-
model: 'linkBoundaries',
94-
view: highlightDescriptor
95-
} ) );
85+
const view = editor.editing.view;
86+
const highlightedLinks = new Set();
9687

97-
// Create marker over whole link when selection has "linkHref" attribute.
98-
doc.registerPostFixer( writer => {
99-
const selection = doc.selection;
100-
const marker = model.markers.get( 'linkBoundaries' );
88+
// Adding the class.
89+
view.document.registerPostFixer( writer => {
90+
const selection = editor.model.document.selection;
10191

102-
// Create marker over link when selection is inside or remove marker otherwise.
10392
if ( selection.hasAttribute( 'linkHref' ) ) {
10493
const modelRange = findLinkRange( selection.getFirstPosition(), selection.getAttribute( 'linkHref' ) );
105-
106-
if ( !marker || !marker.getRange().isEqual( modelRange ) ) {
107-
writer.setMarker( 'linkBoundaries', modelRange );
108-
return true;
94+
const viewRange = editor.editing.mapper.toViewRange( modelRange );
95+
96+
// There might be multiple `a` elements in the `viewRange`, for example, when the `a` element is
97+
// broken by a UIElement.
98+
for ( const item of viewRange.getItems() ) {
99+
if ( item.is( 'a' ) ) {
100+
writer.addClass( HIGHLIGHT_CLASSES, item );
101+
highlightedLinks.add( item );
102+
}
109103
}
110-
} else if ( marker ) {
111-
writer.removeMarker( 'linkBoundaries' );
112-
return true;
113104
}
114-
115-
return false;
116105
} );
117106

118-
// Custom converter for selection's "linkHref" attribute - when collapsed selection has this attribute it is
119-
// wrapped with <span> similar to that used by highlighting mechanism. This <span> will be merged together with
120-
// highlight wrapper. This prevents link splitting When selection is at the beginning or at the end of the link.
121-
// Without this converter:
122-
//
123-
// <a href="url">{}</a><span class="ck-link_selected"><a href="url">foo</a></span>
124-
//
125-
// When converter is applied:
126-
//
127-
// <span class="ck-link_selected"><a href="url">{}foo</a></span>
128-
editor.editing.downcastDispatcher.on( 'attribute:linkHref', ( evt, data, conversionApi ) => {
129-
const selection = data.item;
130-
131-
if ( !( selection instanceof DocumentSelection || selection instanceof ModelSelection ) || !selection.isCollapsed ) {
132-
return;
107+
// Removing the class.
108+
editor.conversion.for( 'editingDowncast' ).add( dispatcher => {
109+
// Make sure the highlight is removed on every possible event, before conversion is started.
110+
dispatcher.on( 'insert', removeHighlight, { priority: 'highest' } );
111+
dispatcher.on( 'remove', removeHighlight, { priority: 'highest' } );
112+
dispatcher.on( 'attribute', removeHighlight, { priority: 'highest' } );
113+
dispatcher.on( 'selection', removeHighlight, { priority: 'highest' } );
114+
115+
function removeHighlight() {
116+
view.change( writer => {
117+
for ( const item of highlightedLinks.values() ) {
118+
writer.removeClass( HIGHLIGHT_CLASSES, item );
119+
highlightedLinks.delete( item );
120+
}
121+
} );
133122
}
134-
135-
const writer = conversionApi.writer;
136-
const viewSelection = writer.document.selection;
137-
const wrapper = createViewElementFromHighlightDescriptor( highlightDescriptor );
138-
139-
conversionApi.writer.wrap( viewSelection.getFirstRange(), wrapper );
140123
} );
141124
}
142125
}

‎tests/linkediting.js‎

Lines changed: 107 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -10,12 +10,15 @@ import UnlinkCommand from '../src/unlinkcommand';
1010
import VirtualTestEditor from '@ckeditor/ckeditor5-core/tests/_utils/virtualtesteditor';
1111
import Paragraph from '@ckeditor/ckeditor5-paragraph/src/paragraph';
1212
import Enter from '@ckeditor/ckeditor5-enter/src/enter';
13+
import ModelRange from '@ckeditor/ckeditor5-engine/src/model/range';
1314
import { getData as getModelData, setData as setModelData } from '@ckeditor/ckeditor5-engine/src/dev-utils/model';
1415
import { getData as getViewData } from '@ckeditor/ckeditor5-engine/src/dev-utils/view';
1516
import { isLinkElement } from '../src/utils';
1617
import { keyCodes } from '@ckeditor/ckeditor5-utils/src/keyboard';
17-
18-
import { downcastAttributeToElement } from '@ckeditor/ckeditor5-engine/src/conversion/downcast-converters';
18+
import {
19+
downcastMarkerToHighlight,
20+
downcastAttributeToElement
21+
} from '@ckeditor/ckeditor5-engine/src/conversion/downcast-converters';
1922

2023
/* global document */
2124

@@ -146,27 +149,15 @@ describe( 'LinkEditing', () => {
146149
} );
147150
} );
148151

149-
describe( 'highlight link boundaries', () => {
150-
it( 'should create marker in model when selection is inside a link', () => {
151-
expect( model.markers.has( 'linkBoundaries' ) ).to.be.false;
152-
153-
setModelData( model,
154-
'<paragraph>foo <$text linkHref="url">b{}ar</$text> baz</paragraph>'
155-
);
156-
157-
expect( model.markers.has( 'linkBoundaries' ) ).to.be.true;
158-
const marker = model.markers.get( 'linkBoundaries' );
159-
expect( marker.getStart().path ).to.deep.equal( [ 0, 4 ] );
160-
expect( marker.getEnd().path ).to.deep.equal( [ 0, 7 ] );
161-
} );
162-
163-
it( 'should convert link boundaries marker to proper view', () => {
152+
describe( 'link highlighting', () => {
153+
it( 'should convert the highlight to a proper view classes', () => {
164154
setModelData( model,
165155
'<paragraph>foo <$text linkHref="url">b{}ar</$text> baz</paragraph>'
166156
);
167157

158+
expect( model.document.selection.hasAttribute( 'linkHref' ) ).to.be.true;
168159
expect( getViewData( view ) ).to.equal(
169-
'<p>foo <span class="ck ck-link_selected"><a href="url">b{}ar</a></span> baz</p>'
160+
'<p>foo <a class="ck ck-link_selected" href="url">b{}ar</a> baz</p>'
170161
);
171162
} );
172163

@@ -182,13 +173,8 @@ describe( 'LinkEditing', () => {
182173
} );
183174

184175
expect( model.document.selection.hasAttribute( 'linkHref' ) ).to.be.true;
185-
expect( model.markers.has( 'linkBoundaries' ) ).to.be.true;
186-
const marker = model.markers.get( 'linkBoundaries' );
187-
expect( marker.getStart().path ).to.deep.equal( [ 0, 4 ] );
188-
expect( marker.getEnd().path ).to.deep.equal( [ 0, 7 ] );
189-
190176
expect( getViewData( view ) ).to.equal(
191-
'<p>foo <span class="ck ck-link_selected"><a href="url">{}bar</a></span> baz</p>'
177+
'<p>foo <a class="ck ck-link_selected" href="url">{}bar</a> baz</p>'
192178
);
193179
} );
194180

@@ -198,13 +184,8 @@ describe( 'LinkEditing', () => {
198184
);
199185

200186
expect( model.document.selection.hasAttribute( 'linkHref' ) ).to.be.true;
201-
expect( model.markers.has( 'linkBoundaries' ) ).to.be.true;
202-
const marker = model.markers.get( 'linkBoundaries' );
203-
expect( marker.getStart().path ).to.deep.equal( [ 0, 4 ] );
204-
expect( marker.getEnd().path ).to.deep.equal( [ 0, 7 ] );
205-
206187
expect( getViewData( view ) ).to.equal(
207-
'<p>foo <span class="ck ck-link_selected"><a href="url">bar{}</a></span> baz</p>'
188+
'<p>foo <a class="ck ck-link_selected" href="url">bar{}</a> baz</p>'
208189
);
209190
} );
210191

@@ -221,25 +202,19 @@ describe( 'LinkEditing', () => {
221202
);
222203

223204
expect( model.document.selection.hasAttribute( 'linkHref' ) ).to.be.true;
224-
expect( model.markers.has( 'linkBoundaries' ) ).to.be.true;
225-
const marker = model.markers.get( 'linkBoundaries' );
226-
expect( marker.getStart().path ).to.deep.equal( [ 1, 0 ] );
227-
expect( marker.getEnd().path ).to.deep.equal( [ 1, 2 ] );
228205
} );
229206

230-
it( 'should remove marker when selection is moved out from the link', () => {
207+
it( 'should remove classes when selection is moved out from the link', () => {
231208
setModelData( model,
232209
'<paragraph>foo <$text linkHref="url">li{}nk</$text> baz</paragraph>'
233210
);
234211

235212
expect( getViewData( view ) ).to.equal(
236-
'<p>foo <span class="ck ck-link_selected"><a href="url">li{}nk</a></span> baz</p>'
213+
'<p>foo <a class="ck ck-link_selected" href="url">li{}nk</a> baz</p>'
237214
);
238215

239-
expect( model.markers.has( 'linkBoundaries' ) ).to.be.true;
240216
model.change( writer => writer.setSelection( model.document.getRoot().getChild( 0 ), 0 ) );
241217

242-
expect( model.markers.has( 'linkBoundaries' ) ).to.be.false;
243218
expect( getViewData( view ) ).to.equal(
244219
'<p>{}foo <a href="url">link</a> baz</p>'
245220
);
@@ -251,16 +226,106 @@ describe( 'LinkEditing', () => {
251226
);
252227

253228
expect( getViewData( view ) ).to.equal(
254-
'<p>foo <span class="ck ck-link_selected"><a href="url">li{}nk</a></span> baz</p>'
229+
'<p>foo <a class="ck ck-link_selected" href="url">li{}nk</a> baz</p>'
255230
);
256231

257-
expect( model.markers.has( 'linkBoundaries' ) ).to.be.true;
258232
model.change( writer => writer.setSelection( model.document.getRoot().getChild( 0 ), 5 ) );
259233

260-
expect( model.markers.has( 'linkBoundaries' ) ).to.be.true;
261234
expect( getViewData( view ) ).to.equal(
262-
'<p>foo <span class="ck ck-link_selected"><a href="url">l{}ink</a></span> baz</p>'
235+
'<p>foo <a class="ck ck-link_selected" href="url">l{}ink</a> baz</p>'
263236
);
264237
} );
238+
239+
describe( 'downcast conversion integration', () => {
240+
it( 'works for the #insert event', () => {
241+
setModelData( model,
242+
'<paragraph>foo <$text linkHref="url">li{}nk</$text> baz</paragraph>'
243+
);
244+
245+
model.change( writer => {
246+
writer.insertText( 'FOO', { linkHref: 'url' }, model.document.selection.getFirstPosition() );
247+
} );
248+
249+
expect( getViewData( view ) ).to.equal(
250+
'<p>foo <a class="ck ck-link_selected" href="url">liFOO{}nk</a> baz</p>'
251+
);
252+
} );
253+
254+
it( 'works for the #remove event', () => {
255+
setModelData( model,
256+
'<paragraph>foo <$text linkHref="url">li{}nk</$text> baz</paragraph>'
257+
);
258+
259+
model.change( writer => {
260+
writer.remove( ModelRange.createFromParentsAndOffsets(
261+
model.document.getRoot().getChild( 0 ), 0,
262+
model.document.getRoot().getChild( 0 ), 5 )
263+
);
264+
} );
265+
266+
expect( getViewData( view ) ).to.equal(
267+
'<p><a class="ck ck-link_selected" href="url">i{}nk</a> baz</p>'
268+
);
269+
} );
270+
271+
it( 'works for the #attribute event', () => {
272+
setModelData( model,
273+
'<paragraph>foo <$text linkHref="url">li{}nk</$text> baz</paragraph>'
274+
);
275+
276+
model.change( writer => {
277+
writer.setAttribute( 'linkHref', 'new-url', new ModelRange(
278+
model.document.selection.getFirstPosition().getShiftedBy( -1 ),
279+
model.document.selection.getFirstPosition().getShiftedBy( 1 ) )
280+
);
281+
} );
282+
283+
expect( getViewData( view ) ).to.equal(
284+
'<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>'
285+
);
286+
} );
287+
288+
it( 'works for the #selection event', () => {
289+
setModelData( model,
290+
'<paragraph>foo <$text linkHref="url">li{}nk</$text> baz</paragraph>'
291+
);
292+
293+
model.change( writer => {
294+
writer.setSelection( new ModelRange(
295+
model.document.selection.getFirstPosition().getShiftedBy( -1 ),
296+
model.document.selection.getFirstPosition().getShiftedBy( 1 ) )
297+
);
298+
} );
299+
300+
expect( getViewData( view ) ).to.equal(
301+
'<p>foo <a class="ck ck-link_selected" href="url">l{in}k</a> baz</p>'
302+
);
303+
} );
304+
305+
it( 'works for the #addMarker and #removeMarker events', () => {
306+
downcastMarkerToHighlight( { model: 'fooMarker', view: {} } )( editor.editing.downcastDispatcher );
307+
308+
setModelData( model,
309+
'<paragraph>foo <$text linkHref="url">li{}nk</$text> baz</paragraph>'
310+
);
311+
312+
model.change( writer => {
313+
writer.setMarker( 'fooMarker', ModelRange.createFromParentsAndOffsets(
314+
model.document.getRoot().getChild( 0 ), 0,
315+
model.document.getRoot().getChild( 0 ), 5 )
316+
);
317+
} );
318+
319+
expect( getViewData( view ) ).to.equal(
320+
'<p><span>foo </span><a class="ck ck-link_selected" href="url"><span>l</span>i{}nk</a> baz</p>'
321+
);
322+
323+
model.change( writer => writer.removeMarker( 'fooMarker' ) );
324+
325+
expect( getViewData( view ) ).to.equal(
326+
'<p>foo <a class="ck ck-link_selected" href="url">li{}nk</a> baz</p>'
327+
);
328+
} );
329+
} );
265330
} );
266331
} );

‎tests/linkui.js‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -250,11 +250,11 @@ describe( 'LinkUI', () => {
250250
const spy = testUtils.sinon.stub( balloon, 'updatePosition' ).returns( {} );
251251

252252
expect( getViewData( view ) ).to.equal(
253-
'<p><span class="ck ck-link_selected"><a href="url">f{}oo</a></span></p>'
253+
'<p><a class="ck ck-link_selected" href="url">f{}oo</a></p>'
254254
);
255255

256256
const root = viewDocument.getRoot();
257-
const linkElement = root.getChild( 0 ).getChild( 0 ).getChild( 0 );
257+
const linkElement = root.getChild( 0 ).getChild( 0 );
258258
const text = linkElement.getChild( 0 );
259259

260260
// Move selection to foo[].
@@ -319,10 +319,10 @@ describe( 'LinkUI', () => {
319319
const spyUpdate = testUtils.sinon.stub( balloon, 'updatePosition' ).returns( {} );
320320
const spyHide = testUtils.sinon.spy( linkUIFeature, '_hideUI' );
321321

322-
expect( getViewData( view ) ).to.equal( '<p><span class="ck ck-link_selected"><a href="url">f{}oo</a></span></p>' );
322+
expect( getViewData( view ) ).to.equal( '<p><a class="ck ck-link_selected" href="url">f{}oo</a></p>' );
323323

324324
const root = viewDocument.getRoot();
325-
const text = root.getChild( 0 ).getChild( 0 ).getChild( 0 ).getChild( 0 );
325+
const text = root.getChild( 0 ).getChild( 0 ).getChild( 0 );
326326

327327
// Move selection to f[o]o.
328328
view.change( writer => {

0 commit comments

Comments
 (0)