Bläddra i källkod

Reimplemented filler managment in the render.

Piotrek Koszuliński 9 år sedan
förälder
incheckning
a1e595b4e8

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

@@ -104,7 +104,7 @@ export default class Position {
 	}
 
 	/**
-	 * Offset at which this position is located in it's {@link engine.model.Position#parent parent}. It is equal
+	 * Offset at which this position is located in its {@link engine.model.Position#parent parent}. It is equal
 	 * to the last item in position {@link engine.model.Position#path path}.
 	 *
 	 * @type {Number}

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

@@ -486,7 +486,8 @@ export default class DomConverter {
 			} else {
 				const viewBefore = this.getCorrespondingView( domParent.childNodes[ domOffset - 1 ] );
 
-				if ( viewBefore ) {
+				// TODO #663
+				if ( viewBefore && viewBefore.parent ) {
 					return new ViewPosition( viewBefore.parent, viewBefore.index + 1 );
 				}
 			}

+ 67 - 49
packages/ckeditor5-engine/src/view/renderer.js

@@ -88,13 +88,12 @@ export default class Renderer {
 		this.selection = selection;
 
 		/**
-		 * Position of the inline {@link engine.view.filler filler}.
-		 * It should always be put before the text which contains filler.
+		 * TODO
 		 *
 		 * @private
-		 * @member {engine.view.Position} engine.view.Renderer#_inlineFillerPosition
+		 * @member {Text} engine.view.Renderer#_inlineFiller
 		 */
-		this._inlineFillerPosition = null;
+		this._inlineFiller = null;
 
 		/**
 		 * Indicates if view document is focused and selection can be rendered. Selection will not be rendered if
@@ -176,21 +175,29 @@ export default class Renderer {
 	 * removed as long selection is in the text node which needed it at first.
 	 */
 	render() {
-		if ( !this._isInlineFillerAtSelection() ) {
+		let inlineFillerPosition;
+
+		// There was inline filler rendered in the DOM but it's not
+		// at the selection position any more, so we can remove it
+		// (cause even if it's needed, it must be placed in another location).
+		if ( this._inlineFiller && !this._isSelectionInInlineFiller() ) {
 			this._removeInlineFiller();
+		}
 
-			if ( this._needAddInlineFiller() ) {
-				this._inlineFillerPosition = this.selection.getFirstPosition();
-				// Do not use `markToSync` so it will be added even if the parent is already added.
-				this.markedChildren.add( this._inlineFillerPosition.parent );
-			} else {
-				this._inlineFillerPosition = null;
+		if ( this._needsInlineFillerAtSelection() ) {
+			inlineFillerPosition = this.selection.getFirstPosition();
+
+			// Do not use `markToSync` so it will be added even if the parent is already added.
+			this.markedChildren.add( inlineFillerPosition.parent );
+		} else if ( this._inlineFiller ) {
+			if ( this.selection.getFirstPosition().parent instanceof ViewText ) {
+				inlineFillerPosition = ViewPosition.createBefore( this.selection.getFirstPosition().parent );
 			}
 		}
 
 		for ( let node of this.markedTexts ) {
 			if ( !this.markedChildren.has( node.parent ) && this.domConverter.getCorrespondingDom( node.parent ) ) {
-				this._updateText( node );
+				this._updateText( node, { inlineFillerPosition } );
 			}
 		}
 
@@ -199,7 +206,7 @@ export default class Renderer {
 		}
 
 		for ( let element of this.markedChildren ) {
-			this._updateChildren( element );
+			this._updateChildren( element, { inlineFillerPosition } );
 		}
 
 		this._updateSelection();
@@ -208,63 +215,72 @@ export default class Renderer {
 		this.markedTexts.clear();
 		this.markedAttributes.clear();
 		this.markedChildren.clear();
+
+		this._storeInlineFiller( inlineFillerPosition );
+	}
+
+	_storeInlineFiller( fillerPosition ) {
+		if ( !fillerPosition ) {
+			this._inlineFiller = null;
+
+			return;
+		}
+
+		const domPosition = this.domConverter.viewPositionToDom( fillerPosition );
+
+		if ( !domPosition || !startsWithFiller( domPosition.parent ) ) {
+			throw new CKEditorError( 'filler-cannot-find-node' );
+		}
+
+		this._inlineFiller = domPosition.parent;
 	}
 
 	/**
-	 * Returns `true` if the inline filler and selection are in the same place.
-	 * If it is true it means filler had been added for a reason and selection does not
-	 * left text node, user can be in the middle of the composition so it should not be touched.
+	 * Returns `true` if the selection hasn't left the inline filler's text node.
+	 * If it is `true` it means that the filler had been added for a reason and the selection does not
+	 * left the filler's text node. E.g. the user can be in the middle of a composition so it should not be touched.
 	 *
 	 * @private
 	 * @returns {Boolean} True if the inline filler and selection are in the same place.
 	 */
-	_isInlineFillerAtSelection() {
+	_isSelectionInInlineFiller() {
 		if ( this.selection.rangeCount != 1 || !this.selection.isCollapsed ) {
 			return false;
 		}
 
-		const selectionPosition = this.selection.getFirstPosition();
-		const fillerPosition = this._inlineFillerPosition;
+		// Note, we can't check if selection's position equals position of the
+		// this._inlineFiller node, because of #663. We may not be able to calculate
+		// the filler's position in the view at this stage.
+		// Instead, we check it the other way – whether selection is anchored in
+		// that text node or next to it.
 
-		if ( !fillerPosition ) {
-			return false;
-		}
+		// Possible options are:
+		// "FILLER{}"
+		// "FILLERadded-text{}"
 
-		if ( fillerPosition.isEqual( selectionPosition )  ) {
-			return true;
-		}
+		const { parent: domParent } = this.domConverter.viewPositionToDom( this.selection.getFirstPosition() );
 
-		if ( selectionPosition.parent instanceof ViewText ) {
-			if ( fillerPosition.isEqual( ViewPosition.createBefore( selectionPosition.parent ) ) ) {
-				return true;
-			}
+		if ( this.domConverter.isText( domParent ) && startsWithFiller( domParent ) ) {
+			return true;
 		}
 
 		return false;
 	}
 
 	/**
-	 * Removes inline filler.
+	 * Removes the inline filler.
 	 *
 	 * @private
 	 */
 	_removeInlineFiller() {
-		if ( !this._inlineFillerPosition ) {
-			// Nothing to remove.
-			return;
-		}
-
-		const domFillerPosition = this.domConverter.viewPositionToDom( this._inlineFillerPosition );
-		const domFillerNode = domFillerPosition.parent;
+		const domFillerNode = this._inlineFiller;
 
-		// If there is no filler viewPositionToDom will return parent node, so domFillerNode will be an element.
-		if ( !( this.domConverter.isText( domFillerNode ) ) || !startsWithFiller( domFillerNode ) ) {
+		// Something weird happened and the stored node doesn't contain the filler's text.
+		if ( !startsWithFiller( domFillerNode ) ) {
 			/**
-			 * No inline filler on expected position.
-			 *
-			 * @error renderer-render-no-inline-filler.
+			 * TODO
 			 */
-			throw new CKEditorError( 'view-renderer-render-no-inline-filler: No inline filler on expected position.' );
+			throw new CKEditorError( 'todo: TODO' );
 		}
 
 		if ( isInlineFiller( domFillerNode ) ) {
@@ -272,15 +288,17 @@ export default class Renderer {
 		} else {
 			domFillerNode.data = domFillerNode.data.substr( INLINE_FILLER_LENGTH );
 		}
+
+		this._inlineFiller = null;
 	}
 
 	/**
-	 * Checks if the inline {@link engine.view.filler fillers} should be added.
+	 * Checks if the inline {@link engine.view.filler filler} should be added.
 	 *
 	 * @private
 	 * @returns {Boolean} True if the inline fillers should be added.
 	 */
-	_needAddInlineFiller() {
+	_needsInlineFillerAtSelection() {
 		if ( this.selection.rangeCount != 1 || !this.selection.isCollapsed ) {
 			return false;
 		}
@@ -319,14 +337,14 @@ export default class Renderer {
 	 * @private
 	 * @param {engine.view.Text} viewText View text to update.
 	 */
-	_updateText( viewText ) {
+	_updateText( viewText, options ) {
 		const domText = this.domConverter.getCorrespondingDom( viewText );
 		const newDomText = this.domConverter.viewToDom( viewText, domText.ownerDocument );
 
 		const actualText = domText.data;
 		let expectedText = newDomText.data;
 
-		const filler = this._inlineFillerPosition;
+		const filler = options.inlineFillerPosition;
 
 		if ( filler && filler.parent == viewText.parent && filler.offset == viewText.index ) {
 			expectedText = INLINE_FILLER + expectedText;
@@ -367,12 +385,12 @@ export default class Renderer {
 	 * @private
 	 * @param {engine.view.Element} viewElement View element to update.
 	 */
-	_updateChildren( viewElement ) {
+	_updateChildren( viewElement, options ) {
 		const domConverter = this.domConverter;
 		const domElement = domConverter.getCorrespondingDom( viewElement );
 		const domDocument = domElement.ownerDocument;
 
-		const filler = this._inlineFillerPosition;
+		const filler = options.inlineFillerPosition;
 
 		const actualDomChildren = domElement.childNodes;
 		const expectedDomChildren = Array.from( domConverter.viewChildrenToDom( viewElement, domDocument, { bind: true } ) );

+ 0 - 1
packages/ckeditor5-engine/tests/view/document/jumpoverinlinefiller.html

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

+ 61 - 32
packages/ckeditor5-engine/tests/view/document/jumpoverinlinefiller.js

@@ -3,7 +3,7 @@
  * For licensing, see LICENSE.md.
  */
 
-/* globals document */
+/* globals document, Range */
 /* bender-tags: view, browser-only */
 
 import ViewRange from '/ckeditor5/engine/view/range.js';
@@ -11,21 +11,34 @@ import ViewDocument from '/ckeditor5/engine/view/document.js';
 import { INLINE_FILLER_LENGTH, isInlineFiller, startsWithFiller } from '/ckeditor5/engine/view/filler.js';
 
 import { keyCodes } from '/ckeditor5/utils/keyboard.js';
+import createElement from '/ckeditor5/utils/dom/createelement.js';
 
 import { parse, setData } from '/ckeditor5/engine/dev-utils/view.js';
 
 describe( 'Document', () => {
-	let viewDocument;
+	let viewDocument, domRoot;
+
+	beforeEach( () => {
+		domRoot = createElement( document, 'div', {
+			contenteditable: 'true'
+		} );
+		document.body.appendChild( domRoot );
 
-	before( () => {
 		viewDocument = new ViewDocument();
-		viewDocument.createRoot( document.getElementById( 'editor' ) ) ;
+		viewDocument.createRoot( domRoot );
 
 		document.getSelection().removeAllRanges();
 
 		viewDocument.isFocused = true;
 	} );
 
+	afterEach( () => {
+		// There's no destroy() method yet, so let's at least kill the observers.
+		viewDocument.disableObservers();
+
+		domRoot.parentElement.removeChild( domRoot );
+	} );
+
 	describe( 'jump over inline filler hack', () => {
 		it( 'should jump over inline filler when left arrow is pressed after inline filler', () => {
 			setData( viewDocument, '<container:p>foo<attribute:b>[]</attribute:b>bar</container:p>' );
@@ -33,10 +46,11 @@ describe( 'Document', () => {
 
 			viewDocument.fire( 'keydown', { keyCode: keyCodes.arrowleft, domTarget: viewDocument.domRoots.get( 'main' ) } );
 
-			const domRange = document.getSelection().getRangeAt( 0 );
-			expect( isInlineFiller( domRange.startContainer ) ).to.be.true;
-			expect( domRange.startOffset ).to.equal( 0 );
-			expect( domRange.collapsed ).to.be.true;
+			const domSelection = document.getSelection();
+
+			expect( domSelection.anchorNode.data ).to.equal( 'foo' );
+			expect( domSelection.anchorOffset ).to.equal( 3 );
+			expect( domSelection.isCollapsed ).to.be.true;
 		} );
 
 		it( 'should do nothing when another key is pressed', () => {
@@ -45,10 +59,11 @@ describe( 'Document', () => {
 
 			viewDocument.fire( 'keydown', { keyCode: keyCodes.arrowright, domTarget: viewDocument.domRoots.get( 'main' ) } );
 
-			const domRange = document.getSelection().getRangeAt( 0 );
-			expect( isInlineFiller( domRange.startContainer ) ).to.be.true;
-			expect( domRange.startOffset ).to.equal( INLINE_FILLER_LENGTH );
-			expect( domRange.collapsed ).to.be.true;
+			const domSelection = document.getSelection();
+
+			expect( isInlineFiller( domSelection.anchorNode ) ).to.be.true;
+			expect( domSelection.anchorOffset ).to.equal( INLINE_FILLER_LENGTH );
+			expect( domSelection.isCollapsed ).to.be.true;
 		} );
 
 		it( 'should do nothing if range is not collapsed', () => {
@@ -57,43 +72,57 @@ describe( 'Document', () => {
 
 			viewDocument.fire( 'keydown', { keyCode: keyCodes.arrowleft, domTarget: viewDocument.domRoots.get( 'main' ) } );
 
-			const domRange = document.getSelection().getRangeAt( 0 );
-			expect( domRange.startContainer.data ).to.equal( 'x' );
-			expect( domRange.startOffset ).to.equal( 0 );
-			expect( domRange.endContainer.data ).to.equal( 'x' );
-			expect( domRange.endOffset ).to.equal( 1 );
+			const domSelection = document.getSelection();
+
+			expect( domSelection.anchorNode.data ).to.equal( 'x' );
+			expect( domSelection.anchorOffset ).to.equal( 0 );
+			expect( domSelection.focusNode.data ).to.equal( 'x' );
+			expect( domSelection.focusOffset ).to.equal( 1 );
 		} );
 
-		it( 'should do nothing if node does not start with filler', () => {
-			setData( viewDocument, '<container:p>foo<attribute:b>{}x</attribute:b>bar</container:p>' );
-			viewDocument.render();
+		// See #664
+		// it( 'should do nothing if node does not start with the filler', () => {
+		// 	setData( viewDocument, '<container:p>foo<attribute:b>{}x</attribute:b>bar</container:p>' );
+		// 	viewDocument.render();
 
-			viewDocument.fire( 'keydown', { keyCode: keyCodes.arrowleft, domTarget: viewDocument.domRoots.get( 'main' ) } );
+		// 	viewDocument.fire( 'keydown', { keyCode: keyCodes.arrowleft, domTarget: viewDocument.domRoots.get( 'main' ) } );
 
-			const domRange = document.getSelection().getRangeAt( 0 );
-			expect( domRange.startContainer.data ).to.equal( 'x' );
-			expect( domRange.startOffset ).to.equal( 0 );
-			expect( domRange.collapsed ).to.be.true;
-		} );
+		// 	const domSelection = document.getSelection();
+
+		// 	expect( domSelection.anchorNode.data ).to.equal( 'x' );
+		// 	expect( domSelection.anchorOffset ).to.equal( INLINE_FILLER_LENGTH );
+		// 	expect( domSelection.isCollapsed ).to.be.true;
+		// } );
 
-		it( 'should do nothing if caret is not directly before filler', () => {
+		it( 'should do nothing if caret is not directly before the filler', () => {
 			setData( viewDocument, '<container:p>foo<attribute:b>[]</attribute:b>bar</container:p>' );
 			viewDocument.render();
 
-			// '<container:p>foo<attribute:b>x{}</attribute:b>bar</container:p>'
+			// Insert a letter to the <b>: '<container:p>foo<attribute:b>x{}</attribute:b>bar</container:p>'
+			// Do this both in the view and in the DOM to simulate typing and to avoid rendering (which would remove the filler).
 			const viewB = viewDocument.selection.getFirstPosition().parent;
 			const viewTextX = parse( 'x' );
 			viewB.appendChildren( viewTextX );
 			viewDocument.selection.removeAllRanges();
 			viewDocument.selection.addRange( ViewRange.createFromParentsAndOffsets( viewTextX, 1, viewTextX, 1 ) );
+
+			const domB = viewDocument.getDomRoot( 'main' ).querySelector( 'b' );
+			const domSelection = document.getSelection();
+			domB.childNodes[ 0 ].data += 'x';
+
+			const domRange = new Range();
+			domSelection.removeAllRanges();
+			domRange.setStart( domB.childNodes[ 0 ], INLINE_FILLER_LENGTH + 1 );
+			domRange.collapse( true );
+			domSelection.addRange( domRange );
+
 			viewDocument.render();
 
 			viewDocument.fire( 'keydown', { keyCode: keyCodes.arrowleft, domTarget: viewDocument.domRoots.get( 'main' ) } );
 
-			const domRange = document.getSelection().getRangeAt( 0 );
-			expect( startsWithFiller( domRange.startContainer ) ).to.be.true;
-			expect( domRange.startOffset ).to.equal( INLINE_FILLER_LENGTH + 1 );
-			expect( domRange.collapsed ).to.be.true;
+			expect( startsWithFiller( domSelection.anchorNode ) ).to.be.true;
+			expect( domSelection.anchorOffset ).to.equal( INLINE_FILLER_LENGTH + 1 );
+			expect( domSelection.isCollapsed ).to.be.true;
 		} );
 	} );
 } );

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

@@ -637,6 +637,7 @@ describe( 'Renderer', () => {
 			selection.addRange( ViewRange.createFromParentsAndOffsets( viewP, 0, viewP, 0 ) );
 
 			renderer.markToSync( 'children', viewP );
+			renderer.markToSync( 'children', viewP2 );
 			renderer.render();
 
 			// Step 3: Check whether in the first paragrpah there's a <br> filler and that
@@ -771,6 +772,8 @@ describe( 'Renderer', () => {
 		it( 'should handle typing in empty attribute, do nothing if changes are already applied', () => {
 			const domSelection = document.getSelection();
 
+			// 1. Render <p><b>FILLER{}</b>foo</p>.
+
 			const { view: viewP, selection: newSelection } = parse(
 				'<container:p><attribute:b>[]</attribute:b>foo</container:p>' );
 
@@ -780,6 +783,8 @@ describe( 'Renderer', () => {
 			renderer.markToSync( 'children', viewRoot );
 			renderer.render();
 
+			// 2. Check the DOM.
+
 			const domP = domRoot.childNodes[ 0 ];
 
 			expect( domP.childNodes.length ).to.equal( 2 );
@@ -797,7 +802,8 @@ describe( 'Renderer', () => {
 			expect( domSelection.getRangeAt( 0 ).startOffset ).to.equal( INLINE_FILLER_LENGTH );
 			expect( domSelection.getRangeAt( 0 ).collapsed ).to.be.true;
 
-			// Add text node to both DOM and View <p><b>x</b>foo</p>
+			// 3. Add text node to both the DOM and the view: <p><b>FILLERx</b>foo</p>.
+
 			domB.childNodes[ 0 ].data += 'x';
 
 			domSelection.removeAllRanges();
@@ -818,6 +824,8 @@ describe( 'Renderer', () => {
 		it( 'should handle typing in empty attribute as a children change, render if needed', () => {
 			const domSelection = document.getSelection();
 
+			// 1. Render <p><b>FILLER{}</b>foo</p>.
+
 			const { view: viewP, selection: newSelection } = parse(
 				'<container:p><attribute:b>[]</attribute:b>foo</container:p>' );
 
@@ -827,6 +835,8 @@ describe( 'Renderer', () => {
 			renderer.markToSync( 'children', viewRoot );
 			renderer.render();
 
+			// 2. Check the DOM.
+
 			const domP = domRoot.childNodes[ 0 ];
 
 			expect( domP.childNodes.length ).to.equal( 2 );
@@ -844,7 +854,8 @@ describe( 'Renderer', () => {
 			expect( domSelection.getRangeAt( 0 ).startOffset ).to.equal( INLINE_FILLER_LENGTH );
 			expect( domSelection.getRangeAt( 0 ).collapsed ).to.be.true;
 
-			// Add text node only to View <p><b>x</b>foo</p>
+			// 3. Add text node only to the view: <p><b>x{}</b>foo</p>.
+
 			const viewText = new ViewText( 'x' );
 			viewB.appendChildren( viewText );
 			selection.removeAllRanges();
@@ -865,6 +876,8 @@ describe( 'Renderer', () => {
 		it( 'should handle typing in empty attribute as a text change, render if needed', () => {
 			const domSelection = document.getSelection();
 
+			// 1. Render <p><b>FILLER{}</b>foo</p>.
+
 			const { view: viewP, selection: newSelection } = parse(
 				'<container:p><attribute:b>[]</attribute:b>foo</container:p>' );
 
@@ -874,6 +887,8 @@ describe( 'Renderer', () => {
 			renderer.markToSync( 'children', viewRoot );
 			renderer.render();
 
+			// 2. Check the DOM.
+
 			const domP = domRoot.childNodes[ 0 ];
 
 			expect( domP.childNodes.length ).to.equal( 2 );
@@ -891,7 +906,8 @@ describe( 'Renderer', () => {
 			expect( domSelection.getRangeAt( 0 ).startOffset ).to.equal( INLINE_FILLER_LENGTH );
 			expect( domSelection.getRangeAt( 0 ).collapsed ).to.be.true;
 
-			// Add text node only to View <p><b>x</b>foo</p>
+			// 3. Add text node only to the view: <p><b>x{}</b>foo</p>.
+
 			const viewText = new ViewText( 'x' );
 			viewB.appendChildren( viewText );
 			selection.removeAllRanges();
@@ -900,6 +916,8 @@ describe( 'Renderer', () => {
 			renderer.markToSync( 'text', viewText );
 			renderer.render();
 
+			// 4. Check the DOM.
+
 			expect( domB.childNodes.length ).to.equal( 1 );
 			expect( domB.childNodes[ 0 ].data ).to.equal( INLINE_FILLER + 'x' );