Merge branch 't/12621'

CHANGES.md

@@ -17,6 +17,7 @@ Fixed Issues:
 * [#12300](http://dev.ckeditor.com/ticket/12300): Fixed: The [`editor.change`](http://docs.ckeditor.com/#!/api/CKEDITOR.editor-event-change) event fired on first navigation key press after typing.
 * [#12141](http://dev.ckeditor.com/ticket/12141): Fixed: List items are lost when indenting a list item with a content wrapped with a block element.
 * [#12515](http://dev.ckeditor.com/ticket/12515): Fixed: Cursor is in the wrong position when executing undo command after adding image and typing some text.
+* [#12621](http://dev.ckeditor.com/ticket/12621): Fixed: Can't remove styles in empty lines.
 
 Other Changes:
 * [#12550](http://dev.ckeditor.com/ticket/12550): Added CKEDITOR.dtd.main.

core/selection.js

@@ -84,8 +84,28 @@
 	// #### checkSelectionChange : END
 
 	var isVisible = CKEDITOR.dom.walker.invisible( 1 );
+
+	// May absorb the caret if:
+	// * is a visible node,
+	// * is a non-empty element (this rule will accept elements like <strong></strong> because they
+	//	they were not accepted by the isVisible() check, not not <br> which cannot absorb the caret).
+	//	See #12621.
+	function mayAbsorbCaret( node ) {
+		if ( isVisible( node ) )
+			return true;
+
+		if ( node.type == CKEDITOR.NODE_ELEMENT && !node.is( CKEDITOR.dtd.$empty ) )
+			return true;
+
+		return false;
+	}
+
 	function rangeRequiresFix( range ) {
-		function isTextCt( node, isAtEnd ) {
+		// Whether we must prevent from absorbing caret by this context node.
+		// Also checks whether there's an editable position next to that node.
+		function ctxRequiresFix( node, isAtEnd ) {
+			// It's ok for us if a text node absorbs the caret, because
+			// the caret container element isn't changed then.
 			if ( !node || node.type == CKEDITOR.NODE_TEXT )
 				return false;
 
@@ -100,17 +120,18 @@
 
 		var ct = range.startContainer;
 
-		var previous = range.getPreviousNode( isVisible, null, ct ),
-			next = range.getNextNode( isVisible, null, ct );
+		var previous = range.getPreviousNode( mayAbsorbCaret, null, ct ),
+			next = range.getNextNode( mayAbsorbCaret, null, ct );
 
-		// Any adjacent text container may absorb the cursor, e.g.
+		// Any adjacent text container may absorb the caret, e.g.
 		// <p><strong>text</strong>^foo</p>
 		// <p>foo^<strong>text</strong></p>
 		// <div>^<p>foo</p></div>
-		if ( isTextCt( previous ) || isTextCt( next, 1 ) )
+		if ( ctxRequiresFix( previous ) || ctxRequiresFix( next, 1 ) )
 			return true;
 
 		// Empty block/inline element is also affected. <span>^</span>, <p>^</p> (#7222)
+		// If you found this line confusing check #12655.
 		if ( !( previous || next ) && !( ct.type == CKEDITOR.NODE_ELEMENT && ct.isBlockBoundary() && ct.getBogus() ) )
 			return true;
 

tests/core/selection/selection.js

@@ -8,6 +8,13 @@ bender.editor = {
 	}
 };
 
+var htmlComparisonOpts = {
+	compareSelection: true,
+	normalizeSelection: true
+};
+
+var isElement = CKEDITOR.dom.walker.nodeType( CKEDITOR.NODE_ELEMENT );
+
 bender.test( {
 	'test contructor': function() {
 		// Make the DOM selection at the beginning of the document.
@@ -151,13 +158,77 @@ bender.test( {
 		assert.isInnerHtmlMatching(
 			'<p>foo</p><p id="target">^<br /></p><p>bar</p>',
 			bender.tools.selection.getWithHtml( editor ),
-			{
-				compareSelection: true,
-				normalizeSelection: true
-			},
+			htmlComparisonOpts,
 			'selection was placed in the empty paragraph' );
 	},
 
+	'test selectRanges - after empty inline element': function() {
+		// IE8 can't handle this selection.
+		if ( CKEDITOR.env.ie && CKEDITOR.env.version == 8 ) {
+			assert.ignore();
+		}
+
+		var editor = this.editor,
+			range = editor.createRange();
+
+		editor.editable().setHtml( '<p>foo<strong id="target"></strong></p>' );
+
+		var strong = editor.document.getById( 'target' );
+		range.moveToPosition( strong, CKEDITOR.POSITION_AFTER_END );
+		editor.getSelection().selectRanges( [ range ] );
+
+		assert.areSame( strong, editor.getSelection().getRanges()[ 0 ].getPreviousNode( isElement ),
+			'the selection was located after the strong element' );
+	},
+
+	'test selectRanges - after empty inline element with the filler char': function() {
+		// IE8 can't handle this selection.
+		if ( CKEDITOR.env.ie && CKEDITOR.env.version == 8 ) {
+			assert.ignore();
+		}
+
+		var editor = this.editor,
+			range = editor.createRange();
+
+		editor.editable().setHtml( '<p>foo<strong id="target"></strong></p>' );
+
+		// Set the selection inside the strong element, so the filler char is created.
+		var strong = editor.document.getById( 'target' );
+		range.moveToPosition( strong, CKEDITOR.POSITION_AFTER_START );
+		editor.getSelection().selectRanges( [ range ] );
+
+		assert.areSame( 'strong', editor.getSelection().getStartElement().getName(),
+			'the selection was correctly placed inside empty strong element' );
+
+		range.moveToPosition( strong, CKEDITOR.POSITION_AFTER_END );
+		editor.getSelection().selectRanges( [ range ] );
+
+		assert.areSame( strong, editor.getSelection().getRanges()[ 0 ].getPreviousNode( isElement ),
+			'the selection was located after the strong element' );
+	},
+
+	'test selectRanges - after empty inline element with an empty text node': function() {
+		// IE8 can't handle this selection.
+		if ( CKEDITOR.env.ie && CKEDITOR.env.version == 8 ) {
+			assert.ignore();
+		}
+
+		var editor = this.editor,
+			range = editor.createRange();
+
+		editor.editable().setHtml( '<p>foo<strong id="target">x</strong></p>' );
+
+		// Set the selection inside the strong element, so the filler char is created.
+		var strong = editor.document.getById( 'target' );
+		strong.getFirst().setText( '' );
+
+		range.moveToPosition( strong, CKEDITOR.POSITION_AFTER_END );
+		editor.getSelection().selectRanges( [ range ] );
+
+		assert.areSame( strong, editor.getSelection().getRanges()[ 0 ].getPreviousNode( isElement ),
+			'the selection was located after the strong element' );
+	},
+
 	'test getSelectedElement': function() {
 		testSelectedElement( '[<img />]', 'img' );
 		testSelectedElement( '[<hr />]', 'hr' );

tests/tickets/12621/1.js

@@ -0,0 +1,22 @@
+/* bender-tags: selection,styles */
+/* bender-ckeditor-plugins: toolbar,basicstyles */
+
+( function() {
+	'use strict';
+
+	bender.editor = true;
+
+	bender.test( {
+		'test applying and removing bold from empty selection': function() {
+			var editor = this.editor;
+
+			bender.tools.selection.setWithHtml( editor, '<p>foo{}</p>' );
+
+			editor.execCommand( 'bold' );
+			assert.areSame( 'strong', editor.getSelection().getStartElement().getName(), 'start element after applying' );
+
+			editor.execCommand( 'bold' );
+			assert.areSame( 'p', editor.getSelection().getStartElement().getName(), 'start element after removing' );
+		}
+	} );
+} )();