diff --git a/src/delete.js b/src/delete.js index 43190a9..9104f46 100644 --- a/src/delete.js +++ b/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; } ); } } diff --git a/src/deleteobserver.js b/src/deleteobserver.js index fc0b244..878f35f 100644 --- a/src/deleteobserver.js +++ b/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. diff --git a/src/utils/injectunsafekeystrokeshandling.js b/src/utils/injectunsafekeystrokeshandling.js index 49dedbf..a5fb9c4 100644 --- a/src/utils/injectunsafekeystrokeshandling.js +++ b/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 { diff --git a/tests/delete.js b/tests/delete.js index f5473b6..c74bd67 100644 --- a/tests/delete.js +++ b/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 ); + } ); +} ); diff --git a/tests/deleteobserver.js b/tests/deleteobserver.js index 378d6d5..4dd4e61 100644 --- a/tests/deleteobserver.js +++ b/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 ); + } +} ); diff --git a/tests/input.js b/tests/input.js index ae8e452..ec79982 100644 --- a/tests/input.js +++ b/tests/input.js @@ -24,6 +24,7 @@ import { getCode } from '@ckeditor/ckeditor5-utils/src/keyboard'; import { getData as getModelData, setData as setModelData } from '@ckeditor/ckeditor5-engine/src/dev-utils/model'; import { getData as getViewData } from '@ckeditor/ckeditor5-engine/src/dev-utils/view'; +import env from '@ckeditor/ckeditor5-utils/src/env'; /* global document */ @@ -689,6 +690,8 @@ describe( 'Input feature', () => { listenter.listenTo( viewDocument, 'keydown', () => { expect( getModelData( model ) ).to.equal( 'fo[]ar' ); }, { priority: 'lowest' } ); + + viewDocument.fire( 'keydown', { keyCode: getCode( 'y' ) } ); } ); // #97 @@ -995,3 +998,104 @@ describe( 'Input feature', () => { } ); } ); +describe( 'Input feature - Android', () => { + let editor, model, modelRoot, view, viewDocument, listenter, oldEnvIsAndroid; + + testUtils.createSinonSandbox(); + + before( () => { + oldEnvIsAndroid = env.isAndroid; + env.isAndroid = true; + } ); + + beforeEach( () => { + listenter = Object.create( EmitterMixin ); + + const domElement = document.createElement( 'div' ); + document.body.appendChild( domElement ); + + return ClassicTestEditor.create( domElement, { plugins: [ Input, Paragraph, Bold, Italic, List, ShiftEnter, Link ] } ) + .then( newEditor => { + editor = newEditor; + model = editor.model; + modelRoot = model.document.getRoot(); + view = editor.editing.view; + viewDocument = view.document; + + editor.setData( '

foobar

' ); + + model.change( writer => { + writer.setSelection( modelRoot.getChild( 0 ), 3 ); + } ); + } ); + } ); + + afterEach( () => { + listenter.stopListening(); + + return editor.destroy(); + } ); + + after( () => { + env.isAndroid = oldEnvIsAndroid; + } ); + + describe( 'keystroke handling', () => { + it( 'should remove contents', () => { + model.change( writer => { + writer.setSelection( + writer.createRange( + writer.createPositionAt( modelRoot.getChild( 0 ), 2 ), + writer.createPositionAt( modelRoot.getChild( 0 ), 4 ) + ) + ); + } ); + + listenter.listenTo( viewDocument, 'beforeinput', () => { + expect( getModelData( model ) ).to.equal( 'fo[]ar' ); + }, { priority: 'lowest' } ); + + // On Android, `keycode` is set to `229` (in scenarios when `keydown` event is not send). + viewDocument.fire( 'beforeinput', { keyCode: 229 } ); + } ); + + it( 'should remove contents and merge blocks', () => { + setModelData( model, 'fo[ob]ar' ); + + listenter.listenTo( viewDocument, 'beforeinput', () => { + expect( getModelData( model ) ).to.equal( 'fo[]ar' ); + }, { priority: 'lowest' } ); + + // On Android, `keycode` is set to `229` (in scenarios when `keydown` event is not send). + viewDocument.fire( 'beforeinput', { keyCode: 229 } ); + } ); + + it( 'should do nothing if selection is collapsed', () => { + viewDocument.fire( 'beforeinput', { keyCode: 229 } ); + + expect( getModelData( model ) ).to.equal( 'foo[]bar' ); + } ); + + it( 'should not modify document when input command is disabled and selection is collapsed', () => { + setModelData( model, 'foo[]bar' ); + + editor.commands.get( 'input' ).isEnabled = false; + + // On Android, `keycode` is set to `229` (in scenarios when `keydown` event is not send). + viewDocument.fire( 'beforeinput', { keyCode: 229 } ); + + expect( getModelData( model ) ).to.equal( 'foo[]bar' ); + } ); + + it( 'should not modify document when input command is disabled and selection is non-collapsed', () => { + setModelData( model, 'fo[ob]ar' ); + + editor.commands.get( 'input' ).isEnabled = false; + + // On Android, `keycode` is set to `229` (in scenarios when `keydown` event is not send). + viewDocument.fire( 'beforeinput', { keyCode: 229 } ); + + expect( getModelData( model ) ).to.equal( 'fo[ob]ar' ); + } ); + } ); +} );