Bug 238226 – Web Inspector: Elements tab: selection variable not displayed after losing focus

• Component: Web Inspector
• Status: RESOLVED FIXED

[Jeff Johnson]

Created attachment 455430 [details]
Screenshot 1

Steps to reproduce:
1) Open https://webkit.org in Safari
2) Open web inspector
3) Select Elements tab
4) Select a DOM node
5) Notice the selection variable = $0
6) Switch to the Console tab
7) Switch back to the Elements tab
8) Notice the selection variable is not display (this may be a bug already)
9) Select the node again

Expected results: The selection variable = $0 is displayed.

Actual results: The selection variable is not displayed. See attached screenshots in order, 1,2,3.

[Jeff Johnson]

Created attachment 455431 [details]
Screenshot 2

[Jeff Johnson]

Created attachment 455432 [details]
Screenshot 3

[Patrick Angle]

Created attachment 455437 [details]
Patch v1.0

[Devin Rousso]

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");
}
```

[Patrick Angle]

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.

[Patrick Angle]

Created attachment 455440 [details]
Patch v1.1

[Patrick Angle]

Created attachment 455442 [details]
Patch v1.2

[Devin Rousso]

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

[Patrick Angle]

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

[Patrick Angle]

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.

[Radar WebKit Bug Importer]

<rdar://problem/90666532>

[EWS]

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