Bug 174229 – Web Inspector: Styles: make Computed a top-level sidebar tab

• Component: Web Inspector
• Status: RESOLVED FIXED

[Nikita Vasilyev]

Two years ago, Styles second-level tabs were replaced by a dropdown menu: Bug 146878 - Web Inspector: Merge the styles sidebar navigation bar into a selectable item in the elements sidebar.

This added an extra click to switch between most commonly used Style tabs: Rules and Computed.

The two most commonly used tabs should:
1. Take a single click to switch between.
2. Be top-level tabs.
3. Be first on the list, before Node and Layers.

[Radar WebKit Bug Importer]

<rdar://problem/33170193>

[Nikita Vasilyev]

Created attachment 325275 [details]
WIP

Not ready for review yet, but you can apply the patch and try the new tabs.

Known issues:
— Command-clicking on a property in Computed to jump to a corresponding property in Styles is now broken.
— Ideally, the filter field should be shared between Styles and Computed, e.g. switching between Styles and Computed should preserve the filter field value. This isn't the case in Chrome. Also, I haven't yet implemented filtering in the redesigned styles panel.

The newly added StyleRulesDetailsSidebarPanel.js and StyleComputedDetailsSidebarPanel.js are trimmed down versions of CSSStyleDetailsSidebarPanel.js. I'm still thinking what's the best way to reduce code duplication between the two.

[Nikita Vasilyev]

Created attachment 325276 [details]
[Animated GIF] With patch applied

[Nikita Vasilyev]

Created attachment 326055 [details]
[Animated GIF] 4 tabs overflow sidebar

We have code that supposed to determine sidebar's min-width based on the navigation items' width. Unfortunately, it doesn't work.

I'm seeing WI.NavigationBar.prototype._calculateMinimumWidth returning 0 because it gets called when _visibleNavigationItems are empty. This zero value gets cached by minimumWidth getter.

Besides, there's also: Bug 179292 - REGRESSION (r221338): Web Inspector: NavigationBar incorrectly calculates minimumWidth

[Nikita Vasilyev]

Created attachment 326285 [details]
Patch

[Nikita Vasilyev]

Created attachment 326286 [details]
[Animated GIF] With patch applied

[Blaze Burg]

Comment on attachment 326285 [details]
Patch

AFAIK, we have not agreed to phase out Visual Styles Sidebar right now. I think we are going to consider this in a more nuanced way later. For now, it's probably OK to keep it as a dropdown alternative for the "Styles" selector.

[Nikita Vasilyev]

Created attachment 326617 [details]
Patch

I added a setting to show Legacy Visual Styles Panel. It's off by default.

[Nikita Vasilyev]

Created attachment 326618 [details]
[Image] Style sidebar tabs

(In reply to Brian Burg from comment #7)
> Comment on attachment 326285 [details]
> Patch
> 
> AFAIK, we have not agreed to phase out Visual Styles Sidebar right now. I
> think we are going to consider this in a more nuanced way later. For now,
> it's probably OK to keep it as a dropdown alternative for the "Styles"
> selector.

I removed all dropdown tab selectors from the styles sidebar and made Visual a top-level tab, which is turned off by default.

[Matt Baker]

Comment on attachment 326617 [details]
Patch

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

Looking pretty good. The only major comment I have is that it feels like the panel class hierarchy is convoluted. A lot of this was in place before this patch, but now that the panels are all top-level, maybe the *Panel and *SidebarPanel classes could be merged together or something. Just a thought, I think it's okay for now.

Aside: I like that pseudo-class checkboxes are visible in the Visual panel now.

> Source/WebInspectorUI/ChangeLog:11
> +        "Visual" styles is no longer visible in the Styles sidebar tabs by default. It can be enabled in the settings.

Nit: no need to quote Styles, Computed, and Visual. Also this could be all one paragraph.

> Source/WebInspectorUI/ChangeLog:49
> +        CSSStyleDetailsSidebarPanel used to suppost several panels. This is no longer the case with GeneralStyleDetailsSidebarPanel:

suppost -> support

> Source/WebInspectorUI/ChangeLog:73
> +        SidebarNavigationBar wraps navigation items in an element so it can correctly calculate the width of all items with spacing between them.

I don't understand why a new class is needed. Can GroupNavigationItem be used instead?

> Source/WebInspectorUI/UserInterface/Views/StyleComputedDetailsSidebarPanel.js:26
> +WI.StyleComputedDetailsSidebarPanel = class StyleComputedDetailsSidebarPanel extends WI.GeneralStyleDetailsSidebarPanel

Should this be named RulesStyleDetailsSidebarPanel, to be consistent with its RulesStyleDetailsPanel? The same goes for the other subclasses of GeneralStyleDetailsSidebarPanel.

[Nikita Vasilyev]

Comment on attachment 326617 [details]
Patch

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

>> Source/WebInspectorUI/ChangeLog:73
>> +        SidebarNavigationBar wraps navigation items in an element so it can correctly calculate the width of all items with spacing between them.
> 
> I don't understand why a new class is needed. Can GroupNavigationItem be used instead?

It doesn't calculate the minimum width correctly. It doesn't take the margin between the items into account.

[Nikita Vasilyev]

(In reply to Nikita Vasilyev from comment #11)
> Comment on attachment 326617 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=326617&action=review
> 
> >> Source/WebInspectorUI/ChangeLog:73
> >> +        SidebarNavigationBar wraps navigation items in an element so it can correctly calculate the width of all items with spacing between them.
> > 
> > I don't understand why a new class is needed. Can GroupNavigationItem be used instead?
> 
> It doesn't calculate the minimum width correctly. It doesn't take the margin
> between the items into account.

Perhaps I should modify GroupNavigationItem to correctly calculate minimumWidth.
Do you know if GroupNavigationItem.prototype.minimumWidth is used anywhere? So far I couldn't find any instances of it.

[Nikita Vasilyev]

Comment on attachment 326617 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/StyleComputedDetailsSidebarPanel.js:26
>> +WI.StyleComputedDetailsSidebarPanel = class StyleComputedDetailsSidebarPanel extends WI.GeneralStyleDetailsSidebarPanel
> 
> Should this be named RulesStyleDetailsSidebarPanel, to be consistent with its RulesStyleDetailsPanel? The same goes for the other subclasses of GeneralStyleDetailsSidebarPanel.

Did you mean to leave this comment for StyleRulesDetailsSidebarPanel? If so, than yes, it should.

[Matt Baker]

Comment on attachment 326617 [details]
Patch

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

>>>> Source/WebInspectorUI/ChangeLog:73
>>>> +        SidebarNavigationBar wraps navigation items in an element so it can correctly calculate the width of all items with spacing between them.
>>> 
>>> I don't understand why a new class is needed. Can GroupNavigationItem be used instead?
>> 
>> It doesn't calculate the minimum width correctly. It doesn't take the margin between the items into account.
> 
> Perhaps I should modify GroupNavigationItem to correctly calculate minimumWidth.
> Do you know if GroupNavigationItem.prototype.minimumWidth is used anywhere? So far I couldn't find any instances of it.

It's a property on the NavigationItem base class, which some navigation items override. It is called by NavigationBar.prototype._calculateMinimumWidth(), and by GroupNavigationItem when it calculates its own `minimumWidth`.

>>> Source/WebInspectorUI/UserInterface/Views/StyleComputedDetailsSidebarPanel.js:26
>>> +WI.StyleComputedDetailsSidebarPanel = class StyleComputedDetailsSidebarPanel extends WI.GeneralStyleDetailsSidebarPanel
>> 
>> Should this be named RulesStyleDetailsSidebarPanel, to be consistent with its RulesStyleDetailsPanel? The same goes for the other subclasses of GeneralStyleDetailsSidebarPanel.
> 
> Did you mean to leave this comment for StyleRulesDetailsSidebarPanel? If so, than yes, it should.

Oops! I meant to say that StyleComputedDetailsSidebarPanel should be renamed to ComputedStyleDetailsSidebarPanel to match ComputedStyleDetailsPanel. The same goes for RulesStyleDetailsSidebarPanel.

[Nikita Vasilyev]

I tried to use GroupNavigationItem and went down the rabbit hole. To use GroupNavigationItem, I'd have to:
- Copy several NavigationBar's methods to GroupNavigationItem: addNavigationItem, insertNavigationItem, removeNavigationItem, etc.
- Sidebar has a concept of a selected item, but GroupNavigationItem doesn't. I'd have to either implement it on GroupNavigationItem or add this logic to WI.Sidebar.

Considering how simple SidebarNavigationBar is, I'd rather use it instead.

(In reply to Matt Baker from comment #14)
> Comment on attachment 326617 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=326617&action=review
> 
> >>>> Source/WebInspectorUI/ChangeLog:73
> >>>> +        SidebarNavigationBar wraps navigation items in an element so it can correctly calculate the width of all items with spacing between them.
> >>> 
> >>> I don't understand why a new class is needed. Can GroupNavigationItem be used instead?
> >> 
> >> It doesn't calculate the minimum width correctly. It doesn't take the margin between the items into account.
> > 
> > Perhaps I should modify GroupNavigationItem to correctly calculate minimumWidth.
> > Do you know if GroupNavigationItem.prototype.minimumWidth is used anywhere? So far I couldn't find any instances of it.
> 
> It's a property on the NavigationItem base class, which some navigation
> items override. It is called by
> NavigationBar.prototype._calculateMinimumWidth(), and by GroupNavigationItem
> when it calculates its own `minimumWidth`.

I understand. I meant to write that I can't find GroupNavigationItem.prototype.minimumWidth being called anywhere.

[Nikita Vasilyev]

(In reply to Matt Baker from comment #10)
> Comment on attachment 326617 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=326617&action=review
> 
> Looking pretty good. The only major comment I have is that it feels like the
> panel class hierarchy is convoluted. A lot of this was in place before this
> patch, but now that the panels are all top-level, maybe the *Panel and
> *SidebarPanel classes could be merged together or something. Just a thought,
> I think it's okay for now.

I'll revisit this when I bring back filtering to the styles sidebar. Ideally, we want to share the filter field between Styles and Computed panels.

[Nikita Vasilyev]

Created attachment 328187 [details]
Patch

[Matt Baker]

Comment on attachment 328187 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 328187 [details]
Patch

Clearing flags on attachment: 328187

Committed r225547: <https://trac.webkit.org/changeset/225547>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.