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
<rdar://problem/47829275>
Created attachment 361217 [details] [HTML] Test Page
Created attachment 361224 [details] Patch
(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.
(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.
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.
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
Created attachment 361233 [details] Patch
(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!
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);
(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.
Created attachment 361235 [details] Patch
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).
Comment on attachment 361235 [details] Patch https://trac.webkit.org/changeset/241003