jenkins-bot has submitted this change and it was merged.
Change subject: Remove dm.Surface's 'change' event
......................................................................
Remove dm.Surface's 'change' event
ve.dm.Surface.js:
* Stop emitting 'change' and remove its event documentation
ve.ce.Surface.js:
* Listen to 'select' instead of 'change'
* Perform a CE surface update after model-based keydown handling
ve.dm.Surface.test.js:
* Stop asserting that 'change' is emitted
Change-Id: I8f16289493e835d890709c6dfe093d04c18522b6
---
M modules/ve/ce/ve.ce.Surface.js
M modules/ve/dm/ve.dm.Surface.js
M modules/ve/test/dm/ve.dm.Surface.test.js
3 files changed, 79 insertions(+), 81 deletions(-)
Approvals:
Trevor Parscal: Looks good to me, approved
jenkins-bot: Verified
diff --git a/modules/ve/ce/ve.ce.Surface.js b/modules/ve/ce/ve.ce.Surface.js
index 902405c..82be00d 100644
--- a/modules/ve/ce/ve.ce.Surface.js
+++ b/modules/ve/ce/ve.ce.Surface.js
@@ -51,7 +51,7 @@
this.pasting = false;
this.clickHistory = [];
this.focusedNode = null;
- // This is set on entering changeModelSelection, then unset when
leaving.
+ // This is set on entering changeModel, then unset when leaving.
// It is used to test whether a reflected change event is emitted.
this.newModelSelection = null;
@@ -59,7 +59,8 @@
this.surfaceObserver.connect(
this, { 'contentChange': 'onContentChange', 'selectionChange':
'onSelectionChange' }
);
- this.model.connect( this, { 'change': 'onChange', 'lock': 'onLock',
'unlock': 'onUnlock' } );
+ this.model.connect( this,
+ { 'select': 'onModelSelect', 'lock': 'onLock', 'unlock':
'onUnlock' } );
$documentNode = this.documentView.getDocumentNode().$;
$documentNode.on( {
@@ -323,7 +324,7 @@
// Calling focus sets the cursor to zero offset, so we need to restore
scrollTop
$window.scrollTop( scrollTop );
this.focusedNode = null;
- this.onChange( null, this.surface.getModel().selection );
+ this.onModelSelect( this.surface.getModel().selection );
};
/*! Native Browser Events */
@@ -498,7 +499,8 @@
* @fires selectionStart
*/
ve.ce.Surface.prototype.onDocumentKeyDown = function ( e ) {
- var trigger;
+ var trigger,
+ updateFromModel = false;
// Ignore keydowns while in IME mode but do not preventDefault them (so
text actually appear on
// the screen).
@@ -537,32 +539,41 @@
this.handleLeftOrRightArrowKey( e );
} else {
this.handleUpOrDownArrowKey( e );
+ updateFromModel = true;
}
break;
case ve.Keys.ENTER:
e.preventDefault();
this.handleEnter( e );
+ updateFromModel = true;
break;
case ve.Keys.BACKSPACE:
e.preventDefault();
this.handleDelete( e, true );
+ updateFromModel = true;
break;
case ve.Keys.DELETE:
e.preventDefault();
this.handleDelete( e, false );
+ updateFromModel = true;
break;
default:
trigger = new ve.ui.Trigger( e );
if ( trigger.isComplete() && this.surface.execute(
trigger ) ) {
e.preventDefault();
+ updateFromModel = true;
}
break;
}
- this.incRenderLock();
+ if ( !updateFromModel ) {
+ this.incRenderLock();
+ }
try {
this.surfaceObserver.pollOnce();
} finally {
- this.decRenderLock();
+ if ( !updateFromModel ) {
+ this.decRenderLock();
+ }
}
this.surfaceObserver.startTimerLoop();
};
@@ -577,7 +588,8 @@
var selection, prevNode, documentModel = this.model.getDocument();
// Prevent IE from editing Aliens/Entities
- // TODO: Better comment about what's going on here is needed.
+ // This is for cases like <p><div>alien</div></p>, to put the cursor
outside
+ // the alien tag.
if ( $.browser.msie === true ) {
selection = this.model.getSelection();
if ( selection.start !== 0 && selection.isCollapsed() ) {
@@ -913,65 +925,62 @@
/*! Custom Events */
/**
- * Handle change events.
+ * Handle model select events.
*
* @see ve.dm.Surface#method-change
*
* @method
- * @param {ve.dm.Transaction|null} transaction
- * @param {ve.Range|undefined} selection
+ * @param {ve.Range} selection
*/
-ve.ce.Surface.prototype.onChange = function ( transaction, selection ) {
+ve.ce.Surface.prototype.onModelSelect = function ( selection ) {
var start, end, rangySel, rangyRange,
next = null,
previous = this.focusedNode;
- if ( selection ) {
- // Detect when only a single inline element is selected
- if ( !selection.isCollapsed() ) {
- start =
this.documentView.getDocumentNode().getNodeFromOffset( selection.start + 1 );
- if ( start.isFocusable() ) {
- end =
this.documentView.getDocumentNode().getNodeFromOffset( selection.end - 1 );
- if ( start === end ) {
- next = start;
- }
- }
- } else {
- // Check we haven't been programmatically placed inside
a focusable node with a collapsed selection
- start =
this.documentView.getDocumentNode().getNodeFromOffset( selection.start );
- if ( start.isFocusable() ) {
+ // Detect when only a single inline element is selected
+ if ( !selection.isCollapsed() ) {
+ start = this.documentView.getDocumentNode().getNodeFromOffset(
selection.start + 1 );
+ if ( start.isFocusable() ) {
+ end =
this.documentView.getDocumentNode().getNodeFromOffset( selection.end - 1 );
+ if ( start === end ) {
next = start;
}
}
- // Update nodes if something changed
- if ( previous !== next ) {
- if ( previous ) {
- previous.setFocused( false );
- this.focusedNode = null;
- }
- if ( next ) {
- next.setFocused( true );
- this.focusedNode = start;
- // As FF won't fire a copy event with nothing
selected, make
- // a dummy selection of one space in the
pasteTarget.
- // onCopy will ignore this native selection and
use the DM selection
- this.$pasteTarget.text( ' ' );
- rangySel = rangy.getSelection(
this.getElementDocument() );
- rangyRange = rangy.createRange(
this.getElementDocument() );
- rangyRange.setStart( this.$pasteTarget[0], 0 );
- rangyRange.setEnd( this.$pasteTarget[0], 1 );
- rangySel.removeAllRanges();
- this.$pasteTarget[0].focus();
- rangySel.addRange( rangyRange, false );
- }
+ } else {
+ // Check we haven't been programmatically placed inside a
focusable node with a collapsed selection
+ start = this.documentView.getDocumentNode().getNodeFromOffset(
selection.start );
+ if ( start.isFocusable() ) {
+ next = start;
}
+ }
+ // Update nodes if something changed
+ if ( previous !== next ) {
+ if ( previous ) {
+ previous.setFocused( false );
+ this.focusedNode = null;
+ }
+ if ( next ) {
+ next.setFocused( true );
+ this.focusedNode = start;
+ // As FF won't fire a copy event with nothing selected,
make
+ // a dummy selection of one space in the pasteTarget.
+ // onCopy will ignore this native selection and use the
DM selection
+ this.$pasteTarget.text( ' ' );
+ rangySel = rangy.getSelection(
this.getElementDocument() );
+ rangyRange = rangy.createRange(
this.getElementDocument() );
+ rangyRange.setStart( this.$pasteTarget[0], 0 );
+ rangyRange.setEnd( this.$pasteTarget[0], 1 );
+ rangySel.removeAllRanges();
+ this.$pasteTarget[0].focus();
+ rangySel.addRange( rangyRange, false );
+ }
+ }
- // If there is no focused node, use native selection, but
ignore the selection if
- // changeModelSelection is currently being called with the same
(object-identical)
- // selection object (i.e. if the model is calling us back)
- if ( !this.focusedNode && !this.isRenderingLocked() &&
selection !== this.newModelSelection ) {
- this.showSelection( selection );
- }
+ // If there is no focused node, use native selection, but ignore the
selection if
+ // changeModelSelection is currently being called with the same
(object-identical)
+ // selection object (i.e. if the model is calling us back)
+ if ( !this.focusedNode && !this.isRenderingLocked() && selection !==
this.newModelSelection ) {
+ this.showSelection( selection );
}
};
@@ -991,7 +1000,7 @@
}
this.incRenderLock();
try {
- this.changeModelSelection( newRange );
+ this.changeModel( null, newRange );
} finally {
this.decRenderLock();
}
@@ -1060,7 +1069,7 @@
}
this.incRenderLock();
try {
- this.model.change(
+ this.changeModel(
ve.dm.Transaction.newFromInsertion(
this.documentView.model,
previous.range.start, data
),
@@ -1081,7 +1090,7 @@
}
this.incRenderLock();
try {
- this.model.change(
+ this.changeModel(
ve.dm.Transaction.newFromRemoval(
this.documentView.model,
range ),
next.range
@@ -1145,7 +1154,7 @@
}
if ( data.length > 0 ) {
- this.model.change(
+ this.changeModel(
ve.dm.Transaction.newFromInsertion(
this.documentView.model, nodeOffset + 1
+ fromLeft,
data
@@ -1154,7 +1163,7 @@
);
}
if ( fromLeft + fromRight < previousData.length ) {
- this.model.change(
+ this.changeModel(
ve.dm.Transaction.newFromRemoval(
this.documentView.model,
new ve.Range(
@@ -1399,6 +1408,7 @@
if ( selection.from !== selection.to ) {
tx = ve.dm.Transaction.newFromRemoval( documentModel, selection
);
selection = tx.translateRange( selection );
+ // We do want this to propagate to the surface
this.model.change( tx, selection );
}
@@ -1883,21 +1893,23 @@
};
/**
- * Change selection in the model only, not the CE surface
+ * Change the model only, not the CE surface
*
* This avoids event storms when the CE surface is already correct
*
* @method
- * @param {ve.Range} range New selection for model
- * @throws {Error} If calls to the method are nested
+ * @param {ve.dm.Transaction|ve.dm.Transaction[]|null} transactions One or
more transactions to
+ * process, or null to process none
+ * @param {ve.Range} new selection
+ * @throws {Error} If calls to this method are nested
*/
-ve.ce.Surface.prototype.changeModelSelection = function ( range ) {
+ve.ce.Surface.prototype.changeModel = function ( transaction, range ) {
if ( this.newModelSelection !== null ) {
- throw new Error( 'Nested changeModelSelection' );
+ throw new Error( 'Nested change of newModelSelection' );
}
this.newModelSelection = range;
try {
- this.model.change( null, range );
+ this.model.change( transaction, range );
} finally {
this.newModelSelection = null;
}
diff --git a/modules/ve/dm/ve.dm.Surface.js b/modules/ve/dm/ve.dm.Surface.js
index a10ea0b..9ad1095 100644
--- a/modules/ve/dm/ve.dm.Surface.js
+++ b/modules/ve/dm/ve.dm.Surface.js
@@ -60,13 +60,6 @@
*/
/**
- * @event change
- * @see #method-change
- * @param {ve.dm.Transaction|null} transaction
- * @param {ve.Range|undefined} selection
- */
-
-/**
* @event history
*/
@@ -306,7 +299,6 @@
* @fires select
* @fires transact
* @fires contextChange
- * @fires change
* @fires unlock
*/
ve.dm.Surface.prototype.change = function ( transactions, selection ) {
@@ -413,8 +405,6 @@
if ( contextChange ) {
this.emit( 'contextChange' );
}
-
- this.emit( 'change', transactions, selection );
// Continue observation polling, we want to know about things that
change from here on out
this.emit( 'unlock' );
diff --git a/modules/ve/test/dm/ve.dm.Surface.test.js
b/modules/ve/test/dm/ve.dm.Surface.test.js
index 5284f0e..bd99959 100644
--- a/modules/ve/test/dm/ve.dm.Surface.test.js
+++ b/modules/ve/test/dm/ve.dm.Surface.test.js
@@ -37,8 +37,7 @@
tx = new ve.dm.Transaction(),
events = {
'transact': 0,
- 'select': 0,
- 'change': 0
+ 'select': 0
};
surface.on( 'transact', function () {
@@ -47,15 +46,12 @@
surface.on( 'select', function () {
events.select++;
} );
- surface.on( 'change', function () {
- events.change++;
- } );
surface.change( tx );
- assert.deepEqual( events, { 'transact': 1, 'select': 0, 'change': 1 },
'transaction without selection' );
+ assert.deepEqual( events, { 'transact': 1, 'select': 0 }, 'transaction
without selection' );
surface.change( null, new ve.Range( 2, 2 ) );
- assert.deepEqual( events, { 'transact': 1, 'select': 1, 'change': 2 },
'selection without transaction' );
+ assert.deepEqual( events, { 'transact': 1, 'select': 1 }, 'selection
without transaction' );
surface.change( tx, new ve.Range( 3, 3 ) );
- assert.deepEqual( events, { 'transact': 2, 'select': 2, 'change': 3 },
'transaction and selection' );
+ assert.deepEqual( events, { 'transact': 2, 'select': 2 }, 'transaction
and selection' );
} );
QUnit.test( 'breakpoint', 7, function ( assert ) {
--
To view, visit https://gerrit.wikimedia.org/r/86802
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I8f16289493e835d890709c6dfe093d04c18522b6
Gerrit-PatchSet: 25
Gerrit-Project: mediawiki/extensions/VisualEditor
Gerrit-Branch: master
Gerrit-Owner: Divec <[email protected]>
Gerrit-Reviewer: Catrope <[email protected]>
Gerrit-Reviewer: Divec <[email protected]>
Gerrit-Reviewer: Krinkle <[email protected]>
Gerrit-Reviewer: Trevor Parscal <[email protected]>
Gerrit-Reviewer: jenkins-bot
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
