Bug 146878 – Web Inspector: Merge the styles sidebar navigation bar into a selectable item in the elements sidebar

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Devin Rousso]

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.

[Radar WebKit Bug Importer]

<rdar://problem/21782447>

[Devin Rousso]

Created attachment 256661 [details]
Patch

[Devin Rousso]

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

[Timothy Hatcher]

Interesting! I am not 100% sold on this approach just yet. I'll think on it.

[Devin Rousso]

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

[Nikita Vasilyev]

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.

[Timothy Hatcher]

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.

[Brian Burg]

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

[Devin Rousso]

Created attachment 257723 [details]
Patch

[Timothy Hatcher]

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.

[Devin Rousso]

Created attachment 257805 [details]
Patch

[Timothy Hatcher]

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.

[Devin Rousso]

Created attachment 257817 [details]
Patch

[Timothy Hatcher]

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.

[Timothy Hatcher]

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.

[Devin Rousso]

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.

[Devin Rousso]

Created attachment 257856 [details]
Patch

[Timothy Hatcher]

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

[Devin Rousso]

Created attachment 258028 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 258028 [details]
Patch

Clearing flags on attachment: 258028

Committed r187895: <http://trac.webkit.org/changeset/187895>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Chris Chiera]

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.

[Timothy Hatcher]

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.

[Chris Chiera]

No worries boss, thank you for the update and look forward to when it lands in Safari stable.

[Chris Chiera]

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!

[Timothy Hatcher]

Update to OS X El Capitan 10.11.2 to get the fix to allow WebKit nightly builds to work again.

[Chris Chiera]

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!

[Timothy Hatcher]

Yes, the nightly was silently failing (even when it launched fine) on earlier 10.11 builds.