Bug 161405 – Web Inspector: Allow hiding of CSS variables in Computed styles panel

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

[Devin Rousso]

Since CSS variables are displayed as part of the computed style, it can get annoying sometimes (especially when inspecting WebInspector) to see computed properties for the selected element.  I think the checkbox should be changed to allow more flexibility with what is displayed.

[Radar WebKit Bug Importer]

<rdar://problem/28083228>

[Devin Rousso]

Created attachment 287440 [details]
Patch

[Devin Rousso]

Created attachment 287441 [details]
[Image] After Patch is applied

[Matt Baker]

Comment on attachment 287440 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:130
> +    get alwaysShowPropertyNames() { return Object.keys(this._alwaysShowPropertyNames); }

Nit: I think the soft rule is that we only make trivial getters one line. Since this calls a method it probably shouldn't be changed.

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:144
> +        if (!Object.values(WebInspector.CSSStyleDeclarationTextEditor.VariableVisibility).includes(variableVisibility))

Remove. Sanity checking for enum values isn't normally done.

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:1687
> +    Hidden: "css-style-declaration-text-editor-variable-visibility-hidden",

We've started using Symbol for enums, with debug descriptions that are usually the "leaf" part of the old-style string:

Hidden: Symbol("variable-visibility-hidden").

[Nikita Vasilyev]

Comment on attachment 287440 [details]
Patch

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

I'd place the Variables section under the Properties section. I think the Properties section is way more commonly used.

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:1690
> +};

Why do we need 3 states? I'm particularly confused by Only.

[Matt Baker]

Created attachment 287449 [details]
[Image] patch applied, double border

2x border above Variables section.

Also we should either remove the Variables section when empty, or style it like the other empty sections (see Layers > Layer Info for a node with no layers).

[Nikita Vasilyev]

(In reply to comment #5)
> Comment on attachment 287440 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=287440&action=review
> 
> I'd place the Variables section under the Properties section. I think the
> Properties section is way more commonly used.
> 
> > Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:1690
> > +};
> 
> Why do we need 3 states? I'm particularly confused by Only.

Now I see. WebInspector.CSSStyleDeclarationTextEditor.VariableVisibility.Only means that the Variable section is expanded and Properties section is collapsed.

The WebInspector.CSSStyleDeclarationTextEditor.VariableVisibility is only used by _iterateOverProperties to filter properties.

[Nikita Vasilyev]

Created attachment 287456 [details]
[Image] Empty section

(In reply to comment #6)
> Also we should either remove the Variables section when empty, or style it
> like the other empty sections (see Layers > Layer Info for a node with no
> layers).

Agreed with Matt. There is too much spacing under "No properties" and not enough below it.

[Devin Rousso]

(In reply to comment #6)
> Created attachment 287449 [details]
> [Image] patch applied, double border
> 
> 2x border above Variables section.
> 
> Also we should either remove the Variables section when empty, or style it
> like the other empty sections (see Layers > Layer Info for a node with no
> layers).

I just tested this on my computer and didn't see the a double border.  Any steps to reproduce?  See below for comment on empty sections.

(In reply to comment #7)
> (In reply to comment #5)
> > Comment on attachment 287440 [details]
> > > Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:1690
> > > +};
> > 
> > Why do we need 3 states? I'm particularly confused by Only.
> 
> Now I see.
> WebInspector.CSSStyleDeclarationTextEditor.VariableVisibility.Only means
> that the Variable section is expanded and Properties section is collapsed.
> 
> The WebInspector.CSSStyleDeclarationTextEditor.VariableVisibility is only
> used by _iterateOverProperties to filter properties.

As you said, the three states are used by _iterateOverProperties.  There need to be three states because there are three different ways of displaying CSS variables:
1) Shown:  included with regular properties (like in Rules panel)
2) Hidden: don't display variables (like Properties section in Computed panel)
3) Only:   don't display CSS properties that aren't CSS variables (like Variables section in Computed panel)

(In reply to comment #8)
> (In reply to comment #6)
> > Also we should either remove the Variables section when empty, or style it
> > like the other empty sections (see Layers > Layer Info for a node with no
> > layers).

I think it is better to just hide the section if there are no CSS variables.

> Agreed with Matt. There is too much spacing under "No properties" and not
> enough below it.

The spacing issue likely an CSS problem with CSSStyleDeclarationTextEditor and the Computed panel.

[Devin Rousso]

(In reply to comment #9)
> The spacing issue likely an CSS problem with CSSStyleDeclarationTextEditor
> and the Computed panel.

It is actually an issue with WebInspector.DetailsSection, specifically the set height (and padding) in DetailsSection.css:

.details-section > .header {
    height: 23px;
    padding: 4px 5px 4px 0;
}

Not sure how to tackle this, other than adding extra padding below the content of the section, which I don't think is a great idea because that is a lot of wasted space.

[Devin Rousso]

Created attachment 287468 [details]
Patch

[Matt Baker]

(In reply to comment #10)
> (In reply to comment #9)
> > The spacing issue likely an CSS problem with CSSStyleDeclarationTextEditor
> > and the Computed panel.
> 
> It is actually an issue with WebInspector.DetailsSection, specifically the
> set height (and padding) in DetailsSection.css:
> 
> .details-section > .header {
>     height: 23px;
>     padding: 4px 5px 4px 0;
> }
> 
> Not sure how to tackle this, other than adding extra padding below the
> content of the section, which I don't think is a great idea because that is
> a lot of wasted space.

DetailsSectionRow has an `emptyMessage` property for just this situation. There are a bunch of examples of how to use this in the UI. If we're going to always show the Variables section (even if it's empty) it should have a look consistent with the other details sections.

[Devin Rousso]

(In reply to comment #12)
> (In reply to comment #10)
> > (In reply to comment #9)
> > > The spacing issue likely an CSS problem with CSSStyleDeclarationTextEditor
> > > and the Computed panel.
> > 
> > It is actually an issue with WebInspector.DetailsSection, specifically the
> > set height (and padding) in DetailsSection.css:
> > 
> > .details-section > .header {
> >     height: 23px;
> >     padding: 4px 5px 4px 0;
> > }
> > 
> > Not sure how to tackle this, other than adding extra padding below the
> > content of the section, which I don't think is a great idea because that is
> > a lot of wasted space.
> 
> DetailsSectionRow has an `emptyMessage` property for just this situation.
> There are a bunch of examples of how to use this in the UI. If we're going
> to always show the Variables section (even if it's empty) it should have a
> look consistent with the other details sections.

I actually just made it so that the Variables section is hidden when no properties are shown in the editor (via the _shownProperties member variable on CSSStyleDeclarationTextEditor).

[Blaze Burg]

Comment on attachment 287468 [details]
Patch

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

Looking pretty good, but I want to apply it before setting r+.

> Source/WebInspectorUI/ChangeLog:32
> +         - Only (don't show other properties)

Let's rename this as it's causing confusion. It doesn't just control variable visibility:

WebInspector.CSSStyleDeclarationTextEditor.PropertyVisibilityMode = {
ShowAll
HideVariables
HideNonVariables
}

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:1197
> +                if (property.variable && this._variableVisibility === WebInspector.CSSStyleDeclarationTextEditor.VariableVisibility.Hidden)

This entire function is pretty scary and has no test coverage at all. We should think about how to improve this situation.

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:1211
> +                if (!property.variable && this._variableVisibility === WebInspector.CSSStyleDeclarationTextEditor.VariableVisibility.Only)

Please make this into a switch on the editor's propertyVisibilityMode (as renamed above). It will be easier to follow.

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:1221
> +        if (filterFunction) {

We could drop this guard if you initialize filterFunction to an identity function.

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:1691
> +WebInspector.CSSStyleDeclarationTextEditor.VariableVisibility = {

See renaming suggestion above.

[Devin Rousso]

Created attachment 287842 [details]
Patch

[Blaze Burg]

Comment on attachment 287842 [details]
Patch

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

r=me. I found an existing UI bug while testing the change. (https://bugs.webkit.org/show_bug.cgi?id=161652)

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:1222
> +                }

There's a missing case. Make it break; so the switch is complete, and add a default case that throws an exception.

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:1230
> +                properties.sort((a, b) => a.name.localeCompare(b.name));

Nit: extra indent

[WebKit Commit Bot]

Comment on attachment 287842 [details]
Patch

Clearing flags on attachment: 287842

Committed r205518: <http://trac.webkit.org/changeset/205518>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.