Web Inspector: Selecting a DOM Search Result in Search Tab unexpectedly changes Tabs

• Component: Web Inspector
• Status: RESOLVED FIXED

[Joseph Pecoraro]

Selecting a DOM Search Result in Search Tab unexpectedly changes Tabs

Steps to Reproduce:
1. Inspect <https://webkit.org>
2. Global search for "Linux" => End up in Search Tab
3. Select one of the [T] DOM Search results in the sidebar
  => Unexpectedly changed to Elements tab

Expected:
- See a DOM Tree in the Search Tab

[Joseph Pecoraro]

<rdar://problem/33949556>

[Joseph Pecoraro]

Created attachment 325819 [details]
[PATCH] Proposed Fix

[Joseph Pecoraro]

Comment on attachment 325819 [details]
[PATCH] Proposed Fix

Going to not mark this as review right now since switching to the Elements tab sometimes doesn't bring all of its Details sidebars... so probably an existing bug.

[Joseph Pecoraro]

Created attachment 325832 [details]
[PATCH] Proposed Fix

Here we go, fixing the tab switching issue, which ran pretty deep into ContentBrowser.

[Joseph Pecoraro]

Created attachment 325833 [details]
[IMAGE] DOM Tree in Search Tab

[Joseph Pecoraro]

Created attachment 325834 [details]
[IMAGE] Context Menu in Search Tab

[Devin Rousso]

Comment on attachment 325832 [details]
[PATCH] Proposed Fix

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

r=me

> Source/WebInspectorUI/ChangeLog:19
> +        (WI.SearchTabContentView):
> +        Check experimental setting for experimental sidebars.

This is unrelated.  Did you forget to upload part of the diff, or is this from another patch?

> Source/WebInspectorUI/UserInterface/Views/ContentBrowser.js:236
> +        let currentContentView = this.currentContentView;
> +        this._updateContentViewSelectionPathNavigationItem(currentContentView);
> +        this._updateHierarchicalPathNavigationItem(currentContentView ? currentContentView.representedObject : null);

I would personally restructure this to make use of existing functions.

    this._updateContentViewSelectionPathNavigationItem(this.currentContentView);
    this.updateHierarchicalPathForCurrentContentView();

[Joseph Pecoraro]

Comment on attachment 325832 [details]
[PATCH] Proposed Fix

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

>> Source/WebInspectorUI/ChangeLog:19
>> +        Check experimental setting for experimental sidebars.
> 
> This is unrelated.  Did you forget to upload part of the diff, or is this from another patch?

Ross actually landed this part, heh.

>> Source/WebInspectorUI/UserInterface/Views/ContentBrowser.js:236
>> +        this._updateHierarchicalPathNavigationItem(currentContentView ? currentContentView.representedObject : null);
> 
> I would personally restructure this to make use of existing functions.
> 
>     this._updateContentViewSelectionPathNavigationItem(this.currentContentView);
>     this.updateHierarchicalPathForCurrentContentView();

Sounds good!

[Joseph Pecoraro]

<https://trac.webkit.org/r224499>