Explorar el Código

Merge pull request #513 from ckeditor/t/502

Removing selection from DOM if there is no selection in the view.
Piotr Jasiun hace 9 años
padre
commit
dc453048e1

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

@@ -191,7 +191,7 @@ export default class Document {
 		viewRoot.on( 'change:attributes', ( evt, node ) => this.renderer.markToSync( 'attributes', node ) );
 		viewRoot.on( 'change:text', ( evt, node ) => this.renderer.markToSync( 'text', node ) );
 
-		if ( domRoot instanceof HTMLElement ) {
+		if ( this.domConverter.isElement( domRoot ) ) {
 			this.attachDomRoot( domRoot, name );
 		}
 
@@ -217,6 +217,7 @@ export default class Document {
 		this.domConverter.bindElements( domRoot, viewRoot );
 
 		this.renderer.markToSync( 'children', viewRoot );
+		this.renderer.domDocuments.add( domRoot.ownerDocument );
 
 		for ( let observer of this._observers.values() ) {
 			observer.observe( domRoot, name );

+ 38 - 8
packages/ckeditor5-engine/src/view/domconverter.js

@@ -234,7 +234,7 @@ export default class DomConverter {
 
 			// If there is an inline filler at position return position inside the filler. We should never return
 			// the position before the inline filler.
-			if ( domAfter instanceof Text && startsWithFiller( domAfter ) ) {
+			if ( this.isText( domAfter ) && startsWithFiller( domAfter ) ) {
 				return { parent: domAfter, offset: INLINE_FILLER_LENGTH };
 			}
 
@@ -261,7 +261,7 @@ export default class DomConverter {
 			return null;
 		}
 
-		if ( domNode instanceof Text ) {
+		if ( this.isText( domNode ) ) {
 			if ( isInlineFiller( domNode ) ) {
 				return null;
 			} else {
@@ -274,7 +274,7 @@ export default class DomConverter {
 
 			let viewElement;
 
-			if ( domNode instanceof DocumentFragment ) {
+			if ( this.isDocumentFragment( domNode ) ) {
 				// Create view document fragment.
 				viewElement = new ViewDocumentFragment();
 
@@ -383,7 +383,7 @@ export default class DomConverter {
 			return this.domPositionToView( domParent.parentNode, indexOf( domParent ) );
 		}
 
-		if ( domParent instanceof Text ) {
+		if ( this.isText( domParent ) ) {
 			if ( isInlineFiller( domParent ) ) {
 				return this.domPositionToView( domParent.parentNode, indexOf( domParent ) );
 			}
@@ -435,11 +435,11 @@ export default class DomConverter {
 	 * @returns {engine.view.Node|engine.view.DocumentFragment|null} Corresponding view item.
 	 */
 	getCorrespondingView( domNode ) {
-		if ( domNode instanceof HTMLElement ) {
+		if ( this.isElement( domNode ) ) {
 			return this.getCorrespondingViewElement( domNode );
-		} else if ( domNode instanceof DocumentFragment ) {
+		} else if ( this.isDocumentFragment( domNode ) ) {
 			return this.getCorrespondingViewDocumentFragment( domNode );
-		} else if ( domNode instanceof Text ) {
+		} else if ( this.isText( domNode ) ) {
 			return this.getCorrespondingViewText( domNode );
 		}
 
@@ -495,7 +495,7 @@ export default class DomConverter {
 
 		// Try to use previous sibling to find the corresponding text node.
 		if ( previousSibling ) {
-			if ( !( previousSibling instanceof HTMLElement ) ) {
+			if ( !( this.isElement( previousSibling ) ) ) {
 				// The previous is text or comment.
 				return null;
 			}
@@ -619,4 +619,34 @@ export default class DomConverter {
 			domEditable.focus();
 		}
 	}
+
+	/**
+	 * Returns `true` when `node.nodeType` equals `Node.TEXT_NODE`.
+	 *
+	 * @param {Node} node Node to check.
+	 * @returns {Boolean}
+	 */
+	isText( node ) {
+		return node && node.nodeType == Node.TEXT_NODE;
+	}
+
+	/**
+	 * Returns `true` when `node.nodeType` equals `Node.ELEMENT_NODE`.
+	 *
+	 * @param {Node} node Node to check.
+	 * @returns {Boolean}
+	 */
+	isElement( node ) {
+		return node && node.nodeType == Node.ELEMENT_NODE;
+	}
+
+	/**
+	 * Returns `true` when `node.nodeType` equals `Node.DOCUMENT_FRAGMENT_NODE`.
+	 *
+	 * @param {Node} node Node to check.
+	 * @returns {Boolean}
+	 */
+	isDocumentFragment( node ) {
+		return node && node.nodeType == Node.DOCUMENT_FRAGMENT_NODE;
+	}
 }

+ 33 - 4
packages/ckeditor5-engine/src/view/renderer.js

@@ -40,6 +40,13 @@ export default class Renderer {
 	 * @param {engine.view.Selection} selection View selection.
 	 */
 	constructor( domConverter, selection ) {
+		/**
+		 * Set of DOM Documents instances.
+		 *
+		 * @member {Set.<Document>} engine.view.Renderer#domDocuments
+		 */
+		this.domDocuments = new Set();
+
 		/**
 		 * Converter instance.
 		 *
@@ -93,7 +100,7 @@ export default class Renderer {
 		 * Indicates if view document is focused and selection can be rendered. Selection will not be rendered if
 		 * this is set to `false`.
 		 *
-		 * @type {boolean}
+		 * @member {Boolean} engine.view.Renderer#isFocused
 		 */
 		this.isFocused = false;
 	}
@@ -243,7 +250,7 @@ export default class Renderer {
 		const domFillerNode = domFillerPosition.parent;
 
 		// If there is no filler viewPositionToDom will return parent node, so domFillerNode will be an element.
-		if ( !( domFillerNode instanceof Text ) || !startsWithFiller( domFillerNode ) ) {
+		if ( !( this.domConverter.isText( domFillerNode ) ) || !startsWithFiller( domFillerNode ) ) {
 			/**
 			 * No inline filler on expected position.
 			 *
@@ -364,7 +371,7 @@ export default class Renderer {
 		if ( filler && filler.parent == viewElement ) {
 			const expectedNodeAfterFiller = expectedDomChildren[ filler.offset ];
 
-			if ( expectedNodeAfterFiller instanceof Text ) {
+			if ( this.domConverter.isText( expectedNodeAfterFiller ) ) {
 				expectedNodeAfterFiller.data = INLINE_FILLER + expectedNodeAfterFiller.data;
 			} else {
 				expectedDomChildren.splice( filler.offset, 0, domDocument.createTextNode( INLINE_FILLER ) );
@@ -392,7 +399,7 @@ export default class Renderer {
 				return true;
 			}
 			// Texts.
-			else if ( actualDomChild instanceof Text && expectedDomChild instanceof Text ) {
+			else if ( domConverter.isText( actualDomChild ) && domConverter.isText( expectedDomChild ) ) {
 				return actualDomChild.data === expectedDomChild.data;
 			}
 			// Block fillers.
@@ -412,6 +419,13 @@ export default class Renderer {
 	 * @private
 	 */
 	_updateSelection() {
+		// If there is no selection - remove it from DOM elements that belongs to the editor.
+		if ( this.selection.rangeCount === 0 ) {
+			this._removeDomSelction();
+
+			return;
+		}
+
 		if ( !this.isFocused ) {
 			return;
 		}
@@ -445,6 +459,21 @@ export default class Renderer {
 		}
 	}
 
+	_removeDomSelction() {
+		for ( let doc of this.domDocuments ) {
+			const domSelection = doc.getSelection();
+
+			if ( domSelection.rangeCount ) {
+				const activeDomElement = doc.activeElement;
+				const viewElement = this.domConverter.getCorrespondingViewElement( activeDomElement );
+
+				if ( activeDomElement && viewElement ) {
+					doc.getSelection().removeAllRanges();
+				}
+			}
+		}
+	}
+
 	/**
 	 * Checks if focus needs to be updated and possibly updates it.
 	 *

+ 46 - 0
packages/ckeditor5-engine/tests/view/domconverter/domconverter.js

@@ -65,4 +65,50 @@ describe( 'DomConverter', () => {
 			expect( focusSpy.calledOnce ).to.be.true;
 		} );
 	} );
+
+	describe( 'DOM nodes type checking', () => {
+		let text, element, documentFragment;
+
+		before( () => {
+			text = document.createTextNode( 'test' );
+			element = document.createElement( 'div' );
+			documentFragment = document.createDocumentFragment();
+		} );
+
+		describe( 'isText', () => {
+			it( 'should return true for Text nodes', () => {
+				expect( converter.isText( text ) ).to.be.true;
+			} );
+
+			it( 'should return false for other arguments', () => {
+				expect( converter.isText( element ) ).to.be.false;
+				expect( converter.isText( documentFragment ) ).to.be.false;
+				expect( converter.isText( {} ) ).to.be.false;
+			} );
+		} );
+
+		describe( 'isElement', () => {
+			it( 'should return true for HTMLElement nodes', () => {
+				expect( converter.isElement( element ) ).to.be.true;
+			} );
+
+			it( 'should return false for other arguments', () => {
+				expect( converter.isElement( text ) ).to.be.false;
+				expect( converter.isElement( documentFragment ) ).to.be.false;
+				expect( converter.isElement( {} ) ).to.be.false;
+			} );
+		} );
+
+		describe( 'isDocumentFragment', () => {
+			it( 'should return true for HTMLElement nodes', () => {
+				expect( converter.isDocumentFragment( documentFragment ) ).to.be.true;
+			} );
+
+			it( 'should return false for other arguments', () => {
+				expect( converter.isDocumentFragment( text ) ).to.be.false;
+				expect( converter.isDocumentFragment( element ) ).to.be.false;
+				expect( converter.isDocumentFragment( {} ) ).to.be.false;
+			} );
+		} );
+	} );
 } );

+ 1 - 0
packages/ckeditor5-engine/tests/view/manual/noselection-iframe.html

@@ -0,0 +1 @@
+<iframe id="iframe" src="__template__.html"></iframe>

+ 26 - 0
packages/ckeditor5-engine/tests/view/manual/noselection-iframe.js

@@ -0,0 +1,26 @@
+/**
+ * @license Copyright (c) 2003-2015, CKSource - Frederico Knabben. All rights reserved.
+ * For licensing, see LICENSE.md.
+ */
+
+'use strict';
+
+import Document from '/ckeditor5/engine/view/document.js';
+import MutationObserver from '/ckeditor5/engine/view/observer/mutationobserver.js';
+import SelectionObserver from '/ckeditor5/engine/view/observer/selectionobserver.js';
+import FocusObserver from '/ckeditor5/engine/view/observer/focusobserver.js';
+import { setData } from '/tests/engine/_utils/view.js';
+
+const viewDocument = new Document();
+const iframe = document.getElementById( 'iframe' );
+viewDocument.createRoot( iframe.contentWindow.document.getElementById( 'editor' ) );
+
+viewDocument.addObserver( MutationObserver );
+viewDocument.addObserver( SelectionObserver );
+viewDocument.addObserver( FocusObserver );
+
+setData( viewDocument,
+	'<container:p>foo</container:p>' +
+	'<container:p>bar</container:p>' );
+
+viewDocument.render();

+ 4 - 0
packages/ckeditor5-engine/tests/view/manual/noselection-iframe.md

@@ -0,0 +1,4 @@
+@bender-ui: collapsed
+@bender-tags: view
+
+It should not be possible to put selection in the editor placed in iframe.

+ 1 - 0
packages/ckeditor5-engine/tests/view/renderer.js

@@ -28,6 +28,7 @@ describe( 'Renderer', () => {
 		selection = new Selection();
 		domConverter = new DomConverter();
 		renderer = new Renderer( domConverter, selection );
+		renderer.domDocuments.add( document );
 	} );
 
 	describe( 'markToSync', () => {