Browse Source

Merge branch 'master' into t/1555

# Conflicts:
#	src/model/model.js
#	src/view/position.js
#	tests/model/position.js
#	tests/view/position.js
Maciej Gołaszewski 7 years ago
parent
commit
c3c8399c32

+ 6 - 2
packages/ckeditor5-engine/src/model/model.js

@@ -309,6 +309,9 @@ export default class Model {
 	 *
 	 *		// Insert text at given position - document selection will not be modified.
 	 *		editor.model.change( writer => {
+	 *			editor.model.insertContent( writer.createText( 'x' ), doc.getRoot(), 2 );
+	 *
+	 *			// Which is a shorthand for:
 	 *			editor.model.insertContent( writer.createText( 'x' ), writer.createPositionAt( doc.getRoot(), 2 ) );
 	 *		} );
 	 *
@@ -329,10 +332,11 @@ export default class Model {
 	 * @fires insertContent
 	 * @param {module:engine/model/documentfragment~DocumentFragment|module:engine/model/item~Item} content The content to insert.
 	 * @param {module:engine/model/selection~Selection|module:engine/model/documentselection~DocumentSelection|
-	 * module:engine/model/position~Position|module:engine/model/element~Element|
+	 * module:engine/model/position~Position|module:engine/model/item~Item|
 	 * Iterable.<module:engine/model/range~Range>|module:engine/model/range~Range|null} [selectable=model.document.selection]
 	 * Selection into which the content should be inserted. If not provided the current model document selection will be used.
-	 * @param {Number|'before'|'end'|'after'|'on'|'in'} [placeOrOffset] Sets place or offset of the selection.
+	 * @param {Number|'before'|'end'|'after'|'on'|'in'} [placeOrOffset] To be used when a model item was passed as `selectable`.
+	 * This param defines a position in relation to that item.
 	 */
 	insertContent( content, selectable, placeOrOffset ) {
 		insertContent( this, content, selectable, placeOrOffset );

+ 8 - 2
packages/ckeditor5-engine/src/model/position.js

@@ -831,9 +831,15 @@ export default class Position {
 			} else if ( offset == 'after' ) {
 				return this._createAfter( node );
 			} else if ( offset !== 0 && !offset ) {
+				/**
+				 * {@link module:engine/model/position~Position.createAt `Position.createAt()`}
+				 * requires the offset to be specified when the first parameter is a model item.
+				 *
+				 * @error model-position-createAt-offset-required
+				 */
 				throw new CKEditorError(
-					'model-position-createAt-required-second-parameter: ' +
-					'Position.createAt requires the second parameter offset when first parameter is a model item.' );
+					'model-position-createAt-offset-required: ' +
+					'Position.createAt() requires the offset when the first parameter is a model item.' );
 			}
 
 			if ( !node.is( 'element' ) && !node.is( 'documentFragment' ) ) {

+ 1 - 1
packages/ckeditor5-engine/src/model/writer.js

@@ -1134,7 +1134,7 @@ export default class Writer {
 	 * {@link #createPositionAt `writer.createPositionAt()`} parameters.
 	 *
 	 * @param {module:engine/model/item~Item|module:engine/model/position~Position} itemOrPosition
-	 * @param {Number|'end'|'before'|'after'} [of~fset=0] Offset or one of the flags. Used only when
+	 * @param {Number|'end'|'before'|'after'} [offset=0] Offset or one of the flags. Used only when
 	 * first parameter is a {@link module:engine/model/item~Item model item}.
 	 */
 	setSelectionFocus( itemOrPosition, offset ) {

+ 9 - 3
packages/ckeditor5-engine/src/view/position.js

@@ -303,7 +303,7 @@ export default class Position {
 	 * * {@link module:engine/view/position~Position._createAfter}.
 	 *
 	 * @protected
-	 * @param {module:engine/view/item~Item|module:engine/model/position~Position} itemOrPosition
+	 * @param {module:engine/view/item~Item|module:engine/view/position~Position} itemOrPosition
 	 * @param {Number|'end'|'before'|'after'} [offset] Offset or one of the flags. Used only when
 	 * first parameter is a {@link module:engine/view/item~Item view item}.
 	 */
@@ -320,9 +320,15 @@ export default class Position {
 			} else if ( offset == 'after' ) {
 				return this._createAfter( node );
 			} else if ( offset !== 0 && !offset ) {
+				/**
+				 * {@link module:engine/view/position~Position.createAt `Position.createAt()`}
+				 * requires the offset to be specified when the first parameter is a view item.
+				 *
+				 * @error view-position-createAt-offset-required
+				 */
 				throw new CKEditorError(
-					'view-position-createAt-required-second-parameter: ' +
-					'Position.createAt requires the second parameter offset when first parameter is a view item.' );
+					'view-position-createAt-offset-required: ' +
+					'Position.createAt() requires the offset when the first parameter is a view item.' );
 			}
 
 			return new Position( node, offset );

+ 33 - 0
packages/ckeditor5-engine/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 ) );
+	}
+}

+ 1 - 0
packages/ckeditor5-engine/tests/manual/tickets/1439/1.html

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

+ 33 - 0
packages/ckeditor5-engine/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 );
+	} );

+ 11 - 0
packages/ckeditor5-engine/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**.

+ 69 - 0
packages/ckeditor5-engine/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(