Bug 192681

Summary: Web Inspector: Elements tab: multiple selection lost after navigating to another tab
Product: WebKit Reporter: Matt Baker <mattbaker>
Component: Web InspectorAssignee: Matt Baker <mattbaker>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, hi, inspector-bugzilla-changes, nvasilyev, timothy, webkit-bug-importer
Priority: P3 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Matt Baker 2018-12-13 14:23:29 PST
Summary:
Multiple selection lost after navigating to another tab.

Steps to Reproduce:
1. Open Inspector > Elements tab
2. Select more than one DOM node
3. Select another tab
4. Return to Elements tab
  => Multiple selection is changed to single selection
Comment 1 Radar WebKit Bug Importer 2018-12-13 14:23:55 PST
<rdar://problem/46709392>
Comment 2 Matt Baker 2019-02-20 14:06:21 PST
Created attachment 362538 [details]
Patch
Comment 3 Nikita Vasilyev 2019-02-20 15:48:18 PST
Comment on attachment 362538 [details]
Patch

Looks good to me. However, I'm not very familiar with intricacies of SelectionController. Even if I was a reviewer, I'd suggest somebody else to review as well.

View in context: https://bugs.webkit.org/attachment.cgi?id=362538&action=review

> Source/WebInspectorUI/UserInterface/Base/Utilities.js:195
> +        let iterator = this.values();
> +        let result = iterator.next().value;
> +        for (let value of iterator)

I was going to suggest to convert it to an array before iterating to make it faster, but it doesn't really matter any longer. Iterators are pretty fast nowadays.

https://jsperf.com/set-iterator-vs-foreach/4

> Source/WebInspectorUI/UserInterface/Controllers/SelectionController.js:106
> +    selectItems(items, extendSelection = false)

Is `extendSelection` currently always false? If so, I'd prefer to remove it until we use it.
Comment 4 Matt Baker 2019-02-20 16:09:13 PST
Comment on attachment 362538 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=362538&action=review

>> Source/WebInspectorUI/UserInterface/Controllers/SelectionController.js:106
>> +    selectItems(items, extendSelection = false)
> 
> Is `extendSelection` currently always false? If so, I'd prefer to remove it until we use it.

Initially I included this parameter in TreeOutline.prototype.selectTreeElements(), but then removed it to limit the scope of this patch since it wasn't used. Will remove.
Comment 5 Matt Baker 2019-02-20 16:13:36 PST
Created attachment 362563 [details]
Patch
Comment 6 Devin Rousso 2019-02-25 17:22:40 PST
Comment on attachment 362563 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=362563&action=review

> Source/WebInspectorUI/ChangeLog:11
> +        (value.defaultComparator):
> +        (value):

These ChangeLog function entires are quite useless :|
Either remove them or replace them with `Set.prototype.maxValue` and `Set.prototype.union` entries.

> Source/WebInspectorUI/ChangeLog:27
> +        * UserInterface/Test.html:
> +        New files for testing.

NIT: I'd put this at the end of the ChangeLog, as it's not very related to the main functionality of this patch/fix.

> Source/WebInspectorUI/UserInterface/Base/Utilities.js:195
> +        for (let value of iterator)

Style: missing braces.

> Source/WebInspectorUI/UserInterface/Base/Utilities.js:207
> +        let result = new Set(other);

Why use `other` and not `this`?

> Source/WebInspectorUI/UserInterface/Base/Utilities.js:212
> +

Style: unnecessary newline.

> Source/WebInspectorUI/UserInterface/Views/DOMTreeOutline.js:192
> +                let domNode = oldTreeElement.representedObject;

NIT: you could inline this.

> Source/WebInspectorUI/UserInterface/Views/DOMTreeOutline.js:195
> +                    treeElement = treeElement.children.lastValue;

This is a scary assumption to make.  Why don't we pass the actual `WI.DOMTreeElement` as `elementCloseTag` instead of just a true/false?  That way, we can retrieve it directly here (e.g. `treeElement.associatedCloseTagTreeElement`).

> Source/WebInspectorUI/UserInterface/Views/DOMTreeOutline.js:199
> +            this.selectTreeElements(selectedTreeElements);

Will this interfere with the selection set by `_revealAndSelectNode` above?  Should we reverse the order of the two, and maybe filter out `selectedNode` from the list of `selectedTreeElements`?

> LayoutTests/inspector/unit-tests/set-utilities.html:142
> +        name: "Set.prototype.maxValue",

We should also test `Set.prototype.union`.
Comment 7 Matt Baker 2019-02-26 09:33:12 PST
Comment on attachment 362563 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=362563&action=review

>> Source/WebInspectorUI/ChangeLog:11
>> +        (value):
> 
> These ChangeLog function entires are quite useless :|
> Either remove them or replace them with `Set.prototype.maxValue` and `Set.prototype.union` entries.

I agree, but am not interested in playing games with our changelog generator,

>> Source/WebInspectorUI/ChangeLog:27
>> +        New files for testing.
> 
> NIT: I'd put this at the end of the ChangeLog, as it's not very related to the main functionality of this patch/fix.

Reordering change log entries isn't a good use of time. Inevitably people will disagree over what the most appropriate order is, and pretty soon we'll be peppering these entries with nits requesting they be reordered. I really don't want to go there. I'll just drop the comment.

>> Source/WebInspectorUI/UserInterface/Base/Utilities.js:195
>> +        for (let value of iterator)
> 
> Style: missing braces.

Oops!

>> Source/WebInspectorUI/UserInterface/Base/Utilities.js:207
>> +        let result = new Set(other);
> 
> Why use `other` and not `this`?

Why use `this` and not `other`?

>> Source/WebInspectorUI/UserInterface/Views/DOMTreeOutline.js:195
>> +                    treeElement = treeElement.children.lastValue;
> 
> This is a scary assumption to make.  Why don't we pass the actual `WI.DOMTreeElement` as `elementCloseTag` instead of just a true/false?  That way, we can retrieve it directly here (e.g. `treeElement.associatedCloseTagTreeElement`).

Nice idea. Will change.

>> Source/WebInspectorUI/UserInterface/Views/DOMTreeOutline.js:199
>> +            this.selectTreeElements(selectedTreeElements);
> 
> Will this interfere with the selection set by `_revealAndSelectNode` above?  Should we reverse the order of the two, and maybe filter out `selectedNode` from the list of `selectedTreeElements`?

Actually reversing the order will cause it to break:

1. Shift select some nodes, in a bottom-to-top direction. For example:

<div>
    <p></p>    <-- Then shift-click to extend the selection to this node (2)
    <p></p>
    <p></p>    <-- Click to select this node first (1)
</div>

2. The topmost (2) selected item's WI.DOMNode is shown in the details sidebar
3. Change to a different tab, then back to Elements
=> Selection restored, but the WI.DOMNode for the bottommost selected item (1) is now shown in the sidebar

I'll add a comment, or find a way to rewrite it to not depend on this subtlety.
Comment 8 Matt Baker 2019-02-26 09:54:06 PST
Comment on attachment 362563 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=362563&action=review

>> Source/WebInspectorUI/UserInterface/Base/Utilities.js:212
>> +
> 
> Style: unnecessary newline.

I wanted to be consistent with the surrounding code. I wish we had better guidelines for these cases. For example, why should return be on its own line in `maxValue` above, but not here? It feels very arbitrary.
Comment 9 Devin Rousso 2019-02-26 09:59:31 PST
Comment on attachment 362563 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=362563&action=review

>>> Source/WebInspectorUI/UserInterface/Base/Utilities.js:207
>>> +        let result = new Set(other);
>> 
>> Why use `other` and not `this`?
> 
> Why use `this` and not `other`?

Touche.  The other `Set.prototype` functions follow that pattern.
Comment 10 Devin Rousso 2019-03-01 15:53:40 PST
Comment on attachment 362563 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=362563&action=review

>>> Source/WebInspectorUI/UserInterface/Views/DOMTreeOutline.js:195
>>> +                    treeElement = treeElement.children.lastValue;
>> 
>> This is a scary assumption to make.  Why don't we pass the actual `WI.DOMTreeElement` as `elementCloseTag` instead of just a true/false?  That way, we can retrieve it directly here (e.g. `treeElement.associatedCloseTagTreeElement`).
> 
> Nice idea. Will change.

r-, as it seems like you're going to make some changes
Comment 11 Matt Baker 2019-03-06 16:23:23 PST
Created attachment 363813 [details]
Patch
Comment 12 Devin Rousso 2019-03-06 17:42:13 PST
Comment on attachment 363813 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=363813&action=review

> Source/WebInspectorUI/UserInterface/Base/Utilities.js:195
> +        for (let value of iterator)

Style: missing braces.

> Source/WebInspectorUI/UserInterface/Controllers/SelectionController.js:120
> +        this._shiftAnchorItem = null;

Looking at it more, I think this may introduce unexpected behavior, as it would change the `_shiftAnchorItem`.  If I select item 1, shift-click to item 4, the `_lastSelectedItem` would be item 1 and the `_shiftAnchorItem` would be also be item 1.  This could would clear `_shiftAnchorItem`, even though it would keep `_lastSelectedItem` the same.

> Source/WebInspectorUI/UserInterface/Views/DOMTreeElement.js:535
> +            this._closeTagTreeElement = this.insertChildElement(this.representedObject, this.children.length, true);

While you're making this change, we could also introduce an `openTagTreeElement` and have a link in both directions (it would involve passing `this` instead of `true` as the last argument).

> Source/WebInspectorUI/UserInterface/Views/DOMTreeOutline.js:202
> +            this.selectTreeElements(selectedTreeElements);

You said you were going to add a comment explaining why this must be called in this particular order?

> LayoutTests/inspector/unit-tests/set-utilities.html:125
> +            return true;

NIT: in a sync test suite, the return is unnecessary (unless you want to fail the test case and stop running the other test cases).
Comment 13 Matt Baker 2019-03-25 17:56:33 PDT
Comment on attachment 363813 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=363813&action=review

>> Source/WebInspectorUI/UserInterface/Controllers/SelectionController.js:120
>> +        this._shiftAnchorItem = null;
> 
> Looking at it more, I think this may introduce unexpected behavior, as it would change the `_shiftAnchorItem`.  If I select item 1, shift-click to item 4, the `_lastSelectedItem` would be item 1 and the `_shiftAnchorItem` would be also be item 1.  This could would clear `_shiftAnchorItem`, even though it would keep `_lastSelectedItem` the same.

This method isn't called when shift-clicking. That's done by handleItemMouseDown, which calls _updateSelection directly when selecting multiple items. In the future we might want to retain the anchor when programmatically selecting items, but let's wait until the need arises.

>> Source/WebInspectorUI/UserInterface/Views/DOMTreeOutline.js:202
>> +            this.selectTreeElements(selectedTreeElements);
> 
> You said you were going to add a comment explaining why this must be called in this particular order?

I'm going to try refactoring this slightly to eliminate the need for a comment.
Comment 14 Matt Baker 2019-03-25 21:37:26 PDT
Created attachment 365941 [details]
Patch
Comment 15 Matt Baker 2019-03-25 21:48:24 PDT
Created attachment 365945 [details]
Patch
Comment 16 Matt Baker 2019-03-25 21:49:35 PDT
(In reply to Matt Baker from comment #15)
> Created attachment 365945 [details]
> Patch

I simplified SelectionController.selectItem, but need to make another change for this to work correctly.
Comment 17 Matt Baker 2019-03-27 01:12:01 PDT
Created attachment 366063 [details]
Patch
Comment 18 Matt Baker 2019-03-27 10:43:40 PDT
A few notes on the latest patch:

- Removed Set utility methods maxValue and union. No longer used.
- Added Set utility method lastValue.
- SelectionController.prototype.selectItems will set _lastSelectedItem to the last item being selected. SelectionController tracks its selected items in the order in which they are selected:

    let sc = new SelectionController(...);
    let items = sc.selectedItems;
    let lastSelectedItem = sc.lastSelectedItem;

    console.assert(items.lastValue === lastSelectedItem);
Comment 19 Devin Rousso 2019-03-29 13:52:06 PDT
Comment on attachment 363813 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=363813&action=review

>>> Source/WebInspectorUI/UserInterface/Controllers/SelectionController.js:120
>>> +        this._shiftAnchorItem = null;
>> 
>> Looking at it more, I think this may introduce unexpected behavior, as it would change the `_shiftAnchorItem`.  If I select item 1, shift-click to item 4, the `_lastSelectedItem` would be item 1 and the `_shiftAnchorItem` would be also be item 1.  This could would clear `_shiftAnchorItem`, even though it would keep `_lastSelectedItem` the same.
> 
> This method isn't called when shift-clicking. That's done by handleItemMouseDown, which calls _updateSelection directly when selecting multiple items. In the future we might want to retain the anchor when programmatically selecting items, but let's wait until the need arises.

I understand that this path isn't called by shift-clicking, but it may be called after `_shiftAnchorItem` was set by a previous shift-click.  As an example, if I shift-click a few nodes, delete one of them via JavaScript (e.g. `node.remove()`), and then try to shift-click again, I'd experience unexpected behavior.
Comment 20 Devin Rousso 2019-03-29 14:12:03 PDT
Comment on attachment 366063 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=366063&action=review

> Source/WebInspectorUI/UserInterface/Base/Utilities.js:176
> +        let iterator = this.values();
> +        for (let i = 0; i < this.size - 1; ++i)
> +            iterator.next();
> +        return iterator.next().value;

I think it would be "cleaner" to just use `return Array.from(this).lastValue`.  It effectively does the same logic, albeit in a slightly different way (it's also much simpler).

> Source/WebInspectorUI/UserInterface/Views/DOMTreeElement.js:540
> +            this._closeTagTreeElement = this.insertChildElement(this.representedObject, this.children.length, true);

We should clear this at the beginning of this function.  If we change node type (e.g. Edit > Tag), we'd want to make sure that this isn't incorrect (or keeping some other object alive).

> LayoutTests/inspector/tree-outline/tree-outline-selection.html:24
> +                addObject(treeElement, child);

`appendObject`?

> LayoutTests/inspector/tree-outline/tree-outline-selection.html:35
> +        InspectorTest.expectEqual(treeOutline.selectedTreeElement, treeElement, `TreeOutline should have selected TreeElement "${treeElement.title}".`);

We  should also `assert`/`expectEqual` that the size of `selectedTreeElements` (or even the actual values) matches what we'd expect.

> LayoutTests/inspector/tree-outline/tree-outline-selection.html:50
> +        name: "TreeOutline.FindTreeElement",

What about a negative case?  Where a `representedObject` doesn't exist?

> LayoutTests/inspector/tree-outline/tree-outline-selection.html:51
> +        description: "Find TreeElement for given represented object.",

This isn't a very good description :(

> LayoutTests/inspector/tree-outline/tree-outline-selection.html:69
> +        description: "Select a tree element, then select a different tree element.",

Ditto (>51).

> LayoutTests/inspector/tree-outline/tree-outline-selection.html:76
> +                InspectorTest.expectThat(treeElement.selected, `TreeElement "${treeElement.title}" should have selected = true.`);

Rather than use "selected = false", how about just "should be selected"?

> LayoutTests/inspector/tree-outline/tree-outline-selection.html:79
> +                InspectorTest.expectThat(otherTreeElementsAreDeselected, "Other TreeElements should have selected = false.");

Ditto (>76).

> LayoutTests/inspector/tree-outline/tree-outline-selection.html:95
> +        name: "TreeOutline.AllowsMultipleSelection",

What about a test for what happens when we have a selection and change `allowsMultipleSelection` (e.g. turning it off should "clamp" the selection to just one item).

> LayoutTests/inspector/tree-outline/tree-outline-selection.html:107
> +    function triggerSelectTreeElements(treeOutline, treeElements) {

Please put this before any test cases, as otherwise it's harder to find/distinguish.

> LayoutTests/inspector/tree-outline/tree-outline-selection.html:108
> +        let displayText = `${treeElements.map((x) => `"${x.title}"`)}`;

This is an odd way of getting a stringified array.  Please use `JSON.stringify` instead.

> LayoutTests/inspector/tree-outline/tree-outline-selection.html:110
> +        InspectorTest.log(`Selecting TreeElements [${displayText}].`);

NIT: we normally put "..." at the end of any log before an action.

> LayoutTests/inspector/tree-outline/tree-outline-selection.html:115
> +        let treeElementsSelected = treeElements.every((x) => x.selected);

I'd inline this.

> LayoutTests/inspector/tree-outline/tree-outline-selection.html:116
> +        InspectorTest.expectThat(treeElementsSelected, `TreeElements [${displayText}] should have selected = true.`);

Ditto (>76).

> LayoutTests/inspector/tree-outline/tree-outline-selection.html:118
> +        let otherTreeElementsNotSelected = treeOutline.children.every((x) => !x.selected || treeElements.includes(x));

Ditto (>115).

> LayoutTests/inspector/tree-outline/tree-outline-selection.html:119
> +        InspectorTest.expectThat(otherTreeElementsNotSelected, "All other TreeElements should have selected = false.");

Ditto (>76).

> LayoutTests/inspector/tree-outline/tree-outline-selection.html:124
> +        description: "Select two tree elements, then select two different tree elements.",

Ditto (>51).
Comment 21 Matt Baker 2019-04-05 17:55:38 PDT
Created attachment 366864 [details]
Patch
Comment 22 Devin Rousso 2019-04-10 14:44:53 PDT
Created attachment 367164 [details]
Patch
Comment 23 WebKit Commit Bot 2019-04-10 15:44:17 PDT
Comment on attachment 367164 [details]
Patch

Clearing flags on attachment: 367164

Committed r244154: <https://trac.webkit.org/changeset/244154>
Comment 24 WebKit Commit Bot 2019-04-10 15:44:19 PDT
All reviewed patches have been landed.  Closing bug.