Bug 189718 – Web Inspector: Table should support shift-extending the row selection

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Matt Baker]

Summary:
Table should support shift-extending the row selection.

This patch will add Shift-click and Shift-arrow-key support for extending the selection.

[Radar WebKit Bug Importer]

<rdar://problem/44577942>

[Matt Baker]

Created attachment 353004 [details]
Patch

[Matt Baker]

(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.

[Matt Baker]

Created attachment 353007 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Matt Baker]

Created attachment 353753 [details]
Patch

[Nikita Vasilyev]

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.

[Matt Baker]

(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.

[Nikita Vasilyev]

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.

[Nikita Vasilyev]

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.

[Matt Baker]

(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.

[Matt Baker]

Created attachment 353938 [details]
Patch

[Nikita Vasilyev]

(In reply to Matt Baker from comment #17)
> Created attachment 353938 [details]
> Patch

Looks good! I'd r=me'd if I could :)

[Matt Baker]

(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.

[Devin Rousso]

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)

[Matt Baker]

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.

[Matt Baker]

Created attachment 354258 [details]
Patch

[Devin Rousso]

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.

[Matt Baker]

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.

[Matt Baker]

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

[Matt Baker]

Created attachment 354623 [details]
Patch

[Matt Baker]

(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.

[EWS Watchlist]

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

[EWS Watchlist]

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

[Matt Baker]

Created attachment 354633 [details]
Patch

[Devin Rousso]

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)

[Devin Rousso]

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]

[Matt Baker]

Created attachment 354638 [details]
Patch for landing

[Matt Baker]

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

[WebKit Commit Bot]

Comment on attachment 354638 [details]
Patch for landing

Clearing flags on attachment: 354638

Committed r238121: <https://trac.webkit.org/changeset/238121>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.