浏览代码

Merge branch 'master' into t/399

Piotrek Koszuliński 9 年之前
父节点
当前提交
eff17721fc

+ 29 - 1
packages/ckeditor5-engine/src/view/domconverter.js

@@ -360,12 +360,15 @@ export default class DomConverter {
 	domSelectionToView( domSelection ) {
 		const viewSelection = new ViewSelection();
 
+		const isBackward = this.isDomSelectionBackward( domSelection );
+
 		for ( let i = 0; i < domSelection.rangeCount; i++ ) {
+			// DOM Range have correct start and end, no matter what is the DOM Selection direction. So we don't have to fix anything.
 			const domRange = domSelection.getRangeAt( i );
 			const viewRange = this.domRangeToView( domRange );
 
 			if ( viewRange ) {
-				viewSelection.addRange( viewRange );
+				viewSelection.addRange( viewRange, isBackward );
 			}
 		}
 
@@ -675,6 +678,31 @@ export default class DomConverter {
 	}
 
 	/**
+	 * Returns `true` if given selection is a backward selection, that is, if it's `focus` is before `anchor`.
+	 *
+	 * @param {Selection} DOM Selection instance to check.
+	 * @returns {Boolean}
+	 */
+	isDomSelectionBackward( selection ) {
+		if ( selection.isCollapsed ) {
+			return false;
+		}
+
+		// Since it takes multiple lines of code to check whether a "DOM Position" is before/after another "DOM Position",
+		// we will use the fact that range will collapse if it's end is before it's start.
+		const range = new Range();
+
+		range.setStart( selection.anchorNode, selection.anchorOffset );
+		range.setEnd( selection.focusNode, selection.focusOffset );
+
+		const backward = range.collapsed;
+
+		range.detach();
+
+		return backward;
+	}
+
+	/**
 	 * Takes text data from given {@link engine.view.Text#data} and processes it so it is correctly displayed in DOM.
 	 *
 	 * Following changes are done:

+ 3 - 0
packages/ckeditor5-engine/src/view/observer/mutationobserver.js

@@ -216,6 +216,9 @@ export default class MutationObserver extends Observer {
 		}
 
 		this.document.fire( 'mutations', viewMutations, viewSelection );
+
+		// If nothing changes on `mutations` event, at this point we have "dirty DOM" (changed) and de-synched
+		// view (which has not been changed). In order to "reset DOM" we render the view again.
 		this.document.render();
 	}
 }

+ 2 - 0
packages/ckeditor5-engine/src/view/observer/selectionobserver.js

@@ -153,6 +153,8 @@ export default class SelectionObserver extends Observer {
 			domSelection: domSelection
 		} );
 
+		// If nothing changes on `selectionChange` event, at this point we have "dirty DOM" (changed) and de-synched
+		// view (which has not been changed). In order to "reset DOM" we render the view again.
 		this.document.render();
 	}
 

+ 12 - 17
packages/ckeditor5-engine/src/view/renderer.js

@@ -3,8 +3,6 @@
  * For licensing, see LICENSE.md.
  */
 
-/* globals Range */
-
 import ViewText from './text.js';
 import ViewElement from './element.js';
 import ViewPosition from './position.js';
@@ -422,7 +420,7 @@ export default class Renderer {
 	_updateSelection() {
 		// If there is no selection - remove it from DOM elements that belongs to the editor.
 		if ( this.selection.rangeCount === 0 ) {
-			this._removeDomSelction();
+			this._removeDomSelection();
 
 			return;
 		}
@@ -445,22 +443,19 @@ export default class Renderer {
 			return;
 		}
 
-		domSelection.removeAllRanges();
-
-		for ( let range of this.selection.getRanges() ) {
-			// Update ranges only in currently selected editable.
-			if ( range.start.parent.root == selectedEditable ) {
-				const domRangeStart = this.domConverter.viewPositionToDom( range.start );
-				const domRangeEnd = this.domConverter.viewPositionToDom( range.end );
-				const domRange = new Range();
-				domRange.setStart( domRangeStart.parent, domRangeStart.offset );
-				domRange.setEnd( domRangeEnd.parent, domRangeEnd.offset );
-				domSelection.addRange( domRange );
-			}
-		}
+		// Multi-range selection is not available in most browsers, and, at least in Chrome, trying to
+		// set such selection, that is not continuous, throws an error. Because of that, we will just use anchor
+		// and focus of view selection.
+		// Since we are not supporting multi-range selection, we also do not need to check if proper editable is
+		// selected. If there is any editable selected, it is okay (editable is taken from selection anchor).
+		const anchor = this.domConverter.viewPositionToDom( this.selection.anchor );
+		const focus = this.domConverter.viewPositionToDom( this.selection.focus );
+
+		domSelection.collapse( anchor.parent, anchor.offset );
+		domSelection.extend( focus.parent, focus.offset );
 	}
 
-	_removeDomSelction() {
+	_removeDomSelection() {
 		for ( let doc of this.domDocuments ) {
 			const domSelection = doc.getSelection();
 

+ 2 - 2
packages/ckeditor5-engine/src/view/selection.js

@@ -117,8 +117,8 @@ export default class Selection {
 	 * @type {engine.view.EditableElement|null}
 	 */
 	get editableElement() {
-		if ( this.rangeCount ) {
-			return this.getFirstPosition().editableElement;
+		if ( this.anchor ) {
+			return this.anchor.editableElement;
 		}
 
 		return null;

+ 7 - 0
packages/ckeditor5-engine/tests/tickets/401/1.html

@@ -0,0 +1,7 @@
+<head>
+	<link rel="stylesheet" href="%APPS_DIR%ckeditor/build/modules/amd/theme/ckeditor.css">
+</head>
+
+<div id="editor">
+	<p>Lorem <strong>ipsum</strong> dol &nbsp; &nbsp;or<strong> sit </strong>amet.</p>
+</div>

+ 19 - 0
packages/ckeditor5-engine/tests/tickets/401/1.js

@@ -0,0 +1,19 @@
+/**
+ * @license Copyright (c) 2003-2016, CKSource - Frederico Knabben. All rights reserved.
+ * For licensing, see LICENSE.md.
+ */
+
+/* globals console, document, window */
+
+import ClassicEditor from '/ckeditor5/editor-classic/classic.js';
+
+ClassicEditor.create( document.getElementById( 'editor' ), {
+	features: [ 'enter', 'typing', 'paragraph', 'basic-styles/bold' ],
+	toolbar: [ 'bold' ]
+} )
+.then( editor => {
+	window.editor = editor;
+} )
+.catch( err => {
+	console.error( err.stack );
+} );

+ 16 - 0
packages/ckeditor5-engine/tests/tickets/401/1.md

@@ -0,0 +1,16 @@
+@bender-ui: collapsed
+@bender-tags: ticket, 401, iteration4
+
+### Selection direction manual test [#401](https://github.com/ckeditor/ckeditor5-engine/issues/401)
+
+1. Click somewhere in word "amet".
+2. Drag slowly to the beginning of text, selecting all other words.
+
+Expected result: selection from "Lorem" to "amet" is made.
+Unexpected result: selection got broken or is incorrect.
+
+1. Focus somewhere in word "amet".
+2. Using <kbd>left arrow</kbd> and <kbd>shift</kbd> expand selection to the beginning of the text.
+
+Expected result: selection from "Lorem" to "amet" is made.
+Unexpected result: selection got broken or is incorrect.

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

@@ -1,5 +1,4 @@
 <head>
-	<meta charset="utf-8">
 	<link rel="stylesheet" href="%APPS_DIR%ckeditor/build/modules/amd/theme/ckeditor.css">
 </head>
 

+ 29 - 2
packages/ckeditor5-engine/tests/view/domconverter/dom-to-view.js

@@ -570,7 +570,7 @@ describe( 'DomConverter', () => {
 	} );
 
 	describe( 'domSelectionToView', () => {
-		it( 'should converter selection', () => {
+		it( 'should convert selection', () => {
 			const domFoo = document.createTextNode( 'foo' );
 			const domBar = document.createTextNode( 'bar' );
 			const domB = createElement( document, 'b', null, domBar );
@@ -597,7 +597,7 @@ describe( 'DomConverter', () => {
 			expect( stringify( viewP, viewSelection.getFirstRange() ) ).to.equal( '<p>f{oo<b>ba}r</b></p>' );
 		} );
 
-		it( 'should converter empty selection to empty selection', () => {
+		it( 'should convert empty selection to empty selection', () => {
 			const domSelection = document.getSelection();
 			domSelection.removeAllRanges();
 
@@ -606,6 +606,33 @@ describe( 'DomConverter', () => {
 			expect( viewSelection.rangeCount ).to.equal( 0 );
 		} );
 
+		it( 'should handle selection direction', () => {
+			const domFoo = document.createTextNode( 'foo' );
+			const domP = createElement( document, 'p', null, [ domFoo ] );
+
+			const viewP = parse( '<p>foo</p>' );
+
+			converter.bindElements( domP, viewP );
+
+			document.body.appendChild( domP );
+
+			const domRange = new Range();
+			domRange.setStart( domFoo, 2 );
+			domRange.collapse( true );
+
+			const domSelection = document.getSelection();
+			domSelection.removeAllRanges();
+			domSelection.addRange( domRange );
+			domSelection.extend( domFoo, 1 );
+
+			const viewSelection = converter.domSelectionToView( domSelection );
+
+			expect( viewSelection.rangeCount ).to.equal( 1 );
+			expect( viewSelection.anchor.offset ).to.equal( 2 );
+			expect( viewSelection.focus.offset ).to.equal( 1 );
+			expect( viewSelection.isBackward ).to.be.true;
+		} );
+
 		it( 'should not add null ranges', () => {
 			const domFoo = document.createTextNode( 'foo' );
 			const domBar = document.createTextNode( 'bar' );

+ 3 - 29
packages/ckeditor5-engine/tests/view/renderer.js

@@ -291,6 +291,8 @@ describe( 'Renderer', () => {
 		} );
 
 		it( 'should not care about filler if there is no DOM', () => {
+			selectionEditable = null;
+
 			const { view: viewP, selection: newSelection } = parse(
 				'<container:p>foo<attribute:b>[]</attribute:b>bar</container:p>' );
 
@@ -918,29 +920,7 @@ describe( 'Renderer', () => {
 			expect( domSelection.getRangeAt( 0 ).collapsed ).to.equal( true );
 		} );
 
-		it( 'should not add ranges if different editable is selected', () => {
-			const domHeader = document.createElement( 'h1' );
-			const viewHeader = new ViewElement( 'h1' );
-			document.body.appendChild( domHeader );
-
-			domConverter.bindElements( domHeader, viewHeader );
-
-			selectionEditable = viewHeader;
-
-			const { view: viewP, selection: newSelection } = parse( '<container:p>fo{o}</container:p>' );
-
-			viewRoot.appendChildren( viewP );
-			selection.setTo( newSelection );
-
-			renderer.render();
-
-			const domSelection = document.getSelection();
-			expect( domSelection.rangeCount ).to.equal( 0 );
-		} );
-
 		it( 'should not add inline filler after text node', () => {
-			const domSelection = document.getSelection();
-
 			const { view: viewP, selection: newSelection } = parse( '<container:p>foo[]</container:p>' );
 
 			viewRoot.appendChildren( viewP );
@@ -951,13 +931,7 @@ describe( 'Renderer', () => {
 
 			const domP = domRoot.childNodes[ 0 ];
 
-			expect( domP.childNodes.length ).to.equal( 1 );
-			expect( domP.childNodes[ 0 ].data ).to.equal( 'foo' );
-
-			expect( domSelection.rangeCount ).to.equal( 1 );
-			expect( domSelection.getRangeAt( 0 ).startContainer ).to.equal( domP );
-			expect( domSelection.getRangeAt( 0 ).startOffset ).to.equal( 1 );
-			expect( domSelection.getRangeAt( 0 ).collapsed ).to.be.true;
+			expect( domP.innerHTML.indexOf( INLINE_FILLER ) ).to.equal( -1 );
 		} );
 
 		it( 'should throw if there is no filler in expected position', () => {