jenkins-bot has submitted this change and it was merged.
Change subject: De-Houdini-fy the handling of observed DOM changes
......................................................................
De-Houdini-fy the handling of observed DOM changes
The SurfaceObserver does far less change calculation now; it simply passes
the old and new RangeState objects to ve.ce.Surface#handleObservedChanges ,
which performs the modifications for contentChanged, selectionChanged and
branchNodeChanged in a single method.
The calculation of a ve.dm.Transaction based on a content change is moved
to a pure function ve.ce.dmChangeFromContentChange.
The cursor focus's annotation sidedness is checked when polling RangeState,
instead of at keyDown, which is better since IMEs may emit no keyDown event.
Bug: T113797
Bug: T113875
Change-Id: I420c14f56187bb881c4b636a746d4383d9ba3146
---
M src/ce/ve.ce.RangeState.js
M src/ce/ve.ce.Surface.js
M src/ce/ve.ce.SurfaceObserver.js
M src/ce/ve.ce.js
M tests/ce/ve.ce.Surface.test.js
5 files changed, 315 insertions(+), 326 deletions(-)
Approvals:
Esanders: Looks good to me, approved
jenkins-bot: Verified
diff --git a/src/ce/ve.ce.RangeState.js b/src/ce/ve.ce.RangeState.js
index d0bedb6..22047d5 100644
--- a/src/ce/ve.ce.RangeState.js
+++ b/src/ce/ve.ce.RangeState.js
@@ -27,6 +27,9 @@
/**
* @property {boolean} contentChanged Whether the content changed
+ *
+ * This is only set to true if both the old and new states have the
+ * same current branch node, whose content has changed
*/
this.contentChanged = false;
@@ -49,6 +52,11 @@
* @property {string|null} DOM Hash of current branch node
*/
this.hash = null;
+
+ /**
+ * @property {boolean|null} focusIsAfterAnnotationBoundary Focus lies
after annotation tag
+ */
+ this.focusIsAfterAnnotationBoundary = null;
this.saveState( old, documentNode, selectionOnly );
};
@@ -133,6 +141,17 @@
( old && old.text ) !== this.text
);
+ if ( old && !this.selectionChanged && !this.contentChanged ) {
+ this.focusIsAfterAnnotationBoundary =
old.focusIsAfterAnnotationBoundary;
+ } else {
+ // Will be null if there is no selection
+ this.focusIsAfterAnnotationBoundary = selection.focusNode &&
+ ve.ce.isAfterAnnotationBoundary(
+ selection.focusNode,
+ selection.focusOffset
+ );
+ }
+
// Save selection for future comparisons. (But it is not properly
frozen, because the nodes
// are live and mutable, and therefore the offsets may come to point to
places that are
// misleadingly different from when the selection was saved).
diff --git a/src/ce/ve.ce.Surface.js b/src/ce/ve.ce.Surface.js
index 7028c4d..ecbc785 100644
--- a/src/ce/ve.ce.Surface.js
+++ b/src/ce/ve.ce.Surface.js
@@ -75,8 +75,7 @@
// Snapshot updated at keyDown. See storeKeyDownState.
this.keyDownState = {
event: null,
- selection: null,
- focusIsAfterAnnotationBoundary: null
+ selection: null
};
this.cursorDirectionality = null;
@@ -2552,60 +2551,83 @@
*
* These are normally caused by the user interacting directly with the
contenteditable.
*
- * @param {Object|null} contentChange Observed change in a CE node's content
- * @param {ve.ce.Node} contentChange.node CE node the change occurred in
- * @param {Object} contentChange.previous Old data
- * @param {Object} contentChange.next New data
- *
- * @param {Object|null} branchNodeChange Change of the selected BranchNode
- * @param {ve.ce.BranchNode} branchNodeChange.oldBranchNode Node from which
the range anchor has just moved
- * @param {ve.ce.BranchNode} branchNodeChange.newBranchNode Node into which
the range anchor has just moved
- *
- * @param {Object|null} rangeChange Change of the DOM selection
- * @param {ve.Range|null} rangeChange.oldRange The old DM range
- * @param {ve.Range|null} rangeChange.newRange The new DM range
- *
- * TODO: Clean up this interface: it's a minimal incremental step from the old
emit-based interface
+ * @param {ve.ce.RangeState|null} oldState The prior range state, if any
+ * @param {ve.ce.RangeState} newState The changed range state
*/
-ve.ce.Surface.prototype.handleObservedChanges = function ( contentChange,
branchNodeChange, rangeChange ) {
- if ( contentChange ) {
- this.onObservedContentChange(
- contentChange.node,
- contentChange.previous,
- contentChange.next
- );
+ve.ce.Surface.prototype.handleObservedChanges = function ( oldState, newState
) {
+ var newSelection, dmContentChange,
+ dmDoc = this.getModel().getDocument(),
+ insertedText = false;
+
+ if ( newState.contentChanged ) {
+ dmContentChange = ve.ce.modelChangeFromContentChange( oldState,
newState );
+ if ( !dmContentChange.rerender ) {
+ this.incRenderLock();
+ }
+ try {
+ this.changeModel( dmContentChange.transaction,
dmContentChange.selection );
+ } finally {
+ if ( !dmContentChange.rerender ) {
+ this.decRenderLock();
+ }
+ }
+
+ insertedText = dmContentChange.transaction.operations.filter(
function ( op ) {
+ return op.type === 'replace' && op.insert.length;
+ } ).length > 0;
}
- if ( branchNodeChange ) {
- this.onObservedBranchNodeChange(
- branchNodeChange.oldBranchNode,
- branchNodeChange.newBranchNode
- );
+
+ if (
+ newState.branchNodeChanged &&
+ oldState &&
+ oldState.node &&
+ oldState.node.root &&
+ oldState.node instanceof ve.ce.ContentBranchNode
+ ) {
+ oldState.node.renderContents();
}
- if ( rangeChange ) {
- this.onObservedRangeChange(
- rangeChange.oldRange,
- rangeChange.newRange
- );
+
+ if ( newState.selectionChanged && !(
+ // Ignore when the newRange is just a flipped oldRange
+ oldState &&
+ oldState.veRange &&
+ newState.veRange &&
+ !newState.veRange.isCollapsed() &&
+ oldState.veRange.equalsSelection( newState.veRange )
+ ) ) {
+ if ( newState.veRange ) {
+ newSelection = new ve.dm.LinearSelection( dmDoc,
newState.veRange );
+ } else {
+ newSelection = new ve.dm.NullSelection( dmDoc );
+ }
+ this.incRenderLock();
+ try {
+ this.changeModel( null, newSelection );
+ } finally {
+ this.decRenderLock();
+ }
+ this.checkUnicorns( false );
+
+ // Firefox lets you create multiple selections within a single
paragraph
+ // which our model doesn't support, so detect and prevent these.
+ // This shouldn't create problems with IME candidates as only
an explicit user
+ // action can create a multiple selection (CTRL+click), and we
remove it
+ // immediately, so there can never be a multiple selection
while the user is
+ // typing text; therefore the selection change will never
commit IME candidates
+ // prematurely.
+ while ( this.nativeSelection.rangeCount > 1 ) {
+ // The current range is the last range, so remove
ranges from the front
+ this.nativeSelection.removeRange(
this.nativeSelection.getRangeAt( 0 ) );
+ }
}
- if ( branchNodeChange && branchNodeChange.newBranchNode ) {
+
+ if ( insertedText ) {
+ this.checkSequences();
+ }
+ if ( newState.branchNodeChanged && newState.node ) {
this.updateCursorHolders();
this.showModelSelection( this.getModel().getSelection() );
- }
-};
-
-/**
- * Handle branch node changes observed from the DOM.
- *
- * @see ve.ce.SurfaceObserver#pollOnce
- *
- * @method
- * @param {ve.ce.BranchNode} oldBranchNode Node from which the range anchor
has just moved
- * @param {ve.ce.BranchNode} newBranchNode Node into which the range anchor
has just moved
- */
-ve.ce.Surface.prototype.onObservedBranchNodeChange = function ( oldBranchNode
) {
- if ( oldBranchNode instanceof ve.ce.ContentBranchNode ) {
- oldBranchNode.renderContents();
}
};
@@ -2638,48 +2660,6 @@
} );
this.onModelSelect();
-};
-
-/**
- * Handle selection changes observed from the DOM.
- *
- * This can be fired by a DOM selection change that doesn't cause any change
in the DM range,
- * with oldRange === newRange .
- *
- * @see ve.ce.SurfaceObserver#pollOnce
- *
- * @method
- * @param {ve.Range|null} oldRange
- * @param {ve.Range|null} newRange
- */
-ve.ce.Surface.prototype.onObservedRangeChange = function ( oldRange, newRange
) {
- if ( newRange && !newRange.isCollapsed() && oldRange &&
oldRange.equalsSelection( newRange ) ) {
- // Ignore when the newRange is just a flipped oldRange
- return;
- }
-
- this.incRenderLock();
- try {
- this.changeModel(
- null,
- newRange ?
- new ve.dm.LinearSelection(
this.getModel().getDocument(), newRange ) :
- new ve.dm.NullSelection(
this.getModel().getDocument() )
- );
- } finally {
- this.decRenderLock();
- }
- this.checkUnicorns( false );
- // Firefox lets you create multiple selections within a single paragraph
- // which our model doesn't support, so detect and prevent these.
- // This shouldn't create problems with IME candidates as only an
explicit user action
- // can create a multiple selection (CTRL+click), and we remove it
immediately, so there can
- // never be a multiple selection while the user is typing text;
therefore the
- // selection change will never commit IME candidates prematurely.
- while ( this.nativeSelection.rangeCount > 1 ) {
- // The current range is the last range, so remove ranges from
the front
- this.nativeSelection.removeRange(
this.nativeSelection.getRangeAt( 0 ) );
- }
};
/**
@@ -2745,174 +2725,6 @@
focusNode: node,
focusOffset: offset
} ) );
-};
-
-/**
- * Handle content changes observed from the DOM.
- *
- * The DOM change is analysed heuristically to build a semantically meaningful
Transaction
- * with the annotations the user intended.
- *
- * @see ve.ce.SurfaceObserver#pollOnce
- *
- * @method
- * @param {ve.ce.Node} node CE node the change occurred in
- * @param {Object} previous Old data
- * @param {Object} previous.text Old plain text content
- * @param {Object} previous.hash Old DOM hash
- * @param {ve.Range} previous.range Old selection
- * @param {Object} next New data
- * @param {Object} next.text New plain text content
- * @param {Object} next.hash New DOM hash
- * @param {ve.Range} next.range New selection
- */
-ve.ce.Surface.prototype.onObservedContentChange = function ( node, previous,
next ) {
- var data, range, len, annotations, offsetDiff, sameLeadingAndTrailing,
- previousStart, nextStart, newRange, replacementRange,
- fromLeft = 0,
- fromRight = 0,
- nodeOffset = node.getModel().getOffset(),
- previousData = previous.text.split( '' ),
- nextData = next.text.split( '' ),
- dmDoc = this.getModel().getDocument(),
- modelData = dmDoc.data,
- lengthDiff = next.text.length - previous.text.length,
- surface = this;
-
- if ( previous.range && next.range ) {
- offsetDiff = ( previous.range.isCollapsed() &&
next.range.isCollapsed() ) ?
- next.range.start - previous.range.start : null;
- previousStart = previous.range.start - nodeOffset - 1;
- nextStart = next.range.start - nodeOffset - 1;
- sameLeadingAndTrailing = offsetDiff !== null && (
- (
- lengthDiff > 0 &&
- previous.text.slice( 0, previousStart ) ===
- next.text.slice( 0, previousStart ) &&
- previous.text.slice( previousStart ) ===
- next.text.slice( nextStart )
- ) ||
- (
- lengthDiff < 0 &&
- previous.text.slice( 0, nextStart ) ===
- next.text.slice( 0, nextStart ) &&
- previous.text.slice( previousStart - lengthDiff
+ offsetDiff ) ===
- next.text.slice( nextStart )
- )
- );
-
- // Simple insertion
- if ( lengthDiff > 0 && offsetDiff === lengthDiff &&
sameLeadingAndTrailing ) {
- data = nextData.slice( previousStart, nextStart );
- // Apply insertion annotations
- if ( node.unicornAnnotations ) {
- annotations = node.unicornAnnotations;
- } else if (
this.keyDownState.focusIsAfterAnnotationBoundary ) {
- annotations =
modelData.getAnnotationsFromOffset(
- nodeOffset + previousStart + 1
- );
- } else {
- annotations =
this.model.getInsertionAnnotations();
- }
-
- if ( annotations.getLength() ) {
- ve.dm.Document.static.addAnnotationsToData(
data, annotations );
- }
-
- this.incRenderLock();
- try {
- this.changeModel(
- ve.dm.Transaction.newFromInsertion(
- this.documentView.model,
previous.range.start, data
- ),
- new ve.dm.LinearSelection(
this.documentView.model, next.range )
- );
- } finally {
- this.decRenderLock();
- }
- setTimeout( function () {
- surface.checkSequences();
- } );
- return;
- }
-
- // Simple deletion
- if ( ( offsetDiff === 0 || offsetDiff === lengthDiff ) &&
sameLeadingAndTrailing ) {
- if ( offsetDiff === 0 ) {
- range = new ve.Range( next.range.start,
next.range.start - lengthDiff );
- } else {
- range = new ve.Range( next.range.start,
previous.range.start );
- }
- this.incRenderLock();
- try {
- this.changeModel(
- ve.dm.Transaction.newFromRemoval(
this.documentView.model,
- range ),
- new ve.dm.LinearSelection(
this.documentView.model, next.range )
- );
- } finally {
- this.decRenderLock();
- }
- return;
- }
- }
-
- // Complex change:
- // 1. Count unchanged characters from left and right;
- // 2. Assume that the minimal changed region indicates the replacement
made by the user;
- // 3. Hence guess how to map annotations.
- // N.B. this logic can go wrong; e.g. this code will see slice->slide
and
- // assume that the user changed 'c' to 'd', but the user could instead
have changed 'ic'
- // to 'id', which would map annotations differently.
-
- len = Math.min( previousData.length, nextData.length );
-
- while ( fromLeft < len && previousData[ fromLeft ] === nextData[
fromLeft ] ) {
- ++fromLeft;
- }
-
- while (
- fromRight < len - fromLeft &&
- previousData[ previousData.length - 1 - fromRight ] ===
- nextData[ nextData.length - 1 - fromRight ]
- ) {
- ++fromRight;
- }
- replacementRange = new ve.Range(
- nodeOffset + 1 + fromLeft,
- nodeOffset + 1 + previousData.length - fromRight
- );
- data = nextData.slice( fromLeft, nextData.length - fromRight );
-
- if ( node.unicornAnnotations ) {
- // This CBN is unicorned. Use the stored annotations.
- annotations = node.unicornAnnotations;
- } else if ( fromLeft + fromRight < previousData.length ) {
- // Content is being removed, so guess that we want to use the
annotations from the
- // start of the removed content.
- annotations = modelData.getAnnotationsFromOffset(
replacementRange.start );
- } else {
- // No content is being removed, so guess that we want to use
the annotations from
- // just before the insertion (which means none at all if the
insertion is at the
- // start of a CBN).
- annotations = modelData.getAnnotationsFromOffset(
replacementRange.start - 1 );
- }
- if ( annotations.getLength() ) {
- ve.dm.Document.static.addAnnotationsToData( data, annotations );
- }
- newRange = next.range;
- if ( newRange.isCollapsed() ) {
- newRange = new ve.Range( dmDoc.getNearestCursorOffset(
newRange.start, 1 ) );
- }
-
- this.changeModel(
- ve.dm.Transaction.newFromReplacement( this.documentView.model,
replacementRange, data ),
- new ve.dm.LinearSelection( this.documentView.model, newRange )
- );
- this.queueCheckSequences = true;
- setTimeout( function () {
- surface.checkSequences();
- } );
};
/**
@@ -3007,28 +2819,19 @@
* modified, because anchorNode/focusNode are live and mutable, and so the
offsets may come to
* point confusingly to different places than they did when the selection was
saved).
*
- * Annotation changes before the cursor focus are detected: see
ve.ce.isAfterAnnotationBoundary .
- *
* @param {jQuery.Event|null} e Key down event; must be active when this call
is made
*/
ve.ce.Surface.prototype.storeKeyDownState = function ( e ) {
this.keyDownState.event = e;
this.keyDownState.selection = null;
- this.keyDownState.focusIsAfterAnnotationBoundary = null;
- if ( this.nativeSelection.rangeCount > 0 ) {
- this.keyDownState.focusIsAfterAnnotationBoundary =
ve.ce.isAfterAnnotationBoundary(
- this.nativeSelection.focusNode,
- this.nativeSelection.focusOffset
- );
- if ( e && (
- e.keyCode === OO.ui.Keys.UP ||
- e.keyCode === OO.ui.Keys.DOWN ||
- e.keyCode === OO.ui.Keys.LEFT ||
- e.keyCode === OO.ui.Keys.RIGHT
- ) ) {
- this.keyDownState.selection = new ve.SelectionState(
this.nativeSelection );
- }
+ if ( this.nativeSelection.rangeCount > 0 && e && (
+ e.keyCode === OO.ui.Keys.UP ||
+ e.keyCode === OO.ui.Keys.DOWN ||
+ e.keyCode === OO.ui.Keys.LEFT ||
+ e.keyCode === OO.ui.Keys.RIGHT
+ ) ) {
+ this.keyDownState.selection = new ve.SelectionState(
this.nativeSelection );
}
};
diff --git a/src/ce/ve.ce.SurfaceObserver.js b/src/ce/ve.ce.SurfaceObserver.js
index 9f36fa2..4ace94b 100644
--- a/src/ce/ve.ce.SurfaceObserver.js
+++ b/src/ce/ve.ce.SurfaceObserver.js
@@ -115,11 +115,7 @@
/**
* Poll for changes.
*
- * TODO: fixing selection in certain cases, handling selection across multiple
nodes in Firefox
- *
* @method
- * @fires contentChange
- * @fires rangeChange
*/
ve.ce.SurfaceObserver.prototype.pollOnce = function () {
this.pollOnceInternal( true );
@@ -148,20 +144,13 @@
/**
* Poll for changes.
*
- * TODO: fixing selection in certain cases, handling selection across multiple
nodes in Firefox
- *
* @method
* @private
* @param {boolean} signalChanges If there changes are observed, call
Surface#handleObservedChange
* @param {boolean} selectionOnly Check for selection changes only
- * @fires contentChange
- * @fires rangeChange
*/
ve.ce.SurfaceObserver.prototype.pollOnceInternal = function ( signalChanges,
selectionOnly ) {
- var oldState, newState,
- contentChange = null,
- branchNodeChange = null,
- rangeChange = null;
+ var oldState, newState;
if ( !this.domDocument || this.disabled ) {
return;
@@ -173,41 +162,16 @@
this.documentView.getDocumentNode(),
selectionOnly
);
-
this.rangeState = newState;
- if ( !selectionOnly && newState.node !== null &&
newState.contentChanged && signalChanges ) {
- contentChange = {
- node: newState.node,
- previous: { text: oldState.text, hash: oldState.hash,
range: oldState.veRange },
- next: { text: newState.text, hash: newState.hash,
range: newState.veRange }
- };
- }
-
- // TODO: Is it correct that branchNode changes are signalled even if
!signalChanges ?
- if ( newState.branchNodeChanged ) {
- branchNodeChange = {
- oldBranchNode: (
- oldState && oldState.node && oldState.node.root
?
- oldState.node :
- null
- ),
- newBranchNode: newState.node
- };
- }
-
- if ( newState.selectionChanged && signalChanges ) {
- // Caution: selectionChanged is true if the CE selection is
different, which can
- // be the case even if the DM selection is unchanged. So the
following line can
- // signal a range change with identical oldRange and newRange.
- rangeChange = {
- oldRange: ( oldState ? oldState.veRange : null ),
- newRange: newState.veRange
- };
- }
-
- if ( contentChange || branchNodeChange || rangeChange ) {
- this.surface.handleObservedChanges( contentChange,
branchNodeChange, rangeChange );
+ if ( signalChanges && (
+ newState.contentChanged ||
+ // TODO: The prior code signalled branchNode changes even if
!signalChanges .
+ // Was this needed?
+ newState.branchNodeChanged ||
+ newState.selectionChanged
+ ) ) {
+ this.surface.handleObservedChanges( oldState, newState );
}
};
diff --git a/src/ce/ve.ce.js b/src/ce/ve.ce.js
index 5ed0e59..b253d66 100644
--- a/src/ce/ve.ce.js
+++ b/src/ce/ve.ce.js
@@ -457,3 +457,158 @@
}
return $( node ).closest( '.ve-ce-linkAnnotation' )[ 0 ];
};
+
+/**
+ * Analyse a DOM content change to build a Transaction
+ *
+ * Content changes have oldState.node === newState.node and
newState.contentChanged === true .
+ * Annotations are inferred heuristically from plaintext to do what the user
intended.
+ * TODO: treat more changes as simple (not needing a re-render); see
+ * https://phabricator.wikimedia.org/T114260 .
+ *
+ * @method
+ * @param {ve.ce.RangeState} oldState The prior range state
+ * @param {ve.ce.RangeState} newState The changed range state
+ *
+ * @return {Object} Results of analysis
+ * @return {ve.dm.Transaction} return.transaction Transaction corresponding to
the DOM content change
+ * @return {ve.dm.Selection} return.selection Changed selection to apply
(TODO: unsafe / useless?)
+ * @return {boolean} return.rerender Whether the DOM needs rerendering after
applying the transaction
+ */
+ve.ce.modelChangeFromContentChange = function ( oldState, newState ) {
+ var data, len, annotations, bothCollapsed, oldStart, newStart,
replacementRange,
+ fromLeft = 0,
+ fromRight = 0,
+ // It is guaranteed that oldState.node === newState.node , so
just call it 'node'
+ node = newState.node,
+ nodeOffset = node.getModel().getOffset(),
+ oldText = oldState.text,
+ newText = newState.text,
+ oldRange = oldState.veRange,
+ newRange = newState.veRange,
+ oldData = oldText.split( '' ),
+ newData = newText.split( '' ),
+ lengthDiff = newText.length - oldText.length,
+ dmDoc = node.getModel().getDocument(),
+ modelData = dmDoc.data;
+
+ bothCollapsed = oldRange.isCollapsed() && newRange.isCollapsed();
+ oldStart = oldRange.start - nodeOffset - 1;
+ newStart = newRange.start - nodeOffset - 1;
+
+ // If the only change is an insertion just before the new cursor, then
apply a
+ // single insertion transaction, using the annotations from the old
start
+ // position (accounting for whether the cursor was before or after an
annotation
+ // boundary)
+ if (
+ bothCollapsed &&
+ lengthDiff > 0 &&
+ oldText.slice( 0, oldStart ) === newText.slice( 0, oldStart ) &&
+ oldText.slice( oldStart ) === newText.slice( newStart )
+ ) {
+ data = newData.slice( oldStart, newStart );
+ if ( node.unicornAnnotations ) {
+ annotations = node.unicornAnnotations;
+ } else {
+ annotations =
modelData.getInsertionAnnotationsFromRange(
+ oldRange,
+ oldState.focusIsAfterAnnotationBoundary
+ );
+ }
+
+ if ( annotations.getLength() ) {
+ ve.dm.Document.static.addAnnotationsToData( data,
annotations );
+ }
+
+ return {
+ transaction: ve.dm.Transaction.newFromInsertion(
+ dmDoc,
+ oldRange.start,
+ data
+ ),
+ selection: new ve.dm.LinearSelection( dmDoc, newRange ),
+ rerender: false
+ };
+ }
+
+ // If the only change is a removal touching the old cursor position,
then apply
+ // a single removal transaction.
+ if (
+ bothCollapsed &&
+ lengthDiff < 0 &&
+ oldText.slice( 0, newStart ) === newText.slice( 0, newStart ) &&
+ oldText.slice( newStart - lengthDiff ) === newText.slice(
newStart )
+ ) {
+ return {
+ transaction: ve.dm.Transaction.newFromRemoval(
+ dmDoc,
+ new ve.Range( newRange.start, newRange.start -
lengthDiff )
+ ),
+ selection: new ve.dm.LinearSelection( dmDoc, newRange ),
+ rerender: false
+ };
+ }
+
+ // Complex change (either removal+insertion or insertion not just
before new cursor)
+ // 1. Count unchanged characters from left and right;
+ // 2. Assume that the minimal changed region indicates the replacement
made by the user;
+ // 3. Hence guess how to map annotations.
+ // N.B. this logic can go wrong; e.g. this code will see slice->slide
and
+ // assume that the user changed 'c' to 'd', but the user could instead
have changed 'ic'
+ // to 'id', which would map annotations differently.
+
+ len = Math.min( oldData.length, newData.length );
+
+ while ( fromLeft < len && oldData[ fromLeft ] === newData[ fromLeft ] )
{
+ ++fromLeft;
+ }
+
+ while (
+ fromRight < len - fromLeft &&
+ oldData[ oldData.length - 1 - fromRight ] ===
+ newData[ newData.length - 1 - fromRight ]
+ ) {
+ ++fromRight;
+ }
+ replacementRange = new ve.Range(
+ nodeOffset + 1 + fromLeft,
+ nodeOffset + 1 + oldData.length - fromRight
+ );
+ data = newData.slice( fromLeft, newData.length - fromRight );
+
+ if ( node.unicornAnnotations ) {
+ // This CBN is unicorned. Use the stored annotations.
+ annotations = node.unicornAnnotations;
+ } else {
+ // Guess the annotations from the (possibly empty) range being
replaced.
+ //
+ // Still consider focusIsAfterAnnotationBoundary, even though
the change is
+ // not necessarily at the cursor: assume the old focus was
inside the same
+ // DOM text node as the insertion, and therefore has the same
annotations.
+ // Otherwise, when using an IME that selects inserted text,
this code path
+ // can cause an annotation discrepancy that triggers an
unwanted re-render,
+ // closing the IME (For example, when typing at the start of
<p><i>x</i></p>
+ // in Windows 8.1 Korean on IE11).
+ annotations = modelData.getInsertionAnnotationsFromRange(
+ replacementRange,
+ oldRange.isCollapsed() &&
oldState.focusIsAfterAnnotationBoundary
+ );
+ }
+ if ( annotations.getLength() ) {
+ ve.dm.Document.static.addAnnotationsToData( data, annotations );
+ }
+ if ( newRange.isCollapsed() ) {
+ // TODO: Remove this, or comment why it's necessary
+ // (When wouldn't we be at a cursor offset?)
+ newRange = new ve.Range( dmDoc.getNearestCursorOffset(
newRange.start, 1 ) );
+ }
+ return {
+ transaction: ve.dm.Transaction.newFromReplacement(
+ dmDoc,
+ replacementRange,
+ data
+ ),
+ selection: new ve.dm.LinearSelection( dmDoc, newRange ),
+ rerender: true
+ };
+};
diff --git a/tests/ce/ve.ce.Surface.test.js b/tests/ce/ve.ce.Surface.test.js
index 57ab75c..9147bd4 100644
--- a/tests/ce/ve.ce.Surface.test.js
+++ b/tests/ce/ve.ce.Surface.test.js
@@ -494,7 +494,7 @@
}
} );
-QUnit.test( 'onObservedContentChange', function ( assert ) {
+QUnit.test( 'handleObservedChanges (content changes)', function ( assert ) {
var i,
cases = [
{
@@ -556,12 +556,53 @@
]
],
msg: 'Replace into non-zero annotation next to
word break'
+ },
+ {
+ prevHtml: '<p><b>X</b></p>',
+ prevRange: new ve.Range( 2 ),
+ nextHtml: '<p><b>XY</b></p>',
+ nextRange: new ve.Range( 3 ),
+ expectedOps: [
+ [
+ { type: 'retain', length: 2 },
+ {
+ type: 'replace',
+ insert: [ [ 'Y', [ 0 ]
] ],
+ remove: [],
+ insertedDataOffset: 0,
+ insertedDataLength: 1
+ },
+ { type: 'retain', length: 3 }
+ ]
+ ],
+ msg: 'Append into bold'
+ },
+ {
+ prevHtml: '<p><b>X</b></p>',
+ prevRange: new ve.Range( 2 ),
+ prevFocusIsAfterAnnotationBoundary: true,
+ nextHtml: '<p><b>X</b>Y</p>',
+ nextRange: new ve.Range( 3 ),
+ expectedOps: [
+ [
+ { type: 'retain', length: 2 },
+ {
+ type: 'replace',
+ insert: [ 'Y' ],
+ remove: [],
+ insertedDataOffset: 0,
+ insertedDataLength: 1
+ },
+ { type: 'retain', length: 3 }
+ ]
+ ],
+ msg: 'Append after bold'
}
];
QUnit.expect( cases.length * 2 );
- function testRunner( prevHtml, prevRange, nextHtml, nextRange,
expectedOps, expectedRange, msg ) {
+ function testRunner( prevHtml, prevRange,
prevFocusIsAfterAnnotationBoundary, nextHtml, nextRange, expectedOps,
expectedRange, msg ) {
var txs, i, ops,
view = ve.test.utils.createSurfaceViewFromHtml(
prevHtml ),
model = view.getModel(),
@@ -569,17 +610,23 @@
prevNode = $( prevHtml )[ 0 ],
nextNode = $( nextHtml )[ 0 ],
prev = {
+ node: node,
text: ve.ce.getDomText( prevNode ),
hash: ve.ce.getDomHash( prevNode ),
- range: prevRange
+ veRange: prevRange,
+ focusIsAfterAnnotationBoundary:
prevFocusIsAfterAnnotationBoundary || false
},
next = {
+ node: node,
text: ve.ce.getDomText( nextNode ),
hash: ve.ce.getDomHash( nextNode ),
- range: nextRange
+ veRange: nextRange,
+ contentChanged: true
};
- view.onObservedContentChange( node, prev, next );
+ // Set model linear selection, so that insertion annotations
are primed correctly
+ model.setLinearSelection( prevRange );
+ view.handleObservedChanges( prev, next );
txs = model.getHistory()[ 0 ].transactions;
ops = [];
for ( i = 0; i < txs.length; i++ ) {
@@ -593,7 +640,8 @@
for ( i = 0; i < cases.length; i++ ) {
testRunner(
- cases[ i ].prevHtml, cases[ i ].prevRange, cases[ i
].nextHtml, cases[ i ].nextRange,
+ cases[ i ].prevHtml, cases[ i ].prevRange, cases[ i
].prevFocusIsAfterAnnotationBoundary || false,
+ cases[ i ].nextHtml, cases[ i ].nextRange,
cases[ i ].expectedOps, cases[ i ].expectedRange ||
cases[ i ].nextRange, cases[ i ].msg
);
}
--
To view, visit https://gerrit.wikimedia.org/r/241144
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I420c14f56187bb881c4b636a746d4383d9ba3146
Gerrit-PatchSet: 15
Gerrit-Project: VisualEditor/VisualEditor
Gerrit-Branch: master
Gerrit-Owner: Divec <[email protected]>
Gerrit-Reviewer: DLynch <[email protected]>
Gerrit-Reviewer: Divec <[email protected]>
Gerrit-Reviewer: Esanders <[email protected]>
Gerrit-Reviewer: Jforrester <[email protected]>
Gerrit-Reviewer: jenkins-bot <>
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
