Summary: Table should support multiple selection and Cmd-click behavior. This patch will add programmatic multiple selection to Table, and Command-click support for selecting/deselecting individual items.
<rdar://problem/44571170>
Created attachment 350041 [details] Patch
To try out multiple selection in the Cookies view, apply the WIP patch here: https://bugs.webkit.org/show_bug.cgi?id=66381.
Comment on attachment 350041 [details] Patch Attachment 350041 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/9259704 New failing tests: inspector/unit-tests/index-set.html
Created attachment 350049 [details] Archive of layout-test-results from ews103 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews103 Port: mac-sierra Platform: Mac OS X 10.12.6
Comment on attachment 350041 [details] Patch Attachment 350041 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/9259837 New failing tests: inspector/unit-tests/index-set.html
Created attachment 350052 [details] Archive of layout-test-results from ews106 for mac-sierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6
Comment on attachment 350041 [details] Patch Attachment 350041 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/9259672 New failing tests: inspector/unit-tests/index-set.html
Created attachment 350055 [details] Archive of layout-test-results from ews112 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews112 Port: mac-sierra Platform: Mac OS X 10.12.6
Created attachment 350062 [details] Patch
Comment on attachment 350041 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=350041&action=review > Source/WebInspectorUI/ChangeLog:13 > + Helper container for managing an ordered sequence of unique positive NIT: I usually like putting these things after the "group", as it doesn't disrupt the flow of the ChangeLog as much. > Source/WebInspectorUI/UserInterface/Base/IndexSet.js:26 > +WI.IndexSet = class IndexSet It seems to me like this should really be called `UniqueOrderedArray`, and be more generic to allow other uses. The naming is also a bit confusing, as sometimes it's not clear whether you mean `index` as in a value (e.g. `this._indexes.includes(index)`) or an index (e.g. `this._indexes[index]`). > Source/WebInspectorUI/UserInterface/Base/IndexSet.js:30 > + console.assert(Array.isArray(indexes) || indexes === undefined); I think just checking for !indexes is fine: console.assert(!indexes || Array.isArray(indexes)); > Source/WebInspectorUI/UserInterface/Base/IndexSet.js:37 > + for (let index of indexes.slice().sort((a, b) => a - b)) { I don't think the sort comparator is necessary. > Source/WebInspectorUI/UserInterface/Base/IndexSet.js:60 > + get size() { return this._indexes.length; } Put this above `firstIndex` and `lastIndex`, since it's a "simple" getter. > Source/WebInspectorUI/UserInterface/Base/IndexSet.js:64 > + this._validateIndex(index); Ditto (208). if (!this._validateIndex(index)) return; > Source/WebInspectorUI/UserInterface/Base/IndexSet.js:75 > + this._validateIndex(index); Ditto (208). if (!this._validateIndex(index)) return; > Source/WebInspectorUI/UserInterface/Base/IndexSet.js:92 > + let position = this._indexes.lowerBound(index); So this should be faster since it's ordered, right? > Source/WebInspectorUI/UserInterface/Base/IndexSet.js:96 > + addRange(startIndex, count) I'm guessing that this was supposed to be used by `WI.Table.prototype._handleMouseDown` when `event.shiftKey` is true? > Source/WebInspectorUI/UserInterface/Base/IndexSet.js:98 > + this._validateIndex(startIndex); Ditto (208). if (!this._validateIndex(startIndex)) return; > Source/WebInspectorUI/UserInterface/Base/IndexSet.js:125 > + this._validateIndex(startIndex); Ditto (208). if (!this._validateIndex(startIndex)) return; > Source/WebInspectorUI/UserInterface/Base/IndexSet.js:155 > + indexGreaterThan(index) In this case, does `index` refer to an index in the array or an index value? The naming is a bit confusing. > Source/WebInspectorUI/UserInterface/Base/IndexSet.js:161 > + indexLessThan(index) Ditto (155). > Source/WebInspectorUI/UserInterface/Base/IndexSet.js:172 > + toArray() I think adding `toArray` doesn't serve much of a purpose over calling `Array.from` on the object itself. That's what I've done with `WI.Collection`. > Source/WebInspectorUI/UserInterface/Base/IndexSet.js:194 > + if (closestIndex === undefined) Is there a better way to check for this? Compare the index vs 0 or the length? > Source/WebInspectorUI/UserInterface/Base/IndexSet.js:197 > + if (index === closestIndex) So in this case, you're saying that we don't have any values close to `index`, or does it mean something else? > Source/WebInspectorUI/UserInterface/Base/IndexSet.js:208 > + throw new TypeError("Index must be an integer."); Woah this seems "scary". I'd rather us just `console.assert` and `return false;` in the case that this (or the if below) fails. Making this function return true/false would also allow it to be used in `.filter()`, which should simplify the logic of the constructor. this._indexes = Array.from(new Set(indexes.filter(this._validateIndex).sort())); > Source/WebInspectorUI/UserInterface/Views/Table.css:129 > + border-top: 1px solid transparent; This will make all of the table rows 1px taller. Does this affect the virtualization at all (e.g. `this._rowHeight`)? > Source/WebInspectorUI/UserInterface/Views/Table.js:130 > + return this._selectedRows.toArray(); Ditto (IndexSet.js:172). > Source/WebInspectorUI/UserInterface/Views/Table.js:216 > + set allowsMultipleSelection(enable) NIT: I'm not sure what our style is on "flag" variable names. I tend to use `flag` or the non-underscore name of the variable (e.g. `allowsMultipleSelection`). > Source/WebInspectorUI/UserInterface/Views/Table.js:1357 > + _deselectAllExcept(rowIndex) NIT: this function could use a better name since it also selects `rowIndex` if it wasn't already selected before. > LayoutTests/ChangeLog:20 > + * inspector/table/table-selection.html: Added. Some tests for Cmd+click and Shift+click functionality (not actually using `.click()`) would be nice :)
Comment on attachment 350041 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=350041&action=review >> Source/WebInspectorUI/UserInterface/Base/IndexSet.js:26 >> +WI.IndexSet = class IndexSet > > It seems to me like this should really be called `UniqueOrderedArray`, and be more generic to allow other uses. The naming is also a bit confusing, as sometimes it's not clear whether you mean `index` as in a value (e.g. `this._indexes.includes(index)`) or an index (e.g. `this._indexes[index]`). I'm okay with the name. This isn't meant to be a general purpose container, but a helper class specific to things like row/column/item indices. It is only really going to be used by Table and TreeOutline. I do agree that some of the names are confusing. Using the name `value` to describe the indices stored in the set should help: >> Source/WebInspectorUI/UserInterface/Base/IndexSet.js:37 >> + for (let index of indexes.slice().sort((a, b) => a - b)) { > > I don't think the sort comparator is necessary. Somewhat surprisingly, it is: > [1, 11, 5, 10].sort() < [1, 10, 11, 5] From MDN (https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/sort#Description): "If compareFunction is not supplied, all non-undefined array elements are sorted by converting them to strings and comparing strings in Unicode code point order." >> Source/WebInspectorUI/UserInterface/Base/IndexSet.js:64 >> + this._validateIndex(index); > > Ditto (208). > > if (!this._validateIndex(index)) > return; IndexSet.prototype._validateIndex throws an exception for invalid indices, so it isn't necessary to return. >> Source/WebInspectorUI/UserInterface/Base/IndexSet.js:92 >> + let position = this._indexes.lowerBound(index); > > So this should be faster since it's ordered, right? Yes, lookup is O(log n). Array.prototype.lowerBound requires that the array be ordered. >> Source/WebInspectorUI/UserInterface/Base/IndexSet.js:96 >> + addRange(startIndex, count) > > I'm guessing that this was supposed to be used by `WI.Table.prototype._handleMouseDown` when `event.shiftKey` is true? Correct. This isn't actually being used yet, so I will remove it for now. >> Source/WebInspectorUI/UserInterface/Base/IndexSet.js:155 >> + indexGreaterThan(index) > > In this case, does `index` refer to an index in the array or an index value? The naming is a bit confusing. It's confusing. I'll rename the parameter value, as discussed above. >> Source/WebInspectorUI/UserInterface/Base/IndexSet.js:172 >> + toArray() > > I think adding `toArray` doesn't serve much of a purpose over calling `Array.from` on the object itself. That's what I've done with `WI.Collection`. I think this has better performance than passing an iteraable to Array.from(), but I don't have data to back that up, and I doubt it matters either way. I'll change it for simplicity's sake. >> Source/WebInspectorUI/UserInterface/Base/IndexSet.js:208 >> + throw new TypeError("Index must be an integer."); > > Woah this seems "scary". I'd rather us just `console.assert` and `return false;` in the case that this (or the if below) fails. > > Making this function return true/false would also allow it to be used in `.filter()`, which should simplify the logic of the constructor. > > this._indexes = Array.from(new Set(indexes.filter(this._validateIndex).sort())); Why is it scary? Constructing an IndexSet with one or more invalid indices is exceptional, as is passing an invalid index to an IndexSet method. This allows us to write layout tests for invalid IndexSet constructor input, and removes the need to check a bunch of return values. >> Source/WebInspectorUI/UserInterface/Views/Table.css:129 >> + border-top: 1px solid transparent; > > This will make all of the table rows 1px taller. Does this affect the virtualization at all (e.g. `this._rowHeight`)? Good catch. >> Source/WebInspectorUI/UserInterface/Views/Table.js:216 >> + set allowsMultipleSelection(enable) > > NIT: I'm not sure what our style is on "flag" variable names. I tend to use `flag` or the non-underscore name of the variable (e.g. `allowsMultipleSelection`). It looks like we use "flag" about twice as often as "enabled", and don't use "enable" at all. >> Source/WebInspectorUI/UserInterface/Views/Table.js:1357 >> + _deselectAllExcept(rowIndex) > > NIT: this function could use a better name since it also selects `rowIndex` if it wasn't already selected before. Agree. I'll come up with something better. >> LayoutTests/ChangeLog:20 >> + * inspector/table/table-selection.html: Added. > > Some tests for Cmd+click and Shift+click functionality (not actually using `.click()`) would be nice :) We can't currently write layout tests for this, which is why tests are limited to programmatic selection.
Comment on attachment 350041 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=350041&action=review >>> Source/WebInspectorUI/UserInterface/Base/IndexSet.js:37 >>> + for (let index of indexes.slice().sort((a, b) => a - b)) { >> >> I don't think the sort comparator is necessary. > > Somewhat surprisingly, it is: This can be simplified a bit. The temporary array is unnecessary: values can be pushed directly into this._indexes, since an invalid value will cause an exception to be thrown and the constructor to fail.
Comment on attachment 350041 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=350041&action=review > Source/WebInspectorUI/UserInterface/Views/Table.js:312 > + this._deselectAllExcept(rowIndex); This should be: if (this._isRowSelected(rowIndex)) { if (!extendSelection) this._deselectAllExcept(rowIndex); return; }
(In reply to Matt Baker from comment #14) > Comment on attachment 350041 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=350041&action=review > > > Source/WebInspectorUI/UserInterface/Views/Table.js:312 > > + this._deselectAllExcept(rowIndex); > > This should be: > > if (this._isRowSelected(rowIndex)) { > if (!extendSelection) > this._deselectAllExcept(rowIndex); > return; > } I've added two additional tests for this case.
Created attachment 350154 [details] Patch
Created attachment 350155 [details] Patch
I simplified the layout tests and switched to a synchronous test suite. Test case names have also been updated for clarity.
Comment on attachment 350155 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=350155&action=review > Source/WebInspectorUI/UserInterface/Base/IndexSet.js:30 > + console.assert(Array.isArray(values) || !values); NIT: I'd reverse the order of these checks. > Source/WebInspectorUI/UserInterface/Base/IndexSet.js:35 > + let previous = NaN; Instead of using `previous`, you can just compare with `this._indexes.lastValue`, or even `this.lastIndex`. > Source/WebInspectorUI/UserInterface/Base/IndexSet.js:36 > + for (let value of values.slice().sort((a, b) => a - b)) { Were you going to remove the `.slice()`? > Source/WebInspectorUI/UserInterface/Base/IndexSet.js:150 > + if (!Number.isInteger(value)) > + throw new TypeError("Index must be an integer."); > + > + if (value < 0) > + throw new RangeError("Index cannot be negative."); My main objection to this is twofold: 1. Throwing uncaught exceptions is very "dangerous" as any wrong move could potentially crash WebInspector, and there's always a chance that we miss a case where this happens 2. Adding an "invalid" value to IndexSet isn't really something that would "break" WebInspector, and it's completely recoverable by just returning early (changing the function to return true/false) I understand that neither of these *should* ever happen, but my fear is that we miss a small case somewhere and it ends up shipping such that anytime the user tries to do that specific action, WebInspector crashes with an uncaught exception. Furthermore, we follow the assert-and-bail pattern everywhere else in WebInspector, so introducing exceptions is something we'd definitely need to discuss/decide more in depth. > Source/WebInspectorUI/UserInterface/Views/DarkMode.css:842 > + .table > .data-container > .data-list > li.selected + li.selected { > + border-top-color: var(--background-color); > + } This file no longer exists, so you'll need to move this to the right spot (or even remove it altogether). > Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:352 > + let rowIndex = table.selectedRow; If we're going to support multiple selection, I'd rather us go all-or-nothing and only have a `selectedRows` that returns an array. In the case where there is only one row selected, it would just be a 1-length array. > Source/WebInspectorUI/UserInterface/Views/Table.js:90 > + this._allowsMultipleSelection = false; Are there any current users of `WI.Table` that we don't want to have support for multi-selection? (I'm just asking, not suggesting anything) If not, is there a plan to eventually remove this member, since all it really does is control whether meta/shift click multi/extend selects more than one row? > Source/WebInspectorUI/UserInterface/Views/Table.js:130 > + return Array.from(this._selectedRows); Is there a reason not to just return the IndexSet? It's API seems fully usable for all the cases in this patch. If we have special model/container objects, I'd like to keep using them as much as possible instead of transforming them into more pliable formats when unnecessary. > LayoutTests/inspector/unit-tests/index-set.html:186 > + // a - 1 -> a While nice, I don't think this diagram is that useful when reading the test messages and code. > LayoutTests/inspector/unit-tests/index-set.html:207 > + // a - 1 -> NaN Ditto (186).
Comment on attachment 350155 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=350155&action=review >> Source/WebInspectorUI/UserInterface/Base/IndexSet.js:35 >> + let previous = NaN; > > Instead of using `previous`, you can just compare with `this._indexes.lastValue`, or even `this.lastIndex`. Good catch, will change. >> Source/WebInspectorUI/UserInterface/Base/IndexSet.js:36 >> + for (let value of values.slice().sort((a, b) => a - b)) { > > Were you going to remove the `.slice()`? I am not a fan of modifying function/constructor arguments, and sort mutates the array. >> Source/WebInspectorUI/UserInterface/Base/IndexSet.js:150 >> + throw new RangeError("Index cannot be negative."); > > My main objection to this is twofold: > 1. Throwing uncaught exceptions is very "dangerous" as any wrong move could potentially crash WebInspector, and there's always a chance that we miss a case where this happens > 2. Adding an "invalid" value to IndexSet isn't really something that would "break" WebInspector, and it's completely recoverable by just returning early (changing the function to return true/false) > I understand that neither of these *should* ever happen, but my fear is that we miss a small case somewhere and it ends up shipping such that anytime the user tries to do that specific action, WebInspector crashes with an uncaught exception. Furthermore, we follow the assert-and-bail pattern everywhere else in WebInspector, so introducing exceptions is something we'd definitely need to discuss/decide more in depth. Okay, I'll switch it to assert-and-bail. Your first objection is especially compelling. I also agree that more widespread adoption of exceptions would require some discussion. >> Source/WebInspectorUI/UserInterface/Views/NetworkTableContentView.js:352 >> + let rowIndex = table.selectedRow; > > If we're going to support multiple selection, I'd rather us go all-or-nothing and only have a `selectedRows` that returns an array. In the case where there is only one row selected, it would just be a 1-length array. Single-selection Tables will not be uncommon, and having this property is nicer than testing the length of the returned array. I'd like to leave it in for now. Also the selectedRow is not necessarily equivalent to selectedRows[0], for Tables where multiple selection is enabled. This distinction may not matter to Table clients, but at this stage I can't be 100% sure of that. >> Source/WebInspectorUI/UserInterface/Views/Table.js:90 >> + this._allowsMultipleSelection = false; > > Are there any current users of `WI.Table` that we don't want to have support for multi-selection? (I'm just asking, not suggesting anything) > > If not, is there a plan to eventually remove this member, since all it really does is control whether meta/shift click multi/extend selects more than one row? The Network table, for example, will not be supporting multiple selection, at least not right away. Table is also meant to be a general purpose. >> Source/WebInspectorUI/UserInterface/Views/Table.js:130 >> + return Array.from(this._selectedRows); > > Is there a reason not to just return the IndexSet? It's API seems fully usable for all the cases in this patch. If we have special model/container objects, I'd like to keep using them as much as possible instead of transforming them into more pliable formats when unnecessary. The IndexSet is an implementation detail of Table, and shouldn't be exposed to the outside world. This keeps the Table API simple -- you don't need to read IndexSet.js to discover how to use the returned rows. Clients will mostly want to iterate over rows or convert it to an array anyway; the IndexSet API is very special-purpose. >> LayoutTests/inspector/unit-tests/index-set.html:186 >> + // a - 1 -> a > > While nice, I don't think this diagram is that useful when reading the test messages and code. Will remove.
Created attachment 351031 [details] Patch
Comment on attachment 351031 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=351031&action=review I'd like to give this an r+, but I don't have a way of testing the functionality of this firsthand. Is there a followup I can apply right now that would let me test this? > Source/WebInspectorUI/UserInterface/Views/Table.css:129 > + border-top: 1px solid transparent; It doesn't look like you've addressed my previous comment: > This will make all of the table rows 1px taller. Does this affect the virtualization at all (e.g. `this._rowHeight`)? My suggestion is to save this height to a CSS variable and add it's numerical value to `this._rowHeight` in `WI.Table`s constructor. ```css --table-border-top-width: 1px; ``` ```js parseInt(window.getComputedStyle(this.element).getPropertyValue("--table-border-top-width")); ``` Alternatively, you could make all of this a JS applied style (at least just for `.table > .data-container > .data-list > li`). > Source/WebInspectorUI/UserInterface/Views/Table.js:1278 > + let extendSelection = event.metaKey && this._allowsMultipleSelection; NIT: I think we usually put the "flag" as the first item in a boolean series. this._allowsMultipleSelection && event.metaKey This way it's clearer for each boolean series as to what is the "flag" that it belongs to.
Comment on attachment 351031 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=351031&action=review To try out cmd-click multiple selection, you can apply https://bugs.webkit.org/show_bug.cgi?id=66381 on top of this. >> Source/WebInspectorUI/UserInterface/Views/Table.css:129 >> + border-top: 1px solid transparent; > > It doesn't look like you've addressed my previous comment: Oops, I had meant to comment on that. This doesn't affect the height of the row, since the li element has box-sizing: border-box. If it were content-box instead it would indeed be 21px instead of 20px. >> Source/WebInspectorUI/UserInterface/Views/Table.js:1278 >> + let extendSelection = event.metaKey && this._allowsMultipleSelection; > > NIT: I think we usually put the "flag" as the first item in a boolean series. > > this._allowsMultipleSelection && event.metaKey > > This way it's clearer for each boolean series as to what is the "flag" that it belongs to. Sure. Will change.
Created attachment 351544 [details] Patch
(In reply to Matt Baker from comment #24) > Created attachment 351544 [details] > Patch Also removed some dark mode styles that are no longer necessary now that https://webkit.org/b189766 has landed.
Comment on attachment 351544 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=351544&action=review > Source/WebInspectorUI/ChangeLog:51 > + Removed styles that are no longer needed after https://webkit.org/b189766. 404. Should be https://webkit.org/b/189766 > Source/WebInspectorUI/UserInterface/Views/Table.css:-183 > - .table, > - .table > .header { > - background: var(--background-color); > - } > - This looks good. I forgot to remove it.
Comment on attachment 351544 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=351544&action=review > Source/WebInspectorUI/UserInterface/Views/Table.css:129 > + border-top: 1px solid transparent; While this doesn't effect the sizing of the rows, it causes the text to be pushed down by 1px. If we want to match the system, what we really should do is insert spacer elements between each row of 1px height and adjust the virtualization code to take that into account (1px * (NumRowsShowing - 1)).
Created attachment 351560 [details] Patch
Comment on attachment 351560 [details] Patch r=me. Nice work!
Created attachment 351563 [details] Patch
Nice! Looks good to me as well. It sounds like Devin reviewed an earlier version of the patch.
(In reply to Joseph Pecoraro from comment #31) > Nice! Looks good to me as well. It sounds like Devin reviewed an earlier > version of the patch. It's ready for his r+/cq+. The only change since he last reviewed was to simplify the CSS used to create the 1px dividers between adjacent selected rows.
Created attachment 351621 [details] Patch for landing
Comment on attachment 351621 [details] Patch for landing Clearing flags on attachment: 351621 Committed r236853: <https://trac.webkit.org/changeset/236853>
All reviewed patches have been landed. Closing bug.