Divec has uploaded a new change for review.
https://gerrit.wikimedia.org/r/239825
Change subject: Support incomparable nodes in ve.compareDocumentOrder
......................................................................
Support incomparable nodes in ve.compareDocumentOrder
When comparing an old position node with a new one, it can happen that the old
node has become detached, which was causing the order comparison to fail. Now
ve.compareDocumentOrder returns null in all cases where the nodes have no
common ancestor.
Bug: T113216
Change-Id: I4592c05eca7f5f57084d13984cd51f87f7d199a8
---
M src/ce/ve.ce.Surface.js
M src/ve.SelectionState.js
M src/ve.utils.js
M tests/ve.test.js
4 files changed, 37 insertions(+), 19 deletions(-)
git pull ssh://gerrit.wikimedia.org:29418/VisualEditor/VisualEditor
refs/changes/25/239825/1
diff --git a/src/ce/ve.ce.Surface.js b/src/ce/ve.ce.Surface.js
index b72af87..0d5e625 100644
--- a/src/ce/ve.ce.Surface.js
+++ b/src/ce/ve.ce.Surface.js
@@ -1385,15 +1385,16 @@
* Compute the direction of cursor movement, if any
*
* Even if the user pressed a cursor key in the interior of the
document, there may not
- * be any movement: browser BIDI and ce=false handling can be quite
quirky
+ * be any movement: browser BIDI and ce=false handling can be quite
quirky.
*
- * @return {number|null} -1 for startwards, 1 for endwards, null for
none
+ * Furthermore, the keydown selection nodes may have become detached
since keydown (e.g.
+ * if ve.ce.ContentBranchNode#renderContents has run).
+ *
+ * @return {number|null} -1 for startwards, 1 for endwards, null for
none/unknown
*/
function getDirection() {
return (
isArrow &&
- surface.keyDownState.selection.focusNode &&
- surface.nativeSelection.focusNode &&
ve.compareDocumentOrder(
surface.nativeSelection.focusNode,
surface.nativeSelection.focusOffset,
@@ -3262,8 +3263,6 @@
// The intended direction is clear, even if the
cursor did not move
// or did something completely preposterous
afterDirection = e.keyCode === OO.ui.Keys.DOWN
? 1 : -1;
- } else if ( !surface.$document[ 0 ].contains(
startFocusNode ) ) {
- afterDirection = 0;
} else {
// Observe which way the cursor moved
afterDirection = ve.compareDocumentOrder(
diff --git a/src/ve.SelectionState.js b/src/ve.SelectionState.js
index 7958f70..23436e7 100644
--- a/src/ve.SelectionState.js
+++ b/src/ve.SelectionState.js
@@ -35,8 +35,8 @@
}
this.isBackwards = selection.isBackwards;
if ( this.isBackwards === undefined ) {
- // Set to false if nodes are null
- this.isBackwards = this.focusNode !== null &&
ve.compareDocumentOrder(
+ // Set to false if nodes are null or focus is no earlier than
anchor
+ this.isBackwards = ve.compareDocumentOrder(
this.focusNode,
this.focusOffset,
this.anchorNode,
diff --git a/src/ve.utils.js b/src/ve.utils.js
index 3e986a9..714ec02 100644
--- a/src/ve.utils.js
+++ b/src/ve.utils.js
@@ -1300,8 +1300,8 @@
/**
* Find the nearest common ancestor of DOM nodes
*
- * @param {...Node} DOM nodes in the same document
- * @return {Node|null} Nearest common ancestor node
+ * @param {...Node|null} DOM nodes
+ * @return {Node|null} Nearest common ancestor; or null if there is none / an
argument is null
*/
ve.getCommonAncestor = function () {
var i, j, nodeCount, chain, node,
@@ -1321,9 +1321,11 @@
node = node.parentNode;
}
if ( chain.length === 0 ) {
+ // args[ i ] was null (so no common ancestor)
return null;
}
if ( i > 0 && chain[ 0 ] !== chains[ chains.length - 1 ][ 0 ] )
{
+ // no common ancestor (different documents or
unattached branches)
return null;
}
if ( minHeight === null || minHeight > chain.length ) {
@@ -1332,8 +1334,9 @@
chains.push( chain );
}
- // Step through chains in parallel, until they differ
- // All chains are guaranteed to start with documentNode
+ // Step through chains in parallel, until they differ.
+ // All chains are guaranteed to start with the common document element
(or the common root
+ // of an unattached branch)
for ( i = 1; i < minHeight; i++ ) {
node = chains[ 0 ][ i ];
for ( j = 1; j < nodeCount; j++ ) {
@@ -1402,16 +1405,24 @@
/**
* Compare two nodes for position in document
*
- * @param {Node} node1 First node
- * @param {number} offset1 First offset
- * @param {Node} node2 Second node
- * @param {number} offset2 Second offset
- * @return {number} negative, zero or positive number
+ * Return null if either position is either null or incomparable (e.g. where
one of the nodes
+ * is detached or the nodes are from different documents).
+ *
+ * @param {Node|null} node1 First node
+ * @param {number|null} offset1 First offset
+ * @param {Node|null} node2 Second node
+ * @param {number|null} offset2 Second offset
+ * @return {number|null} negative, zero or positive number, or null if nodes
null or incomparable
*/
+
ve.compareDocumentOrder = function ( node1, offset1, node2, offset2 ) {
var commonAncestor = ve.getCommonAncestor( node1, node2 );
if ( commonAncestor === null ) {
- throw new Error( 'No common ancestor' );
+ // Signal no common ancestor. In theory we could disallow this
case, and check
+ // the nodes for detachedness and same-documentness before each
call, but such
+ // guard checks would duplicate (either explicitly or
implicitly) much of the
+ // branch traversal performed in this method.
+ return null;
}
return ve.compareTuples(
ve.getOffsetPath( commonAncestor, node1, offset1 ),
diff --git a/tests/ve.test.js b/tests/ve.test.js
index a80d979..1829504 100644
--- a/tests/ve.test.js
+++ b/tests/ve.test.js
@@ -880,7 +880,13 @@
{ nodes: 'img textE', ancestor: 'body' },
{ nodes: 'textA textB textC textD', ancestor: 'div' },
{ nodes: 'textA i b textC', ancestor: 'p' },
- { nodes: 'body div head p', ancestor: 'html' }
+ { nodes: 'body div head p', ancestor: 'html' },
+ { nodes: 'b null', ancestor: 'null' },
+ { nodes: 'null b', ancestor: 'null' },
+ { nodes: 'b i null', ancestor: 'null' },
+ { nodes: 'b null i', ancestor: 'null' },
+ { nodes: 'b unattached', ancestor: 'null' },
+ { nodes: 'unattached b', ancestor: 'null' }
];
nodes = {};
nodes.html = doc.documentElement;
@@ -896,6 +902,8 @@
nodes.textC = nodes.p.childNodes[ 2 ];
nodes.textD = nodes.div.childNodes[ 1 ];
nodes.textE = nodes.body.childNodes[ 1 ];
+ nodes.null = null;
+ nodes.unattached = doc.createElement( 'div' ).appendChild(
doc.createElement( 'span' ) );
function getNode( name ) {
return nodes[ name ];
}
--
To view, visit https://gerrit.wikimedia.org/r/239825
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I4592c05eca7f5f57084d13984cd51f87f7d199a8
Gerrit-PatchSet: 1
Gerrit-Project: VisualEditor/VisualEditor
Gerrit-Branch: master
Gerrit-Owner: Divec <[email protected]>
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
