Bug 189718

<rdar://problem/44577942>

Created attachment 353004 [details] Patch

(In reply to Matt Baker from comment #2) > Created attachment 353004 [details] > Patch WIP: Adds support for Shift-clicking a range of rows to add to the selection. It does not add Shift-arrow key handling.

Created attachment 353007 [details] Patch

Comment on attachment 353007 [details] Patch Attachment 353007 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/9711142 New failing tests: inspector/unit-tests/index-set.html

Created attachment 353010 [details] Archive of layout-test-results from ews102 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews102 Port: mac-sierra Platform: Mac OS X 10.12.6

Comment on attachment 353007 [details] Patch Attachment 353007 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/9711163 New failing tests: inspector/unit-tests/index-set.html

Created attachment 353012 [details] Archive of layout-test-results from ews105 for mac-sierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews105 Port: mac-sierra-wk2 Platform: Mac OS X 10.12.6

Comment on attachment 353007 [details] Patch Attachment 353007 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/9711215 New failing tests: inspector/unit-tests/index-set.html

Created attachment 353014 [details] Archive of layout-test-results from ews113 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews113 Port: mac-sierra Platform: Mac OS X 10.12.6

Created attachment 353753 [details] Patch

In Finder, Command has a precedence over Shift - Command-Shift click behaves as Command click. In your patch, it's the other way around. I'm reviewing the rest of the patch.

(In reply to Nikita Vasilyev from comment #12) > In Finder, Command has a precedence over Shift - Command-Shift click behaves > as Command click. In your patch, it's the other way around. We definitely want to match the system behavior. I'll address this in the next rev.

Comment on attachment 353753 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=353753&action=review Looks good! > Source/WebInspectorUI/UserInterface/Views/Table.js:94 > + this._shiftExtendAnchorIndex = NaN; Nit: _anchorIndex sounds descriptive enough to me. > Source/WebInspectorUI/UserInterface/Views/Table.js:1366 > this.deselectRow(rowIndex) Nit: missing a semicolon.

Comment on attachment 353753 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=353753&action=review > Source/WebInspectorUI/UserInterface/Views/Table.js:1345 > + _handleKeyUp(event) > + { > + if (event.keyIdentifier === "Shift") > + this._shiftExtendAnchorIndex = NaN; > + } Try this: Press Shift Click row #2 Click row #3 Release Shift Press Shift Click row #1 With this patch: rows 1-3 are selected. In Finder: rows 1-2 are selected. This is because Finder doesn't clear anchorIndex when the Shift key is released.

(In reply to Nikita Vasilyev from comment #15) > Comment on attachment 353753 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=353753&action=review > > > Source/WebInspectorUI/UserInterface/Views/Table.js:1345 > > + _handleKeyUp(event) > > + { > > + if (event.keyIdentifier === "Shift") > > + this._shiftExtendAnchorIndex = NaN; > > + } > > Try this: > > Press Shift > Click row #2 > Click row #3 > Release Shift > Press Shift > Click row #1 > > With this patch: rows 1-3 are selected. > > In Finder: rows 1-2 are selected. This is because Finder doesn't clear > anchorIndex when the Shift key is released. Interesting! I'll make this change, and rename _shiftExtendAnchorIndex to _anchorRowIndex, which pairs better with _selectedRowIndex.

Created attachment 353938 [details] Patch

(In reply to Matt Baker from comment #17) > Created attachment 353938 [details] > Patch Looks good! I'd r=me'd if I could :)

(In reply to Nikita Vasilyev from comment #18) > (In reply to Matt Baker from comment #17) > > Created attachment 353938 [details] > > Patch > > Looks good! I'd r=me'd if I could :) Thanks! I’ll ping Devin for a second pass.

Comment on attachment 353938 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=353938&action=review r- due to the fact that shift-selection doesn't work when there is no previous selection Doing this in Finder selects from the first item to wherever you clicked, which is (I guess) what you're trying to match. > Source/WebInspectorUI/UserInterface/Base/IndexSet.js:96 > + this._validateIndex(startIndex); Should this return if this validation fails? > Source/WebInspectorUI/UserInterface/Base/IndexSet.js:99 > + if (!count) NIT: in order to prevent an infinite loop, you should make this `count <= 0`, not just that it's falsy > Source/WebInspectorUI/UserInterface/Base/IndexSet.js:123 > + this._validateIndex(startIndex); Ditto (96) > Source/WebInspectorUI/UserInterface/Base/IndexSet.js:126 > + if (!count) Ditto (99) > Source/WebInspectorUI/UserInterface/Views/Table.js:90 > + this._anchorRowIndex = NaN; So, assuming I am understanding this correctly, this variable holds the last specifically selected index before the most recent shift-selection? In that case, this needs a much more specific name, as I spent a good amount of time trying to figure out how it played into everything (e.g. I was wondering why it's set to `NaN` inside `selectRow` when `_selectedRowIndex` isn't). > Source/WebInspectorUI/UserInterface/Views/Table.js:233 > + if (this._selectedRows.size) { Not that I have anything against this change, but is there a reason for it (other than avoiding work when unnecessary)? > Source/WebInspectorUI/UserInterface/Views/Table.js:1288 > + if (!this.numberOfRows) This should be the first early return, not the last. > Source/WebInspectorUI/UserInterface/Views/Table.js:1307 > + _selectRowsFromArrowKey(goingUp, shiftKey) NIT: `goingUp` is only used once, so I'd rather you call it `direction` and check if it's equal to "up" or "down", rather than pass a boolean NIT: considering that this only deals with up/down, this might be better named as `_selectRowsFromVerticalArrowKey` (if we ever add nesting to `WI.Table` in the future, left/right might also need to be supported) > Source/WebInspectorUI/UserInterface/Views/Table.js:1324 > + let priorRowIndex = this._selectedRowIndex - rowIncrement; This seems wrong. If I've selected the first row and try to move down, there is no `priorRowIndex` as it'd be `-1`. I think we just want to check whether the `_selectedRowIndex` is selected or not, and base our determination off of that. > Source/WebInspectorUI/UserInterface/Views/Table.js:1377 > + function normalizeRangeExcludingAnchor(anchorIndex, rowIndex) { NIT: `anchorIndex` is always passed in as `_anchorRowIndex`, so why not just make this an arrow function and use that instead? > Source/WebInspectorUI/UserInterface/Views/Table.js:1380 > + let startIndex; > + let endIndex; I tend to prefer to always giving variables a starting value (in this case, probably NaN) > Source/WebInspectorUI/UserInterface/Views/Table.js:1400 > + if (isNaN(this._anchorRowIndex)) > + this._anchorRowIndex = this._selectedRowIndex; This doesn't handle the case that we shift-select before anything has been selected. If that's not covered by this patch, a FIXME should be added, as well as an early return somewhere. > Source/WebInspectorUI/UserInterface/Views/Table.js:1412 > + this._selectedRows.addRange(startIndex, 1 + endIndex - startIndex); Style: `endIndex - startIndex + 1` > Source/WebInspectorUI/UserInterface/Views/Table.js:1417 > + this._notifySelectionDidChange(); Should this also be called when the shift-selection goes back go `_anchorRowIndex`? 1. Select row 2 2. Shift-Select row 4 (2-4 selected) 3. Shift-Select row 2 (2 selected) => I'd expect a "selection changed" event to be fired > LayoutTests/inspector/unit-tests/index-set-expected.txt:79 > +Given an IndexSet with values [10,11,12]: Style: these could use newlines after each sub-test for readability > LayoutTests/inspector/unit-tests/index-set.html:204 > + } This test is missing a `return true` > LayoutTests/inspector/unit-tests/index-set.html:207 > + function rangeToArray(startIndex, count) { Style: we usually put utility functions before the suite (top of `test` function) > LayoutTests/inspector/unit-tests/index-set.html:258 > + description: "Add range overlapping the middle.", Can you add a version of this test for `remove` too? > LayoutTests/inspector/unit-tests/index-set.html:343 > + } Ditto (204)

Comment on attachment 353938 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=353938&action=review >> Source/WebInspectorUI/UserInterface/Base/IndexSet.js:96 >> + this._validateIndex(startIndex); > > Should this return if this validation fails? Yep. >> Source/WebInspectorUI/UserInterface/Base/IndexSet.js:99 >> + if (!count) > > NIT: in order to prevent an infinite loop, you should make this `count <= 0`, not just that it's falsy It could be even more robust as `!(count > 0`), which I kind of like. >> Source/WebInspectorUI/UserInterface/Views/Table.js:90 >> + this._anchorRowIndex = NaN; > > So, assuming I am understanding this correctly, this variable holds the last specifically selected index before the most recent shift-selection? In that case, this needs a much more specific name, as I spent a good amount of time trying to figure out how it played into everything (e.g. I was wondering why it's set to `NaN` inside `selectRow` when `_selectedRowIndex` isn't). That's correct. That index has special meaning for future shift-click operations (the operation pivots around, or begins from, the anchor), and this lasts until the next selection operation that doesn't use shift. Originally it was named `_shiftExtendAnchorIndex`, but that was even worse. Another thing to keep in mind is this variable will be moved out of Table, into a helper class that will be used by both Table and TreeOutline. So having "row" in the name won't make sense in that context. What about `_shiftSelectAnchorIndex` or just `_shiftAnchorIndex`? Either way it probably deserves an explanatory comment at the declaration site. >> Source/WebInspectorUI/UserInterface/Views/Table.js:233 >> + if (this._selectedRows.size) { > > Not that I have anything against this change, but is there a reason for it (other than avoiding work when unnecessary)? I considered removing this step altogether, since the typical usage will be to set `allowsMultipleSelection` once and forget it. But I think doing something sensible here is worthwhile, to keep the class internally consistent. That said this looks wrong. Even in single selection mode, the following should be true: `this._selectedRowIndex === this._selectedRows.firstIndex`. >> Source/WebInspectorUI/UserInterface/Views/Table.js:1288 >> + if (!this.numberOfRows) > > This should be the first early return, not the last. Seems sensible. >> Source/WebInspectorUI/UserInterface/Views/Table.js:1307 >> + _selectRowsFromArrowKey(goingUp, shiftKey) > > NIT: `goingUp` is only used once, so I'd rather you call it `direction` and check if it's equal to "up" or "down", rather than pass a boolean > NIT: considering that this only deals with up/down, this might be better named as `_selectRowsFromVerticalArrowKey` (if we ever add nesting to `WI.Table` in the future, left/right might also need to be supported) - I like passing a bool, so that this function doesn't deal with event keyIdentifier strings. That should stay in the event handler. - If we add hierarchical navigation to Table, I'll consider renaming this. I like the name for now. >> Source/WebInspectorUI/UserInterface/Views/Table.js:1324 >> + let priorRowIndex = this._selectedRowIndex - rowIncrement; > > This seems wrong. If I've selected the first row and try to move down, there is no `priorRowIndex` as it'd be `-1`. I think we just want to check whether the `_selectedRowIndex` is selected or not, and base our determination off of that. If the first row is selected and shift-down arrow is pressed, this test will fail and the selection is being extended. You can't determine whether you are increasing or decreasing the selection without checking the `priorRowIndex`. >> Source/WebInspectorUI/UserInterface/Views/Table.js:1377 >> + function normalizeRangeExcludingAnchor(anchorIndex, rowIndex) { > > NIT: `anchorIndex` is always passed in as `_anchorRowIndex`, so why not just make this an arrow function and use that instead? So it is! Will change. >> Source/WebInspectorUI/UserInterface/Views/Table.js:1412 >> + this._selectedRows.addRange(startIndex, 1 + endIndex - startIndex); > > Style: `endIndex - startIndex + 1` Nice. >> Source/WebInspectorUI/UserInterface/Views/Table.js:1417 >> + this._notifySelectionDidChange(); > > Should this also be called when the shift-selection goes back go `_anchorRowIndex`? > > 1. Select row 2 > 2. Shift-Select row 4 (2-4 selected) > 3. Shift-Select row 2 (2 selected) > => I'd expect a "selection changed" event to be fired It should, good catch.

Created attachment 354258 [details] Patch

Comment on attachment 354258 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=354258&action=review > Source/WebInspectorUI/UserInterface/Base/IndexSet.js:100 > + if (!(count > 0)) Style: this should be `count <= 0`. The only time I ever think we should use `!(...)` is for `instanceof` and `in` checks. > Source/WebInspectorUI/UserInterface/Base/IndexSet.js:128 > + if (!(count > 0)) Ditto (100) > Source/WebInspectorUI/UserInterface/Views/Table.js:371 > + this._shiftAnchorIndex = NaN; It looks like Finder tries to reset its "anchor" to the closest index nearby. Not sure if we want to mirror that, but it's something to consider. > Source/WebInspectorUI/UserInterface/Views/Table.js:1329 > + if (!this.isRowSelected(priorRowIndex)) { If `_selectedRowIndex` is 0 and `rowIncrement` is 1, then `priorRowIndex` will be -1, which can never be selected, meaning that `isRowSelected` will return false, and `_selectedRowIndex` will be deselected. We should just check to see if `_selectedRowIndex` is selected. > Source/WebInspectorUI/UserInterface/Views/Table.js:1381 > + // FIXMNE: <https://webkit.org/b/191429> Web Inspector: Table with no selection should select rows on shift-click Typo: FIXME > Source/WebInspectorUI/UserInterface/Views/Table.js:1388 > + console.assert(this._shiftAnchorIndex !== rowIndex); You should add an early return in the case that `event.shiftKey && this.isRowSelected(this._selectedRowIndex)` for the case that the user shift-clicks on the same row multiple times in a row.

Comment on attachment 354258 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=354258&action=review >> Source/WebInspectorUI/UserInterface/Views/Table.js:371 >> + this._shiftAnchorIndex = NaN; > > It looks like Finder tries to reset its "anchor" to the closest index nearby. Not sure if we want to mirror that, but it's something to consider. Interesting. Will consider for a follow-up. I don't want to get too far into the weeds handling all these edge cases (there are probably more too) >> Source/WebInspectorUI/UserInterface/Views/Table.js:1329 >> + if (!this.isRowSelected(priorRowIndex)) { > > If `_selectedRowIndex` is 0 and `rowIncrement` is 1, then `priorRowIndex` will be -1, which can never be selected, meaning that `isRowSelected` will return false, and `_selectedRowIndex` will be deselected. We should just check to see if `_selectedRowIndex` is selected. True. But if `_selectedRowIndex` is 0 and `rowIncrement` is 1, then the branch above (line 1320) is taken, so `priorRowIndex` will never have this value.

I found a bug in _handleMouseDown. It's an ugly method, so I'm going to try cleaning it up now rather than later.

Created attachment 354623 [details] Patch

(In reply to Matt Baker from comment #26) > Created attachment 354623 [details] > Patch The improved _handleMouseDown handles the behavior described here: https://bugs.webkit.org/show_bug.cgi?id=191429. I will close that bug.

Comment on attachment 354623 [details] Patch Attachment 354623 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/9967006 New failing tests: inspector/unit-tests/index-set.html

Created attachment 354630 [details] Archive of layout-test-results from ews101 for mac-sierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-sierra Platform: Mac OS X 10.12.6

Created attachment 354633 [details] Patch

Comment on attachment 354623 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=354623&action=review r=me, please wait for EWS > Source/WebInspectorUI/UserInterface/Base/IndexSet.js:112 > + if (!this.size || (this.firstIndex >= range[0] && this.lastIndex <= range.lastValue)) { It's times like these that I wish we had a `firstIndex` too, but I also realize how completely unnecessary that would be :| > Source/WebInspectorUI/UserInterface/Base/IndexSet.js:140 > + if (startIndex <= this.firstIndex && lastIndex >= this.lastIndex) { Style: this is the reverse order of `addRange` (e.g. whether you use `this._<blah>` on the left or the right) > Source/WebInspectorUI/UserInterface/Base/IndexSet.js:174 > + difference(indexSet) NIT: I'd personally call this `subtract` > Source/WebInspectorUI/UserInterface/Base/IndexSet.js:179 > + return []; NIT: should this be `return new IndexSet` to match the return type of the other `return`? > Source/WebInspectorUI/UserInterface/Views/Table.js:232 > Style: remove newline > Source/WebInspectorUI/UserInterface/Views/Table.js:1288 > + if (event.metaKey || event.ctrlKey) `event.altKey`? > Source/WebInspectorUI/UserInterface/Views/Table.js:1370 > + if (event.metaKey) { This should be `event.ctrlKey` on windows. I think @Nikita added an `Event.prototype.commandOrControl`? > Source/WebInspectorUI/UserInterface/Views/Table.js:1388 > + if (isNaN(this._selectedRowIndex)) { Alternatively, you could check/assert that `newSelectedRows` is empty > Source/WebInspectorUI/UserInterface/Views/Table.js:1415 > + newSelectedRows.addRange(startIndex, endIndex - startIndex + 1); It's cases like this that make me consider whether `addRange` should take a `endIndex` instead of `count` 🤔 > Source/WebInspectorUI/UserInterface/Views/Table.js:1592 > + row.classList.toggle("selected", flag); NIT: due to the nature of `undefined`, I always add `!!` before the flag argument for `classList.toggle` (passing `undefined` makes `classList.toggle` act like a flip-flop) > LayoutTests/inspector/unit-tests/index-set.html:361 > + name: "IndexSet.prototype.equals", You're missing the expected results for this > LayoutTests/inspector/unit-tests/index-set.html:376 > + name: "IndexSet.prototype.difference", Ditto (361)

Comment on attachment 354633 [details] Patch Oh, looks like you already uploaded a new patch with the results. r=me, please wait for EWS See comments on attachment 354623 [details]

Created attachment 354638 [details] Patch for landing

Comment on attachment 354623 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=354623&action=review >> Source/WebInspectorUI/UserInterface/Base/IndexSet.js:174 >> + difference(indexSet) > > NIT: I'd personally call this `subtract` I wanted the name to match the standard set operation. >> Source/WebInspectorUI/UserInterface/Base/IndexSet.js:179 >> + return []; > > NIT: should this be `return new IndexSet` to match the return type of the other `return`? Oops! >> Source/WebInspectorUI/UserInterface/Views/Table.js:1288 >> + if (event.metaKey || event.ctrlKey) > > `event.altKey`? We didn't check `event.altKey` previously, so I left it out. Also, we may want to add special handling in the future: Web Inspector: Table should select the first/last row when pressing option-up/down arrow https://bugs.webkit.org/show_bug.cgi?id=191576

Comment on attachment 354638 [details] Patch for landing Clearing flags on attachment: 354638 Committed r238121: <https://trac.webkit.org/changeset/238121>

All reviewed patches have been landed. Closing bug.