Bug 238226

Created attachment 455431 [details]
Screenshot 2

Created attachment 455432 [details]
Screenshot 3

Created attachment 455437 [details]
Patch v1.0

Comment on attachment 455437 [details]
Patch v1.0

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

> Source/WebInspectorUI/UserInterface/Views/DOMTreeOutline.js:185
> +                if (node === WI.domManager.inspectedNode)
> +                    treeElement.listItemElement.classList.add("inspected-node");

Do we also need to do this inside `if (this._includeRootDOMNode) {`?

Also, this logic only seems to handle the top-level nodes.  What if the inspected node is nested deeper?

Maybe we should just throw something like this right before the first early-return, as I think by that point we should've fully reconstructed the visible DOM?
```
let inspectedNodeTreeElement = this.findTreeElement(WI.domManager.inspectedNode);
if (inspectedNodeTreeElement) {
    inspectedNodeTreeElement.reveal();
    inspectedNodeTreeElement.listItemElement.classList.add("inspected-node");
}
```

Comment on attachment 455437 [details]
Patch v1.0

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

>> Source/WebInspectorUI/UserInterface/Views/DOMTreeOutline.js:185
>> +                    treeElement.listItemElement.classList.add("inspected-node");
> 
> Do we also need to do this inside `if (this._includeRootDOMNode) {`?
> 
> Also, this logic only seems to handle the top-level nodes.  What if the inspected node is nested deeper?
> 
> Maybe we should just throw something like this right before the first early-return, as I think by that point we should've fully reconstructed the visible DOM?
> ```
> let inspectedNodeTreeElement = this.findTreeElement(WI.domManager.inspectedNode);
> if (inspectedNodeTreeElement) {
>     inspectedNodeTreeElement.reveal();
>     inspectedNodeTreeElement.listItemElement.classList.add("inspected-node");
> }
> ```

🤦🏻‍♂️ yep.

Created attachment 455440 [details]
Patch v1.1

Created attachment 455442 [details]
Patch v1.2

Comment on attachment 455442 [details]
Patch v1.2

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

r=me, nice!

... but I did get a bit curious 😅

I think the backtrace for this bug is when `WI.DOMTreeOutline.prototype.update` is called by `WI.DOMTreeOutline.prototype.setVisible`.

Looking at `WI.DOMTreeOutline.prototype.setVisible`, I think there may be some other (possibly previously undiscovered) bugs in that we call `this.update()` _last_, which seems to completely wipe out the entire tree, meaning that
- `this._updateModifiedNodes()` is kinda pointless since we recreate all the `WI.DOMTreeElement`
- `this._revealAndSelectNode(this._selectedDOMNode, omitFocus)` might not end up doing anything since the `WI.DOMTreeElement` that we might reveal and select (assuming `this._selectedDOMNode` is set) is then immediately removed by `this.update()`
We might need to move the `this.update()` down below (and maybe get rid of `this._updateModifiedNodes()`), or do some other restructuring of things.  Not really sure.

Either way, I think what you've done here is probably fine :)

> Source/WebInspectorUI/UserInterface/Views/DOMTreeOutline.js:195
> +                inspectedNodeTreeElement.reveal();

NIT: I wonder if we should also do this inside `WI.DOMTreeOutline.prototype._handleInspectedNodeChanged` 🤔

(In reply to Devin Rousso from comment #8)
> ... but I did get a bit curious 😅
> 
> I think the backtrace for this bug is when
> `WI.DOMTreeOutline.prototype.update` is called by
> `WI.DOMTreeOutline.prototype.setVisible`.
> 
> Looking at `WI.DOMTreeOutline.prototype.setVisible`, I think there may be
> some other (possibly previously undiscovered) bugs in that we call
> `this.update()` _last_, which seems to completely wipe out the entire tree,
> meaning that
> - `this._updateModifiedNodes()` is kinda pointless since we recreate all the
> `WI.DOMTreeElement`
> - `this._revealAndSelectNode(this._selectedDOMNode, omitFocus)` might not
> end up doing anything since the `WI.DOMTreeElement` that we might reveal and
> select (assuming `this._selectedDOMNode` is set) is then immediately removed
> by `this.update()`
> We might need to move the `this.update()` down below (and maybe get rid of
> `this._updateModifiedNodes()`), or do some other restructuring of things. 
> Not really sure.
> 
> Either way, I think what you've done here is probably fine :)

Yeah, so currently `update` will actually remember what is selected before it removes all the items and rebuilds them, and then reselect stuff. Certainly not the most efficient, but I think selection should be correctly maintained during updates due to this. Some deeper work is almost certainly in order to untangle the order of operations here, though.

Comment on attachment 455442 [details]
Patch v1.2

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

>> Source/WebInspectorUI/UserInterface/Views/DOMTreeOutline.js:195
>> +                inspectedNodeTreeElement.reveal();
> 
> NIT: I wonder if we should also do this inside `WI.DOMTreeOutline.prototype._handleInspectedNodeChanged` 🤔

I don't think it's currently possible for the inspected node to be set (thus causing this handler to be called) without it being visible in the DOM tree to select it in the first place.

<rdar://problem/90666532>

Committed r291729 (248761@main): <https://commits.webkit.org/248761@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 455442 [details].