Bug 175343 – Web Inspector: Styles Redesign: hook up real data to spreadsheet style editor

• Component: Web Inspector
• Status: RESOLVED FIXED

[Nikita Vasilyev]

Replace static HTML added in bug 174838: Web Inspector: Styles: Add pre-populated data to spreadsheet-style view
with actual CSS rules of the inspected element.

<rdar://problem/33525145>

[Radar WebKit Bug Importer]

<rdar://problem/33784793>

[Nikita Vasilyev]

Created attachment 319057 [details]
[Screenshot] With patch applied

Not implemented yet:
- "Inherited from: [selector]" sections
- "@media query" sections

[Nikita Vasilyev]

Created attachment 319128 [details]
WIP

Not ready for review, but you can apply the patch and play around with the new UI.

[Nikita Vasilyev]

Created attachment 319146 [details]
Patch

[Nikita Vasilyev]

Created attachment 319161 [details]
Patch

Introduced SpreadsheetCSSStyleDeclarationEditor.css file.

[Devin Rousso]

Comment on attachment 319146 [details]
Patch

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

The UI of this looks great.  Can't wait to see it fully working 😁

Mostly style comments and some questions, but I'd like to see these answered/updated before committing.

> Source/WebInspectorUI/ChangeLog:9
> +        Replace static HTML added in Bug 174838 with an actual data.

Using a link here would probably be easier to read.

    Replace static HTML added in <https://webkit.org/b/174838> with an actual data.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:43
> +    render()

Would `update()` be a better name here?  Render doesn't really fit IMO :/

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:54
> +        this.element.removeChildren();
> +        let properties = this._propertiesToRender;
> +
> +        if (!properties.length)
> +            this.element.classList.add("no-properties");
> +        else
> +            this.element.classList.remove("no-properties");
> +
> +        for (let property of properties)
> +            this.element.append(this.renderProperty(property));

Style: add newline between.

    this.element.removeChildren();

    let properties = this._propertiesToRender;
    this.element.classList.toggle("no-properties", !properties.length);

    for (let property of properties)
        this.element.append(this.renderProperty(property));

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:61
> +        let propertyElement = document.createElement("div");
> +        let nameElement = document.createElement("span");
> +        let valueElement = document.createElement("span");

The ordering of these variables is odd to me.  I think you can structure it so that if flows a bit better.

    let propertyElement = document.createElement("div");
    propertyElement.classList.add("property");

    let nameElement = propertyElement.appendChild(document.createElement("span"));
    nameElement.classList.add("name");
    nameElement.textContent = cssProperty.name;

    propertyElement.append(": ");

    let valueElement = propertyElement.appendChild(document.createElement("span"));
    valueElement.classList.add("value");
    valueElement.textContent = cssProperty.value;

    propertyElement.append(";");

    return propertyElement;

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:74
> +    get delegate() { return this._delegate; }

Style: either put both the get/set on one line or make them both multi-line.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:80
> +    get style() { return this._style; }

Ditto.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:100
> +        if (style._styleSheetTextRange)
> +            return style.visibleProperties;
> +        else
> +            return style.properties;

Why is this branch needed?  Is there a case where we want to show all properties, instead of just the visible ones?  AFAIU, visibleProperties exists so that longhand properties that are created for shorthand properties are not visible in the editor.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.js:26
> +WI.SpreadsheetCSSStyleDeclarationSection = class SpreadsheetCSSStyleDeclarationSection extends WI.Object

It seems like you've added functions used by View, so is there a reason you don't want to just subclass View?  I think we are trying to move larger UI constructs in that direction anyways.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.js:36
> +        this._style = style || null;

NIT: since you assert above, I would drop the "|| null".

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.js:39
> +        this._element = document.createElement("section");

FWICT, we haven't ever used a <section> in WebInspector.  Is there a reason we want to use it here?

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.js:46
> +        this._originElement = this._headerElement.createChild("span", "origin");
> +        this._selectorElement = this._headerElement.createChild("span", "selector");

NIT: I think Joe wants to phase out usage of createChild :(

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.js:59
> +        this._element.append(this._headerElement, openBrace, this._propertiesEditor.element, closeBrace);

I personally like to call append/appendChild in the same line as the construction of the element, but I am willing to go either way on this.  I think, however, that it warrants a discussion as to how we intend to use append() in the future, and whether we want to replace appendChild() with append().

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.js:64
> +        if (!style.editable)
> +            this._element.classList.add(WI.SpreadsheetCSSStyleDeclarationSection.LockedStyleClassName);
> +        else if (!style.ownerRule)
> +            this._element.classList.add(WI.SpreadsheetCSSStyleDeclarationSection.SelectorLockedStyleClassName);

NIT: these classes are only ever used here.  I think you can remove the value on WI.SpreadsheetCSSStyleDeclarationSection and just inline the string.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.js:74
> +    get element()
> +    {
> +        return this._element;
> +    }

Style: one line.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.js:79
> +    get style()
> +    {
> +        return this._style;
> +    }

Style: one line.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.js:81
> +    render()

Same comment about render() as above.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.js:89
> +        function appendSelector(selector, matched)
> +        {

Style: same line bracket.

Also, instead of invoking this with call(), I think it's easier to just create it as a variable arrow function.

    let appendSelector = (selector, matched) => {
        ...
    };

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.js:116
> +            this._selectorElement.appendChild(selectorElement);

Move this onto the same line as createElement.

    let selectorElement = this._selectorElement.appendChild(document.createElement("span"));

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.js:121
> +        function appendSelectorTextKnownToMatch(selectorText)
> +        {

Style: same line bracket.

Also, instead of invoking this with call(), I think it's easier to just create it as a variable arrow function.

    let appendSelectorTextKnownToMatch = (selectorText) => {
        ...
    };

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.js:125
> +            this._selectorElement.appendChild(selectorElement);

Move this onto the same line as createElement.

    let selectorElement = this._selectorElement.appendChild(document.createElement("span"));

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.js:145
> +                this._element.classList.toggle(WI.SpreadsheetCSSStyleDeclarationSection.PseudoElementSelectorStyleClassName, hasMatchingPseudoElementSelector);

NIT: this class is only ever used here.  I think you can remove the value on WI.SpreadsheetCSSStyleDeclarationSection and just inline the string.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.js:147
> +                appendSelectorTextKnownToMatch.call(this, this._style.ownerRule.selectorText);

See above.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.js:155
> +                };
> +                if (this._style.ownerStyleSheet.isInspectorStyleSheet()) {

Style: add newline.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.js:163
> +                let originString;

Style: give this a default value.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.js:187
> +                    let styleLabel;

Style: give this a default value.
Style: I think `styleTitle` would be a better name.  When I read "label" I think of a <form><label>.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.js:213
> +        // Unimplemented.

Why is this needed?  See comment on line 26.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.js:218
> +        // Unimplemented.

Is this needed?  If so, please create a bug and link it here.  Otherwise, I'd rather you change the call-site to make it check for the existence of the function and not add it at all.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.js:223
> +        if (typeof this._delegate.cssStyleDeclarationSectionEditorFocused === "function")

You should also check that `this._delegate` is not null.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.js:229
> +        // Unimplemented.

Is this needed?  If so, please create a bug and link it here.  Otherwise, I'd rather you change the call-site to make it check for the existence of the function and not add it at all.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.js:234
> +        // Unimplemented.

Ditto.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.js:239
> +        // Unimplemented.

Ditto.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.js:250
> +    get selectorEditable()
> +    {
> +        return !this.locked && this._style.ownerRule;
> +    }
> +
> +    get locked()
> +    {
> +        return !this._style.editable;
> +    }

NIT: I would switch the order of these two functions, since locked() is used by selectorEditable().

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.js:256
> +WI.SpreadsheetCSSStyleDeclarationSection.Event = {
> +    Blurred: "css-style-declaration-sections-blurred"
> +};

This isn't used in this patch.  Please remove it.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStylesDeclarationSection.css:1
> +.spreadsheet-css-declaration {

Is this file a copy of another file?  I typically follow an ordering to my CSS:

display (and related properties, such as for flexbox)
position (and related properties, such as top/bottom)
float
width/height
margin
padding
content (font, color, etc)
background
border
outline
miscellaneous

It's up to you as to how you want to organize it, but I find that it makes CSS much easier to read.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetRulesStyleDetailsPanel.js:30
> +        super(delegate, "rules", "rules", WI.UIString("Styles \u2014 Rules"));

Style: use const variables for the inline values.

    const className = "rules";
    const identifier = "rules";
    const label = WI.UIString("Styles \u2014 Rules");
    super(delegate, className, identifier, label);

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetRulesStyleDetailsPanel.js:44
> +        this._emptyFilterResultsElement.appendChild(this._emptyFilterResultsMessage);

Move this onto the same line as createElement.

    this._emptyFilterResultsMessage = this._emptyFilterResultsElement.appendChild(document.createElement("div"));

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetRulesStyleDetailsPanel.js:63
> +        function uniqueOrderedStyles(orderedStyles)

I think this function can entirely be replaced by creating a Set from orderedStyles.

    let uniqueOrderedStyles = new Set(this.nodeStyles.orderedStyles);

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetRulesStyleDetailsPanel.js:88
> +        // FIXME: Insert matching pseudo-styles.

Please create a bug for this and link it here.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetRulesStyleDetailsPanel.js:91
> +        function appendStyleSection(style)
> +        {

Style: same line bracket.

Also, instead of invoking this with call(), I think it's easier to just create it as a variable arrow function.

    let appendStyleSection = (style) => {
        ...
    };

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetRulesStyleDetailsPanel.js:92
> +            let section = style.__rulesSection;

I think we now prefer to use symbols for these types of expando properties.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetRulesStyleDetailsPanel.js:114
> +            // FIXME: Insert pseudo-style and inherited headers.

Please create a bug for this and link it here.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetRulesStyleDetailsPanel.js:122
> +        this.element.append(newDOMFragment, this._emptyFilterResultsElement);

Instead of creating a DocumentFragment and appending it here, why not utilize this._sections in the loop below and add each there?

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetRulesStyleDetailsPanel.js:127
> +        for (let i = 0; i < this._sections.length; ++i)
> +            this._sections[i].updateLayout();

With the above comment:

    for (let section of this._sections) {
        this.element.appendChild(section.element);
        section.updateLayout();
    }

    this.element.appendChild(this._emptyFilterResultsElement);

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetRulesStyleDetailsPanel.js:135
> +        // Unimplemented.

Is this needed?  If so, please create a bug and link it here.  Otherwise, I'd rather you change the call-site to make it check for the existence of the function and not add it at all.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetRulesStyleDetailsPanel.js:140
> +        // Unimplemented.

Ditto.

[Devin Rousso]

Comment on attachment 319146 [details]
Patch

Oops.  Looks like I was reviewing an old copy :(

[Nikita Vasilyev]

(In reply to Devin Rousso from comment #7)
> Comment on attachment 319146 [details]
> Patch
> 
> Oops.  Looks like I was reviewing an old copy :(

It's fine. You didn't comment on anything that is different between the two, it seems.

[Nikita Vasilyev]

Comment on attachment 319146 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:43
>> +    render()
> 
> Would `update()` be a better name here?  Render doesn't really fit IMO :/

Should I subclass WI.View and use initialLayout & layout instead?

>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:54
>> +            this.element.append(this.renderProperty(property));
> 
> Style: add newline between.
> 
>     this.element.removeChildren();
> 
>     let properties = this._propertiesToRender;
>     this.element.classList.toggle("no-properties", !properties.length);
> 
>     for (let property of properties)
>         this.element.append(this.renderProperty(property));

Good catch. I forgot about classList.toggle!

>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:74
>> +    get delegate() { return this._delegate; }
> 
> Style: either put both the get/set on one line or make them both multi-line.

This is straight from CSSStyleDeclarationTextEditor.js. I suppose it didn't follow our guidelines.

>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:100
>> +            return style.properties;
> 
> Why is this branch needed?  Is there a case where we want to show all properties, instead of just the visible ones?  AFAIU, visibleProperties exists so that longhand properties that are created for shorthand properties are not visible in the editor.

visibleProperties are the ones that have a resource with a corresponding text range:

    get visibleProperties()
    {
        if (this._visibleProperties)
            return this._visibleProperties;

        this._visibleProperties = this._properties.filter(function(property) {
            return !!property.styleDeclarationTextRange;
        });

        return this._visibleProperties;
    }

For example, user agent styles never have visibleProperties. For example, inspect body on any page and look for:

    body {
        display: block;
        margin-top: 8px;
        margin-right: 8px;
        margin-bottom: 8px;
        margin-left: 8px;
    }

visibleProperties have nothing to do with shorthand/longhand properties.

I don't know why they are called visibleProperties. The fact that they mislead you too, makes me think we should at least rename them or change CSSStyleDeclaration model logic.

>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.js:39
>> +        this._element = document.createElement("section");
> 
> FWICT, we haven't ever used a <section> in WebInspector.  Is there a reason we want to use it here?

I think we should use more semantic elements, such as <section>, <nav>, <header>, <footer>, and <aside>, to make the content more accessible. The fact that we haven't used them is rather oversight. I believe these elements should give tools such as VoiceOver and Jaws extra cues about web page structure.

>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.js:59
>> +        this._element.append(this._headerElement, openBrace, this._propertiesEditor.element, closeBrace);
> 
> I personally like to call append/appendChild in the same line as the construction of the element, but I am willing to go either way on this.  I think, however, that it warrants a discussion as to how we intend to use append() in the future, and whether we want to replace appendChild() with append().

I like Element#append. It can take several arguments. It can take strings — no need to document.createTextNode("}").

The only downside of Element#append that I can think of, is that Element#append returns undefined and it doesn't fit into a pattern we sometimes use:

    var aChild = someElement.appendChild(document.createElement("div"))

I personally find this hard to read, and I'd rather see this instead:

    var aChild = document.createElement("div")
    someElement.append(aChild)

[Nikita Vasilyev]

Comment on attachment 319146 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.js:89
>> +        {
> 
> Style: same line bracket.
> 
> Also, instead of invoking this with call(), I think it's easier to just create it as a variable arrow function.
> 
>     let appendSelector = (selector, matched) => {
>         ...
>     };

WI.SpreadsheetCSSStyleDeclarationSection#render is primarily a copy/paste of WI.CSSStyleDeclarationSection#refresh, which is very old. I'm going to rewrite most of it in the following couple of weeks. That being said, I'll address you comments in this patch.

>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.js:250
>> +    }
> 
> NIT: I would switch the order of these two functions, since locked() is used by selectorEditable().

selectorEditable reads poorly. I'll rewrite it to:

    return this._style.editable && this._style.ownerRule;

[Nikita Vasilyev]

Comment on attachment 319146 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetRulesStyleDetailsPanel.js:63
>> +        function uniqueOrderedStyles(orderedStyles)
> 
> I think this function can entirely be replaced by creating a Set from orderedStyles.
> 
>     let uniqueOrderedStyles = new Set(this.nodeStyles.orderedStyles);

Unfortunately, it's more complex than that...

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetRulesStyleDetailsPanel.js:76
> +                    if (rule.isEqualTo(existingStyle.ownerRule)) {

because `new Set([iterable])` uses strict equality for [iterable] values. I need to use rule.isEqualTo, which is:

    return Object.shallowEqual(this._id, rule.id);

>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetRulesStyleDetailsPanel.js:135
>> +        // Unimplemented.
> 
> Is this needed?  If so, please create a bug and link it here.  Otherwise, I'd rather you change the call-site to make it check for the existence of the function and not add it at all.

I suppose I could add

    // Protected

    filterDidChange(filterBar)
    {
        // Implemented by subclasses.
    }

to WI.StyleDetailsPanel. Both WI.SpreadsheetRulesStyleDetailsPanel and WI.RulesStyleDetailsPanel extend WI.StyleDetailsPanel.

[Nikita Vasilyev]

Created attachment 319531 [details]
Patch

[Nikita Vasilyev]

Comment on attachment 319531 [details]
Patch

A couple of small things I noticed after posting this patch. Since I know Devin is reviewing it already, I'll just mention them here instead of uploading a new patch.

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

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:33
> +        this.element.textContent = style.text;

This line should be removed. I used it for debugging.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetRulesStyleDetailsPanel.js:127
> +        super.refresh();

super.refresh(significantChange);

[Devin Rousso]

Comment on attachment 319531 [details]
Patch

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

I think it's very close to being good to land.  I'd like to see one more revision.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:51
> +        super.layout();

Is there a reason that this is at the end?  I would expect setup style functions to have super as the first line.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:54
> +    renderProperty(cssProperty)

NIT: This could be private.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:75
> +    set delegate(delegate) { this._delegate = delegate || null; }

NIT: this isn't used.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:100
> +        else

Style: since the `if` returns, this `else` isn't needed.

    if (this._style.styleSheetTextRange)
        return this._style.visibleProperties;
    return this._style.properties;

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.css:37
> +    float: right;

Is there no way around using floats?  They tend to just cause so much havoc... :(

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.js:44
> +    get element() { return this._element; }

This getter already exists on WI.View.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.js:82
> +        super.initialLayout();

See comment on SpreadsheetCSSStyleDeclarationEditor.js:51

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.js:103
> +                let tooltip = WI.UIString("Specificity: (%d, %d, %d)").format(specificity[0], specificity[1], specificity[2]);

NIT: you can use a spread here:

    let tooltip = WI.UIString("Specificity: (%d, %d, %d)").format(...specificity);

Also, do we want to call toLocaleString() on these numbers?

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.js:115
> +                let tooltip = WI.UIString("Specificity: No value for selected element");
> +                tooltip += "\n";
> +                tooltip += WI.UIString("Dynamically calculated for the selected element and did not match");

I think we usually just put the \n and second line in the same UIString.

    selectorElement.title = WI.UIString("Specificity: No value for selected element\nDynamically calculated for the selected element and did not match");

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.js:126
> +        let appendSelectorTextKnownToMatch = (selectorText) => {
> +            let selectorElement = this._selectorElement.appendChild(document.createElement("span"));
> +            selectorElement.textContent = selectorText;
> +            selectorElement.classList.add(WI.SpreadsheetCSSStyleDeclarationSection.MatchedSelectorElementStyleClassName);
> +        };

This function duplicates some logic inside `appendSelector`.  You could rework this function to return `selectorElement` and take another `matched` parameter, and then use it inside `appendSelector` so that this code only exists once.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.js:211
> +        super.layout();

See comment on SpreadsheetCSSStyleDeclarationEditor.js:51.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetRulesStyleDetailsPanel.js:56
> +            super.refresh();

You should also pass along `significantChange`, just for completeness.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetRulesStyleDetailsPanel.js:115
> +            previousSection.lastInGroup = true;

I realize that you copied much of this, but `lastInGroup` doesn't seem to be used anywhere.  It is used by CSSStyleDeclarationSection.js, but not in this patch.  I think that for now it would be better to remove it so we don't potentially bring unneeded code into the future.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetRulesStyleDetailsPanel.js:117
> +        this.removeAllSubviews();

It reads awkwardly to have this be after a loop that calls a function called `appendStyleSection`, especially since `appendStyleSection` doesn't actually add the section to the DOM.  Can you move this avoid the for-loop, or better yet combine/rework both loops into one?

[Nikita Vasilyev]

Comment on attachment 319531 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.css:37
>> +    float: right;
> 
> Is there no way around using floats?  They tend to just cause so much havoc... :(

I don't know any better alternative here. Moving the origin name on its own line looks wasteful when it's as short as "style.css:123".
Using columns (implemented via flexbox or tables) leaves very little space a CSS selector when the origin name is long, like "SpreadsheetCSSStyleDeclarationSection.css:1:12345".

>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.js:126
>> +        };
> 
> This function duplicates some logic inside `appendSelector`.  You could rework this function to return `selectorElement` and take another `matched` parameter, and then use it inside `appendSelector` so that this code only exists once.

I just tried this and I think it made code harder to follow.

        let appendSelectorText = (selectorText, matched) => {
            let selectorElement = this._selectorElement.appendChild(document.createElement("span"));
            selectorElement.textContent = selectorText;

            if (matched)
                selectorElement.classList.add(WI.SpreadsheetCSSStyleDeclarationSection.MatchedSelectorElementStyleClassName);
            
            return selectorElement;
        };

1. appendSelector uses selectorElement, so I need to return selectorElement from appendSelectorText.

2. appendSelectorTextKnownToMatch(selectorText) is used in two other places. If I convert it to appendSelectorText(selectorText, matched), every time I use it, I'd have to call it like this:
    
    const matched = true;
    appendSelectorText(this._style.node.displayName, matched);

[Nikita Vasilyev]

Comment on attachment 319531 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.js:115
>> +                tooltip += WI.UIString("Dynamically calculated for the selected element and did not match");
> 
> I think we usually just put the \n and second line in the same UIString.
> 
>     selectorElement.title = WI.UIString("Specificity: No value for selected element\nDynamically calculated for the selected element and did not match");

I imagine this would make it harder for localizers to keep consistent with other "Dynamically calculated for ..." strings.

[Nikita Vasilyev]

Created attachment 319559 [details]
Patch

[Devin Rousso]

Comment on attachment 319559 [details]
Patch

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

r=me

This looks really nice!  I know I'm in the minority here, but I miss the selector icons :(

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:33
> +        this.element.classList.add(WI.SpreadsheetCSSStyleDeclarationEditor.StyleClassName);
> +        this._style = style;

Style: add newline

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:63
> +        this.layout();

NIT: should be `needsLayout(WI.View.LayoutReason.Dirty)`

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.js:38
> +        this._delegate = delegate || null;
> +
> +        this._style = style;

Style: unnecessary newline

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.js:165
> +                let originString = "";
> +                switch (this._style.ownerRule.type) {

Style: I'd personally add a newline for readability, but it's up to you

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.js:201
> +            this._selectorElement.append(WI.UIString("Style Attribute"));

NIT: this could just be `textContent`

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetRulesStyleDetailsPanel.js:92
> +            let section = style[WI.SpreadsheetRulesStyleDetailsPanel.RuleSection];
> +
> +            if (!section) {

Style: unnecessary newline

[Nikita Vasilyev]

Created attachment 319859 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 319859 [details]
Patch

Clearing flags on attachment: 319859

Committed r221594: <http://trac.webkit.org/changeset/221594>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.