Merge pull request #1582 from ckeditor/t/1439

Fix: Firefox should visually move the caret to the new line after a soft break. Closes #1439.

Author: Reinmar

src/view/renderer.js

@@ -3,6 +3,8 @@
  * For licensing, see LICENSE.md.
  */
 
+/* globals Node */
+
 /**
  * @module engine/view/renderer
  */
@@ -20,6 +22,7 @@ import CKEditorError from '@ckeditor/ckeditor5-utils/src/ckeditorerror';
 import isText from '@ckeditor/ckeditor5-utils/src/dom/istext';
 import isNode from '@ckeditor/ckeditor5-utils/src/dom/isnode';
 import fastDiff from '@ckeditor/ckeditor5-utils/src/fastdiff';
+import env from '@ckeditor/ckeditor5-utils/src/env';
 
 /**
  * Renderer is responsible for updating the DOM structure and the DOM selection based on
@@ -752,6 +755,11 @@ export default class Renderer {
 
 		domSelection.collapse( anchor.parent, anchor.offset );
 		domSelection.extend( focus.parent, focus.offset );
+
+		// Firefox–specific hack (https://github.com/ckeditor/ckeditor5-engine/issues/1439).
+		if ( env.isGecko ) {
+			fixGeckoSelectionAfterBr( focus, domSelection );
+		}
 	}
 
 	/**
@@ -923,3 +931,28 @@ function sameNodes( blockFiller, actualDomChild, expectedDomChild ) {
 	// Not matching types.
 	return false;
 }
+
+// The following is a Firefox–specific hack (https://github.com/ckeditor/ckeditor5-engine/issues/1439).
+// When the native DOM selection is at the end of the block and preceded by <br /> e.g.
+//
+//		<p>foo<br/>[]</p>
+//
+// which happens a lot when using the soft line break, the browser fails to (visually) move the
+// caret to the new line. A quick fix is as simple as force–refreshing the selection with the same range.
+function fixGeckoSelectionAfterBr( focus, domSelection ) {
+	const parent = focus.parent;
+
+	// This fix works only when the focus point is at the very end of an element.
+	// There is no point in running it in cases unrelated to the browser bug.
+	if ( parent.nodeType != Node.ELEMENT_NODE || focus.offset != parent.childNodes.length - 1 ) {
+		return;
+	}
+
+	const childAtOffset = parent.childNodes[ focus.offset ];
+
+	// To stay on the safe side, the fix being as specific as possible, it targets only the
+	// selection which is at the very end of the element and preceded by <br />.
+	if ( childAtOffset && childAtOffset.tagName == 'BR' ) {
+		domSelection.addRange( domSelection.getRangeAt( 0 ) );
+	}
+}

tests/manual/tickets/1439/1.html

@@ -0,0 +1 @@
+<div id="editor"></div>

tests/manual/tickets/1439/1.js

@@ -0,0 +1,33 @@
+/**
+ * @license Copyright (c) 2003-2018, CKSource - Frederico Knabben. All rights reserved.
+ * For licensing, see LICENSE.md.
+ */
+
+/* globals console, window, document */
+
+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: {
+			items: [
+				'heading',
+				'bold',
+				'italic',
+				'link',
+				'bulletedList',
+				'numberedList',
+				'blockQuote',
+				'undo',
+				'redo'
+			]
+		}
+	} )
+	.then( editor => {
+		window.editor = editor;
+	} )
+	.catch( err => {
+		console.error( err.stack );
+	} );

tests/manual/tickets/1439/1.md

@@ -0,0 +1,11 @@
+## Firefox should visually move the caret to the new line after a soft break [#1439](https://github.com/ckeditor/ckeditor5-engine/issues/1439)
+
+
+1. Open Firefox,
+2. In an empty editor type "foo",
+3. Press <kbd>Shift</kbd>+<kbd>Enter</kbd>.
+
+**Expected**
+
+1. The soft break should be created,
+2. The caret should be **in the new line and blinking**.

tests/view/renderer.js

@@ -25,6 +25,7 @@ import testUtils from '@ckeditor/ckeditor5-core/tests/_utils/utils';
 import createViewRoot from './_utils/createroot';
 import createElement from '@ckeditor/ckeditor5-utils/src/dom/createelement';
 import normalizeHtml from '@ckeditor/ckeditor5-utils/tests/_utils/normalizehtml';
+import env from '@ckeditor/ckeditor5-utils/src/env';
 
 describe( 'Renderer', () => {
 	let selection, domConverter, renderer;
@@ -1714,6 +1715,74 @@ describe( 'Renderer', () => {
 			expect( domRoot.innerHTML ).to.equal( '<ul><li>Foo<ul><li><b>Bar</b><i>Baz</i></li></ul></li></ul>' );
 		} );
 
+		// #1439
+		it( 'should not force–refresh the selection in non–Gecko browsers after a soft break', () => {
+			const domSelection = domRoot.ownerDocument.defaultView.getSelection();
+
+			testUtils.sinon.stub( env, 'isGecko' ).get( () => false );
+			const spy = testUtils.sinon.stub( domSelection, 'addRange' );
+
+			// <p>foo<br/>[]</p>
+			const { view: viewP, selection: newSelection } = parse(
+				'<container:p>' +
+					'foo[]' +
+					'<empty:br></empty:br>[]' +
+				'</container:p>' );
+
+			viewRoot._appendChild( viewP );
+			selection._setTo( newSelection );
+			renderer.markToSync( 'children', viewRoot );
+			renderer.render();
+
+			sinon.assert.notCalled( spy );
+		} );
+
+		// #1439
+		it( 'should force–refresh the selection in Gecko browsers after a soft break to nudge the caret', () => {
+			const domSelection = domRoot.ownerDocument.defaultView.getSelection();
+
+			testUtils.sinon.stub( env, 'isGecko' ).get( () => true );
+			const spy = testUtils.sinon.stub( domSelection, 'addRange' );
+
+			// <p>foo[]<b>bar</b></p>
+			let { view: viewP, selection: newSelection } = parse(
+				'<container:p>' +
+					'foo[]' +
+					'<attribute:b>bar</attribute:b>' +
+				'</container:p>' );
+
+			viewRoot._appendChild( viewP );
+			selection._setTo( newSelection );
+			renderer.markToSync( 'children', viewRoot );
+			renderer.render();
+
+			sinon.assert.notCalled( spy );
+
+			// <p>foo<b>bar</b></p><p>foo[]<br/></p>
+			( { view: viewP, selection: newSelection } = parse(
+				'<container:p>' +
+					'foo[]' +
+					'<empty:br></empty:br>' +
+				'</container:p>' ) );
+
+			viewRoot._appendChild( viewP );
+			selection._setTo( newSelection );
+			renderer.markToSync( 'children', viewRoot );
+			renderer.render();
+
+			sinon.assert.notCalled( spy );
+
+			// <p>foo<b>bar</b></p><p>foo<br/>[]</p>
+			selection._setTo( [
+				new ViewRange( new ViewPosition( viewP, 2 ), new ViewPosition( viewP, 2 ) )
+			] );
+
+			renderer.markToSync( 'children', viewRoot );
+			renderer.render();
+
+			sinon.assert.calledOnce( spy );
+		} );
+
 		describe( 'fake selection', () => {
 			beforeEach( () => {
 				const { view: viewP, selection: newSelection } = parse(