Bug 191984 – Web Inspector: Computed panel: allow to expand properties to show list of overridden values

• Component: Web Inspector
• Status: RESOLVED FIXED

[Nikita Vasilyev]

Redesign Computed panel of the Elements tab.

Each property should expand to show a list of overridden values.

<rdar://problem/44266652>

[Nikita Vasilyev]

Created attachment 355686 [details]
Patch

[Nikita Vasilyev]

Created attachment 355687 [details]
[Image] With patch applied

[Devin Rousso]

Comment on attachment 355686 [details]
Patch

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

r-, as this breaks tabbing

When filtering, I also noticed that the "Show All" checkbox remains visible even if no matches were found.  Filtering for values found in the override chain but not in any computed value is also somewhat "broken", in that the properties are hidden (e.g. if `display: flex;` gets overridden by `display: block;` and I filter for "flex", the property is not visible even if I've expanded `display` and am able to see `display: flex;`).  Not sure how we want that to work 🤔  Frankly, all of the changes like this should be in a separate patch.

I get the following error when tabbing from the pseudo-class checkboxes:
> TypeError:​ this._computedStyleSection.focus is not a function. (In 'this._computedStyleSection.focus()​', 'this._computedStyleSection.focus' is undefined)​ (at ComputedStyleDetailsPanel.js:​87:​41)​
>     focusFirstSection @ ComputedStyleDetailsPanel.js:​87:​41
>     _handleForcedPseudoClassCheckboxKeydown @ GeneralStyleDetailsSidebarPanel.js:​263:​46
>     _handleForcedPseudoClassCheckboxKeydown @ [native code]​

The majority of my comments are related to design.  I think this looks really nice, but the way in which it's built could use some reworking.

Also, is there a reason you're not using this for the Variables section as well?

> Source/WebInspectorUI/ChangeLog:8
> +        Introduce the new experimental Computed panel with traces.

I understand what you're getting at here, but in my mind "trace" has a very specific meaning (backtrace in JS), so maybe use a different word ("override chain")?

> Source/WebInspectorUI/ChangeLog:17
> +        Remove "Properties" section header and move "Show All" checkbox to the left so
> +        it isn't covered by the scrollbar.

Why was this done?  It is weird to have everything else in the sidebar be part of a collapsable section, but not have the computed style be in a collapsable section?

> Source/WebInspectorUI/UserInterface/Base/Setting.js:133
> +    experimentalEnableComputedStylesWithTraces: new WI.Setting("experimental-enable-computed-styles-with-traces", true),

All experimental settings should start as off (`false`)

> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.css:56
> +    padding-left: var(--disclosure-button-size);

-webkit-padding-start, unless this shouldn't be RTL-compatible?

> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.js:50
> +            let propertyTraces = this._computePropertyTraces(this.nodeStyles.uniqueOrderedStyles);
> +            this._computedStyleSection.styleTraces = propertyTraces;

NIT: you could just directly assign the value instead of saving it to a local variable

> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.js:108
> +            computedStyleShowAllLabel.append(this._computedStyleShowAllCheckbox, WI.UIString("Show All"));

See ChangeLog:16

It seems weird to have a standalone label-checkbox be at the top of a section with no title.  I'd expect this to be the last item.

> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.js:124
> +            this._computedStyleSection.propertyVisibilityMode = WI.SpreadsheetCSSStyleDeclarationEditor.PropertyVisibilityMode.HideVariables;

If/When we remove this code, we can also remove all support for `WI.SpreadsheetCSSStyleDeclarationEditor.PropertyVisibilityMode`, as this was the only user =D

> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.js:138
> +            this.element.appendChild(this._propertiesSection.element);

This shouldn't be necessary because of the `addSubview` on line 136

> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.js:157
> -        this.element.appendChild(this._propertiesSection.element);
> +        if (!WI.settings.experimentalEnableComputedStylesWithTraces.value)
> +            this.element.appendChild(this._propertiesSection.element);

Ditto (138)

Not sure why this was added/needed to begin with 🤔

> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.js:186
> +                if (!result.has(property.name))
> +                    result.set(property.name, []);
> +
> +                result.get(property.name).push(property);

Rather than do a `has` and a `get`, you can use the result of `get` to infer `has`:

    let properties = result.get(property.name);
    if (!Array.isArray(properties)) {
        properties = [];
        result.set(property.name, properties);
    }
    properties.push(property);

> Source/WebInspectorUI/UserInterface/Views/ComputedStyleSection.css:27
> +    font: 12px -webkit-system-font, sans-serif;

NIT: I have a personal ordering that I use for CSS:

display/visibility/etc
position/float/etc
sizing
margin
padding
content
background
border
outline
miscellaneous

> Source/WebInspectorUI/UserInterface/Views/ComputedStyleSection.css:28
> +    padding-bottom: 3px;

This causes there to be a small area after the last property if it is expanded that has the non-expanded background color

> Source/WebInspectorUI/UserInterface/Views/ComputedStyleSection.css:37
> +    padding-left: calc(var(--disclosure-button-size));
> +    padding-right: var(--css-declaration-horizontal-padding);

Ditto ComputedStyleDetailsPanel.css:56

> Source/WebInspectorUI/UserInterface/Views/ComputedStyleSection.css:38
> +

Style: don't add extra newlines

> Source/WebInspectorUI/UserInterface/Views/ComputedStyleSection.css:43
> +

Ditto (38)

> Source/WebInspectorUI/UserInterface/Views/ComputedStyleSection.css:49
> +    background-color: var(--background-color);

Why are we changing the background color when we expand the property?  Shouldn't we be always be using this background color, just like the Variables section and all of the sections in the Rules sidebar panel?

> Source/WebInspectorUI/UserInterface/Views/ComputedStyleSection.css:61
> +    margin-left: calc(-1 * var(--disclosure-button-size));

Ditto ComputedStyleDetailsPanel.css:56

> Source/WebInspectorUI/UserInterface/Views/ComputedStyleSection.css:99
> +    text-align: right;

Ditto ComputedStyleDetailsPanel.css:56 (`text-align: end;`)

> Source/WebInspectorUI/UserInterface/Views/ComputedStyleSection.css:105
> +    padding-right: 0.6em;

Ditto ComputedStyleDetailsPanel.css:56

> Source/WebInspectorUI/UserInterface/Views/ComputedStyleSection.js:42
> +        this._propertyVisibilityMode = WI.ComputedStyleSection.PropertyVisibilityMode.ShowAll;
> +        this._hideFilterNonMatchingProperties = false;

Considering this class is only used once, you could remove these values and associated logic for simplicity.

> Source/WebInspectorUI/UserInterface/Views/ComputedStyleSection.js:48
> +    layout()

Please add a `super.layout()` call at the top of this function

> Source/WebInspectorUI/UserInterface/Views/ComputedStyleSection.js:73
> +            let expandableItem = new WI.ExpandableItem(property.name, propertyView.element, traceElement);

`property.name` is not really a specific enough key for a `WI.Setting`, unless you want it to be that every `display` gets auto-expanded once you expand any `display` once?  I'd imagine it to be based on the path of the DOM node for that property (similar to the collapsed state of the "Properties" section for the selected node in the "Node" sidebar panel).

> Source/WebInspectorUI/UserInterface/Views/ComputedStyleSection.js:79
> +    detached()

Please add a `super.detached()` call at the top of this function

> Source/WebInspectorUI/UserInterface/Views/ComputedStyleSection.js:91
> +    get style()

Style: we prefer putting getters/setters above "regular" functions

> Source/WebInspectorUI/UserInterface/Views/ComputedStyleSection.js:102
> +            this._style.removeEventListener(WI.CSSStyleDeclaration.Event.PropertiesChanged, this._propertiesChanged, this);

NIT: I like to prefix all event handler callbacks with `_handle*`

> Source/WebInspectorUI/UserInterface/Views/ComputedStyleSection.js:119
> +        if (this._styleTraces === styleTraces)

Considering that `styleTraces` will be an array, we should also use `Array.shallowEqual` or just drop it completely

> Source/WebInspectorUI/UserInterface/Views/ComputedStyleSection.js:138
> +        this._alwaysShowPropertyNames = new Set(propertyNames);

Instead of constructing a `Set` inside this function, I think it's better to expect that a `Set` be passed in as an argument

> Source/WebInspectorUI/UserInterface/Views/ComputedStyleSection.js:187
> +    highlightProperty(property)

This doesn't appear to be used outside of the Rules sidebar panel

> Source/WebInspectorUI/UserInterface/Views/ComputedStyleSection.js:254
> +            return traceElement;

It seems weird to create an element that has nothing in it and isn't actually displayed

> Source/WebInspectorUI/UserInterface/Views/ComputedStyleSection.js:279
> +            let selectorElement = document.createElement("span");
> +            selectorElement.textContent = selectorText.truncateMiddle(24);
> +            selectorElement.className = "selector";

This has VERY weird wrapping at any semi-small width.  I get what you're trying to go for here (have every value be aligned to the same horizontal start "point"), but I don't think we have enough room to make this feasible, unless we do far more aggressive truncation (e.g. via CSS).

> Source/WebInspectorUI/UserInterface/Views/ExpandableItem.js:26
> +WI.ExpandableItem = class ExpandableItem

NIT: I'd rather you call this something like `WI.ExpandableElement`, as "item" implies a list to me

> Source/WebInspectorUI/UserInterface/Views/ExpandableItem.js:41
> +        let settingKey = "expanded-" + key;

NIT: you can inline this

> Source/WebInspectorUI/UserInterface/Views/ExpandableItem.js:61
> +        const shouldExpand = !this._element.classList.contains("expanded");

You should use the value of the `WI.Setting`, not the DOM for state

    let shouldExpand = !this._expandedSetting.value;
    this._update(shouldExpand);

> Source/WebInspectorUI/UserInterface/Views/SettingsTabContentView.js:279
> +        listenForChange(WI.settings.experimentalEnableComputedStylesWithTraces);

Does this actually require reloading WebInspector?  Wouldn't it use the new UI the next time the user shows the Elements tab?

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:28
> +    constructor(delegate, property, options = {})

Nice!

> Source/WebInspectorUI/UserInterface/Views/StyleOrigin.js:26
> +WI.StyleOrigin = class StyleOrigin

This name makes me think it's a model object.  It should have `View` or something to that effect as a suffix.

> Source/WebInspectorUI/UserInterface/Views/StyleOrigin.js:28
> +    constructor(style)

Rather than having this class effectively be a factory for producing origin DOM structure, I think you'd be better having the DOM creation be inside a `layout` so that `WI.SpreadsheetCSSStyleDeclarationSection` doesn't have to do a `replaceElement` and can instead just call `this._originView..needsLayout()` (the same is true for Computed)

[Nikita Vasilyev]

Comment on attachment 355686 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.js:124
>> +            this._computedStyleSection.propertyVisibilityMode = WI.SpreadsheetCSSStyleDeclarationEditor.PropertyVisibilityMode.HideVariables;
> 
> If/When we remove this code, we can also remove all support for `WI.SpreadsheetCSSStyleDeclarationEditor.PropertyVisibilityMode`, as this was the only user =D

Yes, that's the plan once the feature is no longer experimental!

>> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.js:157
>> +            this.element.appendChild(this._propertiesSection.element);
> 
> Ditto (138)
> 
> Not sure why this was added/needed to begin with 🤔

I preserved the existing logic. Yes, it's convoluted.

>> Source/WebInspectorUI/UserInterface/Views/ComputedStyleSection.css:28
>> +    padding-bottom: 3px;
> 
> This causes there to be a small area after the last property if it is expanded that has the non-expanded background color

This adds a little whitespace between the computed section and the next section (Variables or Box Model).

The one you're referring to is on line 48. The intention was to separate the expanded property from the next one but I don't feel strongly about it.

>> Source/WebInspectorUI/UserInterface/Views/ComputedStyleSection.css:49
>> +    background-color: var(--background-color);
> 
> Why are we changing the background color when we expand the property?  Shouldn't we be always be using this background color, just like the Variables section and all of the sections in the Rules sidebar panel?

I find it helps a lot to see which sections are expanded. Items in the Variables section don't expand.

>> Source/WebInspectorUI/UserInterface/Views/ComputedStyleSection.js:42
>> +        this._hideFilterNonMatchingProperties = false;
> 
> Considering this class is only used once, you could remove these values and associated logic for simplicity.

I'll refactor this once this the old Computed panel is removed.

>> Source/WebInspectorUI/UserInterface/Views/ComputedStyleSection.js:48
>> +    layout()
> 
> Please add a `super.layout()` call at the top of this function

Whoops.

>> Source/WebInspectorUI/UserInterface/Views/ComputedStyleSection.js:73
>> +            let expandableItem = new WI.ExpandableItem(property.name, propertyView.element, traceElement);
> 
> `property.name` is not really a specific enough key for a `WI.Setting`, unless you want it to be that every `display` gets auto-expanded once you expand any `display` once?  I'd imagine it to be based on the path of the DOM node for that property (similar to the collapsed state of the "Properties" section for the selected node in the "Node" sidebar panel).

Yes, I want every `display` to be expanded in your example.

When I was using Computed while working on it, I found myself expanding `color` and `font` quite often. It felt right to save the expanded status by property name.

>> Source/WebInspectorUI/UserInterface/Views/ComputedStyleSection.js:138
>> +        this._alwaysShowPropertyNames = new Set(propertyNames);
> 
> Instead of constructing a `Set` inside this function, I think it's better to expect that a `Set` be passed in as an argument

This is exactly as in the old Computed panel. I think we should define this list in WI.ComputedStyleSection and remove the setter entirely.

>> Source/WebInspectorUI/UserInterface/Views/ComputedStyleSection.js:187
>> +    highlightProperty(property)
> 
> This doesn't appear to be used outside of the Rules sidebar panel

Good catch!

>> Source/WebInspectorUI/UserInterface/Views/ComputedStyleSection.js:279
>> +            selectorElement.className = "selector";
> 
> This has VERY weird wrapping at any semi-small width.  I get what you're trying to go for here (have every value be aligned to the same horizontal start "point"), but I don't think we have enough room to make this feasible, unless we do far more aggressive truncation (e.g. via CSS).

Yes, I agree! This is very weird. I plan to replace this table layout with flexbox or even CSS grid, I just didn't have enough time.

>> Source/WebInspectorUI/UserInterface/Views/SettingsTabContentView.js:279
>> +        listenForChange(WI.settings.experimentalEnableComputedStylesWithTraces);
> 
> Does this actually require reloading WebInspector?  Wouldn't it use the new UI the next time the user shows the Elements tab?

It wouldn't use the new UI right away. It's easier to reload.

>> Source/WebInspectorUI/UserInterface/Views/StyleOrigin.js:26
>> +WI.StyleOrigin = class StyleOrigin
> 
> This name makes me think it's a model object.  It should have `View` or something to that effect as a suffix.

Good point.

[Nikita Vasilyev]

(In reply to Devin Rousso from comment #3)
> Comment on attachment 355686 [details]
> 
> I get the following error when tabbing from the pseudo-class checkboxes:
> > TypeError:​ this._computedStyleSection.focus is not a function. (In 'this._computedStyleSection.focus()​', 'this._computedStyleSection.focus' is undefined)​ (at ComputedStyleDetailsPanel.js:​87:​41)​
> >     focusFirstSection @ ComputedStyleDetailsPanel.js:​87:​41
> >     _handleForcedPseudoClassCheckboxKeydown @ GeneralStyleDetailsSidebarPanel.js:​263:​46
> >     _handleForcedPseudoClassCheckboxKeydown @ [native code]​

Good find, but this isn't a regression from the patch. It's broken in ToT.

[Nikita Vasilyev]

Created attachment 355767 [details]
Patch

[Nikita Vasilyev]

Created attachment 355768 [details]
[Image] With patch applied

[Nikita Vasilyev]

Created attachment 355769 [details]
[Image] Current Computed panel in RTL

The current computed section is unusable in RTL.
I'm making the new computed section always LTR because CSS is strictly LTR.

[Nikita Vasilyev]

This patch preserves all the features of the current Computed section. Filtering works, but it doesn't search in the traces.

I'm happy to iterate on the UI in the follow-up patches.

[Devin Rousso]

Created attachment 355774 [details]
[Image] Screenshot of Issue with last property

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

>>> Source/WebInspectorUI/UserInterface/Views/ComputedStyleSection.css:28
>>> +    padding-bottom: 3px;
>> 
>> This causes there to be a small area after the last property if it is expanded that has the non-expanded background color
> 
> This adds a little whitespace between the computed section and the next section (Variables or Box Model).
> 
> The one you're referring to is on line 48. The intention was to separate the expanded property from the next one but I don't feel strongly about it.

I actually was referring specifically to this, in that it adds a few pixels of the non-expanded color below the last property, which is weird when that property is expanded.  I don't think we need extra spacing above/below the section (or the properties themselves).

[Devin Rousso]

Comment on attachment 355767 [details]
Patch

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

r=me, with some expected followups:
 - Add tabbing support
 - Discuss/Finalize styling
 - Move Variables section to use this code as well

Looks great :)

> Source/WebInspectorUI/ChangeLog:97
> +        Replace `_renderOrigin` with WI.StyleOrigin so it could be used by WI.ComputedStyleSection. 

WI.StyleOriginView

> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.css:47
> +    background-color: hsl(0, 0%, 97%);

I personally think that we should keep the collapsed color be the standard `--background-color`, and having this intermediate color is somewhat confusing, but we could discuss/fix that later.

> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.css:51
> +    font: 12px -webkit-system-font, sans-serif;

The `font-size` of the values in the "Variables" and "Box Model" sections is `11px` (this is our standard `font-size` as well).

> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.js:171
> +                if (properties === undefined) {

Style: use `!` instead of `=== undefined` (we aren't that explicit unless `null` or `false` are valid values), or even check with `Array.isArray`.

> Source/WebInspectorUI/UserInterface/Views/ComputedStyleSection.js:235
> +                if (ownerRule)

We should have some text for inline styles as well.

> Source/WebInspectorUI/UserInterface/Views/ExpandableView.js:59
> +        const shouldExpand = !this._expandedSetting.value;

Style: use `let` not `const` (I reserve `const` for values that shouldn't change over the life of the program or even between runs of the program).

> Source/WebInspectorUI/UserInterface/Views/StyleOriginView.js:28
> +    constructor(style)

I'd reiterate my comment in attachment 355686 [details], in that you should rework this class to have an `update` function that takes a `WI.CSSStyleDeclaration` and regenerates its DOM so that you don't need to call `replaceWith`.

[Devin Rousso]

(In reply to Devin Rousso from comment #11)
> r=me, with some expected followups:
>  - Add tabbing support
>  - Discuss/Finalize styling
>  - Move Variables section to use this code as well
We might also want to add go-to arrows for items in the override chain, as well as having filtering be based on expanded override chains too.

[Devin Rousso]

Comment on attachment 355767 [details]
Patch

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

I forgot to mention this:

> Source/WebInspectorUI/ChangeLog:8
> +        Introduce the new experimental Computed panel with traces.

I realize that much of this patch uses the word "Trace", and while that's fine for our class names and file names, I don't think that word is going to be familiar to developers in this context.  I think most developers would be more likely to understand "Overrides" or "Override Chain" or even "Cascade".  If you do simple web searches for "CSS Trace" vs "CSS Override" or "CSS Cascade", I think you'll see what I mean.  We shouldn't use "Trace" in anything shown to the user.

[Nikita Vasilyev]

Created attachment 355790 [details]
Patch

[Nikita Vasilyev]

(In reply to Devin Rousso from comment #11)
> The `font-size` of the values in the "Variables" and "Box Model" sections is
> `11px` (this is our standard `font-size` as well).

I used a non-monospaced font. It allowed fitting significantly more data into the narrow sidebar.

12px San Francisco font (`-webkit-system-font` on macOS 10.13+) vertically is very similar to 11px monospaced Menlo.

In the console, we also use 12px San Francisco.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 355790 [details]:

workers/bomb.html bug 171985 (author: [e-mail])
inspector/console/command-line-api.html bug 192047 (authors: [e-mail] and [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 355790 [details]:

webgl/1.0.2/conformance/more/functions/bufferSubData.html bug 192048 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 355790 [details]:

workers/bomb.html bug 171985 (author: [e-mail])
imported/w3c/web-platform-tests/encoding/legacy-mb-tchinese/big5/big5-encode-form-errors-hangul.html bug 192052 (author: [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 355790 [details]
Patch

Clearing flags on attachment: 355790

Committed r238589: <https://trac.webkit.org/changeset/238589>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.