Bug 161405 - Web Inspector: Allow hiding of CSS variables in Computed styles panel
Summary: Web Inspector: Allow hiding of CSS variables in Computed styles panel
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: Devin Rousso
URL:
Keywords: InRadar
Depends on:
Blocks: 161652
  Show dependency treegraph
 
Reported: 2016-08-30 14:25 PDT by Devin Rousso
Modified: 2016-09-06 16:09 PDT (History)
7 users (show)

See Also:


Attachments
Patch (20.25 KB, patch)
2016-08-30 15:32 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
[Image] After Patch is applied (192.73 KB, image/png)
2016-08-30 15:32 PDT, Devin Rousso
no flags Details
[Image] patch applied, double border (146.82 KB, image/png)
2016-08-30 16:34 PDT, Matt Baker
no flags Details
[Image] Empty section (34.55 KB, image/png)
2016-08-30 17:00 PDT, Nikita Vasilyev
no flags Details
Patch (20.59 KB, patch)
2016-08-30 17:45 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (21.06 KB, patch)
2016-09-02 18:19 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 2016-08-30 14:25:00 PDT
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.
Comment 1 Radar WebKit Bug Importer 2016-08-30 14:25:31 PDT
<rdar://problem/28083228>
Comment 2 Devin Rousso 2016-08-30 15:32:13 PDT
Created attachment 287440 [details]
Patch
Comment 3 Devin Rousso 2016-08-30 15:32:45 PDT
Created attachment 287441 [details]
[Image] After Patch is applied
Comment 4 Matt Baker 2016-08-30 16:27:31 PDT
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").
Comment 5 Nikita Vasilyev 2016-08-30 16:31:40 PDT
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.
Comment 6 Matt Baker 2016-08-30 16:34:59 PDT
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).
Comment 7 Nikita Vasilyev 2016-08-30 16:49:48 PDT
(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.
Comment 8 Nikita Vasilyev 2016-08-30 17:00:10 PDT
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.
Comment 9 Devin Rousso 2016-08-30 17:10:08 PDT
(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.
Comment 10 Devin Rousso 2016-08-30 17:42:43 PDT
(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.
Comment 11 Devin Rousso 2016-08-30 17:45:53 PDT
Created attachment 287468 [details]
Patch
Comment 12 Matt Baker 2016-08-30 18:56:14 PDT
(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.
Comment 13 Devin Rousso 2016-08-30 18:57:58 PDT
(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).
Comment 14 Brian Burg 2016-09-01 12:51:28 PDT
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.
Comment 15 Devin Rousso 2016-09-02 18:19:27 PDT
Created attachment 287842 [details]
Patch
Comment 16 Brian Burg 2016-09-06 16:04:36 PDT
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
Comment 17 WebKit Commit Bot 2016-09-06 16:08:52 PDT
Comment on attachment 287842 [details]
Patch

Clearing flags on attachment: 287842

Committed r205518: <http://trac.webkit.org/changeset/205518>
Comment 18 WebKit Commit Bot 2016-09-06 16:08:57 PDT
All reviewed patches have been landed.  Closing bug.