Bug 179223 - Web Inspector: Selecting a DOM Search Result in Search Tab unexpectedly changes Tabs
Summary: Web Inspector: Selecting a DOM Search Result in Search Tab unexpectedly chang...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Joseph Pecoraro
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-11-02 18:58 PDT by Joseph Pecoraro
Modified: 2017-11-06 10:57 PST (History)
4 users (show)

See Also:

Attachments
[PATCH] Proposed Fix (6.69 KB, patch)
2017-11-02 19:05 PDT, Joseph Pecoraro 		joepeck: commit-queue-
Details | Formatted Diff | Diff
[PATCH] Proposed Fix (9.48 KB, patch)
2017-11-02 20:22 PDT, Joseph Pecoraro 		hi: review+
Details | Formatted Diff | Diff
[IMAGE] DOM Tree in Search Tab (259.60 KB, image/png)
2017-11-02 20:24 PDT, Joseph Pecoraro 		no flags Details
[IMAGE] Context Menu in Search Tab (80.00 KB, image/png)
2017-11-02 20:24 PDT, Joseph Pecoraro 		no flags Details
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Pecoraro 2017-11-02 18:58:46 PDT 
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
Comment 1 Joseph Pecoraro 2017-11-02 18:58:54 PDT 
<rdar://problem/33949556>
Comment 2 Joseph Pecoraro 2017-11-02 19:05:18 PDT 
Created attachment 325819 [details]
[PATCH] Proposed Fix
Comment 3 Joseph Pecoraro 2017-11-02 19:07:36 PDT 
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.
Comment 4 Joseph Pecoraro 2017-11-02 20:22:45 PDT 
Created attachment 325832 [details]
[PATCH] Proposed Fix

Here we go, fixing the tab switching issue, which ran pretty deep into ContentBrowser.
Comment 5 Joseph Pecoraro 2017-11-02 20:24:00 PDT 
Created attachment 325833 [details]
[IMAGE] DOM Tree in Search Tab
Comment 6 Joseph Pecoraro 2017-11-02 20:24:21 PDT 
Created attachment 325834 [details]
[IMAGE] Context Menu in Search Tab
Comment 7 Devin Rousso 2017-11-06 01:10:43 PST 
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();
Comment 8 Joseph Pecoraro 2017-11-06 10:49:08 PST 
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!
Comment 9 Joseph Pecoraro 2017-11-06 10:57:18 PST 
<https://trac.webkit.org/r224499>