Bug 192681 – Web Inspector: Elements tab: multiple selection lost after navigating to another tab

• Component: Web Inspector
• Status: RESOLVED FIXED

[Matt Baker]

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

[Radar WebKit Bug Importer]

<rdar://problem/46709392>

[Matt Baker]

Created attachment 362538 [details]
Patch

[Nikita Vasilyev]

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.

[Matt Baker]

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.

[Matt Baker]

Created attachment 362563 [details]
Patch

[Devin Rousso]

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

[Matt Baker]

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.

[Matt Baker]

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.

[Devin Rousso]

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.

[Devin Rousso]

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

[Matt Baker]

Created attachment 363813 [details]
Patch

[Devin Rousso]

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

[Matt Baker]

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.

[Matt Baker]

Created attachment 365941 [details]
Patch

[Matt Baker]

Created attachment 365945 [details]
Patch

[Matt Baker]

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

[Matt Baker]

Created attachment 366063 [details]
Patch

[Matt Baker]

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);

[Devin Rousso]

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.

[Devin Rousso]

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

[Matt Baker]

Created attachment 366864 [details]
Patch

[Devin Rousso]

Created attachment 367164 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 367164 [details]
Patch

Clearing flags on attachment: 367164

Committed r244154: <https://trac.webkit.org/changeset/244154>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.