Catrope has uploaded a new change for review.
https://gerrit.wikimedia.org/r/165734
Change subject: Followup 2fcc4c0: fix bugs with selection-only polling
......................................................................
Followup 2fcc4c0: fix bugs with selection-only polling
The optimizations in selection-only polling were too aggressive.
When the cursor moved to a different node and the next poll was
a selection-only poll, we would not update this.node because that
code path was skipped for selection-only polls. Changing the
selection by clicking with the mouse will cause both a
selection-only poll (from onDocumentSelectionChange) and
a full poll (from a timeout in onDocumentMouseDown) to occur,
but they can occur in either order, at least in Chrome.
Probably what's going on is that there may or may not be
a timer tick between mousedown and selectionchange, depending
on how fast the user clicks or something.
I think this is related to recent cases I've seen of inserted
characters at the end of a line ending up on the next line.
I would imagine something like this could have caused that:
* User clicks at end of line
* mousedown happens, sets timeout for full poll
* DOM selectionchange event happens, does selectionOnly poll
* Poll emits selectionchange event but does not update this.node
* Timeout fires, full poll happens
* Poll observes that DOM selection hasn't changed, does nothing
* User types 'A', causes full poll
* Poll observes this.node has changed, does not check for content change
* No contentChange emitted, 'A' not added to DM, DM and CE out of sync
* Subsequent polls do nothing because DOM selection didn't change
* User types again
* Poll observes 'B' was added, emits contentChange
* CE sees <p>HelloA|B|</p>, computes insertion offset 7
* DM doesn't have 'A', so "insert B at 7" is <p>Hello</p>|B|
* This is normalized to <p>Hello</p><p>B</p> and committed
* New paragraph is rendered but old paragraph isn't rerendered
* CE DOM now shows <p>HelloAB</p><p>B</p>
* User is confused and goes to Roan's desk
Change-Id: I6333590e46c6c7241b8974afe5e5dfed9d79c877
(cherry picked from commit fdbbad7f29e1159494c0573dd1866a6f2f62d8a1)
---
M src/ce/ve.ce.SurfaceObserver.js
1 file changed, 67 insertions(+), 67 deletions(-)
git pull ssh://gerrit.wikimedia.org:29418/VisualEditor/VisualEditor
refs/changes/34/165734/1
diff --git a/src/ce/ve.ce.SurfaceObserver.js b/src/ce/ve.ce.SurfaceObserver.js
index b786540..073f6de 100644
--- a/src/ce/ve.ce.SurfaceObserver.js
+++ b/src/ce/ve.ce.SurfaceObserver.js
@@ -188,7 +188,7 @@
*/
ve.ce.SurfaceObserver.prototype.pollOnceInternal = function ( emitChanges,
selectionOnly ) {
var $nodeOrSlug, node, text, hash, range, domRange, $slugWrapper,
- domRangeChange = false,
+ anchorNodeChange = false,
slugChange = false,
observer = this;
@@ -201,82 +201,82 @@
domRange = ve.ce.DomRange.newFromDocument( this.domDocument );
if ( !domRange.equals( this.domRange ) ) {
+ if ( !this.domRange || this.domRange.anchorNode !==
domRange.anchorNode ) {
+ anchorNodeChange = true;
+ }
range = domRange.getRange();
this.domRange = domRange;
- domRangeChange = true;
}
- if ( !selectionOnly ) {
- if ( domRangeChange ) {
- node = null;
- $nodeOrSlug = $( domRange.anchorNode ).closest(
'.ve-ce-branchNode, .ve-ce-branchNode-slug' );
- if ( $nodeOrSlug.length ) {
- if ( $nodeOrSlug.hasClass(
've-ce-branchNode-slug' ) ) {
- $slugWrapper = $nodeOrSlug.closest(
'.ve-ce-branchNode-blockSlugWrapper' );
- } else {
- node = $nodeOrSlug.data( 'view' );
- // Check this node belongs to our
document
- if ( node && node.root !==
this.documentView.getDocumentNode() ) {
- node = null;
- range = null;
- }
+ if ( anchorNodeChange ) {
+ node = null;
+ $nodeOrSlug = $( domRange.anchorNode ).closest(
'.ve-ce-branchNode, .ve-ce-branchNode-slug' );
+ if ( $nodeOrSlug.length ) {
+ if ( $nodeOrSlug.hasClass( 've-ce-branchNode-slug' ) ) {
+ $slugWrapper = $nodeOrSlug.closest(
'.ve-ce-branchNode-blockSlugWrapper' );
+ } else {
+ node = $nodeOrSlug.data( 'view' );
+ // Check this node belongs to our document
+ if ( node && node.root !==
this.documentView.getDocumentNode() ) {
+ node = null;
+ range = null;
}
- }
-
- if ( this.$slugWrapper && !this.$slugWrapper.is(
$slugWrapper ) ) {
- this.$slugWrapper
- .addClass(
've-ce-branchNode-blockSlugWrapper-unfocused' )
- .removeClass(
've-ce-branchNode-blockSlugWrapper-focused' );
- this.$slugWrapper = null;
- slugChange = true;
- }
-
- if ( $slugWrapper && !$slugWrapper.is(
this.$slugWrapper) ) {
- this.$slugWrapper = $slugWrapper
- .addClass(
've-ce-branchNode-blockSlugWrapper-focused' )
- .removeClass(
've-ce-branchNode-blockSlugWrapper-unfocused' );
- slugChange = true;
- }
-
- if ( slugChange ) {
- // Emit 'position' on the surface view after
the animation completes
- this.setTimeout( function () {
- if ( observer.surface ) {
- observer.surface.emit(
'position' );
- }
- }, 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 ( 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 }
- );
+ if ( this.$slugWrapper && !this.$slugWrapper.is( $slugWrapper )
) {
+ this.$slugWrapper
+ .addClass(
've-ce-branchNode-blockSlugWrapper-unfocused' )
+ .removeClass(
've-ce-branchNode-blockSlugWrapper-focused' );
+ this.$slugWrapper = null;
+ slugChange = true;
+ }
+
+ if ( $slugWrapper && !$slugWrapper.is( this.$slugWrapper ) ) {
+ this.$slugWrapper = $slugWrapper
+ .addClass(
've-ce-branchNode-blockSlugWrapper-focused' )
+ .removeClass(
've-ce-branchNode-blockSlugWrapper-unfocused' );
+ slugChange = true;
+ }
+
+ if ( slugChange ) {
+ // Emit 'position' on the surface view after the
animation completes
+ this.setTimeout( function () {
+ if ( observer.surface ) {
+ observer.surface.emit( 'position' );
}
- this.text = text;
- this.hash = hash;
+ }, 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;
}
}
--
To view, visit https://gerrit.wikimedia.org/r/165734
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: I6333590e46c6c7241b8974afe5e5dfed9d79c877
Gerrit-PatchSet: 1
Gerrit-Project: VisualEditor/VisualEditor
Gerrit-Branch: wmf/1.25wmf2
Gerrit-Owner: Catrope <[email protected]>
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
