Bug 146878 - Web Inspector: Merge the styles sidebar navigation bar into a selectable item in the elements sidebar
Summary: Web Inspector: Merge the styles sidebar navigation bar into a selectable item...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Devin Rousso
URL:
Keywords: InRadar
Depends on:
Blocks: 145982
  Show dependency treegraph
 
Reported: 2015-07-11 11:59 PDT by Devin Rousso
Modified: 2015-12-08 11:26 PST (History)
10 users (show)

See Also:


Attachments
Patch (21.89 KB, patch)
2015-07-11 12:12 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
After Patch is applied (2.14 MB, video/quicktime)
2015-07-11 12:14 PDT, Devin Rousso
no flags Details
Patch (25.22 KB, patch)
2015-07-28 19:19 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (25.23 KB, patch)
2015-07-29 20:08 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (25.32 KB, patch)
2015-07-29 22:21 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (24.56 KB, patch)
2015-07-30 14:49 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (24.55 KB, patch)
2015-08-02 00:51 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Devin Rousso 2015-07-11 11:59:58 PDT
Since space is limited in the styles sidebar, removing the second navigation bar and moving the styles sidebar navigation items (Computed, Rules, and Metrics) into one selectable element will allow more content to be shown in that space.
Comment 1 Radar WebKit Bug Importer 2015-07-11 12:00:13 PDT
<rdar://problem/21782447>
Comment 2 Devin Rousso 2015-07-11 12:12:19 PDT
Created attachment 256661 [details]
Patch
Comment 3 Devin Rousso 2015-07-11 12:14:51 PDT
Created attachment 256662 [details]
After Patch is applied

When hovering the "Styles" navigation item, the title will show the label of the currently selected sidebar panel (Computed, Rules, or Metrics).
Comment 4 Timothy Hatcher 2015-07-11 14:39:27 PDT
Interesting! I am not 100% sold on this approach just yet. I'll think on it.
Comment 5 Devin Rousso 2015-07-11 14:49:40 PDT
I definitely agree.  I don't think that the fact that you can switch style panel views (Rules, Computed, and Metrics) is visible enough, but I also think that adding some sort of label below or next to the "Styles" would make it a bit too cluttered.  Not really sure what the best approach is...
Comment 6 Nikita Vasilyev 2015-07-11 19:11:21 PDT
I switch between Computed and Rules very often. This change introduces an extra click. We don't have usage stats, but I believe Styles and Computed are by far the most commonly used tabs in that sidebar.

I was thinking of:
— Removing the metrics tab and displaying Box Model at the bottom of the Rules, like in Chrome.
— Merging two tab bars into the one: Node, Styles, Computed, Layers. It would save us 30px of vertical space.

In addition to that, I'd put the Node tab the last or second to the last.
Comment 7 Timothy Hatcher 2015-07-11 20:33:13 PDT
Metrics should merge with Computed, not Rules. It is computed info it is displaying.

I think Computed is too generic of a term to put in the nav bar alone.
Comment 8 Brian Burg 2015-07-16 09:37:05 PDT
(In reply to comment #7)
> Metrics should merge with Computed, not Rules. It is computed info it is
> displaying.
> 
> I think Computed is too generic of a term to put in the nav bar alone.

Agree. I would like to see this as a "Layout" section, where the user can see how the element is used during layout. This includes its computed properties (determined in layout) but also position on page and other things.

As shown in the video, I don't like this change. It adds another click to the most common workflow, as Nikita points out.
Comment 9 Devin Rousso 2015-07-28 19:19:29 PDT
Created attachment 257723 [details]
Patch
Comment 10 Timothy Hatcher 2015-07-29 09:00:00 PDT
Comment on attachment 257723 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDetailsSidebarPanel.js:203
> +        var selectedPanel = this._panelMatchingIdentifier(selectedItem);

Is selectedItem a panel or identifier? The fiction takes an identifier, but the var name implies it is a navigation item object.
Comment 11 Devin Rousso 2015-07-29 20:08:03 PDT
Created attachment 257805 [details]
Patch
Comment 12 Timothy Hatcher 2015-07-29 22:12:58 PDT
Comment on attachment 257805 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/ScopeRadioButtonNavigationItem.js:68
> +    get selectedItem()

I see, the confusion was here. This should be selectedItemIdentifier to avoid sounding like an object.
Comment 13 Devin Rousso 2015-07-29 22:21:29 PDT
Created attachment 257817 [details]
Patch
Comment 14 Timothy Hatcher 2015-07-30 11:19:03 PDT
Comment on attachment 257817 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationSection.js:-553
> -WebInspector.CSSStyleDeclarationSection.prototype.__proto__ = WebInspector.StyleDetailsPanel.prototype;

Still needed I think.
Comment 15 Timothy Hatcher 2015-07-30 11:20:14 PDT
Comment on attachment 257817 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/NavigationBar.js:300
> +        && this.selectedNavigationItem.dontPreventDefaultOnNavigationBarMouseDown()
> +        && this._previousSelectedNavigationItem === this.selectedNavigationItem)

We typically indent these too.

> Source/WebInspectorUI/UserInterface/Views/ScopeRadioButtonNavigationItem.js:55
> +
> +

Extra new lines.
Comment 16 Devin Rousso 2015-07-30 14:44:42 PDT
Comment on attachment 257817 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationSection.js:-553
>> -WebInspector.CSSStyleDeclarationSection.prototype.__proto__ = WebInspector.StyleDetailsPanel.prototype;
> 
> Still needed I think.

I couldn't find any instances where CSSStyleDeclarationSection used something from StyleDetailsPanel, but I'll add it back in just in case.
Comment 17 Devin Rousso 2015-07-30 14:49:48 PDT
Created attachment 257856 [details]
Patch
Comment 18 Timothy Hatcher 2015-07-30 16:07:28 PDT
(In reply to comment #16)
> Comment on attachment 257817 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=257817&action=review
> 
> >> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationSection.js:-553
> >> -WebInspector.CSSStyleDeclarationSection.prototype.__proto__ = WebInspector.StyleDetailsPanel.prototype;
> > 
> > Still needed I think.
> 
> I couldn't find any instances where CSSStyleDeclarationSection used
> something from StyleDetailsPanel, but I'll add it back in just in case.

Oh, sorry. I didn't read the whole thing. That is the wrong inheritance! It should be WebIndpector.Object, or nothing. File a new bug for that.
Comment 19 Devin Rousso 2015-08-02 00:51:28 PDT
Created attachment 258028 [details]
Patch
Comment 20 WebKit Commit Bot 2015-08-04 15:32:21 PDT
Comment on attachment 258028 [details]
Patch

Clearing flags on attachment: 258028

Committed r187895: <http://trac.webkit.org/changeset/187895>
Comment 21 WebKit Commit Bot 2015-08-04 15:32:26 PDT
All reviewed patches have been landed.  Closing bug.
Comment 22 Chris Chiera 2015-09-29 22:17:41 PDT
Noticed this patch was available in Nightly Webkit previously, but downloading the latest Nightly on 10.11 beta it doesn't seem to be showing, same for bug 147360. Do you know if when 10.11 final is released tomorrow if these two bug patches will be available in the final Safari and or the Nightly version tomorrow? Thanks for your time and sorry to awaken this closed thread.
Comment 23 Timothy Hatcher 2015-09-30 16:26:17 PDT
We did not ship this in Safari 9. There is also a big where nightly builds are not working on El Cap. We are working on fixing that. Sorry for the trouble.
Comment 24 Chris Chiera 2015-09-30 16:28:06 PDT
No worries boss, thank you for the update and look forward to when it lands in Safari stable.
Comment 25 Chris Chiera 2015-12-01 17:47:33 PST
Just checking in to see if there has been progress on this. As currently all the great web inspector updates haven't made it to Safari or Webkit Nightly yet. Do you know when they might show up or if there is another thread that we can track the progress as this one has it listed as RESOLVED FIXED. Thanks guys!
Comment 26 Timothy Hatcher 2015-12-08 10:48:50 PST
Update to OS X El Capitan 10.11.2 to get the fix to allow WebKit nightly builds to work again.
Comment 27 Chris Chiera 2015-12-08 11:17:00 PST
Thank you! To note, I've been able to install and use Webkit on 10.11.1 without a problem. The only issue was that the webkit inspector wasn't showing that visual improvements.

On 10.11.1 Webkit Nightly showed version: Webkit: Version 9.0.1 (11601.2.7.2)

Just updated to 10.11.2 and reopened Webkit and now it's automatically showing version "Version 9.0.2 (11601.3.9, r192763)" and the inspector is now showing all those visual improvements. Awesome, thanks!
Comment 28 Timothy Hatcher 2015-12-08 11:26:49 PST
Yes, the nightly was silently failing (even when it launched fine) on earlier 10.11 builds.