Merge pull request #10357 from ckeditor/ck/2616

Other (highlight): Toggling highlight doesn't remove it when the caret is at the end of the highlighted range. Closes #2616.
ckeditor · Aug 17, 2021 · d1a271d · d1a271d
2 parents d884b5c + b0ff7b9
commit d1a271d
Show file tree

Hide file tree

Showing 2 changed files with 60 additions and 4 deletions.
diff --git a/packages/ckeditor5-highlight/src/highlightcommand.js b/packages/ckeditor5-highlight/src/highlightcommand.js
@@ -57,8 +57,6 @@ export default class HighlightCommand extends Command {
 		const highlighter = options.value;
 
 		model.change( writer => {
-			const ranges = model.schema.getValidRanges( selection.getRanges(), 'highlight' );
-
 			if ( selection.isCollapsed ) {
 				const position = selection.getFirstPosition();
 
@@ -77,17 +75,29 @@ export default class HighlightCommand extends Command {
 					// Then depending on current value...
 					if ( !highlighter || this.value === highlighter ) {
 						// ...remove attribute when passing highlighter different then current or executing "eraser".
-						writer.removeAttribute( 'highlight', highlightRange );
+
+						// If we're at the end of the highlighted range, we don't want to remove highlight of the range.
+						if ( !position.isEqual( highlightEnd ) ) {
+							writer.removeAttribute( 'highlight', highlightRange );
+						}
+
 						writer.removeSelectionAttribute( 'highlight' );
 					} else {
 						// ...update `highlight` value.
-						writer.setAttribute( 'highlight', highlighter, highlightRange );
+
+						// If we're at the end of the highlighted range, we don't want to change the highlight of the range.
+						if ( !position.isEqual( highlightEnd ) ) {
+							writer.setAttribute( 'highlight', highlighter, highlightRange );
+						}
+
 						writer.setSelectionAttribute( 'highlight', highlighter );
 					}
 				} else if ( highlighter ) {
 					writer.setSelectionAttribute( 'highlight', highlighter );
 				}
 			} else {
+				const ranges = model.schema.getValidRanges( selection.getRanges(), 'highlight' );
+
 				for ( const range of ranges ) {
 					if ( highlighter ) {
 						writer.setAttribute( 'highlight', highlighter, range );

diff --git a/packages/ckeditor5-highlight/tests/highlightcommand.js b/packages/ckeditor5-highlight/tests/highlightcommand.js
@@ -187,6 +187,40 @@ describe( 'HighlightCommand', () => {
 					expect( command.value ).to.equal( 'pinkMarker' );
 					expect( doc.selection.getAttribute( 'highlight' ) ).to.equal( 'pinkMarker' );
 				} );
+
+				it( 'should not change entire highlight when at the end of highlighted text', () => {
+					setData( model, '<p>abc<$text highlight="yellowMarker">foobar</$text>[]xyz</p>' );
+
+					expect( command.value ).to.equal( 'yellowMarker' );
+
+					command.execute( { value: 'greenMarker' } );
+
+					expect( getData( model, { withoutSelection: true } ) ).to.equal(
+						'<p>abc<$text highlight="yellowMarker">foobar</$text>xyz</p>'
+					);
+
+					expect( command.value ).to.equal( 'greenMarker' );
+				} );
+
+				it( 'should not remove entire highlight when at the end of highlighted text of the same value', () => {
+					setData( model, '<p>abc<$text highlight="yellowMarker">foobar</$text>[]xyz</p>' );
+
+					expect( command.value ).to.equal( 'yellowMarker' );
+
+					command.execute( { value: 'yellowMarker' } );
+
+					expect( getData( model ) ).to.equal( '<p>abc<$text highlight="yellowMarker">foobar</$text>[]xyz</p>' );
+
+					expect( command.value ).to.be.undefined;
+				} );
+
+				it( 'should change selection attribute when at the end of highlighted text', () => {
+					setData( model, '<p>abc<$text highlight="yellowMarker">foobar</$text>[]xyz</p>' );
+
+					command.execute( { value: 'greenMarker' } );
+
+					expect( doc.selection.getAttribute( 'highlight' ) ).to.equal( 'greenMarker' );
+				} );
 			} );
 
 			describe( 'on not collapsed range', () => {
@@ -250,6 +284,18 @@ describe( 'HighlightCommand', () => {
 
 					expect( command.value ).to.be.undefined;
 				} );
+
+				it( 'should not remove entire highlight when at the end of highlighted text', () => {
+					setData( model, '<p>abc<$text highlight="yellowMarker">foobar</$text>[]xyz</p>' );
+
+					expect( command.value ).to.equal( 'yellowMarker' );
+
+					command.execute();
+
+					expect( getData( model ) ).to.equal( '<p>abc<$text highlight="yellowMarker">foobar</$text>[]xyz</p>' );
+
+					expect( command.value ).to.be.undefined;
+				} );
 			} );
 
 			describe( 'on not collapsed range', () => {