Bug 189803

<rdar://problem/44655709>

Created attachment 352992 [details] Patch

I missed a row removal use case. When right-clicking in a macOS tree or table that supports item removal (Finder, Mail, Xcode) brings up a context menu with a "Delete" menu item. When a selection exists in the tree/table, the target of the delete will be: a) All the selected items, if the target item is selected b) The target item only, if the target item is not selected Supporting scenario b) requires an additional method, `Table.prototype.removeRow(rowIndex)`.

Created attachment 353069 [details] Patch

(In reply to Matt Baker from comment #4) > Created attachment 353069 [details] > Patch This can be tested by applying https://bugs.webkit.org/show_bug.cgi?id=66381.

Comment on attachment 353069 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=353069&action=review r=me, with some NITs I'd love to see a test (if you can) of what would happen when selection/deletion occurs outside of what's been cached (see Table.js:1432). > Source/WebInspectorUI/UserInterface/Base/IndexSet.js:99 > + clone() NIT: I think we've preferred `copy` in the past > Source/WebInspectorUI/UserInterface/Views/Table.js:388 > + if (rowIndex >= this.numberOfRows) While we're at it, should we also check that it's non-negative? Maybe an assertion too? > Source/WebInspectorUI/UserInterface/Views/Table.js:406 > + Style: extra newline > Source/WebInspectorUI/UserInterface/Views/Table.js:1432 > + let newSelectedRow = this._cachedRows.get(rowIndex); So this means that if we don't have a cached row for the index, we don't select it? Is it possible for us to be in a situation where we don't have a cached row (e.g. it's far off the screen) and try to select it? It seems like we should do some sort of "generate rows if necessary" here to make sure that we never "lose" a selection. > Source/WebInspectorUI/UserInterface/Views/Table.js:1442 > + _removeRows(rowIndexes) NIT: just `indexes` would be clear enough IMO > Source/WebInspectorUI/UserInterface/Views/Table.js:1446 > + let adjustRowAtIndex = (rowIndex) => { Ditto (1442) > LayoutTests/inspector/table/resources/table-utilities.js:27 > + for (let i = rowIndexes.length - 1; i >= 0; --i) NIT: I'd call this `index` instead of `i` since it's a value, not "just" an index in an array.

Comment on attachment 353069 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=353069&action=review >> Source/WebInspectorUI/UserInterface/Base/IndexSet.js:99 >> + clone() > > NIT: I think we've preferred `copy` in the past Will change. >> Source/WebInspectorUI/UserInterface/Views/Table.js:388 >> + if (rowIndex >= this.numberOfRows) > > While we're at it, should we also check that it's non-negative? Maybe an assertion too? I took another look at public Table methods taking a row index: deselectRow, selectRow, and removeRow. Only removeRow has any kind of validation. cellForRowAndColumn returns early if the row isn't visible, so an invalid index won't matter. I'll remove the check from removeRow, and add an assertion to all three: console.assert(rowIndex >= 0 && rowIndex < this.numberOfRows); >> Source/WebInspectorUI/UserInterface/Views/Table.js:1432 >> + let newSelectedRow = this._cachedRows.get(rowIndex); > > So this means that if we don't have a cached row for the index, we don't select it? Is it possible for us to be in a situation where we don't have a cached row (e.g. it's far off the screen) and try to select it? It seems like we should do some sort of "generate rows if necessary" here to make sure that we never "lose" a selection. It shouldn't be possible for us to to get in this situation currently, since a row needs to be visible (cached) to be interacted with/selected. Adding the index to `this._selectedRows` shouldn't be conditional though; good catch!

(In reply to Devin Rousso from comment #6) > Comment on attachment 353069 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=353069&action=review > > r=me, with some NITs > > I'd love to see a test (if you can) of what would happen when > selection/deletion occurs outside of what's been cached (see Table.js:1432). Good idea; I tested deselectRow, selectRow, and removeRow with an uncached row (row 999 of 1000). deselectRow fails but the others work as expected. I added a test to table-remove.rows.html, and will fix deselectRow (and add tests) in a follow-up: Web Inspector: Table selection should not require that rows be in the cache https://bugs.webkit.org/show_bug.cgi?id=190993

Created attachment 353248 [details] Patch for landing

Comment on attachment 353248 [details] Patch for landing Clearing flags on attachment: 353248 Committed r237495: <https://trac.webkit.org/changeset/237495>

All reviewed patches have been landed. Closing bug.