Merge 94891ee into 85ea134

src/delete.js

@@ -67,7 +67,6 @@ export default class Delete extends Plugin {
 		// on DOM selection level, because on `keyup` the model selection is still the same as it was just after deletion, so it
 		// wouldn't be changed and the fix would do nothing.
 		//
-		/* istanbul ignore if */
 		if ( env.isAndroid ) {
 			let domSelectionAfterDeletion = null;
 
@@ -83,14 +82,12 @@ export default class Delete extends Plugin {
 			}, { priority: 'lowest' } );
 
 			this.listenTo( viewDocument, 'keyup', ( evt, data ) => {
-				if ( domSelectionAfterDeletion ) {
-					const domSelection = data.domTarget.ownerDocument.defaultView.getSelection();
+				const domSelection = data.domTarget.ownerDocument.defaultView.getSelection();
 
-					domSelection.collapse( domSelectionAfterDeletion.anchorNode, domSelectionAfterDeletion.anchorOffset );
-					domSelection.extend( domSelectionAfterDeletion.focusNode, domSelectionAfterDeletion.focusOffset );
+				domSelection.collapse( domSelectionAfterDeletion.anchorNode, domSelectionAfterDeletion.anchorOffset );
+				domSelection.extend( domSelectionAfterDeletion.focusNode, domSelectionAfterDeletion.focusOffset );
 
-					domSelectionAfterDeletion = null;
-				}
+				domSelectionAfterDeletion = null;
 			} );
 		}
 	}

src/deleteobserver.js

@@ -51,7 +51,6 @@ export default class DeleteObserver extends Observer {
 		} );
 
 		// `beforeinput` is handled only for Android devices. Desktop Chrome and iOS are skipped because they are working fine now.
-		/* istanbul ignore if */
 		if ( env.isAndroid ) {
 			document.on( 'beforeinput', ( evt, data ) => {
 				// If event type is other than `deleteContentBackward` then this is not deleting.

src/utils/injectunsafekeystrokeshandling.js

@@ -24,7 +24,6 @@ export default function injectUnsafeKeystrokesHandling( editor ) {
 	const inputCommand = editor.commands.get( 'input' );
 
 	// For Android, we want to handle keystrokes on `beforeinput` to be sure that code in `DeleteObserver` already had a chance to be fired.
-	/* istanbul ignore if */
 	if ( env.isAndroid ) {
 		view.document.on( 'beforeinput', ( evt, evtData ) => handleUnsafeKeystroke( evtData ), { priority: 'lowest' } );
 	} else {

tests/delete.js

@@ -4,10 +4,12 @@
  */
 
 import Delete from '../src/delete';
+import Paragraph from '@ckeditor/ckeditor5-paragraph/src/paragraph';
 import DomEventData from '@ckeditor/ckeditor5-engine/src/view/observer/domeventdata';
 import ClassicTestEditor from '@ckeditor/ckeditor5-core/tests/_utils/classictesteditor';
+import env from '@ckeditor/ckeditor5-utils/src/env';
 
-/* globals document */
+/* globals window, document */
 
 describe( 'Delete feature', () => {
 	let element, editor, viewDocument;
@@ -106,3 +108,88 @@ describe( 'Delete feature', () => {
 		};
 	}
 } );
+
+describe( 'Delete feature - Android', () => {
+	let element, editor, oldEnvIsAndroid;
+
+	before( () => {
+		oldEnvIsAndroid = env.isAndroid;
+		env.isAndroid = true;
+	} );
+
+	beforeEach( () => {
+		element = document.createElement( 'div' );
+		document.body.appendChild( element );
+
+		return ClassicTestEditor
+			.create( element, { plugins: [ Delete, Paragraph ] } )
+			.then( newEditor => {
+				editor = newEditor;
+
+				const modelRoot = editor.model.document.getRoot();
+
+				editor.model.change( writer => {
+					writer.insertElement( 'paragraph', modelRoot, 0 );
+					writer.insertText( 'Foobar', modelRoot.getChild( 0 ), 0 );
+
+					writer.setSelection( modelRoot.getChild( 0 ), 3 );
+				} );
+			} );
+	} );
+
+	afterEach( () => {
+		element.remove();
+		return editor.destroy();
+	} );
+
+	after( () => {
+		env.isAndroid = oldEnvIsAndroid;
+	} );
+
+	it( 'should re-set selection on keyup event if it was changed after deletion but before the input was fired (Android)', () => {
+		// This test covers a quirk on Android. We will recreate what browser does in this scenario.
+		// The test is not perfect because there are difficulties converting model selection to DOM in unit tests.
+		const view = editor.editing.view;
+		const viewDocument = view.document;
+
+		const domEvt = {
+			preventDefault: sinon.spy()
+		};
+
+		const domRoot = view.getDomRoot();
+		const domSelection = window.getSelection();
+		const domText = domRoot.childNodes[ 0 ].childNodes[ 0 ];
+
+		// Change the selection ("manual conversion").
+		// Because it all works quite bad the selection will be moved to quite a random place after delete is fired but all we care is
+		// checking if the selection is reversed on `keyup` event.
+		domSelection.collapse( domText, 3 );
+
+		// On `delete` the selection is saved.
+		viewDocument.fire( 'delete', new DomEventData( viewDocument, domEvt, {
+			direction: 'backward',
+			unit: 'character',
+			sequence: 1,
+			domTarget: domRoot
+		} ) );
+
+		// Store what was the selection when it was saved in `delete`.
+		const anchorNodeBefore = domSelection.anchorNode;
+		const anchorOffsetBefore = domSelection.anchorOffset;
+		const focusNodeBefore = domSelection.focusNode;
+		const focusOffsetBefore = domSelection.focusOffset;
+
+		// Change the selection.
+		domSelection.collapse( domText, 0 );
+
+		// On `keyup` it should be reversed.
+		viewDocument.fire( 'keyup', new DomEventData( viewDocument, domEvt, {
+			domTarget: domRoot
+		} ) );
+
+		expect( domSelection.anchorNode ).to.equal( anchorNodeBefore );
+		expect( domSelection.anchorOffset ).to.equal( anchorOffsetBefore );
+		expect( domSelection.focusNode ).to.equal( focusNodeBefore );
+		expect( domSelection.focusOffset ).to.equal( focusOffsetBefore );
+	} );
+} );

tests/deleteobserver.js

@@ -3,7 +3,7 @@
  * For licensing, see LICENSE.md or https://ckeditor.com/legal/ckeditor-oss-license
  */
 
-/* globals document */
+/* globals window, document */
 
 import DeleteObserver from '../src/deleteobserver';
 import View from '@ckeditor/ckeditor5-engine/src/view/view';
@@ -260,3 +260,149 @@ describe( 'DeleteObserver', () => {
 		};
 	}
 } );
+
+describe( 'DeleteObserver - Android', () => {
+	let view, viewDocument, oldEnvIsAndroid, domElement, viewRoot, domText;
+
+	testUtils.createSinonSandbox();
+
+	before( () => {
+		oldEnvIsAndroid = env.isAndroid;
+		env.isAndroid = true;
+	} );
+
+	beforeEach( () => {
+		domElement = document.createElement( 'div' );
+		domElement.contenteditable = true;
+
+		document.body.appendChild( domElement );
+
+		view = new View();
+		viewDocument = view.document;
+		view.addObserver( DeleteObserver );
+
+		viewRoot = createViewRoot( viewDocument );
+		view.attachDomRoot( domElement );
+
+		view.change( writer => {
+			const p = writer.createContainerElement( 'p' );
+
+			writer.insert( writer.createPositionAt( viewRoot, 0 ), p );
+
+			const text = writer.createText( 'foo' );
+
+			writer.insert( writer.createPositionAt( p, 0 ), text );
+		} );
+
+		domText = domElement.childNodes[ 0 ].childNodes[ 0 ];
+	} );
+
+	afterEach( () => {
+		view.destroy();
+		domElement.remove();
+	} );
+
+	after( () => {
+		env.isAndroid = oldEnvIsAndroid;
+	} );
+
+	describe( 'delete event', () => {
+		it( 'is fired on beforeinput', () => {
+			const spy = sinon.spy();
+
+			viewDocument.on( 'delete', spy );
+
+			setDomSelection( domText, 1, domText, 2 );
+
+			viewDocument.fire( 'beforeinput', new DomEventData( viewDocument, getDomEvent( 'deleteContentBackward' ), {
+				domTarget: domElement
+			} ) );
+
+			expect( spy.calledOnce ).to.be.true;
+
+			const data = spy.args[ 0 ][ 1 ];
+			expect( data ).to.have.property( 'direction', 'backward' );
+			expect( data ).to.have.property( 'unit', 'codepoint' );
+			expect( data ).to.have.property( 'sequence', 1 );
+			expect( data ).not.to.have.property( 'selectionToRemove' );
+		} );
+
+		it( 'should set selectionToRemove if DOM selection size is different than 1', () => {
+			// In real scenarios, before `beforeinput` is fired, browser changes DOM selection to a selection that contains
+			// all content that should be deleted. If the selection is big (> 1 character) we need to pass special parameter
+			// so that `DeleteCommand` will know what to delete. This test checks that case.
+			const spy = sinon.spy();
+
+			viewDocument.on( 'delete', spy );
+
+			setDomSelection( domText, 0, domText, 3 );
+
+			viewDocument.fire( 'beforeinput', new DomEventData( viewDocument, getDomEvent( 'deleteContentBackward' ), {
+				domTarget: domElement
+			} ) );
+
+			expect( spy.calledOnce ).to.be.true;
+
+			const data = spy.args[ 0 ][ 1 ];
+			expect( data ).to.have.property( 'selectionToRemove' );
+
+			const viewText = viewRoot.getChild( 0 ).getChild( 0 );
+			const range = data.selectionToRemove.getFirstRange();
+
+			expect( range.start.offset ).to.equal( 0 );
+			expect( range.start.parent ).to.equal( viewText );
+			expect( range.end.offset ).to.equal( 3 );
+			expect( range.end.parent ).to.equal( viewText );
+		} );
+
+		it( 'is not fired on beforeinput when event type is other than deleteContentBackward', () => {
+			const spy = sinon.spy();
+
+			viewDocument.on( 'delete', spy );
+
+			viewDocument.fire( 'keydown', new DomEventData( viewDocument, getDomEvent( 'insertText' ), {
+				domTarget: domElement
+			} ) );
+
+			expect( spy.calledOnce ).to.be.false;
+		} );
+
+		it( 'should stop beforeinput event when delete event is stopped', () => {
+			const keydownSpy = sinon.spy();
+			viewDocument.on( 'beforeinput', keydownSpy );
+			viewDocument.on( 'delete', evt => evt.stop() );
+
+			viewDocument.fire( 'beforeinput', new DomEventData( viewDocument, getDomEvent( 'deleteContentBackward' ), {
+				domTarget: domElement
+			} ) );
+
+			sinon.assert.notCalled( keydownSpy );
+		} );
+
+		it( 'should not stop keydown event when delete event is not stopped', () => {
+			const keydownSpy = sinon.spy();
+			viewDocument.on( 'beforeinput', keydownSpy );
+			viewDocument.on( 'delete', evt => evt.stop() );
+
+			viewDocument.fire( 'beforeinput', new DomEventData( viewDocument, getDomEvent( 'insertText' ), {
+				domTarget: domElement
+			} ) );
+
+			sinon.assert.calledOnce( keydownSpy );
+		} );
+	} );
+
+	function getDomEvent( inputType ) {
+		return {
+			inputType,
+			preventDefault: sinon.spy()
+		};
+	}
+
+	function setDomSelection( anchorNode, anchorOffset, focusNode, focusOffset ) {
+		const selection = window.getSelection();
+
+		selection.collapse( anchorNode, anchorOffset );
+		selection.extend( focusNode, focusOffset );
+	}
+} );