Bug 183627 – Web Inspector: Styles Redesign: rework Computed panel to use Spreadsheet classes

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

[Devin Rousso]

It seems very unnecessary to use CodeMirror for the Computed panel, especially since none of it is editable.  This would massively simplify (and lighten the load) of the Computed panel, and remove some bugs in the process.

[Devin Rousso]

Created attachment 335796 [details]
Patch

This is a reviewable patch, but it is missing tabbing support.  Not sure whether it'll be better to address that in this bug or a followup, as I think it'll be pretty hefty.

[Devin Rousso]

Created attachment 348275 [details]
Patch

Rebase

[Joseph Pecoraro]

In general I think this looks and feels good. A few things that are different (which may just because that is how it is implemented in the SpreadSheet editor).

    1. Less syntax highlighting (numbers, keywords, etc)
    2. Filtering now matches the entire property + value instead of just the search term
    3. Sometimes when switching between nodes the pseudo class checkboxes are visible where they weren't before
    4. Text selection of properties where `display`, `width`, or `height` had an implicit value displays poorly
    5. PRO: Color values have color swatches

I also saw multiple existing bugs, which exhibited before and after the patch.

I certainly think this is a step in the right direction to be more consistent.

[Joseph Pecoraro]

Comment on attachment 348275 [details]
Patch

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

I think this is very close to landing.

I did notice one difference which seems like a bug. Filtering did not behave as expected in the Variables section:

Steps to Reproduce:
1. Inspect the inspector
2. Select an element in inspector²
3. Show Computed styles
4. Filter for "size"
    => Should show `box-size` in properties, and some variables. Actually shows lots of variables...

Perhaps we should be setting:

    this._variablesTextEditor.hideFilterNonMatchingProperties = true;

> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.js:141
> +        this._variablesTextEditor = new WI.SpreadsheetCSSStyleDeclarationEditor(this);

Should we enable `hideFilterNonMatchingProperties`?

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:55
> -        if (!this.style.editable)
> +        if (!this._style || !this._style.editable)

When might this._style be null?

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:202
> +        if (this._style) {

Style: Early return to avoid nesting the entire function.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:208
> +            if (this._style.type === WI.CSSStyleDeclaration.Type.Computed) {

It is weird that we only apply the `propertyVisibilityMode` and `showsImplicitProperties` filters when the type is Computed. Nothing about those editor properties seems specific to Computed. I know they aren't used outside of Computed right now though.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:315
> +                if (this._hideFilterNonMatchingProperties)
> +                    this.element.append(propertyView.element);

Maybe this should avoid calling `append` if `this.element.parentNode` is non-null? Otherwise it might try to remove and re-append at the end?

[Devin Rousso]

Comment on attachment 348275 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.js:141
>> +        this._variablesTextEditor = new WI.SpreadsheetCSSStyleDeclarationEditor(this);
> 
> Should we enable `hideFilterNonMatchingProperties`?

Yup!  Oops :P

>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:55
>> +        if (!this._style || !this._style.editable)
> 
> When might this._style be null?

When created by the computed tab, they are initialized inside `initialLayout`, meaning that the `refresh` call won't happen until later, which is where the `style` is set.

>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:208
>> +            if (this._style.type === WI.CSSStyleDeclaration.Type.Computed) {
> 
> It is weird that we only apply the `propertyVisibilityMode` and `showsImplicitProperties` filters when the type is Computed. Nothing about those editor properties seems specific to Computed. I know they aren't used outside of Computed right now though.

Agreed.  Will change.

>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:315
>> +                    this.element.append(propertyView.element);
> 
> Maybe this should avoid calling `append` if `this.element.parentNode` is non-null? Otherwise it might try to remove and re-append at the end?

We don't want to do that actually, because otherwise previously removed `propertyView` won't get inserted at the right spot.  By re-adding to the bottom, we effectively keep the same order as before because they ALL get re-added to the end.

[Devin Rousso]

Created attachment 349916 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 349916 [details]
Patch

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

r=me

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:234
> +
> +

Style: Extra empty line.

[Devin Rousso]

Created attachment 350282 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 350282 [details]
Patch

Clearing flags on attachment: 350282

Committed r236297: <https://trac.webkit.org/changeset/236297>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/44664917>