Web Inspector: REGRESSION: deleting an audit puts selection in a selected but invisible state

[Devin Rousso]

# STEPS TO REPRODUCE:
1. open WebInspector
2. enable the Audits tab
3. add the default audits (assuming they aren't already there)
4. recursively expand all default audits so everything is visible
5. select the first top-level default audit ("Demo Audit")
6. press delete ⌫
 => the first top-level default audit ("Demo Audit") will disappear, and the selection styles will apply to the new first top-level default audit ("Accessibility") (which was the second top-level default audit)
7. press delete ⌫
 => nothing happens
8. press up ↑
 => the "valid-tabindex" test becomes selected, while the first top-level default audit ("Accessibility") is also selected

[Radar WebKit Bug Importer]

<rdar://problem/46875285>

[Matt Baker]

Created attachment 358026 [details]
Patch

[Devin Rousso]

Comment on attachment 358026 [details]
Patch

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

r-, as I think there's a simpler (and safer) approach to `removeChildren`, namely just entirely wiping out the `_selectionController`'s list of `_selectedIndexes`.  Since we're removing all children from the `WI.TreeOutline`, there shouldn't be anything left to select, meaning that all we really need to do is call `this._selectionController.deselectAll`.

> Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:1103
> +        // Include the index of the subtree's root, unless it's the TreeOutline root.
> +        if (!treeElement.root)
> +            startIndex--;

Also, why is this needed?  None of the other index-related functions care about the `root`?  This should either be removed or have a better comment.

Also, AFACIT, the root is only set on `WI.TreeOutline`, so I don't see how this would ever not get called?

[Matt Baker]

(In reply to Devin Rousso from comment #3)
> Comment on attachment 358026 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=358026&action=review
> 
> r-, as I think there's a simpler (and safer) approach to `removeChildren`,
> namely just entirely wiping out the `_selectionController`'s list of
> `_selectedIndexes`.  Since we're removing all children from the
> `WI.TreeOutline`, there shouldn't be anything left to select, meaning that
> all we really need to do is call `this._selectionController.deselectAll`.

The reason we can't do this is that `this` can be a TreeElement:

class TreeElement
{
    removeChildren() { return WI.TreeOutline.prototype.removeChildren.apply(this, arguments); }
}

[Devin Rousso]

(In reply to Matt Baker from comment #4)
> The reason we can't do this is that `this` can be a TreeElement:
> 
> class TreeElement
> {
>     removeChildren() { return WI.TreeOutline.prototype.removeChildren.apply(this, arguments); }
> }

Ah, right.  I forgot that we do that.  At the very least then, we could create a union of the indexes for all the children and then just call it once.

    removeChildren(suppressOnDeselect)
    {
        let removedIndexes = new IndexSet;

        while (this.children.length) {
            let child = this.children[0];
            child.deselect(suppressOnDeselect);

            let treeOutline = child.treeOutline;
            if (treeOutline) {
                treeOutline._forgetTreeElement(child);
                treeOutline._forgetChildrenRecursive(child);
            }

            removedIndexes = removedIndexes.union(treeOutline._indexesForSubtree(child));

            child._detach();
            child.treeOutline = null;
            child.parent = null;
            child.nextSibling = null;
            child.previousSibling = null;

            this.children.shift();

            if (treeOutline)
                treeOutline.dispatchEventToListeners(WI.TreeOutline.Event.ElementRemoved, {element: child});
        }

        if (this.treeOutline)
            this.treeOutline._selectionController.didRemoveItems(removedIndexes);
    }

[Matt Baker]

(In reply to Devin Rousso from comment #5)
> (In reply to Matt Baker from comment #4)
> > The reason we can't do this is that `this` can be a TreeElement:
> > 
> > class TreeElement
> > {
> >     removeChildren() { return WI.TreeOutline.prototype.removeChildren.apply(this, arguments); }
> > }
> 
> Ah, right.  I forgot that we do that.  At the very least then, we could
> create a union of the indexes for all the children and then just call it
> once.

My concern with this approach is that ElementRemoved listeners would then execute before the SelectionController is updated.

[Devin Rousso]

Comment on attachment 358026 [details]
Patch

r=me, after re-reading the code the `root` check makes more sense

[WebKit Commit Bot]

Comment on attachment 358026 [details]
Patch

Clearing flags on attachment: 358026

Committed r239870: <https://trac.webkit.org/changeset/239870>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.