jenkins-bot has submitted this change and it was merged.
Change subject: Refactor SurfaceObserver pollOnceInternal
......................................................................
Refactor SurfaceObserver pollOnceInternal
Refactor pollOnceInternal to make its logic clearer.
* Remove ve.ce.DomRange class (had misleadingly live node properties)
* Move much of the SurfaceObserver state to new ve.ce.RangeState class
Change-Id: Iccb6d39022ee06c8bd1fb687343efa160dbac44f
---
M .docs/categories.json
M .docs/eg-iframe.html
M build/modules.json
M demos/ve/desktop.html
M demos/ve/mobile.html
D src/ce/ve.ce.DomRange.js
A src/ce/ve.ce.RangeState.js
M src/ce/ve.ce.SurfaceObserver.js
M tests/index.html
9 files changed, 266 insertions(+), 178 deletions(-)
Approvals:
Catrope: Looks good to me, approved
jenkins-bot: Verified
diff --git a/.docs/categories.json b/.docs/categories.json
index 5a5e4a4..9b5d74b 100644
--- a/.docs/categories.json
+++ b/.docs/categories.json
@@ -96,7 +96,7 @@
"ve.ce.NodeFactory",
"ve.ce.Surface",
"ve.ce.SurfaceObserver",
- "ve.ce.DomRange"
+ "ve.ce.RangeState"
]
},
{
diff --git a/.docs/eg-iframe.html b/.docs/eg-iframe.html
index 9562a05..a475983 100644
--- a/.docs/eg-iframe.html
+++ b/.docs/eg-iframe.html
@@ -210,7 +210,7 @@
<script
src="../src/dm/metaitems/ve.dm.CommentMetaItem.js"></script>
<script src="../src/dm/nodes/ve.dm.CommentNode.js"></script>
<script src="../src/ce/ve.ce.js"></script>
- <script src="../src/ce/ve.ce.DomRange.js"></script>
+ <script src="../src/ce/ve.ce.RangeState.js"></script>
<script src="../src/ce/ve.ce.AnnotationFactory.js"></script>
<script src="../src/ce/ve.ce.NodeFactory.js"></script>
<script src="../src/ce/ve.ce.Document.js"></script>
diff --git a/build/modules.json b/build/modules.json
index c96abff..0f8adb3 100644
--- a/build/modules.json
+++ b/build/modules.json
@@ -235,7 +235,7 @@
"src/dm/metaitems/ve.dm.CommentMetaItem.js",
"src/dm/nodes/ve.dm.CommentNode.js",
"src/ce/ve.ce.js",
- "src/ce/ve.ce.DomRange.js",
+ "src/ce/ve.ce.RangeState.js",
"src/ce/ve.ce.AnnotationFactory.js",
"src/ce/ve.ce.NodeFactory.js",
"src/ce/ve.ce.Document.js",
diff --git a/demos/ve/desktop.html b/demos/ve/desktop.html
index 41ab04d..49ddc20 100644
--- a/demos/ve/desktop.html
+++ b/demos/ve/desktop.html
@@ -222,7 +222,7 @@
<script
src="../../src/dm/metaitems/ve.dm.CommentMetaItem.js"></script>
<script src="../../src/dm/nodes/ve.dm.CommentNode.js"></script>
<script src="../../src/ce/ve.ce.js"></script>
- <script src="../../src/ce/ve.ce.DomRange.js"></script>
+ <script src="../../src/ce/ve.ce.RangeState.js"></script>
<script src="../../src/ce/ve.ce.AnnotationFactory.js"></script>
<script src="../../src/ce/ve.ce.NodeFactory.js"></script>
<script src="../../src/ce/ve.ce.Document.js"></script>
diff --git a/demos/ve/mobile.html b/demos/ve/mobile.html
index ed878ed..6997b4f 100644
--- a/demos/ve/mobile.html
+++ b/demos/ve/mobile.html
@@ -223,7 +223,7 @@
<script
src="../../src/dm/metaitems/ve.dm.CommentMetaItem.js"></script>
<script src="../../src/dm/nodes/ve.dm.CommentNode.js"></script>
<script src="../../src/ce/ve.ce.js"></script>
- <script src="../../src/ce/ve.ce.DomRange.js"></script>
+ <script src="../../src/ce/ve.ce.RangeState.js"></script>
<script src="../../src/ce/ve.ce.AnnotationFactory.js"></script>
<script src="../../src/ce/ve.ce.NodeFactory.js"></script>
<script src="../../src/ce/ve.ce.Document.js"></script>
diff --git a/src/ce/ve.ce.DomRange.js b/src/ce/ve.ce.DomRange.js
deleted file mode 100644
index 7f818c8..0000000
--- a/src/ce/ve.ce.DomRange.js
+++ /dev/null
@@ -1,69 +0,0 @@
-/*!
- * VisualEditor DomRange class.
- *
- * @copyright 2011-2014 VisualEditor Team and others; see
http://ve.mit-license.org
- */
-
-/**
- * DOM range
- *
- * @class
- * @constructor
- * @param {HTMLElement} focusNode Selection focus node
- * @param {number} focusOffset Selection focus offset
- * @param {HTMLElement} anchorNode Selection anchor node
- * @param {number} anchorOffset Selection anchor offset
- */
-ve.ce.DomRange = function VeCeDomRange( focusNode, focusOffset, anchorNode,
anchorOffset ) {
- this.focusNode = focusNode;
- this.focusOffset = focusOffset;
- this.anchorNode = anchorNode;
- this.anchorOffset = anchorOffset;
-};
-
-/* Static Methods */
-
-/**
- * Create a new DOM range from a document's native selection
- *
- * @param {HTMLDocument} doc Document to get selection from
- * @return {ve.ce.DomRange} DOM range
- */
-ve.ce.DomRange.newFromDocument = function ( doc ) {
- var selection = doc.getSelection();
- return new ve.ce.DomRange(
- selection.focusNode, selection.focusOffset,
selection.anchorNode, selection.anchorOffset
- );
-};
-
-/* Methods */
-
-/**
- * Check if a DOM range is equal to another DOM range
- *
- * @param {ve.ce.DomRange} other DOM range to compare to
- * @return {boolean} The other DOM range is equal to this one
- */
-ve.ce.DomRange.prototype.equals = function ( other ) {
- return other &&
- this.focusNode === other.focusNode &&
- this.focusOffset === other.focusOffset &&
- this.anchorNode === other.anchorNode &&
- this.anchorOffset === other.anchorOffset;
-};
-
-/**
- * Get a linear model ve.Range for the DOM range
- *
- * @returns {ve.Range|null} Linear model range, or null if out of bounds
- */
-ve.ce.DomRange.prototype.getRange = function () {
- try {
- return new ve.Range(
- ve.ce.getOffset( this.anchorNode, this.anchorOffset ),
- ve.ce.getOffset( this.focusNode, this.focusOffset )
- );
- } catch ( e ) {
- return null;
- }
-};
diff --git a/src/ce/ve.ce.RangeState.js b/src/ce/ve.ce.RangeState.js
new file mode 100644
index 0000000..a719214
--- /dev/null
+++ b/src/ce/ve.ce.RangeState.js
@@ -0,0 +1,214 @@
+/*!
+ * VisualEditor Content Editable Range State class
+ *
+ * @copyright 2011-2014 VisualEditor Team and others; see
http://ve.mit-license.org
+ */
+
+/**
+ * ContentEditable range state (a snapshot of CE selection/content state)
+ *
+ * @class
+ *
+ * @constructor
+ * @param {ve.ce.RangeState|null} old Previous range state
+ * @param {ve.ce.DocumentNode} docNode The current document node
+ * @param {boolean} selectionOnly The caller promises the content has not
changed from old
+ */
+ve.ce.RangeState = function VeCeRangeState( old, docNode, selectionOnly ) {
+ /**
+ * @property {boolean} branchNodeChanged Whether the CE branch node
changed
+ */
+ this.branchNodeChanged = null;
+
+ /**
+ * @property {boolean} selectionChanged Whether the DOM range changed
+ */
+ this.selectionChanged = null;
+
+ /**
+ * @property {boolean} contentChanged Whether the content changed
+ */
+ this.contentChanged = null;
+
+ /**
+ * @property {boolean} leftBlockSlug Whether the range left a block slug
+ */
+ this.leftBlockSlug = null;
+
+ /**
+ * @property {boolean} enteredBlockSlug Whether the range entered a
block slug
+ */
+ this.enteredBlockSlug = null;
+
+ /**
+ * @property {ve.Range|null} veRange The current selection range
+ */
+ this.veRange = null;
+
+ /**
+ * @property {ve.ce.BranchNode|null} node The current branch node
+ */
+ this.node = null;
+
+ /**
+ * @property {jQuery|null} $slugWrapper The current slug wrapper
+ */
+ this.$slugWrapper = null;
+
+ /**
+ * @property {string} text Plain text of current branch node
+ */
+ this.text = null;
+
+ /**
+ * @property {string} DOM Hash of current branch node
+ */
+ this.hash = null;
+
+ this.saveState( old, docNode, selectionOnly );
+};
+
+/* Inheritance */
+
+OO.initClass( ve.ce.RangeState );
+
+/* Methods */
+
+/**
+ * Saves a snapshot of the current range state
+ * @method
+ * @param {ve.ce.RangeState|null} old Previous range state
+ * @param {ve.ce.DocumentNode} docNode The current document node
+ * @param {boolean} selectionOnly The caller promises the content has not
changed from old
+ */
+ve.ce.RangeState.prototype.saveState = function ( old, docNode, selectionOnly
) {
+ var $nodeOrSlug, selection, anchorNodeChanged;
+
+ // Freeze selection out of live object.
+ selection = ( function ( liveSelection ) {
+ return {
+ focusNode: liveSelection.focusNode,
+ focusOffset: liveSelection.focusOffset,
+ anchorNode: liveSelection.anchorNode,
+ anchorOffset: liveSelection.anchorOffset
+ };
+ } ( docNode.getElementDocument().getSelection() ) );
+
+ // Get new range information
+ if ( old && !old.compareSelection( selection ) ) {
+ // No change; use old values for speed
+ this.selectionChanged = false;
+ this.veRange = old.veRange;
+ this.$slugWrapper = old.$slugWrapper;
+ this.leftBlockSlug = false;
+ this.enteredBlockSlug = false;
+ } else {
+ this.selectionChanged = true;
+ try {
+ this.veRange = new ve.Range(
+ ve.ce.getOffset( selection.anchorNode,
selection.anchorOffset ),
+ ve.ce.getOffset( selection.focusNode,
selection.focusOffset )
+ );
+ } catch ( e ) {
+ this.veRange = null;
+ }
+ }
+
+ anchorNodeChanged = !old || old.compareAnchorNode( selection );
+
+ if ( !anchorNodeChanged ) {
+ this.node = old.node;
+ this.$slugWrapper = old.$slugWrapper;
+ } else {
+ $nodeOrSlug = $( selection.anchorNode ).closest(
+ '.ve-ce-branchNode, .ve-ce-branchNode-blockSlugWrapper'
+ );
+ if ( $nodeOrSlug.length === 0 ) {
+ this.node = null;
+ this.$slugWrapper = null;
+ } else if ( $nodeOrSlug.hasClass(
've-ce-branchNode-blockSlugWrapper' ) ) {
+ this.node = null;
+ this.$slugWrapper = $nodeOrSlug;
+ } else {
+ this.node = $nodeOrSlug.data( 'view' );
+ this.$slugWrapper = null;
+ // Check this node belongs to our document
+ if ( this.node && this.node.root !== docNode ) {
+ this.node = null;
+ this.veRange = null;
+ }
+ }
+ }
+
+ this.branchNodeChanged = ( !old || this.node !== old.node );
+
+ // Compute text/hash, for change comparison
+ if ( selectionOnly && !anchorNodeChanged ) {
+ this.text = old.text;
+ this.hash = old.hash;
+ } else if ( this.node === null ) {
+ this.text = null;
+ this.hash = null;
+ } else {
+ this.text = ve.ce.getDomText( this.node.$element[0] );
+ this.hash = ve.ce.getDomHash( this.node.$element[0] );
+ }
+
+ this.leftBlockSlug = (
+ old &&
+ old.$slugWrapper &&
+ !old.$slugWrapper.is( this.$slugWrapper )
+ );
+ this.enteredBlockSlug = (
+ old &&
+ this.$slugWrapper &&
+ this.$slugWrapper.length > 0 &&
+ !this.$slugWrapper.is( old.$slugWrapper )
+ );
+
+ // Only set contentChanged if we're still in the same branch node/block
slug
+ this.contentChanged = (
+ !selectionOnly &&
+ old &&
+ old.node === this.node && (
+ old.hash === null ||
+ old.text === null ||
+ old.hash !== this.hash ||
+ old.text !== this.text
+ )
+ );
+
+ // 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).
+ this.misleadingSelection = selection;
+};
+
+/**
+ * Compare a selection object for changes from the snapshotted state.
+ *
+ * The meaning of "changes" is slightly misleading, because the offsets were
taken
+ * at two different instants, between which content outside of the selection
may
+ * have changed. This can in theory cause false negatives (unnoticed changes).
+ *
+ * @param {Object} selection Selection to compare
+ * @returns {boolean} Whether there is a change
+ */
+ve.ce.RangeState.prototype.compareSelection = function ( selection ) {
+ return (
+ this.misleadingSelection.focusNode !== selection.focusNode ||
+ this.misleadingSelection.focusOffset !== selection.focusOffset
||
+ this.misleadingSelection.anchorNode !== selection.anchorNode ||
+ this.misleadingSelection.anchorOffset !== selection.anchorOffset
+ );
+};
+
+/**
+ * Compare a selection object for a change of anchor node from the snapshotted
state.
+ *
+ * @param {Object} selection Selection to compare
+ * @returns {boolean} Whether the anchor node has changed
+ */
+ve.ce.RangeState.prototype.compareAnchorNode = function ( selection ) {
+ return this.misleadingSelection.anchorNode !== selection.anchorNode;
+};
diff --git a/src/ce/ve.ce.SurfaceObserver.js b/src/ce/ve.ce.SurfaceObserver.js
index ad1564a..95b4da7 100644
--- a/src/ce/ve.ce.SurfaceObserver.js
+++ b/src/ce/ve.ce.SurfaceObserver.js
@@ -25,9 +25,7 @@
this.disabled = false;
this.timeoutId = null;
this.pollInterval = 250; // ms
-
- // Initialization
- this.clear();
+ this.rangeState = null;
};
/* Inheritance */
@@ -74,15 +72,9 @@
* Clear polling data.
*
* @method
- * @param {ve.Range} range Initial range to use
*/
-ve.ce.SurfaceObserver.prototype.clear = function ( range ) {
- this.domRange = null;
- this.range = range || null;
- this.node = null;
- this.text = null;
- this.hash = null;
- this.$slugWrapper = null;
+ve.ce.SurfaceObserver.prototype.clear = function () {
+ this.rangeState = null;
};
/**
@@ -209,113 +201,61 @@
* @fires slugEnter
*/
ve.ce.SurfaceObserver.prototype.pollOnceInternal = function ( emitChanges,
selectionOnly ) {
- var $nodeOrSlug, node, text, hash, range, domRange, $slugWrapper,
- anchorNodeChange = false,
- enteredSlug = false,
- leftSlug = false,
+ var oldState, newState,
observer = this;
if ( !this.domDocument || this.disabled ) {
return;
}
- range = this.range;
- node = this.node;
- domRange = ve.ce.DomRange.newFromDocument( this.domDocument );
+ oldState = this.rangeState;
+ newState = new ve.ce.RangeState(
+ oldState,
+ this.documentView.getDocumentNode(),
+ selectionOnly
+ );
- if ( !domRange.equals( this.domRange ) ) {
- if ( !this.domRange || this.domRange.anchorNode !==
domRange.anchorNode ) {
- anchorNodeChange = true;
- }
- range = domRange.getRange();
- this.domRange = domRange;
+ if ( newState.leftBlockSlug ) {
+ oldState.$slugWrapper
+ .addClass(
've-ce-branchNode-blockSlugWrapper-unfocused' )
+ .removeClass(
've-ce-branceNode-blockSlugWrapper-focused' );
}
- if ( anchorNodeChange ) {
- node = null;
- $nodeOrSlug = $( domRange.anchorNode ).closest(
'.ve-ce-branchNode, .ve-ce-branchNode-blockSlugWrapper' );
- if ( $nodeOrSlug.length ) {
- if ( $nodeOrSlug.hasClass(
've-ce-branchNode-blockSlugWrapper' ) ) {
- $slugWrapper = $nodeOrSlug;
- } else {
- node = $nodeOrSlug.data( 'view' );
- // Check this node belongs to our document
- if ( node && node.root !==
this.documentView.getDocumentNode() ) {
- node = null;
- range = null;
- }
+ if ( newState.enteredBlockSlug ) {
+ newState.$slugWrapper
+ .addClass( 've-ce-branchNode-blockSlugWrapper-focused' )
+ .removeClass(
've-ce-branchNode-blockSlugWrapper-unfocused' );
+ }
+
+ this.rangeState = newState;
+
+ if ( newState.enteredBlockSlug || newState.leftBlockSlug ) {
+ // Emit 'position' on the surface view after the animation
completes
+ this.setTimeout( function () {
+ if ( observer.surface ) {
+ observer.surface.emit( 'position' );
}
- }
-
- if ( this.$slugWrapper && !this.$slugWrapper.is( $slugWrapper )
) {
- this.$slugWrapper
- .addClass(
've-ce-branchNode-blockSlugWrapper-unfocused' )
- .removeClass(
've-ce-branchNode-blockSlugWrapper-focused' );
- this.$slugWrapper = null;
- leftSlug = true;
- }
-
- if ( $slugWrapper && $slugWrapper.length && !$slugWrapper.is(
this.$slugWrapper ) ) {
- this.$slugWrapper = $slugWrapper
- .addClass(
've-ce-branchNode-blockSlugWrapper-focused' )
- .removeClass(
've-ce-branchNode-blockSlugWrapper-unfocused' );
- enteredSlug = true;
- }
-
- if ( enteredSlug || leftSlug ) {
- // Emit 'position' on the surface view after the
animation completes
- this.setTimeout( function () {
- if ( observer.surface ) {
- observer.surface.emit( 'position' );
- }
- }, 200 );
- }
+ }, 200 );
}
- if ( this.node !== node ) {
- if ( node === null ) {
- this.text = null;
- this.hash = null;
- this.node = null;
- } else {
- this.text = ve.ce.getDomText( node.$element[0] );
- this.hash = ve.ce.getDomHash( node.$element[0] );
- this.node = node;
- }
- } else if ( !selectionOnly && node !== null ) {
- text = ve.ce.getDomText( node.$element[0] );
- hash = ve.ce.getDomHash( node.$element[0] );
- if ( this.text !== text || this.hash !== hash ) {
- if ( emitChanges ) {
- this.emit(
- 'contentChange',
- node,
- {
- text: this.text,
- hash: this.hash,
- range: this.range
- },
- { text: text, hash: hash, range: range }
- );
- }
- this.text = text;
- this.hash = hash;
- }
+ if ( !selectionOnly && newState.node !== null &&
newState.contentChanged && emitChanges ) {
+ this.emit(
+ 'contentChange',
+ newState.node,
+ { text: oldState.text, hash: oldState.hash, range:
oldState.veRange },
+ { text: newState.text, hash: newState.hash, range:
newState.veRange }
+ );
}
- // Only emit rangeChange event if there's a meaningful range difference
- if ( ( this.range && range ) ? !this.range.equals( range ) : (
this.range !== range ) ) {
- if ( emitChanges ) {
- this.emit(
- 'rangeChange',
- this.range,
- range
- );
- }
- this.range = range;
+ if ( newState.selectionChanged && emitChanges ) {
+ this.emit(
+ 'rangeChange',
+ ( oldState ? oldState.veRange : null ),
+ newState.veRange
+ );
}
- if ( emitChanges && enteredSlug ) {
+ if ( newState.enteredBlockSlug && emitChanges ) {
this.emit( 'slugEnter' );
}
};
@@ -338,5 +278,8 @@
* @return {ve.Range} Range
*/
ve.ce.SurfaceObserver.prototype.getRange = function () {
- return this.range;
+ if ( !this.rangeState ) {
+ return null;
+ }
+ return this.rangeState.veRange;
};
diff --git a/tests/index.html b/tests/index.html
index 4778413..393d97e 100644
--- a/tests/index.html
+++ b/tests/index.html
@@ -173,7 +173,7 @@
<script
src="../src/dm/metaitems/ve.dm.CommentMetaItem.js"></script>
<script src="../src/dm/nodes/ve.dm.CommentNode.js"></script>
<script src="../src/ce/ve.ce.js"></script>
- <script src="../src/ce/ve.ce.DomRange.js"></script>
+ <script src="../src/ce/ve.ce.RangeState.js"></script>
<script src="../src/ce/ve.ce.AnnotationFactory.js"></script>
<script src="../src/ce/ve.ce.NodeFactory.js"></script>
<script src="../src/ce/ve.ce.Document.js"></script>
--
To view, visit https://gerrit.wikimedia.org/r/166188
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: Iccb6d39022ee06c8bd1fb687343efa160dbac44f
Gerrit-PatchSet: 19
Gerrit-Project: VisualEditor/VisualEditor
Gerrit-Branch: master
Gerrit-Owner: Divec <[email protected]>
Gerrit-Reviewer: Catrope <[email protected]>
Gerrit-Reviewer: Divec <[email protected]>
Gerrit-Reviewer: Esanders <[email protected]>
Gerrit-Reviewer: jenkins-bot <>
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
