jenkins-bot has submitted this change and it was merged.
Change subject: Don't unmerge spanned cells when spanned cell is removed
......................................................................
Don't unmerge spanned cells when spanned cell is removed
Previously we inserted regular cells in place of every placeholder in
the matrix, which meant that deleting a row which happened to contain
a rowspanned cell would split the rest of the cell apart, and the
contents would be lost. (Same for columns.)
Now we only replace one of the placeholders with a new spanned cell,
appropriately smaller. The new cell inherits all of the properties
and content of the removed cell, with the span adjusted.
Bonus: When unmerging a header cell, all newly split cells also get
header style. Previously first row would get header style and the rest
would get data style, which was an accidental effect of how
ve.dm.TableMatrix#findClosestCell works.
Bug: 73213
Change-Id: I62e56657c18bc67be859ece882770ce008bc3838
---
M src/dm/nodes/ve.dm.TableCellNode.js
M src/ui/actions/ve.ui.TableAction.js
2 files changed, 39 insertions(+), 26 deletions(-)
Approvals:
Esanders: Looks good to me, approved
jenkins-bot: Verified
diff --git a/src/dm/nodes/ve.dm.TableCellNode.js
b/src/dm/nodes/ve.dm.TableCellNode.js
index c9017dc..662cfe8 100644
--- a/src/dm/nodes/ve.dm.TableCellNode.js
+++ b/src/dm/nodes/ve.dm.TableCellNode.js
@@ -88,22 +88,29 @@
/**
* Creates data that can be inserted into the model to create a new table cell.
*
- * @param {Object} [options] An object with property 'style' which can be
either 'header' or 'data'.
+ * @param {Object} [options]
+ * @param {string} [options.style='data'] Either 'header' or 'data'
+ * @param {number} [options.rowspan=1] Number of rows the cell spans
+ * @param {number} [options.colspan=1] Number of columns the cell spans
+ * @param {Array} [options.content] Linear model data, defaults to empty
wrapper paragraph
* @return {Array} Model data for a new table cell
*/
ve.dm.TableCellNode.static.createData = function ( options ) {
+ var opening, content;
options = options || {};
- return [
- {
- type: 'tableCell',
- attributes: {
- style: options.style || 'data'
- }
- },
+ opening = {
+ type: 'tableCell',
+ attributes: {
+ style: options.style || 'data',
+ rowspan: options.rowspan || 1,
+ colspan: options.colspan || 1
+ }
+ };
+ content = options.content || [
{ type: 'paragraph', internal: { generated: 'wrapper' } },
- { type: '/paragraph' },
- { type: '/tableCell' }
+ { type: '/paragraph' }
];
+ return [ opening ].concat( content ).concat( [ { type: '/tableCell' } ]
);
};
/* Methods */
diff --git a/src/ui/actions/ve.ui.TableAction.js
b/src/ui/actions/ve.ui.TableAction.js
index 56574f0..8e47a2d 100644
--- a/src/ui/actions/ve.ui.TableAction.js
+++ b/src/ui/actions/ve.ui.TableAction.js
@@ -199,7 +199,11 @@
);
for ( i = cells.length - 1; i >= 1; i-- ) {
txs.push(
- this.replacePlaceholder(
selection.getTableNode().getMatrix(), cells[i] )
+ this.replacePlaceholder(
+ selection.getTableNode().getMatrix(),
+ cells[i],
+ { style: cells[0].node.getStyle() }
+ )
);
}
} else {
@@ -452,10 +456,10 @@
surfaceModel = this.surface.getModel();
// Deleting cells can have two additional consequences:
- // 1. The cell is a Placeholder. The owner's span might be decreased.
+ // 1. The cell is a Placeholder. The owner's span must be decreased.
// 2. The cell is owner of placeholders which get orphaned by the
deletion.
- // New, empty cells must be inserted to replace the placeholders and
keep the
- // table in proper shape.
+ // The first of the placeholders now becomes the real cell, with the
span adjusted.
+ // It also inherits all of the properties and content of the removed
cell.
// Insertions and deletions of cells must be done in an appropriate
order, so that the transactions
// do not interfere with each other. To achieve that, we record
insertions and deletions and
// sort them by the position of the cell (row, column) in the table
matrix.
@@ -502,11 +506,14 @@
endCol = cell.col + cell.node.getColspan() - 1;
// Record the insertion to apply it later
- for ( row = startRow; row <= endRow; row++ ) {
- for ( col = startCol; col <= endCol; col++ ) {
- actions.push( { action: 'insert', cell:
matrix.getCell( row, col ) } );
- }
- }
+ actions.push( {
+ action: 'insert',
+ cell: matrix.getCell( startRow, startCol ),
+ colspan: 1 + endCol - startCol,
+ rowspan: 1 + endRow - startRow,
+ style: cell.node.getStyle(),
+ content: surfaceModel.getDocument().getData(
cell.node.getRange() )
+ } );
}
// Cell nodes only get deleted when deleting columns (otherwise
row nodes)
@@ -526,7 +533,7 @@
// First replace orphaned placeholders which are below the last
deleted row,
// thus, this works with regard to transaction offsets
for ( i = 0; i < actions.length; i++ ) {
- txs.push( this.replacePlaceholder( matrix,
actions[i].cell ) );
+ txs.push( this.replacePlaceholder( matrix,
actions[i].cell, actions[i] ) );
}
// Remove rows in reverse order to have valid transaction
offsets
for ( row = maxIndex; row >= minIndex; row-- ) {
@@ -536,7 +543,7 @@
} else {
for ( i = 0; i < actions.length; i++ ) {
if ( actions[i].action === 'insert' ) {
- txs.push( this.replacePlaceholder( matrix,
actions[i].cell ) );
+ txs.push( this.replacePlaceholder( matrix,
actions[i].cell, actions[i] ) );
} else {
txs.push( ve.dm.Transaction.newFromRemoval(
surfaceModel.getDocument(), actions[i].cell.node.getOuterRange() ) );
}
@@ -550,10 +557,11 @@
*
* @param {ve.dm.TableMatrix} matrix Table matrix
* @param {ve.dm.TableMatrixCell} placeholder Placeholder cell to replace
+ * @param {Object} [options] Options to pass to
ve.dm.TableCellNode.static.createData
* @return {ve.dm.Transaction} Transaction
*/
-ve.ui.TableAction.prototype.replacePlaceholder = function ( matrix,
placeholder ) {
- var range, offset, data, style,
+ve.ui.TableAction.prototype.replacePlaceholder = function ( matrix,
placeholder, options ) {
+ var range, offset, data,
// For inserting the new cell a reference cell node
// which is used to get an insertion offset.
refCell = matrix.findClosestCell( placeholder ),
@@ -562,14 +570,12 @@
if ( refCell ) {
range = refCell.node.getOuterRange();
offset = ( placeholder.col < refCell.col ) ? range.start :
range.end;
- style = refCell.node.getStyle();
} else {
// if there are only placeholders in the row, the row node's
inner range is used
range = matrix.getRowNode( placeholder.row ).getRange();
offset = range.start;
- style = placeholder.node.getStyle();
}
- data = ve.dm.TableCellNode.static.createData( { style: style } );
+ data = ve.dm.TableCellNode.static.createData( options );
return ve.dm.Transaction.newFromInsertion( surfaceModel.getDocument(),
offset, data );
};
--
To view, visit https://gerrit.wikimedia.org/r/172193
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I62e56657c18bc67be859ece882770ce008bc3838
Gerrit-PatchSet: 6
Gerrit-Project: VisualEditor/VisualEditor
Gerrit-Branch: master
Gerrit-Owner: Bartosz Dziewoński <matma....@gmail.com>
Gerrit-Reviewer: Bartosz Dziewoński <matma....@gmail.com>
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
