8
0
Просмотр исходного кода

Fixed: Prevent DOM Selection from ending up in ui elements.

Szymon Cofalik 8 лет назад
Родитель
Сommit
de2d1ef70b

+ 3 - 5
packages/ckeditor5-engine/src/conversion/view-selection-to-model-converters.js

@@ -39,10 +39,8 @@ export function convertSelectionChange( modelDocument, mapper ) {
 
 		modelSelection.setRanges( ranges, viewSelection.isBackward );
 
-		if ( !modelSelection.isEqual( modelDocument.selection ) ) {
-			modelDocument.enqueueChanges( () => {
-				modelDocument.selection.setTo( modelSelection );
-			} );
-		}
+		modelDocument.enqueueChanges( () => {
+			modelDocument.selection.setTo( modelSelection );
+		} );
 	};
 }

+ 29 - 0
packages/ckeditor5-engine/src/view/document.js

@@ -20,6 +20,7 @@ import KeyObserver from './observer/keyobserver';
 import FakeSelectionObserver from './observer/fakeselectionobserver';
 import mix from '@ckeditor/ckeditor5-utils/src/mix';
 import ObservableMixin from '@ckeditor/ckeditor5-utils/src/observablemixin';
+import { keyCodes } from '@ckeditor/ckeditor5-utils/src/keyboard';
 
 /**
  * Document class creates an abstract layer over the content editable area.
@@ -127,6 +128,8 @@ export default class Document {
 
 		injectQuirksHandling( this );
 
+		this.on( 'keydown', ( evt, data ) => _jumpOverUiElement( evt, data, this.domConverter ) );
+
 		this.decorate( 'render' );
 	}
 
@@ -347,3 +350,29 @@ mix( Document, ObservableMixin );
  *
  * @event render
  */
+
+// Selection cannot be placed in a `UIElement`. Whenever it is placed there, it is moved before it. This
+// causes a situation when it is impossible to jump over `UIElement` using right arrow key, because the selection
+// ends up in ui element (in DOM) and is moved back to the left. This handler fixes this situation.
+function _jumpOverUiElement( evt, data, domConverter ) {
+	if ( data.keyCode == keyCodes.arrowright ) {
+		const domSelection = data.domTarget.ownerDocument.defaultView.getSelection();
+
+		if ( domSelection.rangeCount == 1 && domSelection.getRangeAt( 0 ).collapsed ) {
+			const domParent = domSelection.getRangeAt( 0 ).startContainer;
+			const domOffset = domSelection.getRangeAt( 0 ).startOffset;
+
+			const viewPosition = domConverter.domPositionToView( domParent, domOffset );
+			// Skip all following ui elements.
+			const nextViewPosition = viewPosition.getLastMatchingPosition( value => value.item.is( 'uiElement' ) );
+
+			// If anything has been skipped, fix position.
+			// This `if` could be possibly omitted but maybe it is better not to mess with DOM selection if not needed.
+			if ( !viewPosition.isEqual( nextViewPosition ) ) {
+				const newDomPosition = domConverter.viewPositionToDom( nextViewPosition );
+
+				domSelection.collapse( newDomPosition.parent, newDomPosition.offset );
+			}
+		}
+	}
+}

+ 44 - 0
packages/ckeditor5-engine/src/view/domconverter.js

@@ -838,6 +838,50 @@ export default class DomConverter {
 		return null;
 	}
 
+	/**
+	 * Checks if given {Selection DOM Selection} boundaries are in correct places.
+	 *
+	 * Incorrect places for selection are:
+	 * * before or in the middle of inline filler sequence,
+	 * * inside DOM element that represents {@link module:engine/view/uielement~UIElement view ui element}.
+	 *
+	 * @param {Selection} domSelection DOM Selection object to be checked.
+	 * @returns {Boolean} `true` if given selection is at correct place, `false` otherwise.
+	 */
+	isCorrectDomSelection( domSelection ) {
+		return this._isCorrectDomSelectionPosition( domSelection.anchorNode, domSelection.anchorOffset ) &&
+			this._isCorrectDomSelectionPosition( domSelection.focusNode, domSelection.focusOffset );
+	}
+
+	/**
+	 * Checks if given DOM position is a correct place for selection boundary. See {@link ~isCorrectDomSelection}.
+	 *
+	 * @private
+	 * @param {Node} domParent Position parent.
+	 * @param {Number} offset Position offset.
+	 * @returns {Boolean} `true` if given position is correct place for selection boundary, `false` otherwise.
+	 */
+	_isCorrectDomSelectionPosition( domParent, offset ) {
+		// If selection is before or in the middle of inline filler string, it is incorrect.
+		if ( this.isText( domParent ) && startsWithFiller( domParent ) && offset < INLINE_FILLER_LENGTH ) {
+			// Selection in a text node, at wrong position (before or in the middle of filler).
+			return false;
+		} else if ( this.isElement( domParent ) && startsWithFiller( domParent.childNodes[ offset ] ) ) {
+			// Selection in an element node, before filler text node.
+			return false;
+		}
+
+		const viewParent = this.mapDomToView( domParent );
+
+		// If selection is in `view.UIElement`, it is incorrect. Note that `mapDomToView()` returns `view.UIElement`
+		// also for any dom element that is inside the view ui element (so we don't need to perform any additional checks).
+		if ( viewParent && viewParent.is( 'uiElement' ) ) {
+			return false;
+		}
+
+		return true;
+	}
+
 	/**
 	 * Takes text data from given {@link module:engine/view/text~Text#data} and processes it so it is correctly displayed in DOM.
 	 *

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

@@ -148,7 +148,7 @@ export default class SelectionObserver extends Observer {
 		const domSelection = domDocument.defaultView.getSelection();
 		const newViewSelection = this.domConverter.domSelectionToView( domSelection );
 
-		if ( this.selection.isEqual( newViewSelection ) ) {
+		if ( this.selection.isEqual( newViewSelection ) && this.domConverter.isCorrectDomSelection( domSelection ) ) {
 			return;
 		}
 

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

@@ -602,24 +602,33 @@ export default class Renderer {
 	 */
 	_updateDomSelection( domRoot ) {
 		const domSelection = domRoot.ownerDocument.defaultView.getSelection();
-		const oldViewSelection = domSelection && this.domConverter.domSelectionToView( domSelection );
 
-		if ( oldViewSelection && this.selection.isEqual( oldViewSelection ) ) {
-			return;
-		}
-
-		if ( oldViewSelection && areSimilarSelections( oldViewSelection, this.selection ) ) {
-			const data = {
-				oldSelection: oldViewSelection,
-				currentSelection: this.selection
-			};
-
-			log.warn(
-				'renderer-skipped-selection-rendering: The selection was not rendered due to its similarity to the current one.',
-				data
-			);
-
-			return;
+		// Below we will check whether DOM Selection needs updating at all.
+		// We need to update DOM Selection if either:
+		// * it is at incorrect position, or
+		// * it has changed (when compared to view selection).
+		if ( this.domConverter.isCorrectDomSelection( domSelection ) ) {
+			// DOM Selection is at correct position. Check whether it has changed.
+			const viewSelectionFromDom = this.domConverter.domSelectionToView( domSelection );
+
+			// Compare view selection assumed from dom with current view selection.
+			if ( this.selection.isCollapsed && this.selection.isEqual( viewSelectionFromDom ) ) {
+				// Selection did not changed and is correct, do not update.
+				return;
+			} else if ( areSimilarSelections( viewSelectionFromDom, this.selection ) ) {
+				const data = {
+					oldSelection: viewSelectionFromDom,
+					currentSelection: this.selection
+				};
+
+				log.warn(
+					'renderer-skipped-selection-rendering: The selection was not rendered due to its similarity to the current one.',
+					data
+				);
+
+				// Selection did not changed and is correct, do not update.
+				return;
+			}
 		}
 
 		// Multi-range selection is not available in most browsers, and, at least in Chrome, trying to

+ 3 - 2
packages/ckeditor5-engine/tests/conversion/view-selection-to-model-converters.js

@@ -109,7 +109,8 @@ describe( 'convertSelectionChange', () => {
 		expect( model.selection.isBackward ).to.true;
 	} );
 
-	it( 'should not enqueue changes if selection has not changed', () => {
+	it( 'should re-convert selection even if it has not changed in model', () => {
+		// Selection might not have changed in model but it needs to be reconverted because it ended up in incorrect place in DOM.
 		const viewSelection = new ViewSelection();
 		viewSelection.addRange( ViewRange.createFromParentsAndOffsets(
 			viewRoot.getChild( 0 ).getChild( 0 ), 1, viewRoot.getChild( 0 ).getChild( 0 ), 1 ) );
@@ -122,6 +123,6 @@ describe( 'convertSelectionChange', () => {
 
 		convertSelection( null, { newSelection: viewSelection } );
 
-		expect( spy.called ).to.be.false;
+		expect( spy.called ).to.be.true;
 	} );
 } );

+ 87 - 1
packages/ckeditor5-engine/tests/view/domconverter/domconverter.js

@@ -8,7 +8,8 @@
 import DomConverter from '../../../src/view/domconverter';
 import ViewEditable from '../../../src/view/editableelement';
 import ViewDocument from '../../../src/view/document';
-import { BR_FILLER, NBSP_FILLER } from '../../../src/view/filler';
+import ViewUIElement from '../../../src/view/uielement';
+import { BR_FILLER, NBSP_FILLER, INLINE_FILLER, INLINE_FILLER_LENGTH } from '../../../src/view/filler';
 import testUtils from '@ckeditor/ckeditor5-core/tests/_utils/utils';
 import global from '@ckeditor/ckeditor5-utils/src/dom/global';
 
@@ -190,4 +191,89 @@ describe( 'DomConverter', () => {
 			} );
 		} );
 	} );
+
+	describe( 'isCorrectDomSelection', () => {
+		function domSelection( anchorParent, anchorOffset, focusParent, focusOffset ) {
+			const sel = document.getSelection();
+
+			sel.collapse( anchorParent, anchorOffset );
+			sel.extend( focusParent, focusOffset );
+
+			return sel;
+		}
+
+		let domP, domFillerTextNode, domUiSpan;
+
+		beforeEach( () => {
+			// <p>INLINE_FILLERfoo<span></span></p>.
+			domP = document.createElement( 'p' );
+			domFillerTextNode = document.createTextNode( INLINE_FILLER + 'foo' );
+			domUiSpan = document.createElement( 'span' );
+
+			const viewUiSpan = new ViewUIElement( 'span' );
+
+			domP.appendChild( domFillerTextNode );
+			domP.appendChild( domUiSpan );
+
+			converter.bindElements( domUiSpan, viewUiSpan );
+
+			document.body.appendChild( domP );
+		} );
+
+		it( 'should return true for correct dom selection', () => {
+			// <p>INLINE_FILLER{foo}<span></span></p>.
+			const sel1 = domSelection( domFillerTextNode, INLINE_FILLER_LENGTH, domFillerTextNode, INLINE_FILLER_LENGTH + 3 );
+			expect( converter.isCorrectDomSelection( sel1 ) ).to.be.true;
+
+			// <p>INLINE_FILLERfoo[]<span></span></p>.
+			const sel2 = domSelection( domP, 1, domP, 1 );
+			expect( converter.isCorrectDomSelection( sel2 ) ).to.be.true;
+
+			// <p>INLINE_FILLERfoo<span></span>[]</p>.
+			const sel3 = domSelection( domP, 2, domP, 2 );
+			expect( converter.isCorrectDomSelection( sel3 ) ).to.be.true;
+		} );
+
+		describe( 'should return false', () => {
+			it( 'if anchor or focus is before filler node', () => {
+				// Tests forward and backward selection.
+				// <p>[INLINE_FILLERfoo]<span></span></p>.
+				const sel1 = domSelection( domP, 0, domP, 1 );
+				expect( converter.isCorrectDomSelection( sel1 ) ).to.be.false;
+
+				const sel2 = domSelection( domP, 1, domP, 0 );
+				expect( converter.isCorrectDomSelection( sel2 ) ).to.be.false;
+			} );
+
+			it( 'if anchor or focus is before filler sequence', () => {
+				// Tests forward and backward selection.
+				// <p>{INLINE_FILLERfoo}<span></span></p>.
+				const sel1 = domSelection( domFillerTextNode, 0, domFillerTextNode, INLINE_FILLER_LENGTH + 3 );
+				expect( converter.isCorrectDomSelection( sel1 ) ).to.be.false;
+
+				const sel2 = domSelection( domFillerTextNode, INLINE_FILLER_LENGTH + 3, domFillerTextNode, 0 );
+				expect( converter.isCorrectDomSelection( sel2 ) ).to.be.false;
+			} );
+
+			it( 'if anchor or focus is in the middle of filler sequence', () => {
+				// Tests forward and backward selection.
+				// <p>I{NLINE_FILLERfoo}<span></span></p>.
+				const sel1 = domSelection( domFillerTextNode, 1, domFillerTextNode, INLINE_FILLER_LENGTH + 3 );
+				expect( converter.isCorrectDomSelection( sel1 ) ).to.be.false;
+
+				const sel2 = domSelection( domFillerTextNode, INLINE_FILLER_LENGTH + 3, domFillerTextNode, 1 );
+				expect( converter.isCorrectDomSelection( sel2 ) ).to.be.false;
+			} );
+
+			it( 'if anchor or focus is inside dom element that represents view ui element', () => {
+				// Tests forward and backward selection.
+				// <p>INLINE_FILLER{foo<span>]</span></p>.
+				const sel1 = domSelection( domFillerTextNode, INLINE_FILLER_LENGTH + 3, domUiSpan, 0 );
+				expect( converter.isCorrectDomSelection( sel1 ) ).to.be.false;
+
+				const sel2 = domSelection( domUiSpan, 0, domFillerTextNode, INLINE_FILLER_LENGTH + 3 );
+				expect( converter.isCorrectDomSelection( sel2 ) ).to.be.false;
+			} );
+		} );
+	} );
 } );

+ 41 - 4
packages/ckeditor5-engine/tests/view/observer/selectionobserver.js

@@ -34,7 +34,7 @@ describe( 'SelectionObserver', () => {
 		viewRoot = viewDocument.getRoot();
 
 		viewRoot.appendChildren( parse(
-			'<container:p>xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx</container:p>' +
+			'<container:p>xxx<ui:span></ui:span></container:p>' +
 			'<container:p>yyyyyyyyyyyyyyyyyyyyyyyyyyyyyyyy</container:p>' ) );
 
 		viewDocument.render();
@@ -67,7 +67,7 @@ describe( 'SelectionObserver', () => {
 			expect( data.newSelection.rangeCount ).to.equal( 1 );
 
 			const newViewRange = data.newSelection.getFirstRange();
-			const viewFoo = viewDocument.getRoot().getChild( 0 ).getChild( 0 );
+			const viewFoo = viewDocument.getRoot().getChild( 1 ).getChild( 0 );
 
 			expect( newViewRange.start.parent ).to.equal( viewFoo );
 			expect( newViewRange.start.offset ).to.equal( 2 );
@@ -264,7 +264,7 @@ describe( 'SelectionObserver', () => {
 				expect( data.newSelection.rangeCount ).to.equal( 1 );
 
 				const newViewRange = data.newSelection.getFirstRange();
-				const viewFoo = viewDocument.getRoot().getChild( 0 ).getChild( 0 );
+				const viewFoo = viewDocument.getRoot().getChild( 1 ).getChild( 0 );
 
 				expect( newViewRange.start.parent ).to.equal( viewFoo );
 				expect( newViewRange.start.offset ).to.equal( 3 );
@@ -301,9 +301,46 @@ describe( 'SelectionObserver', () => {
 		}, 100 );
 	} );
 
+	it( 'should fire selectionChange event even if selections are similar if DOM selection is in incorrect place', done => {
+		const sel = domDocument.getSelection();
+
+		// Add rendering on selectionChange event to check this feature.
+		viewDocument.on( 'selectionChange', () => {
+			// Manually set selection because no handlers are set for selectionChange event in this test.
+			// Normally this is handled by view -> model -> view selection converters chain.
+			const viewSel = viewDocument.selection;
+
+			const viewAnchor = viewDocument.domConverter.domPositionToView( sel.anchorNode, sel.anchorOffset );
+			const viewFocus = viewDocument.domConverter.domPositionToView( sel.focusNode, sel.focusOffset );
+
+			viewSel.collapse( viewAnchor );
+			viewSel.setFocus( viewFocus );
+
+			viewDocument.render();
+		} );
+
+		viewDocument.once( 'selectionChange', () => {
+			viewDocument.once( 'selectionChange', ( evt, data ) => {
+				// 3. Selection change event was correctly fired.
+				// Check whether new and old view selection were in fact equal.
+				expect( data.oldSelection.isEqual( data.newSelection ) ).to.be.true;
+
+				done();
+			}, { priority: 'lowest' } );
+
+			// 2. Selection change has been handled and proper event has been fired.
+			// Now, collapse selection in similar position, but in UI element.
+			// Current and new selection position are same in view.
+			sel.collapse( domMain.childNodes[ 0 ].childNodes[ 1 ], 0 );
+		}, { priority: 'lowest' } );
+
+		// 1. Collapse before ui element and wait for async selectionchange to fire selection change handling.
+		sel.collapse( domMain.childNodes[ 0 ], 1 );
+	} );
+
 	function changeDomSelection() {
 		const domSelection = domDocument.getSelection();
-		const domFoo = domMain.childNodes[ 0 ].childNodes[ 0 ];
+		const domFoo = domMain.childNodes[ 1 ].childNodes[ 0 ];
 		const offset = domSelection.anchorOffset;
 
 		domSelection.removeAllRanges();

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

@@ -1604,6 +1604,35 @@ describe( 'Renderer', () => {
 				expect( logWarnStub.notCalled ).to.true;
 			} );
 
+			it( 'should always render selection (even if it is same in view) if current dom selection is in incorrect place', () => {
+				const domSelection = document.getSelection();
+
+				const { view: viewP, selection: newSelection } = parse( '<container:p>foo[]<ui:span></ui:span></container:p>' );
+
+				viewRoot.appendChildren( viewP );
+				selection.setTo( newSelection );
+
+				renderer.markToSync( 'children', viewRoot );
+				renderer.render();
+
+				// In DOM, set position to: <p>foo<span>[]</span></p>. This is incorrect DOM selection (it is in view ui element).
+				// Do not change view selection.
+				// When renderer will check if the DOM selection changed, it will convert DOM selection to a view selection.
+				// Selections (current view selection and view-from-dom selection) will be equal but we will still expect re-render
+				// because DOM selection is in incorrect place.
+				const domP = domRoot.childNodes[ 0 ];
+				const domSpan = domP.childNodes[ 1 ];
+				domSelection.collapse( domSpan, 0 );
+
+				renderer.render();
+
+				// Expect that after calling `renderer.render()` the DOM selection was re-rendered (and set at correct position).
+				expect( domSelection.anchorNode ).to.equal( domP );
+				expect( domSelection.anchorOffset ).to.equal( 1 );
+				expect( domSelection.focusNode ).to.equal( domP );
+				expect( domSelection.focusOffset ).to.equal( 1 );
+			} );
+
 			it( 'should not render non-collapsed selection it is similar (element start)', () => {
 				const domSelection = document.getSelection();