jenkins-bot has submitted this change and it was merged. Change subject: Move insertion annotation calc logic from DM Surface to ElementLinearData ......................................................................
Move insertion annotation calc logic from DM Surface to ElementLinearData Previously, the logic to calculate insertion annotations was buried inside ve.dm.Surface#setSelection . Moves the logic into its own pure function, ve.dm.ElementLinearData#getInsertionAnnotationsFromRange , and apply unit tests. The actual implementation will be refactored in a subsequent commit. Change-Id: I4397f988c430a8cece59861b468160d87f0e8187 --- M src/dm/lineardata/ve.dm.ElementLinearData.js M src/dm/ve.dm.Surface.js M tests/dm/lineardata/ve.dm.ElementLinearData.test.js 3 files changed, 113 insertions(+), 49 deletions(-) Approvals: Esanders: Looks good to me, approved jenkins-bot: Verified diff --git a/src/dm/lineardata/ve.dm.ElementLinearData.js b/src/dm/lineardata/ve.dm.ElementLinearData.js index fc10960..f11f492 100644 --- a/src/dm/lineardata/ve.dm.ElementLinearData.js +++ b/src/dm/lineardata/ve.dm.ElementLinearData.js @@ -531,6 +531,56 @@ }; /** + * Get the insertion annotations that should apply to a range. + * + * The semantics are intended to match Chromium's behaviour. + * TODO: This cannot match Firefox behaviour, which depends on the cursor's annotation + * boundary side, and performs a union of the annotations at each end of the selection; + * see https://phabricator.wikimedia.org/T113869 . + * + * @param {ve.Range} range The range into which text would be inserted + * @return {ve.dm.AnnotationSet} The insertion annotations that should apply + */ +ve.dm.ElementLinearData.prototype.getInsertionAnnotationsFromRange = function ( range ) { + var left, right, leftAnnotations, rightAnnotations; + + if ( range.isCollapsed() ) { + // Get annotations from either side of the cursor + left = Math.max( 0, range.start - 1 ); + if ( !this.isContentOffset( left ) ) { + left = -1; + } + right = Math.max( 0, range.start ); + if ( !this.isContentOffset( right ) ) { + right = -1; + } + } else { + // Get annotations from the first character of the range + // TODO: The use of getNearestContentOffset no longer makes much sense here + left = this.getNearestContentOffset( range.start ); + right = this.getNearestContentOffset( range.end ); + } + if ( left === -1 ) { + // No content offset to our left, use empty set + return new ve.dm.AnnotationSet( this.getStore() ); + } + + // Include each annotation on the left that either continues on the right + // or should get added to appended content + leftAnnotations = this.getAnnotationsFromOffset( left ); + if ( right === -1 ) { + // TODO: "return leftAnnotations" would match prior behaviour. Was that a mistake? + rightAnnotations = new ve.dm.AnnotationSet( this.getStore() ); + } else { + rightAnnotations = this.getAnnotationsFromOffset( right ); + } + return leftAnnotations.filter( function ( annotation ) { + return annotation.constructor.static.applyToAppendedContent || + rightAnnotations.containsComparable( annotation ); + } ); +}; + +/** * Check if the range has any annotations * * @method diff --git a/src/dm/ve.dm.Surface.js b/src/dm/ve.dm.Surface.js index 4b3d0e7..c8cb5bb 100644 --- a/src/dm/ve.dm.Surface.js +++ b/src/dm/ve.dm.Surface.js @@ -625,8 +625,7 @@ * @fires contextChange */ ve.dm.Surface.prototype.setSelection = function ( selection ) { - var left, right, leftAnnotations, rightAnnotations, insertionAnnotations, - startNode, selectedNode, range, selectedAnnotations, isCollapsed, + var insertionAnnotations, startNode, selectedNode, range, selectedAnnotations, oldSelection = this.selection, branchNodes = {}, selectionChange = false, @@ -668,48 +667,24 @@ } } - // Figure out which annotations to use for insertions - if ( range.isCollapsed() ) { - // Get annotations from either side of the cursor - left = Math.max( 0, range.start - 1 ); - if ( !linearData.isContentOffset( left ) ) { - left = -1; - } - right = Math.max( 0, range.start ); - if ( !linearData.isContentOffset( right ) ) { - right = -1; - } - selectedAnnotations = linearData.getAnnotationsFromOffset( range.start ); - isCollapsed = true; - } else { - // Get annotations from the first character of the range - left = linearData.getNearestContentOffset( range.start ); - right = linearData.getNearestContentOffset( range.end ); - selectedAnnotations = linearData.getAnnotationsFromRange( range, true ); - isCollapsed = false; - } - if ( left === -1 ) { - // No content offset to our left, use empty set - insertionAnnotations = new ve.dm.AnnotationSet( this.getDocument().getStore() ); - } else { - // Include annotations on the left that should be added to appended content, or ones that - // are on both the left and the right that should not - leftAnnotations = linearData.getAnnotationsFromOffset( left ); - if ( right !== -1 ) { - rightAnnotations = linearData.getAnnotationsFromOffset( right ); - insertionAnnotations = leftAnnotations.filter( function ( annotation ) { - return annotation.constructor.static.applyToAppendedContent || - rightAnnotations.containsComparable( annotation ); - } ); - } else { - insertionAnnotations = leftAnnotations; - } - } - - // Only emit an annotations change event if there's a difference - // Note that ANY difference matters here, even order + // Reset insertionAnnotations based on the neighbouring document data + insertionAnnotations = linearData.getInsertionAnnotationsFromRange( range ); + // If there's *any* difference in insertion annotations (even order), then: + // * emit insertionAnnotationsChange + // * emit contextChange (TODO: is this desirable?) if ( !insertionAnnotations.equalsInOrder( this.insertionAnnotations ) ) { this.setInsertionAnnotations( insertionAnnotations ); + } + + // Reset selectedAnnotations + if ( range.isCollapsed() ) { + selectedAnnotations = linearData.getAnnotationsFromOffset( range.start ); + } else { + selectedAnnotations = linearData.getAnnotationsFromRange( range, true ); + } + if ( !selectedAnnotations.compareTo( this.selectedAnnotations ) ) { + this.selectedAnnotations = selectedAnnotations; + contextChange = true; } } else if ( selection instanceof ve.dm.TableSelection ) { if ( selection.isSingleCell() ) { @@ -720,16 +695,11 @@ contextChange = true; } - if ( selectedAnnotations && !selectedAnnotations.compareTo( this.selectedAnnotations ) ) { - this.selectedAnnotations = selectedAnnotations; - contextChange = true; - } - - if ( isCollapsed !== this.isCollapsed ) { + if ( range && range.isCollapsed() !== this.isCollapsed ) { // selectedAnnotations won't have changed if going from insertion annotations to // selection of the same annotations, but some tools will consider that a context change // (e.g. ClearAnnotationTool). - this.isCollapsed = isCollapsed; + this.isCollapsed = range.isCollapsed(); contextChange = true; } diff --git a/tests/dm/lineardata/ve.dm.ElementLinearData.test.js b/tests/dm/lineardata/ve.dm.ElementLinearData.test.js index bad1033..e2d8d52 100644 --- a/tests/dm/lineardata/ve.dm.ElementLinearData.test.js +++ b/tests/dm/lineardata/ve.dm.ElementLinearData.test.js @@ -381,6 +381,50 @@ } } ); +QUnit.test( 'getInsertionAnnotationsFromRange', function ( assert ) { + var html, linearData, tests, + u = ve.dm.example.underline; + + // <h1>:0 a:1 b:2 c:3 d:4 e:5 f:6 </h1>:7 <p>:8 g:9 </p>:10 <div>:11 </div>:12 + html = '<h1>ab<u>cd</u>ef</h1><p><u>g</u></p><div></div>'; + tests = [ + { range: [ 1, 1 ], expected: [], msg: 'plain start at block start' }, + { range: [ 2, 2 ], expected: [], msg: 'plain interior' }, + { range: [ 3, 3 ], expected: [], msg: 'plain end before u' }, + { range: [ 4, 4 ], expected: [ u ], msg: 'u start' }, + { range: [ 5, 5 ], expected: [ u ], msg: 'u interior' }, + { range: [ 6, 6 ], expected: [], msg: 'plain start after u' }, + { range: [ 7, 7 ], expected: [], msg: 'plain end at block end' }, + { range: [ 9, 9 ], expected: [], msg: 'block start before u' }, + { range: [ 10, 10 ], expected: [ u ], msg: 'u end before block end' }, + { range: [ 12, 12 ], expected: [], msg: 'empty block' }, + { range: [ 2, 3 ], expected: [], msg: 'forward to u start' }, + { range: [ 3, 2 ], expected: [], msg: 'backward to u start' }, + { range: [ 2, 4 ], expected: [], msg: 'forward past u start' }, + { range: [ 4, 2 ], expected: [], msg: 'backward past u start' }, + { range: [ 3, 4 ], expected: [ u ], msg: 'forward to u end' }, + { range: [ 4, 3 ], expected: [ u ], msg: 'backward to u end' }, + { range: [ 3, 5 ], expected: [ u ], msg: 'forward past u end' }, + { range: [ 5, 3 ], expected: [ u ], msg: 'backward past u end' } + ]; + + QUnit.expect( tests.length ); + linearData = ve.dm.converter.getModelFromDom( + ve.createDocumentFromHtml( html ) + ).data; + tests.forEach( function ( test ) { + var observed = linearData.getInsertionAnnotationsFromRange( + new ve.Range( test.range[ 0 ], test.range[ 1 ] ) + ).get().map( function ( annotation ) { + return { + type: annotation.element.type, + attributes: annotation.element.attributes + }; + } ); + assert.deepEqual( observed, test.expected, test.msg ); + } ); +} ); + QUnit.test( 'getAnnotatedRangeFromOffset', 1, function ( assert ) { var i, data, doc, cases = [ -- To view, visit https://gerrit.wikimedia.org/r/241555 To unsubscribe, visit https://gerrit.wikimedia.org/r/settings Gerrit-MessageType: merged Gerrit-Change-Id: I4397f988c430a8cece59861b468160d87f0e8187 Gerrit-PatchSet: 3 Gerrit-Project: VisualEditor/VisualEditor Gerrit-Branch: master Gerrit-Owner: Divec <da...@troi.org> Gerrit-Reviewer: Catrope <roan.katt...@gmail.com> Gerrit-Reviewer: Divec <da...@troi.org> Gerrit-Reviewer: Esanders <esand...@wikimedia.org> Gerrit-Reviewer: jenkins-bot <> _______________________________________________ MediaWiki-commits mailing list MediaWiki-commits@lists.wikimedia.org https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
