Bug 193940 – Web Inspector: Changes: group CSS rules by resource

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

[Nikita Vasilyev]

Created attachment 360403 [details]
[Video] Demo

Currently, Changes panel doesn't have any resource links.

Group CSS rules by their resource. Create resource sections with their headers being source links.

[Radar WebKit Bug Importer]

<rdar://problem/47617785>

[Nikita Vasilyev]

Created attachment 360411 [details]
Patch

[Nikita Vasilyev]

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

[Matt Baker]

Comment on attachment 360411 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.css:39
> +    padding-top: var(--css-declaration-vertical-padding);

We should style the empty message like the one in the Canvas tab > Traces sidebar panel. The message should be centered vertically and horizontally, and use title case to be consistent with similar panels in native apps and Web Inspector. I know this is somewhat unrelated, but what about changing the text to just "No CSS Changes"?

> Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.css:68
> +.changes-panel .unchanged.property {

Writing the selectors like this sorts better visually:

.changes-panel .property.unchanged {
    ...
}

.changes-panel .property.added {
    ...
}

.changes-panel .property.removed {
    ...
}

But I think the way you have them reads better (verb-object). Does anyone else have an opinion? I'll leave it up to you.

> Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.css:74
> +    background-color: hsl(70, 90%, 86%);

What prompted the background-color tweaks for .added and .removed?

> Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.css:104
> +        background-color: hsl(5, 40%, 28%);

Same question as above.

> Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.js:85
> +            let rulesList = stylesheets.get(cssRule.ownerStyleSheet);

First the map value is referred to as `rulesList`, then later when iterating it is `cssRules`. Let's pick one or the other.

> Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.js:114
> +    _renderRule(cssRule)

Since this creates and returns an element, how about `_createRuleElement(cssRule)`?

[Matt Baker]

Comment on attachment 360411 [details]
Patch

I noticed that adding a new rule, and changing an existing rule, causes two .resource-sections to be added for the same file. Is this intentional?

[Devin Rousso]

Comment on attachment 360411 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.css:33
> +    padding-top: var(--css-declaration-vertical-padding);

NIT: I typically put `padding` (a "positional" property) above `font` (a "content" property).

> Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.css:44
> +    border-bottom: 0.5px solid var(--text-color-quaternary);

How does this look in @1x?

> Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.css:47
> +.changes-panel .resource-section > .header {

NIT: I'd reorder this as follows:

    position: -webkit-sticky;
    top: 0;
    z-index: var(--z-index-header);
    padding: 4px 8px;
    background-color: var(--panel-background-color);
    border-bottom: 0.5px solid var(--text-color-quaternary);
    white-space: nowrap;
    overflow: hidden;
    text-overflow: ellipsis;

> Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.css:51
> +    border-bottom: 0.5px solid var(--text-color-quaternary);

Ditto (>44).

>> Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.css:68
>> +.changes-panel .unchanged.property {
> 
> Writing the selectors like this sorts better visually:
> 
> .changes-panel .property.unchanged {
>     ...
> }
> 
> .changes-panel .property.added {
>     ...
> }
> 
> .changes-panel .property.removed {
>     ...
> }
> 
> But I think the way you have them reads better (verb-object). Does anyone else have an opinion? I'll leave it up to you.

Personally, I like having commonality when reading (e.g. the common bits are first, so just by looking at the end we can see what's different).

> Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.css:83
> +    content: "-";

NIT: I typically put `content` below positional properties (e.g. `position`, `margin`, `padding`, etc) but above content properties (e.g. `background`, `border`, `pointer-events`, etc).

> Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.css:90
> +    content: "+";

Ditto (>83).

> Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.js:83
> +        let stylesheets = new Map();

This name implies an array, not a map.  How about `rulesForStylesheet`?

> Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.js:93
> +        stylesheets.forEach((cssRules, styleSheet) => {

Style: we prefer `for...of` vs `forEach`.

   for (let [styleSheet, cssRules] of stylesheets)

> Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.js:94
> +            let resourceSection = document.createElement("section");

For cases like these, I'd rather you put it all on one line for brevity.

    let resourceSection = this.element.append(document.createElement("section"));

> Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.js:97
> +            let resourceHeader = document.createElement("div");

Ditto (>94).

    let resourceHeader = resourceSection.append(document.createElement("div"));

> Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.js:99
> +            resourceHeader.append(this._createLocationLink(styleSheet));

Style: add a newline before this line.

> Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.js:104
> +                let ruleElement = this._renderRule(cssRule);

NIT: you can just inline this.

> Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.js:122
> +        ruleElement.append(selectorElement);

Ditto (>94).

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

> Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.js:124
> +        let appendProperty = (cssProperty, className) => {

NIT: there's no reason to have this be an arrow function, since you don't use this.  Let's avoid the "cost" of capturing `this`.

> Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.js:128
> +            stylePropertyView.element.insertAdjacentText("afterbegin", WI.indentString());
> +            stylePropertyView.element.insertAdjacentText("beforeend", "\n");

I'm not a fan of adding DOM to "encapsulated" UI objects, as it effectively requires editors of `WI.SpreadsheetStyleProperty` to have to know to never call `this.element.removeChildren` (as an example).  Is there a way you can wrap the entire `WI.SpreadsheetStyleProperty` in a container element (and apply `className` to the container as well) and put these before/after within the container?

> Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.js:158
> +        let options = {

NIT: this should be `const`.

> Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.js:166
> +        let sourceCodeLocation = styleSheet.createSourceCodeLocation(0, 0);

NIT: these values should be `const`.

    const lineNumber = 0;
    const columnNumber = 0;
    let sourceCodeLocation = styleSheet.createSourceCodeLocation(lineNumber, columnNumber);

[Nikita Vasilyev]

Comment on attachment 360411 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.css:47
>> +.changes-panel .resource-section > .header {
> 
> NIT: I'd reorder this as follows:
> 
>     position: -webkit-sticky;
>     top: 0;
>     z-index: var(--z-index-header);
>     padding: 4px 8px;
>     background-color: var(--panel-background-color);
>     border-bottom: 0.5px solid var(--text-color-quaternary);
>     white-space: nowrap;
>     overflow: hidden;
>     text-overflow: ellipsis;

Could you document your preferred order style on https://trac.webkit.org/wiki/WebInspectorCodingStyleGuide#CSS ?

Once we agree on the order, I'd like to write a script to sort properties.

[Devin Rousso]

Comment on attachment 360411 [details]
Patch

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

>>> Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.css:47
>>> +.changes-panel .resource-section > .header {
>> 
>> NIT: I'd reorder this as follows:
>> 
>>     position: -webkit-sticky;
>>     top: 0;
>>     z-index: var(--z-index-header);
>>     padding: 4px 8px;
>>     background-color: var(--panel-background-color);
>>     border-bottom: 0.5px solid var(--text-color-quaternary);
>>     white-space: nowrap;
>>     overflow: hidden;
>>     text-overflow: ellipsis;
> 
> Could you document your preferred order style on https://trac.webkit.org/wiki/WebInspectorCodingStyleGuide#CSS ?
> 
> Once we agree on the order, I'd like to write a script to sort properties.

It's more of a "general feeling" and less of a hard-set order.  Regardless, I'll write up a short list of common properties (obviously I won't list all 500-something CSS properties 😛).

[Nikita Vasilyev]

Created attachment 360533 [details]
[Image] section borders @1x

(In reply to Devin Rousso from comment #6)
> > Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.css:44
> > +    border-bottom: 0.5px solid var(--text-color-quaternary);
> 
> How does this look in @1x?

It looks good, I think. WebKit rounds 0.5px to 1px (started doing it 3 years ago).

[Nikita Vasilyev]

Comment on attachment 360411 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.css:74
>> +    background-color: hsl(70, 90%, 86%);
> 
> What prompted the background-color tweaks for .added and .removed?

I very slightly decreased the contrast after making the lines span the entire width of the panel.

>> Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.css:83
>> +    content: "-";
> 
> NIT: I typically put `content` below positional properties (e.g. `position`, `margin`, `padding`, etc) but above content properties (e.g. `background`, `border`, `pointer-events`, etc).

It seems to me that `content` is the most important property, when present.

>> Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.js:128
>> +            stylePropertyView.element.insertAdjacentText("beforeend", "\n");
> 
> I'm not a fan of adding DOM to "encapsulated" UI objects, as it effectively requires editors of `WI.SpreadsheetStyleProperty` to have to know to never call `this.element.removeChildren` (as an example).  Is there a way you can wrap the entire `WI.SpreadsheetStyleProperty` in a container element (and apply `className` to the container as well) and put these before/after within the container?

Sounds reasonable.

[Nikita Vasilyev]

Created attachment 360616 [details]
Patch

[Matt Baker]

Comment on attachment 360616 [details]
Patch

r=me, nice work!

[Devin Rousso]

Comment on attachment 360411 [details]
Patch

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

>>> Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.css:83
>>> +    content: "-";
>> 
>> NIT: I typically put `content` below positional properties (e.g. `position`, `margin`, `padding`, etc) but above content properties (e.g. `background`, `border`, `pointer-events`, etc).
> 
> It seems to me that `content` is the most important property, when present.

`content` only really has an "important" effect for ::before/::after.  And even then, if we set a `content: "foo";` but had `display: none;`, having set a `content` property is effectively irrelevant as we aren't showing anything.

[WebKit Commit Bot]

Comment on attachment 360616 [details]
Patch

Clearing flags on attachment: 360616

Committed r240741: <https://trac.webkit.org/changeset/240741>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.