From 5e477cbe9848a34d2b71657de38dc9258e1e74f6 Mon Sep 17 00:00:00 2001 From: Trevor Parscal Date: Tue, 27 Nov 2012 17:29:09 -0800 Subject: [PATCH] (bug 42401) Cursor movement fixes ve.ce.Surface * Added ve.ce.Surface.adjustCursor, which replaces repetitive and buggy code that was handling left and right arrow key presses * New method only affects the selection target, so it won't collapse the selection on you - this was what caused bug 42401 * Made hasSlugAtOffset() actually return a boolean ve.dm.Document * Fixed turn-around issue in ve.dm.Document.getRelativeOffset - if the offset is already valid and we can't move in the direction we want, we should just leave it be, not turn around * Since this method was being used by ve.ce.Surface to correct the cursor position on arrow key presses, it was causing the strange cursor jumping when you pressed an arrow key while at the edge of a document ve.dm.SurfaceFragment * Fixed typo where getAnnotationRangeFromSelection was preserving selection direction, but checking the wrong properties ve.dm.Document.test * Added tests that verify turn-around issue is fixed Change-Id: Iba55cfc3d531e7d1333b78c94912ff22179aace8 --- modules/ve/ce/ve.ce.Surface.js | 113 +++++++++++----------- modules/ve/dm/ve.dm.Document.js | 7 ++ modules/ve/dm/ve.dm.SurfaceFragment.js | 3 +- modules/ve/test/dm/ve.dm.Document.test.js | 12 +++ 4 files changed, 80 insertions(+), 55 deletions(-) diff --git a/modules/ve/ce/ve.ce.Surface.js b/modules/ve/ce/ve.ce.Surface.js index f9cdfc83fe..c77f2e51b5 100644 --- a/modules/ve/ce/ve.ce.Surface.js +++ b/modules/ve/ce/ve.ce.Surface.js @@ -413,67 +413,16 @@ ve.ce.Surface.prototype.onKeyDown = function ( e ) { this.emit( 'selectionStart' ); } - var offset, - relativeContentOffset, - relativeStructuralOffset, - relativeStructuralOffsetNode, - hasSlug, - newOffset; switch ( e.keyCode ) { // Left arrow case 37: - offset = this.model.getSelection().start; - relativeContentOffset = this.documentView.model.getRelativeContentOffset( offset, -1 ); - relativeStructuralOffset = this.documentView.model.getRelativeStructuralOffset( offset - 1, -1, true ); - relativeStructuralOffsetNode = this.documentView.documentNode.getNodeFromOffset( relativeStructuralOffset ); - hasSlug = this.documentView.getSlugAtOffset( relativeStructuralOffset ) || false; - - if ( hasSlug ) { - if ( relativeContentOffset > offset ) { - // If relativeContentOffset returns a greater number, there's nowhere to go toward the left. Go right. - newOffset = relativeStructuralOffset; - } else { - // Move cursor to whichever is nearest to the original offset. - newOffset = Math.max( relativeContentOffset, relativeStructuralOffset ); - } - } else if ( relativeContentOffset !== offset - 1 ) { - // The closest content offet is further away than just one offset. Don't trust the browser. Move programatically. - newOffset = relativeContentOffset; - } - - if ( newOffset ) { - this.model.change( - null, - new ve.Range( newOffset ) - ); + if ( this.adjustCursor( -1 ) ) { e.preventDefault(); } break; // Right arrow case 39: - offset = this.model.getSelection().start; - relativeContentOffset = this.documentView.model.getRelativeContentOffset( offset, 1 ); - relativeStructuralOffset = this.documentView.model.getRelativeStructuralOffset( offset + 1, 1, true ); - relativeStructuralOffsetNode = this.documentView.documentNode.getNodeFromOffset( relativeStructuralOffset ); - hasSlug = this.documentView.getSlugAtOffset( relativeStructuralOffset ) || false; - - if ( hasSlug ) { - if ( relativeContentOffset < offset ) { - // If relativeContentOffset returns a lesser number, there's nowhere to go toward the right. Go left. - newOffset = relativeStructuralOffset; - } else { - // Move cursor to whichever is nearest to the original offset. - newOffset = Math.min( relativeContentOffset, relativeStructuralOffset ); - } - } else if ( relativeContentOffset !== offset + 1 ) { - newOffset = relativeContentOffset; - } - - if ( newOffset ) { - this.model.change( - null, - new ve.Range( newOffset ) - ); + if ( this.adjustCursor( 1 ) ) { e.preventDefault(); } break; @@ -775,6 +724,62 @@ ve.ce.Surface.prototype.handleEnter = function ( e ) { this.surfaceObserver.start(); }; +/** + * Adjusts the cursor position in a given distance. + * + * This method only affects the selection target, preserving selections that are not collapsed and + * the direction of the selection. + * + * @method + * @param {Number} adjustment Distance to adjust the cursor, can be positive or negative + * @returns {Boolean} Cursor was moved + */ +ve.ce.Surface.prototype.adjustCursor = function ( adjustment ) { + // Bypass for zero-adjustment + if ( !adjustment ) { + return false; + } + var adjustedTargetOffset, + bias = adjustment > 0 ? 1 : -1, + selection = this.model.getSelection(), + targetOffset = selection.to, + documentModel = this.model.getDocument(), + relativeContentOffset = documentModel.getRelativeContentOffset( targetOffset, adjustment ), + relativeStructuralOffset = documentModel.getRelativeStructuralOffset( + targetOffset + bias, adjustment, true + ); + // Check if we've moved into a slug + if ( this.hasSlugAtOffset( relativeStructuralOffset ) ) { + // Check if the relative content offset is in the opposite direction we are trying to go + if ( ( relativeContentOffset - targetOffset < 0 ? -1 : 1 ) !== bias ) { + // There's nothing past the slug we are already in, stay in it + adjustedTargetOffset = relativeStructuralOffset; + } else { + // There's a slug neaby, go into it if it's closer + adjustedTargetOffset = adjustment < 0 ? + Math.max( relativeContentOffset, relativeStructuralOffset ) : + Math.min( relativeContentOffset, relativeStructuralOffset ); + } + } + // Check if we've moved a different distance than we asked for + else if ( relativeContentOffset !== targetOffset + adjustment ) { + // We can't trust the browser, move programatically + adjustedTargetOffset = relativeContentOffset; + } + // If the target changed, update the model + if ( adjustedTargetOffset ) { + this.model.change( + null, + new ve.Range( + selection.isCollapsed() ? + adjustedTargetOffset : selection.from, adjustedTargetOffset + ) + ); + return true; + } + return false; +}; + /** * Responds to backspace and delete key events. * @@ -1062,7 +1067,7 @@ ve.ce.Surface.prototype.getNearestCorrectOffset = function ( offset, direction ) * @returns {Boolean} A slug exists at the given offset */ ve.ce.Surface.prototype.hasSlugAtOffset = function ( offset ) { - return this.documentView.getSlugAtOffset( offset ) || false; + return !!this.documentView.getSlugAtOffset( offset ); }; /** diff --git a/modules/ve/dm/ve.dm.Document.js b/modules/ve/dm/ve.dm.Document.js index 4930f98997..ea90a9d81c 100644 --- a/modules/ve/dm/ve.dm.Document.js +++ b/modules/ve/dm/ve.dm.Document.js @@ -805,6 +805,8 @@ ve.dm.Document.prototype.rebuildNodes = function ( parent, index, numNodes, offs * Gets an offset a given distance from another using a callback to check if offsets are valid. * * - If {offset} is not already valid, one step will be used to move it to an valid one. + * - If {offset} is already valid and can not be moved in the direction of {distance} and still be + * valid, it will be left where it is * - If {distance} is zero the result will either be {offset} if it's already valid or the * nearest valid offset to the right if possible and to the left otherwise. * - If {offset} is after the last valid offset and {distance} is >= 1, or if {offset} if @@ -862,6 +864,11 @@ ve.dm.Document.prototype.getRelativeOffset = function ( offset, distance, callba // Only turn around if we're about to reach the edge ( ( direction < 0 && i === 0 ) || ( direction > 0 && i === this.data.length ) ) ) { + // Before we turn around, let's see if we are at a valid position + if ( callback.apply( window, [this.data, start].concat( args ) ) ) { + // Stay where we are + return start; + } // Start over going in the opposite direction direction *= -1; i = start; diff --git a/modules/ve/dm/ve.dm.SurfaceFragment.js b/modules/ve/dm/ve.dm.SurfaceFragment.js index 7920ad8ee7..74d824d4fa 100644 --- a/modules/ve/dm/ve.dm.SurfaceFragment.js +++ b/modules/ve/dm/ve.dm.SurfaceFragment.js @@ -204,7 +204,8 @@ ve.dm.SurfaceFragment.prototype.expandRange = function ( scope, type ) { range = this.document.getAnnotatedRangeFromSelection( this.range, type ); // Adjust selection if it does not contain the annotated range if ( this.range.start > range.start || this.range.end < range.end ) { - if ( this.range.from > this.range.start ) { + // Maintain range direction + if ( this.range.from > this.range.to ) { range.flip(); } } else { diff --git a/modules/ve/test/dm/ve.dm.Document.test.js b/modules/ve/test/dm/ve.dm.Document.test.js index 91ed0f9b99..4b1104d6cf 100644 --- a/modules/ve/test/dm/ve.dm.Document.test.js +++ b/modules/ve/test/dm/ve.dm.Document.test.js @@ -1013,6 +1013,18 @@ QUnit.test( 'getRelativeContentOffset', function ( assert ) { 'distance': 1, 'expected': 60 }, + { + 'msg': 'stop at left edge if already valid', + 'offset': 1, + 'distance': -1, + 'expected': 1 + }, + { + 'msg': 'stop at right edge if already valid', + 'offset': 60, + 'distance': 1, + 'expected': 60 + }, { 'msg': 'first content offset is farthest left', 'offset': 2,