Skip to content

Commit

Permalink
Merge "[INTERNAL][FIX] CellSelector: Range limit should not influence…
Browse files Browse the repository at this point in the history 
… selection" into rel-1.120
  • Loading branch information
Marcos Gavilan de Paz authored and Gerrit Code Review committed Jul 1, 2024
2 parents 4e4d4c6 + 2664b6b commit 2121aa4
Show file tree
Hide file tree
Showing 3 changed files with 71 additions and 58 deletions.
52 changes: 26 additions & 26 deletions src/sap.m/src/sap/m/plugins/CellSelector.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 +155,10 @@ sap.ui.define([
}

oEvent.preventDefault();
} else if (this._bSelecting && isKeyCombination(oEvent, KeyCodes.SPACE, false, true)) {
}
/*
Deactivate as feature did not work anyways and will be handled in separate BLI
else if (this._bSelecting && isKeyCombination(oEvent, KeyCodes.SPACE, false, true)) {
if (!this._inSelection(oEvent.target)) {
// If focus is on cell outside of selection, select focused column
var oInfo = this.getConfig("getCellInfo", this.getControl(), oEvent.target);
Expand All @@ -167,6 +170,7 @@ sap.ui.define([
oEvent.preventDefault();
}
*/
},
onmousedown: function(oEvent) {
if (oEvent.isMarked?.() || oEvent.button != 0) {
Expand Down Expand Up @@ -203,6 +207,10 @@ sap.ui.define([
this._scrollSelect(this._oSession.scrollForward, this._oSession.isVertical, oEvent);
} else {
this._selectCells();
if (!this._oSession.mSource || !this._oSession.mTarget) {
return;
}
this._drawSelection(this._oSession.mSource, this._oSession.mTarget);
}
}.bind(this);
this._fnOnMouseEnter = this._onmouseenter.bind(this);
Expand Down Expand Up @@ -301,7 +309,7 @@ sap.ui.define([
return null;
}

var mSelectionRange = this._getNormalizedBounds(this._oSession.mSource, this._oSession.mTarget, true);
var mSelectionRange = this._getNormalizedBounds(this._oSession.mSource, this._oSession.mTarget);
if (isNaN(mSelectionRange.from.rowIndex) || isNaN(mSelectionRange.to.rowIndex)) {
return null;
}
Expand Down Expand Up @@ -600,26 +608,21 @@ sap.ui.define([

mFrom = mFrom ? mFrom : this._oSession.mSource;
mTo = mTo ? mTo : this._oSession.mTarget;
var mBounds = this._getNormalizedBounds(mFrom, mTo);

if (mTo.rowIndex == Infinity || mFrom.rowIndex == Infinity) {
this.getConfig("loadContexts", this.getControl(), mBounds.from.rowIndex, this.getRangeLimit());
this.getConfig("loadContexts", this.getControl(), Math.max(Math.min(mFrom, mTo), 0), this.getRangeLimit());
}

this._drawSelection(mBounds);
this._drawSelection(mFrom, mTo);

this._oSession.mSource = mFrom;
this._oSession.mTarget = mTo;
};

/**
* Draws the selection for the given bounds.
* @param {Object} mBounds object containing the bounds information (from, to)
* @param {Object} mBounds.from from position
* @param {Object} mBounds.to to position
* @private
*/
CellSelector.prototype._drawSelection = function (mBounds) {
CellSelector.prototype._drawSelection = function (mFrom, mTo) {
const bAdjustBounds = !isFinite(mFrom.rowIndex) || !isFinite(mTo.rowIndex);
const mBounds = this._getNormalizedBounds(mFrom, mTo, bAdjustBounds);

if (!mBounds.from || !mBounds.to) {
return;
}
Expand Down Expand Up @@ -765,23 +768,20 @@ sap.ui.define([

/**
* Returns an object containing normalized coordinates for the given bounding area.
* <code>from</code> will contain the coordinates for the upper left corner of the bounding area,
* while <code>to</code> contains the coordinates of the lower right corner of the bounding area.
* @param {Object} mFrom
* @param {int} mFrom.rowIndex row index
* @param {int} mFrom.colIndex column index
* @param {Object} mTo
* @param {int} mTo.rowIndex row index
* @param {int} mTo.colIndex column index
* @returns object containing coordinates for from and to
* <code>from</code> contains the coordinates for the upper left corner of the bounding area,
* <code>to</code> contains the coordinates of the lower right corner of the bounding area.
* @param {Object} mFrom Source cell coordinates
* @param {Object} mTo Target cell coordinates
* @param {boolean} bAdjustBounds bounds are adjusted to fit into limit/table boundaries (e.g. range selection)
* @returns {object} Object containing coordinates for the bounding area
*/
CellSelector.prototype._getNormalizedBounds = function(mFrom, mTo, bKeepBounds) {
const iMaxColumns = this.getConfig("getVisibleColumns", this.getControl()).length;
CellSelector.prototype._getNormalizedBounds = function(mFrom, mTo, bAdjustBounds) {
const iMaxColumns = this.getConfig("numberOfColumns", this.getControl());
const iMaxRows = this.getRangeLimit() == 0 ? this.getConfig("getRowCount", this.getControl()) : this.getRangeLimit();

let toRowIndex = Math.max(mFrom.rowIndex, mTo.rowIndex), toColIndex = Math.max(mFrom.colIndex, mTo.colIndex);
if (!bKeepBounds) {
toRowIndex = Math.min(iMaxRows - 1, toRowIndex);
if (bAdjustBounds) {
toRowIndex = Math.min(iMaxRows, toRowIndex);
toColIndex = Math.min(iMaxColumns, toColIndex);
}

Expand Down
40 changes: 28 additions & 12 deletions src/sap.m/test/sap/m/qunit/plugins/CellSelector.qunit.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -71,26 +71,42 @@ sap.ui.define([

this.oTable.attachEventOnce("rowsUpdated", () => {
var oBinding = this.oTable.getBinding("rows");
var oGetContextsSpy = sinon.spy(oBinding, "getContexts");
// var oGetContextsSpy = sinon.spy(oBinding, "getContexts");
assert.ok(oBinding.getLength() > this.oCellSelector.getRangeLimit());

var oCell = getCell(this.oTable, 1, 0); // first cell of first row
qutils.triggerKeydown(oCell, KeyCodes.SPACE); // select first cell of first row
assert.equal(oBinding.getAllCurrentContexts().length, this.oTable.getThreshold() + this.oTable.getRowMode().getRowCount());

qutils.triggerKeyup(oCell, KeyCodes.SPACE, false, false, true /* Ctrl */); // enlarge selection to all rows and cells
assert.equal(oGetContextsSpy.callCount, 1);
assert.ok(oGetContextsSpy.calledWithExactly(0, this.oCellSelector.getRangeLimit(), 0, true));
assert.deepEqual(this.oCellSelector.getSelectionRange(), {from: {rowIndex: 0, colIndex: 0}, to: {rowIndex: Infinity, colIndex: 0}});
// qutils.triggerKeyup(oCell, KeyCodes.SPACE, false, false, true /* Ctrl */); // enlarge selection to all rows and cells
// assert.equal(oGetContextsSpy.callCount, 1);
// assert.ok(oGetContextsSpy.calledWithExactly(0, this.oCellSelector.getRangeLimit(), 0, true));
// assert.deepEqual(this.oCellSelector.getSelectionRange(), {from: {rowIndex: 0, colIndex: 0}, to: {rowIndex: Infinity, colIndex: 0}});

oBinding.attachEventOnce("dataReceived", setTimeout.bind(0, () => {
assert.equal(oBinding.getAllCurrentContexts().length, this.oCellSelector.getRangeLimit());
assert.equal(this.oCellSelector.getSelectedRowContexts().length, this.oCellSelector.getRangeLimit());
assert.deepEqual(this.oCellSelector.getSelectedRowContexts(), oBinding.getAllCurrentContexts().slice(0, this.oCellSelector.getRangeLimit()));
qutils.triggerKeydown(oCell, KeyCodes.ARROW_RIGHT, true, false, false);
qutils.triggerKeyup(oCell, KeyCodes.ARROW_RIGHT, true, false, false);

oGetContextsSpy.restore();
done();
}));
oCell = getCell(this.oTable, 1, 1);
qutils.triggerKeydown(oCell, KeyCodes.ARROW_DOWN, true, false, false);
qutils.triggerKeyup(oCell, KeyCodes.ARROW_DOWN, true, false, false);

// Commenting as Column Selection feature will be changed/adjusted in a separate BLI
// qutils.triggerKeyup(oCell, KeyCodes.SPACE, false, false, true /* Ctrl */); // enlarge selection to all rows and cells
// assert.equal(oGetContextsSpy.callCount, 1);
// assert.ok(oGetContextsSpy.calledWithExactly(0, this.oCellSelector.getRangeLimit(), 0, true));

assert.deepEqual(this.oCellSelector.getSelectionRange(), {from: {rowIndex: 1, colIndex: 0}, to: {rowIndex: 2, colIndex: 1}});
assert.deepEqual(this.oCellSelector.getSelectedRowContexts(), oBinding.getAllCurrentContexts().slice(1, 3));

// Commenting as Column Selection feature will be changed/adjusted in a separate BLI
// oBinding.attachEventOnce("dataReceived", () => {
// assert.equal(oBinding.getAllCurrentContexts().length, this.oCellSelector.getRangeLimit());
// assert.equal(this.oCellSelector.getSelectedRowContexts().length, this.oCellSelector.getRangeLimit());
// assert.deepEqual(this.oCellSelector.getSelectedRowContexts(), oBinding.getAllCurrentContexts().slice(0, this.oCellSelector.getRangeLimit()));
// assert.deepEqual(this.oCellSelector.getSelectedRowContexts(), oBinding.getAllCurrentContexts().slice(1, 2));

// oGetContextsSpy.restore();
done();
});
});

Expand Down
37 changes: 17 additions & 20 deletions src/sap.m/test/sap/m/qunit/plugins/opa/CellSelector/test/CellSelectorOPA.qunit.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -312,20 +312,6 @@ sap.ui.define([
When.Keyboard.iRemoveSelection(true);
Then.iSeeCellsSelected();

// Trying to clear with 2x CTRL + A with no Select All => nothing happens
selectBlock();
When.iFocusCell(1, 3);
When.Keyboard.iSelectAll();
Then.iSeeCellsSelected({ rowIndex: 1, colIndex: 1 }, { rowIndex: 1, colIndex: 3 });
Then.iSeeRowsSelected();

When.Keyboard.iSelectAll();
Then.iSeeCellsSelected({ rowIndex: 1, colIndex: 1 }, { rowIndex: 1, colIndex: 3 });
Then.iSeeRowsSelected();

When.Keyboard.iRemoveSelection(true);
Then.iSeeCellsSelected();

// Trying to clear with 2x CTRL + A with Select All => clears selection, also clears cells
Given.iChangeSelectAllState(true);

Expand All @@ -344,6 +330,8 @@ sap.ui.define([
Given.iChangeSelectAllState(false);
});

/*
Commenting as Column Selection feature will be changed/adjusted in a separate BLI
opaTest("Column Selection", function(Given, When, Then) {
let rangeLimit = 200; // Default range limit
Expand All @@ -354,7 +342,7 @@ sap.ui.define([
Then.iSeeCellFocused({ rowIndex: 1, colIndex: 1 });
When.Keyboard.iSelectColumns();
Then.iSeeCellsSelected({ rowIndex: 0, colIndex: 1 }, { rowIndex: rangeLimit - 1, colIndex: 1 });
Then.iSeeCellsSelected({ rowIndex: 0, colIndex: 1 }, { rowIndex: rangeLimit, colIndex: 1 });
When.Keyboard.iRemoveSelection();
Then.iSeeCellsSelected();
Expand All @@ -370,7 +358,7 @@ sap.ui.define([
Then.iSeeCellFocused({ rowIndex: 1, colIndex: 1 });
When.Keyboard.iSelectColumns();
Then.iSeeCellsSelected({ rowIndex: 0, colIndex: 1 }, { rowIndex: rangeLimit - 1, colIndex: 1 });
Then.iSeeCellsSelected({ rowIndex: 0, colIndex: 1 }, { rowIndex: rangeLimit, colIndex: 1 });
When.Keyboard.iRemoveSelection();
Then.iSeeCellsSelected();
Expand All @@ -388,17 +376,25 @@ sap.ui.define([
Then.iSeeCellFocused({ rowIndex: 1, colIndex: 3 });
When.Keyboard.iSelectColumns();
Then.iSeeCellsSelected({ rowIndex: 0, colIndex: 1 }, { rowIndex: rangeLimit - 1, colIndex: 3 });
Then.iSeeCellsSelected({ rowIndex: 0, colIndex: 1 }, { rowIndex: rangeLimit, colIndex: 3 });
Then.iSeeCellFocused({ rowIndex: 1, colIndex: 3});
// Extend Column Selection
When.Keyboard.iSelectNextCell(true, !oConfig.forward);
Then.iSeeCellsSelected({ rowIndex: 0, colIndex: 1 }, { rowIndex: rangeLimit - 1, colIndex: 2 });
Then.iSeeCellsSelected({ rowIndex: 0, colIndex: 1 }, { rowIndex: rangeLimit, colIndex: 2 });
Then.iSeeCellFocused({ rowIndex: 1, colIndex: 2});
When.Keyboard.iRemoveSelection();
Then.iSeeCellsSelected();
// Select cell outside of range limit
When.iFocusCell(7, 1);
When.Keyboard.iSelectDeselectCell();
Then.iSeeCellsSelected({ rowIndex: 7, colIndex: 1 }, { rowIndex: 7, colIndex: 1 });
When.Keyboard.iRemoveSelection();
Then.iSeeCellsSelected();
// Select column, then focus cell that is not part of selection -> columns of focused cell should be selected
When.iFocusCell(1, 1);
Expand All @@ -412,18 +408,19 @@ sap.ui.define([
Then.iSeeCellFocused({ rowIndex: 1, colIndex: 3 });
When.Keyboard.iSelectColumns();
Then.iSeeCellsSelected({ rowIndex: 0, colIndex: 1 }, { rowIndex: rangeLimit - 1, colIndex: 3 });
Then.iSeeCellsSelected({ rowIndex: 0, colIndex: 1 }, { rowIndex: rangeLimit, colIndex: 3 });
Then.iSeeCellFocused({ rowIndex: 1, colIndex: 3});
When.iFocusCell(4, 4);
When.Keyboard.iSelectColumns();
Then.iSeeCellsSelected({ rowIndex: 0, colIndex: 4 }, { rowIndex: rangeLimit - 1, colIndex: 4 });
Then.iSeeCellsSelected({ rowIndex: 0, colIndex: 4 }, { rowIndex: rangeLimit, colIndex: 4 });
When.Keyboard.iRemoveSelection();
Then.iSeeCellsSelected();
Given.iChangeRangeLimit(200);
});
*/

opaTest("Row- and Cell Selection interaction", function(Given, When, Then) {
// No selection at all => cells should be selected
Expand Down

0 comments on commit 2121aa4

Please sign in to comment.