Bug 194300 – Web Inspector: Elements tab: selection is broken after deleting the selected node

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

[Matt Baker]

Summary:
Selection is broken after deleting the selected node.

Deleting a selected node causes a new node to be selected, as expected, and the details sidebar and DOM path reflect the new selection. However, clicking on another node does not deselect the current node. A new node is selected, but the previous selection remains visible.

Steps to Reproduce:
1. Inspect test page
2. Elements tab
3. Select DIV "node-2"
4. Press Delete
5. DIV "node-3" becomes selected in the tree
6. Hit up arrow key

Actual:
  => HTML closing tag (the last node) is selected
Expected:
  => DIV "node-1" is selected

[Radar WebKit Bug Importer]

<rdar://problem/47829275>

[Matt Baker]

Created attachment 361217 [details]
[HTML] Test Page

[Matt Baker]

Created attachment 361224 [details]
Patch

[Matt Baker]

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

Hmm, I think `numberOfElementsInSubtree` needs to account for the closing tag TreeElement. Checking now.

[Matt Baker]

(In reply to Matt Baker from comment #4)
> (In reply to Matt Baker from comment #3)
> > Created attachment 361224 [details]
> > Patch
> 
> Hmm, I think `numberOfElementsInSubtree` needs to account for the closing
> tag TreeElement. Checking now.

Nope! The TreeElement for the closing tag is a child of TreeElement for the open tag.

[Nikita Vasilyev]

Comment on attachment 361224 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:1105
> +        }

How often do we call _indexesForSubtree?

This seems fairly expensive. It doesn't seem to be any worse than before but I wonder if we can make it even better. If it's performance sensitive, perhaps we can optimize breadth-first search here. For instance, avoid using shift() since it changes indexes of array items every time.

[Nikita Vasilyev]

Comment on attachment 361224 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:1105
>> +        }
> 
> How often do we call _indexesForSubtree?
> 
> This seems fairly expensive. It doesn't seem to be any worse than before but I wonder if we can make it even better. If it's performance sensitive, perhaps we can optimize breadth-first search here. For instance, avoid using shift() since it changes indexes of array items every time.

A straightforward optimization here is to replace shift with pop and use a depth-first search.

https://jsperf.com/pop-vs-shift-on-a-array/1

[Matt Baker]

Created attachment 361233 [details]
Patch

[Matt Baker]

(In reply to Nikita Vasilyev from comment #7)
> Comment on attachment 361224 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=361224&action=review
> 
> >> Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:1105
> >> +        }
> > 
> > How often do we call _indexesForSubtree?

Not often. Only when removing things from the DOM tree. However, a series of node removals triggered from script could potentially be slow.

> > This seems fairly expensive. It doesn't seem to be any worse than before but I wonder if we can make it even better. If it's performance sensitive, perhaps we can optimize breadth-first search here. For instance, avoid using shift() since it changes indexes of array items every time.
> 
> A straightforward optimization here is to replace shift with pop and use a
> depth-first search.
> 
> https://jsperf.com/pop-vs-shift-on-a-array/1

Done!

[Devin Rousso]

Comment on attachment 361233 [details]
Patch

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

> Source/WebInspectorUI/ChangeLog:18
> +        This method did not stay within the subtree rooted at `treeElement`.

If you're saying that `stayWithin` wasn't working, isn't that a bug worth fixing?

> Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:1097
> +                let child = elements.pop();

I don't like the idea of constantly pushing/popping to an array.  Do you have any performance numbers as to whether this approach is faster than using recursion?  A recursive approach would only modify count, which should be less work.

    function numberOfElementsInSubtree(treeElement) {
        let count = 0;
        if (treeElement.revealed()) {
            ++count;
            for (let child of treeElement.children)
                count += numberOfElementsInSubtree(child);
        }
        return count;
    }

> Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:1102
> +                elements.push(...child.children);

This should definitely be a `concat` instead of a spread.  I've often found that spread is often not that great, performance wise.

    elements = elements.concat(child.children);

[Matt Baker]

(In reply to Devin Rousso from comment #10)
> Comment on attachment 361233 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=361233&action=review
> 
> > Source/WebInspectorUI/ChangeLog:18
> > +        This method did not stay within the subtree rooted at `treeElement`.
> 
> If you're saying that `stayWithin` wasn't working, isn't that a bug worth
> fixing?

It seems like a bug, but I'm not 100% sure what the intention was and don't want to change that here, as it would have such a large ripple effect.

> > Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:1097
> > +                let child = elements.pop();
> 
> I don't like the idea of constantly pushing/popping to an array.  Do you
> have any performance numbers as to whether this approach is faster than
> using recursion?  A recursive approach would only modify count, which should
> be less work.
> 
>     function numberOfElementsInSubtree(treeElement) {
>         let count = 0;
>         if (treeElement.revealed()) {
>             ++count;
>             for (let child of treeElement.children)
>                 count += numberOfElementsInSubtree(child);
>         }
>         return count;
>     }

I wanted to avoid a recursive approach just in case a very large subtree is being traversed. I seem to recall this being a thing at one point. Secondly, we can't use treeElement.revealed(), since that will return false for children with a collapsed parent, and we want to count all TreeElements that are in the tree.

> > Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:1102
> > +                elements.push(...child.children);
> 
> This should definitely be a `concat` instead of a spread.  I've often found
> that spread is often not that great, performance wise.
> 
>     elements = elements.concat(child.children);

I actually meant to change that. Thanks for reminding.

[Matt Baker]

Created attachment 361235 [details]
Patch

[Devin Rousso]

Comment on attachment 361235 [details]
Patch

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

rs=me

> Source/WebInspectorUI/UserInterface/Views/TreeOutline.js:1096
> +                let child = elements.pop();

Rather than pop, perhaps you can just keep an index and traverse the array with the index (instead of checking `elements.length`, you could check `index < elements.length`).  My gut is telling me we should avoid modifying the array as much as possible (lots of `length` churn).

[Matt Baker]

Comment on attachment 361235 [details]
Patch

https://trac.webkit.org/changeset/241003