Diff
Modified: trunk/LayoutTests/ChangeLog (237494 => 237495)
--- trunk/LayoutTests/ChangeLog 2018-10-27 21:17:21 UTC (rev 237494)
+++ trunk/LayoutTests/ChangeLog 2018-10-27 23:30:03 UTC (rev 237495)
@@ -1,3 +1,29 @@
+2018-10-27 Matt Baker <[email protected]>
+
+ Web Inspector: Table should support deleting rows
+ https://bugs.webkit.org/show_bug.cgi?id=189803
+ <rdar://problem/44655709>
+
+ Reviewed by Devin Rousso.
+
+ * inspector/table/resources/table-utilities.js:
+ (TestPage.registerInitializer.InspectorTest.TableDelegate.prototype.tableDidRemoveRows):
+ (TestPage.registerInitializer.createDataSource):
+ (TestPage.registerInitializer.InspectorTest.createTable):
+ (TestPage.registerInitializer.InspectorTest.createTableWithDelegate):
+ (TestPage.registerInitializer):
+ Update table test utilities to keep TableDataSource backing array in
+ sync when rows are removed. Also allow the number of table rows to be
+ specified when creating a table for testing.
+
+ * inspector/table/table-remove-rows-expected.txt: Added.
+ * inspector/table/table-remove-rows.html: Added.
+ Add tests for new Table methods `removeRow(rowIndex)` and `removeSelectedRows()`.
+
+ * inspector/unit-tests/index-set-expected.txt:
+ * inspector/unit-tests/index-set.html:
+ Add tests for new IndexSet method `copy()`.
+
2018-10-26 Commit Queue <[email protected]>
Unreviewed, rolling out r237479 and r237484.
Modified: trunk/LayoutTests/inspector/table/resources/table-utilities.js (237494 => 237495)
--- trunk/LayoutTests/inspector/table/resources/table-utilities.js 2018-10-27 21:17:21 UTC (rev 237494)
+++ trunk/LayoutTests/inspector/table/resources/table-utilities.js 2018-10-27 23:30:03 UTC (rev 237495)
@@ -21,6 +21,13 @@
this.items = items || [];
}
+ tableDidRemoveRows(table, rowIndexes)
+ {
+ // Prevent data source from getting out of sync.
+ for (let index = rowIndexes.length - 1; index >= 0; --index)
+ this.items.splice(index, 1);
+ }
+
tableSelectionDidChange(table)
{
InspectorTest.pass("Table selection changed.");
@@ -36,9 +43,9 @@
}
};
- function createDataSource() {
+ function createDataSource(numberOfRows = 10) {
let items = [];
- for (let i = 0; i < 10; ++i)
+ for (let i = 0; i < numberOfRows; ++i)
items.push({index: i, name: `Row ${i}`});
return new InspectorTest.TableDataSource(items);
@@ -58,16 +65,16 @@
return table;
}
- InspectorTest.createTable = function() {
- let dataSource = createDataSource();
+ InspectorTest.createTable = function(numberOfRows) {
+ let dataSource = createDataSource(numberOfRows);
let delegate = new InspectorTest.TableDelegate(dataSource.items);
return createTableInternal(dataSource, delegate);
};
- InspectorTest.createTableWithDelegate = function(delegate) {
+ InspectorTest.createTableWithDelegate = function(delegate, numberOfRows) {
InspectorTest.assert(delegate instanceof InspectorTest.TableDelegate);
- let dataSource = createDataSource();
+ let dataSource = createDataSource(numberOfRows);
delegate.items = dataSource.items;
return createTableInternal(dataSource, delegate);
};
Added: trunk/LayoutTests/inspector/table/table-remove-rows-expected.txt (0 => 237495)
--- trunk/LayoutTests/inspector/table/table-remove-rows-expected.txt (rev 0)
+++ trunk/LayoutTests/inspector/table/table-remove-rows-expected.txt 2018-10-27 23:30:03 UTC (rev 237495)
@@ -0,0 +1,84 @@
+Tests that rows can be removed from Table, and that the selection is updated when removing selected rows.
+
+An asterix (*) indicates a selected row; a hyphen (-) indicates a removed row.
+
+
+== Running test suite: Table.RemoveRows
+-- Running test case: Table.RemoveRow.NoneSelected
+Given a Table with selected rows [], remove row 0.
+PASS: Should remove row 0.
+
+-- Running test case: Table.RemoveRow.Selected
+Given a Table with selected rows [0], remove row 0.
+Selection changed to [] before removing row 0.
+PASS: Should remove row 0.
+
+-- Running test case: Table.RemoveSelectedRows.Single.SelectFollowing
+Given a Table with selected rows [0]:
+ * Row 0
+ Row 1
+ Row 2
+ Row 3
+Selection changed before removing rows:
+ - Row 0
+ * Row 1
+ Row 2
+ Row 3
+PASS: Should remove rows [0].
+
+-- Running test case: Table.RemoveSelectedRows.Single.SelectPreceding
+Given a Table with selected rows [3]:
+ Row 0
+ Row 1
+ Row 2
+ * Row 3
+Selection changed before removing rows:
+ Row 0
+ Row 1
+ * Row 2
+ - Row 3
+PASS: Should remove rows [3].
+
+-- Running test case: Table.RemoveSelectedRows.Multiple.SelectFollowing
+Given a Table with selected rows [0,2]:
+ * Row 0
+ Row 1
+ * Row 2
+ Row 3
+Selection changed before removing rows:
+ - Row 0
+ Row 1
+ - Row 2
+ * Row 3
+PASS: Should remove rows [0,2].
+
+-- Running test case: Table.RemoveSelectedRows.Multiple.SelectHole
+Given a Table with selected rows [0,3]:
+ * Row 0
+ Row 1
+ Row 2
+ * Row 3
+Selection changed before removing rows:
+ - Row 0
+ * Row 1
+ Row 2
+ - Row 3
+PASS: Should remove rows [0,3].
+
+-- Running test case: Table.RemoveSelectedRows.Multiple.SelectPreceding
+Given a Table with selected rows [2,3]:
+ Row 0
+ Row 1
+ * Row 2
+ * Row 3
+Selection changed before removing rows:
+ Row 0
+ * Row 1
+ - Row 2
+ - Row 3
+PASS: Should remove rows [2,3].
+
+-- Running test case: Table.RemoveRow.NotCached
+Given a Table with selected rows [], remove row 999.
+PASS: Should remove row 999.
+
Added: trunk/LayoutTests/inspector/table/table-remove-rows.html (0 => 237495)
--- trunk/LayoutTests/inspector/table/table-remove-rows.html (rev 0)
+++ trunk/LayoutTests/inspector/table/table-remove-rows.html 2018-10-27 23:30:03 UTC (rev 237495)
@@ -0,0 +1,206 @@
+<!doctype html>
+<html>
+<head>
+<script src=""
+<script src=""
+<script>
+function test()
+{
+ InspectorTest.redirectRequestAnimationFrame();
+
+ let suite = InspectorTest.createSyncSuite("Table.RemoveRows");
+
+ function logTable(table, rowsToRemove = []) {
+ for (let rowIndex = 0; rowIndex < table.numberOfRows; ++rowIndex) {
+ let type = " ";
+ if (rowsToRemove.includes(rowIndex))
+ type = "-";
+ else if (table.selectedRows.includes(rowIndex))
+ type = "*";
+ InspectorTest.log(` ${type} Row ${rowIndex}`);
+ }
+ }
+
+ class RemoveRowTestDelegate extends InspectorTest.TableDelegate
+ {
+ constructor()
+ {
+ super();
+
+ this._rowIndex = NaN;
+ }
+
+ // Public
+
+ triggerRemoveRow(table, rowIndex)
+ {
+ InspectorTest.assert(isNaN(this._rowIndex));
+
+ this._rowIndex = rowIndex;
+
+ InspectorTest.log(`Given a Table with selected rows [${table.selectedRows}], remove row ${rowIndex}.`);
+ table.removeRow(rowIndex);
+ }
+
+ // Table delegate
+
+ tableDidRemoveRows(table, rowIndexes)
+ {
+ super.tableDidRemoveRows(table, rowIndexes);
+
+ InspectorTest.expectShallowEqual(rowIndexes, [this._rowIndex], `Should remove row ${this._rowIndex}.`);
+ this._rowIndex = NaN;
+ }
+
+ tableSelectionDidChange(table)
+ {
+ if (isNaN(this._rowIndex))
+ return;
+
+ InspectorTest.log(`Selection changed to [${table.selectedRows}] before removing row ${this._rowIndex}.`);
+ }
+ }
+
+ class RemoveSelectedRowsTestDelegate extends InspectorTest.TableDelegate
+ {
+ constructor()
+ {
+ super();
+
+ this._rowIndexes = null;
+ }
+
+ // Public
+
+ triggerRemoveSelectedRows(table)
+ {
+ InspectorTest.assert(!this._rowIndexes);
+
+ this._rowIndexes = table.selectedRows;
+
+ InspectorTest.log(`Given a Table with selected rows [${this._rowIndexes}]:`);
+ logTable(table);
+
+ table.removeSelectedRows();
+ }
+
+ // Table delegate
+
+ tableDidRemoveRows(table, rowIndexes)
+ {
+ super.tableDidRemoveRows(table, rowIndexes);
+
+ InspectorTest.expectShallowEqual(rowIndexes, this._rowIndexes, `Should remove rows [${this._rowIndexes}].`);
+ this._rowIndexes = null;
+ }
+
+ tableSelectionDidChange(table)
+ {
+ if (!this._rowIndexes)
+ return;
+
+ InspectorTest.log(`Selection changed before removing rows:`);
+ logTable(table, this._rowIndexes);
+ }
+ }
+
+ const numberOfRows = 4;
+ const lastRowIndex = numberOfRows - 1;
+
+ suite.addTestCase({
+ name: "Table.RemoveRow.NoneSelected",
+ description: "Remove a row from a table with no selected rows.",
+ test() {
+ let testDelegate = new RemoveRowTestDelegate;
+ let table = InspectorTest.createTableWithDelegate(testDelegate, numberOfRows);
+
+ testDelegate.triggerRemoveRow(table, 0);
+
+ return true;
+ }
+ });
+
+ suite.addTestCase({
+ name: "Table.RemoveRow.Selected",
+ description: "Remove the only selected table row.",
+ test() {
+ let testDelegate = new RemoveRowTestDelegate;
+ let table = InspectorTest.createTableWithDelegate(testDelegate, numberOfRows);
+
+ table.selectRow(0);
+ testDelegate.triggerRemoveRow(table, 0);
+
+ return true;
+ }
+ });
+
+ function addTestCase({name, description, rowIndexes}) {
+ suite.addTestCase({
+ name, description,
+ test() {
+ let testDelegate = new RemoveSelectedRowsTestDelegate;
+ let table = InspectorTest.createTableWithDelegate(testDelegate, numberOfRows);
+ table.allowsMultipleSelection = true;
+
+ for (let rowIndex of rowIndexes)
+ table.selectRow(rowIndex, true);
+
+ testDelegate.triggerRemoveSelectedRows(table);
+
+ return true;
+ }
+ });
+ }
+
+ addTestCase({
+ name: "Table.RemoveSelectedRows.Single.SelectFollowing",
+ description: "Remove the selected row, causing the following row to be selected.",
+ rowIndexes: [0],
+ });
+
+ addTestCase({
+ name: "Table.RemoveSelectedRows.Single.SelectPreceding",
+ description: "Remove the selected row, causing the preceding row to be selected.",
+ rowIndexes: [lastRowIndex],
+ });
+
+ addTestCase({
+ name: "Table.RemoveSelectedRows.Multiple.SelectFollowing",
+ description: "Remove selected rows, causing the row following the selection to be selected.",
+ rowIndexes: [0, lastRowIndex - 1],
+ });
+
+ addTestCase({
+ name: "Table.RemoveSelectedRows.Multiple.SelectHole",
+ description: "Remove selected rows, causing the first deselected row inside the selection to be selected.",
+ rowIndexes: [0, lastRowIndex],
+ });
+
+ addTestCase({
+ name: "Table.RemoveSelectedRows.Multiple.SelectPreceding",
+ description: "Remove selected rows, causing the row preceding the selection to be selected.",
+ rowIndexes: [lastRowIndex - 1, lastRowIndex],
+ });
+
+ suite.addTestCase({
+ name: "Table.RemoveRow.NotCached",
+ description: "Remove a row that is not in the table cache.",
+ test() {
+ let testDelegate = new RemoveRowTestDelegate;
+ let table = InspectorTest.createTableWithDelegate(testDelegate, 1000);
+
+ testDelegate.triggerRemoveRow(table, 999);
+
+ return true;
+ }
+ });
+
+ suite.runTestCasesAndFinish();
+}
+</script>
+</head>
+<body _onLoad_="runTest()">
+ <p>Tests that rows can be removed from Table, and that the selection is updated when removing selected rows.</p>
+ <p>An asterix (*) indicates a selected row; a hyphen (-) indicates a removed row.</p>
+</body>
+</html>
Modified: trunk/LayoutTests/inspector/unit-tests/index-set-expected.txt (237494 => 237495)
--- trunk/LayoutTests/inspector/unit-tests/index-set-expected.txt 2018-10-27 21:17:21 UTC (rev 237494)
+++ trunk/LayoutTests/inspector/unit-tests/index-set-expected.txt 2018-10-27 23:30:03 UTC (rev 237495)
@@ -68,3 +68,7 @@
PASS: Index less than 2 should be 1.
PASS: Index less than 3 should be 2.
+-- Running test case: IndexSet.prototype.copy
+PASS: Copy and original should be different objects.
+PASS: Copy and original should have the same values.
+
Modified: trunk/LayoutTests/inspector/unit-tests/index-set.html (237494 => 237495)
--- trunk/LayoutTests/inspector/unit-tests/index-set.html 2018-10-27 21:17:21 UTC (rev 237494)
+++ trunk/LayoutTests/inspector/unit-tests/index-set.html 2018-10-27 23:30:03 UTC (rev 237495)
@@ -194,6 +194,18 @@
}
});
+ suite.addTestCase({
+ name: "IndexSet.prototype.copy",
+ test() {
+ let original = new WI.IndexSet([1, 2, 3]);
+ let copied = original.copy();
+ InspectorTest.expectNotEqual(copied, original, "Copy and original should be different objects.");
+ InspectorTest.expectShallowEqual(Array.from(copied), Array.from(original), "Copy and original should have the same values.");
+
+ return true;
+ }
+ });
+
suite.runTestCasesAndFinish();
}
</script>
Modified: trunk/Source/WebInspectorUI/ChangeLog (237494 => 237495)
--- trunk/Source/WebInspectorUI/ChangeLog 2018-10-27 21:17:21 UTC (rev 237494)
+++ trunk/Source/WebInspectorUI/ChangeLog 2018-10-27 23:30:03 UTC (rev 237495)
@@ -1,3 +1,41 @@
+2018-10-27 Matt Baker <[email protected]>
+
+ Web Inspector: Table should support deleting rows
+ https://bugs.webkit.org/show_bug.cgi?id=189803
+ <rdar://problem/44655709>
+
+ Reviewed by Devin Rousso.
+
+ Add methods for removing rows from a Table without reloading the data
+ source. This patch adds Table.prototype.removeRow for removing a single
+ row, and Table.prototype.removeSelectedRows for removing the entire selection.
+ The latter also attempts to select a new row, if possible, before removing
+ the selection.
+
+ * UserInterface/Base/IndexSet.js:
+ (WI.IndexSet.prototype.copy):
+
+ * UserInterface/Views/Table.js:
+ (WI.Table):
+ (WI.Table.prototype.get numberOfRows):
+ Cache the number of rows in the table data source. Invalidate cached
+ value whenever table data is reloaded. Removing rows immediately updates
+ cached value, without incurring a potentially expensive reload.
+
+ (WI.Table.prototype.reloadData):
+ (WI.Table.prototype.selectRow):
+ (WI.Table.prototype.deselectRow):
+ (WI.Table.prototype.removeRow):
+ (WI.Table.prototype.removeSelectedRows):
+ Remove the selected rows and select a new row, if possible.
+ (WI.Table.prototype._updateVisibleRows):
+ (WI.Table.prototype._handleKeyDown):
+ (WI.Table.prototype._deselectAllAndSelect):
+ Drive-by fix: should work when `rowToSelect` isn't already selected.
+ (WI.Table.prototype._removeRows):
+ Remove rows and adjust the indexes of rows that are shifted up as a result
+ of preceding rows being removed.
+
2018-10-26 Devin Rousso <[email protected]>
Web Inspector: simplify some editing code checks
Modified: trunk/Source/WebInspectorUI/UserInterface/Base/IndexSet.js (237494 => 237495)
--- trunk/Source/WebInspectorUI/UserInterface/Base/IndexSet.js 2018-10-27 21:17:21 UTC (rev 237494)
+++ trunk/Source/WebInspectorUI/UserInterface/Base/IndexSet.js 2018-10-27 23:30:03 UTC (rev 237495)
@@ -96,6 +96,13 @@
this._indexes = [];
}
+ copy()
+ {
+ let indexSet = new WI.IndexSet;
+ indexSet._indexes = this._indexes.slice();
+ return indexSet;
+ }
+
indexGreaterThan(value)
{
const following = true;
Modified: trunk/Source/WebInspectorUI/UserInterface/Views/Table.js (237494 => 237495)
--- trunk/Source/WebInspectorUI/UserInterface/Views/Table.js 2018-10-27 21:17:21 UTC (rev 237494)
+++ trunk/Source/WebInspectorUI/UserInterface/Views/Table.js 2018-10-27 23:30:03 UTC (rev 237495)
@@ -76,6 +76,7 @@
this._resizersElement.className = "resizers";
this._cachedRows = new Map;
+ this._cachedNumberOfRows = NaN;
this._columnSpecs = new Map;
this._columnOrder = [];
@@ -132,6 +133,14 @@
get scrollContainer() { return this._scrollContainerElement; }
+ get numberOfRows()
+ {
+ if (isNaN(this._cachedNumberOfRows))
+ this._cachedNumberOfRows = this._dataSource.tableNumberOfRows(this);
+
+ return this._cachedNumberOfRows;
+ }
+
get sortOrder()
{
return this._sortOrder;
@@ -241,6 +250,7 @@
this._selectedRowIndex = NaN;
this._selectedRows.clear();
+ this._cachedNumberOfRows = NaN;
this._previousRevealedRowCount = NaN;
this.needsLayout();
}
@@ -309,6 +319,7 @@
selectRow(rowIndex, extendSelection = false)
{
console.assert(!extendSelection || this._allowsMultipleSelection, "Cannot extend selection with multiple selection disabled.");
+ console.assert(rowIndex >= 0 && rowIndex < this.numberOfRows);
if (this._isRowSelected(rowIndex)) {
if (!extendSelection)
@@ -333,6 +344,8 @@
deselectRow(rowIndex)
{
+ console.assert(rowIndex >= 0 && rowIndex < this.numberOfRows);
+
if (!this._isRowSelected(rowIndex))
return;
@@ -373,6 +386,47 @@
this._deselectAllAndSelect(rowIndex);
}
+ removeRow(rowIndex)
+ {
+ console.assert(rowIndex >= 0 && rowIndex < this.numberOfRows);
+
+ if (this._isRowSelected(rowIndex))
+ this.deselectRow(rowIndex);
+
+ this._removeRows(new WI.IndexSet([rowIndex]));
+ }
+
+ removeSelectedRows()
+ {
+ let numberOfSelectedRows = this._selectedRows.size;
+ if (!numberOfSelectedRows)
+ return;
+
+ // Try selecting the row following the selection.
+ let lastSelectedRow = this._selectedRows.lastIndex;
+ let rowToSelect = lastSelectedRow + 1;
+ if (rowToSelect === this.numberOfRows) {
+ // If no row exists after the last selected row, try selecting a
+ // deselected row (hole) within the selection.
+ let firstSelectedRow = this._selectedRows.firstIndex;
+ if (lastSelectedRow - firstSelectedRow > numberOfSelectedRows) {
+ rowToSelect = this._selectedRows.firstIndex + 1;
+ while (this._selectedRows.has(rowToSelect))
+ rowToSelect++;
+ } else {
+ // If the selection contains no holes, try selecting the row
+ // preceding the selection.
+ rowToSelect = firstSelectedRow > 0 ? firstSelectedRow - 1 : NaN;
+ }
+ }
+
+ // Change the selection before removing rows. This matches the behavior
+ // of macOS Finder (in list and column modes) when removing selected items.
+ let oldSelectedRows = this._selectedRows.copy();
+ this._deselectAllAndSelect(rowToSelect);
+ this._removeRows(oldSelectedRows);
+ }
+
columnWithIdentifier(identifier)
{
return this._columnSpecs.get(identifier);
@@ -790,7 +844,7 @@
let availableWidth = this._cachedWidth;
let availableHeight = this._cachedHeight;
- let numberOfRows = this._dataSource.tableNumberOfRows(this);
+ let numberOfRows = this.numberOfRows;
this._cachedNumberOfRows = numberOfRows;
let contentHeight = numberOfRows * this._rowHeight;
@@ -1011,7 +1065,7 @@
if (belowTopThreshold && aboveBottomThreshold && !isNaN(this._previousRevealedRowCount))
return;
- let numberOfRows = this._dataSource.tableNumberOfRows(this);
+ let numberOfRows = this.numberOfRows;
this._previousRevealedRowCount = numberOfRows;
// Scroll back up if the number of rows was reduced such that the existing
@@ -1222,6 +1276,7 @@
if (event.shiftKey || event.metaKey || event.ctrlKey)
return;
+ let numberOfRows = this.numberOfRows;
let rowToSelect = NaN;
if (event.keyIdentifier === "Up") {
@@ -1366,8 +1421,6 @@
return;
for (let selectedRowIndex of this._selectedRows) {
- if (selectedRowIndex === rowIndex)
- continue;
let oldSelectedRow = this._cachedRows.get(selectedRowIndex);
if (oldSelectedRow)
oldSelectedRow.classList.remove("selected");
@@ -1376,12 +1429,70 @@
this._selectedRowIndex = rowIndex;
this._selectedRows.clear();
- if (!isNaN(rowIndex))
+ if (!isNaN(rowIndex)) {
this._selectedRows.add(rowIndex);
+ let newSelectedRow = this._cachedRows.get(rowIndex);
+ if (newSelectedRow)
+ newSelectedRow.classList.add("selected");
+ }
this._notifySelectionDidChange();
}
+ _removeRows(rowIndexes)
+ {
+ let removed = 0;
+
+ let adjustRowAtIndex = (index) => {
+ let newIndex = index - removed;
+ let row = this._cachedRows.get(index);
+ if (row) {
+ this._cachedRows.delete(row.__index);
+ row.__index = newIndex;
+ this._cachedRows.set(newIndex, row);
+ }
+
+ if (this._isRowSelected(index)) {
+ this._selectedRows.delete(index);
+ this._selectedRows.add(newIndex);
+ if (this._selectedRowIndex === index)
+ this._selectedRowIndex = newIndex;
+ }
+ };
+
+ if (rowIndexes.has(this._selectedRowIndex))
+ this._selectedRowIndex = NaN;
+
+ for (let index = rowIndexes.firstIndex; index <= rowIndexes.lastIndex; ++index) {
+ if (rowIndexes.has(index)) {
+ let row = this._cachedRows.get(index);
+ if (row) {
+ this._cachedRows.delete(index);
+ row.remove();
+ }
+ removed++;
+ continue;
+ }
+
+ if (removed)
+ adjustRowAtIndex(index);
+ }
+
+ if (!removed)
+ return;
+
+ for (let index = rowIndexes.lastIndex + 1; index < this._cachedNumberOfRows; ++index)
+ adjustRowAtIndex(index);
+
+ this._cachedNumberOfRows -= removed;
+ console.assert(this._cachedNumberOfRows >= 0);
+
+ if (this._delegate.tableDidRemoveRows) {
+ this._delegate.tableDidRemoveRows(this, Array.from(rowIndexes));
+ console.assert(this._cachedNumberOfRows === this._dataSource.tableNumberOfRows(this), "Table data source should update after removing rows.");
+ }
+ }
+
_isRowSelected(rowIndex)
{
return this._selectedRows.has(rowIndex);
