Bug 193803 – Web Inspector: Add Changes panel to Elements tab

• Component: Web Inspector
• Status: RESOLVED FIXED

[Nikita Vasilyev]

Introduce a new panel that shows a list of CSS changes made via Web Inspector.

rdar://problem/47524618

[Nikita Vasilyev]

Created attachment 360059 [details]
Patch

[EWS Watchlist]

Comment on attachment 360059 [details]
Patch

Attachment 360059 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/10879847

New failing tests:
inspector/css/add-css-property.html
inspector/css/modify-css-property.html

[EWS Watchlist]

Created attachment 360065 [details]
Archive of layout-test-results from ews101 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 360059 [details]
Patch

Attachment 360059 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/10880152

New failing tests:
inspector/css/add-css-property.html
inspector/css/modify-css-property.html

[EWS Watchlist]

Created attachment 360068 [details]
Archive of layout-test-results from ews104 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 360059 [details]
Patch

Attachment 360059 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/10880144

New failing tests:
inspector/css/add-css-property.html
inspector/css/modify-css-property.html

[EWS Watchlist]

Created attachment 360073 [details]
Archive of layout-test-results from ews112 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[Nikita Vasilyev]

Created attachment 360091 [details]
Patch

[Nikita Vasilyev]

Created attachment 360192 [details]
Patch

Clear Changes panel when the inspected page reloads. The rest is the same as the previous patch.

[Matt Baker]

Created attachment 360200 [details]
[Video] Changes panel not updating

[Matt Baker]

Comment on attachment 360192 [details]
Patch

The Changes panel does not update reliably so r- for now. See attached video in the above comment.

I have gotten the panel to start picking up my changes, but cannot easily reproduce it..

[Nikita Vasilyev]

(In reply to Matt Baker from comment #11)
> Comment on attachment 360192 [details]
> Patch
> 
> The Changes panel does not update reliably so r- for now. See attached video
> in the above comment.
> 
> I have gotten the panel to start picking up my changes, but cannot easily
> reproduce it..

It doesn't currently work for inline styles (<style>...</style> and style="...").
Try editing a CSS resource.

[Nikita Vasilyev]

Actually, it works for inline styles, i.e. <style>...</style>. It only doesn't work for style attributes.

I created a follow-up: Bug 193859: Web Inspector: Changes: style attribute changes aren't being tracked

[Matt Baker]

Created attachment 360207 [details]
[Image] Changes panel - alignment issues

Properties are indented consistently in the Changes panel and Styles panel, but the rule text and braces are not (see screenshot).

It would also be nice if the Changes panel did syntax highlighting to match the Styles panel, but that can be a follow up.

[Nikita Vasilyev]

(In reply to Matt Baker from comment #14)
> Created attachment 360207 [details]
> [Image] Changes panel - alignment issues
> 
> Properties are indented consistently in the Changes panel and Styles panel,
> but the rule text and braces are not (see screenshot).

I think this should be a follow-up.

The distance between the purple and the blue line was chosen because I needed to fit checkboxes before every CSS property. In the Changes panel, currently, the indentation is exactly two spaces.

I agree that padding before selectors should match.

[Nikita Vasilyev]

Comment on attachment 360192 [details]
Patch

Resetting to r? since we agreed that tracking changes in style attributes should be fixed as a follow-up.

[Devin Rousso]

Comment on attachment 360192 [details]
Patch

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

r-, as we should have tests for the diff logic

> Source/WebInspectorUI/UserInterface/Controllers/CSSManager.js:356
> +        let cssRules = [];
> +        this._modifiedCSSRules.forEach((value) => { cssRules.push(value) });
> +        return cssRules;

NIT: this can be simpler

    get modifiedCSSRules()
    {
        return Array.from(this._modifiedCSSRules.values());
    }

> Source/WebInspectorUI/UserInterface/Controllers/CSSManager.js:361
> +        let key = cssRule.id.styleSheetId + "/" + cssRule.id.ordinal;

NIT: since this is used in many places, I'd rather we create a `stringId` function on `WI.CSSRule` so that it keeps the logic consistent everywhere.

> Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:103
> +        if (typeof this._text !== "undefined" && this._text !== text)

Why are we also checking that `typeof this._test !== "undefined"`?  Is that for the case where we're creating a new property in the frontend and have yet to add any text (e.g. empty)?

> Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:132
> +        this._markModified();

Wouldn't it be valid to just have this call inside `this._updateStyleOwnerText`?  It seems like all these cases end up there, so it may help centralize the code.

> Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:441
> +        if (!this._initialState) {

Can you inline this in `this._markModified`?

> Source/WebInspectorUI/UserInterface/Models/CSSRule.js:183
> +        if (!this._initialState) {

Ditto (>CSSProperty.js:441).

> Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js:217
> +        if (!this._enabledProperties)
> +            this._enabledProperties = this._properties.filter((property) => property.enabled);

Nice!

> Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js:392
> +    _saveInitialState()

Ditto (>CSSProperty.js:441).

> Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.css:70
> +    }
> +    .changes-panel del {

Style: missing newline.

> Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.js:45
> +        if (this.didInitialLayout)
> +            this.needsLayout();

This check shouldn't be necessary.  We should always be calling `layout` right after we call `shown`.

> Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.js:53
> +    hidden()
> +    {
> +        super.hidden();
> +    }

NIT: unnecessary

> Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.js:67
> +        WI.Frame.addEventListener(WI.Frame.Event.MainResourceDidChange, this._mainResourceDidChange, this);

Please move the addition/removal of event listeners to `attached`/`detached` functions.

> Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.js:73
> +        super.layout();
> +        this.element.removeChildren();

Style: missing newline.

> Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.js:77
> +        this.element.classList.toggle("empty", cssRules.length === 0);

NIT: `!cssRules.length`

> Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.js:84
> +            let selectorElement = document.createElement("span");

NIT: `this.element.append(document.createElement("span"))`

> Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.js:90
> +            const indent = "  ";

NIT: we can have this match `WI.settings.indentUnit`

> Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.js:93
> +                if (action === "equal" || action === "moved") {

Can you use an enum instead of hardcoded strings?

    WI.ChangesDetailsSidebarPanel.DiffAction = {
        Equal: "equal",
        Added: "added",
        Removed: "removed",
        Moved: "moved",
    };

Alternatively, you could have `action` be an integer instead (`0` for a "neutral change", `-1` for deletion, `1` for addition).

> Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.js:103
> +                    delElement.classList.add("css-property", "initial");

It seems like the "initial" and "current" classes aren't used in CSS.  "css-property" is also somewhat redundant since the only things using it are `<ins>` and `<del>` elements.  Perhaps we can remove them to remove duplicate code.

    let tag = null;

    if (action === "added")
        tag = "ins";
    else if (action === "removed")
        tag  = "del";
    else
        tag = "span";

    let propertyElement = this.element.append(document.createElement(tag));
    propertyElement.append(indent, cssProprety.formattedText);

    this.element.append("\n");

> Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.js:139
> +    _diff(listA, listB, onEach)

This seems like a useful enough function to maybe be added to Utilities.  Furthermore, it might deserve a comment (or better variable names) explaining which of `listA` vs `listB` is what's considered "current" (e.g. what will be diff'd against) and what's considered "initial" (e.g. what will bee diff'd from).

We also 100% should have tests for this.

> Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.js:151
> +            if (i >= shortListLength)

This check seems redundant given that you're individually comparing each list and their index on the next if.  Is there a reason to keep it?

> Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.js:168
> +                    --i;
> +                    ++deltaB;

This logic could use an explanation.  I originally thought this would cause skipping, but I after re-reading I see that you're resetting (not adding to) `indexB` each iteration.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.css:139
> +    background-color: var(--background-color-selected) !important;

Can you add a `:not(.selected)` to a different rule instead of using an `!important`?  :(

[Nikita Vasilyev]

Comment on attachment 360192 [details]
Patch

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

Thanks for the timely review! Some comments below.

>> Source/WebInspectorUI/UserInterface/Controllers/CSSManager.js:356
>> +        return cssRules;
> 
> NIT: this can be simpler
> 
>     get modifiedCSSRules()
>     {
>         return Array.from(this._modifiedCSSRules.values());
>     }

Neat!

>> Source/WebInspectorUI/UserInterface/Controllers/CSSManager.js:361
>> +        let key = cssRule.id.styleSheetId + "/" + cssRule.id.ordinal;
> 
> NIT: since this is used in many places, I'd rather we create a `stringId` function on `WI.CSSRule` so that it keeps the logic consistent everywhere.

Makes sense.

>> Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:132
>> +        this._markModified();
> 
> Wouldn't it be valid to just have this call inside `this._updateStyleOwnerText`?  It seems like all these cases end up there, so it may help centralize the code.

I wish it would be that easy. Calling this inside this._updateStyleOwnerText would be too late. For instance, `remove` method modifies this._name before calling this_updateOwnerStyleText.

[Nikita Vasilyev]

Comment on attachment 360192 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.js:93
>> +                if (action === "equal" || action === "moved") {
> 
> Can you use an enum instead of hardcoded strings?
> 
>     WI.ChangesDetailsSidebarPanel.DiffAction = {
>         Equal: "equal",
>         Added: "added",
>         Removed: "removed",
>         Moved: "moved",
>     };
> 
> Alternatively, you could have `action` be an integer instead (`0` for a "neutral change", `-1` for deletion, `1` for addition).

I'll move the `_diff` to Utilities. I'm thinking of calling it Array.diffUniqArrays.

Should the diff actions move to Utilities as well? How about Array.DiffActions?

[Devin Rousso]

Comment on attachment 360192 [details]
Patch

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

>>> Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:132
>>> +        this._markModified();
>> 
>> Wouldn't it be valid to just have this call inside `this._updateStyleOwnerText`?  It seems like all these cases end up there, so it may help centralize the code.
> 
> I wish it would be that easy. Calling this inside this._updateStyleOwnerText would be too late. For instance, `remove` method modifies this._name before calling this_updateOwnerStyleText.

Ah, is this because we save the values in a new object?  If so, that could be documented in the ChangeLog somewhere.  Perhaps then it may be a good idea to add an assert inside `_updateOwnerStyleText` that checks for `this.modified` so that future changes don't "forget" to mark modified.

>>> Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.js:93
>>> +                if (action === "equal" || action === "moved") {
>> 
>> Can you use an enum instead of hardcoded strings?
>> 
>>     WI.ChangesDetailsSidebarPanel.DiffAction = {
>>         Equal: "equal",
>>         Added: "added",
>>         Removed: "removed",
>>         Moved: "moved",
>>     };
>> 
>> Alternatively, you could have `action` be an integer instead (`0` for a "neutral change", `-1` for deletion, `1` for addition).
> 
> I'll move the `_diff` to Utilities. I'm thinking of calling it Array.diffUniqArrays.
> 
> Should the diff actions move to Utilities as well? How about Array.DiffActions?

Using abbreviated names isn't part of our style, so I'd recommend a more "straightforward" name, like `Array.diffArrays` or `Array.forEachDiff` (this helps make it clear that the third argument is supposed to be a callback), or even just "Array.differences".

I personally like the idea of using a number (it matches many of the other array functions), so I'd say we do that.  If you feel strongly about it, then I'd suggest adding it to the function (e.g. `Array.differences.ItemType`), not `Array`.

[Nikita Vasilyev]

Created attachment 360274 [details]
Patch

[Devin Rousso]

Comment on attachment 360274 [details]
Patch

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

r=me, nice test!

One thing I thought of is maybe we should add a "Show All"-style checkbox to the top of the changes panel that shows only rules associated with the current node (e.g. those in the Rules panel) when unchecked.  I could then check the checkbox to see everything as it currently is now.  Being able to see specifically what changed for a given node is quite helpful sometimes.

> Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:103
> +        if (typeof this._text !== "undefined" && this._text !== text)

I'm still not sure why we need to check for `undefined`.  Isn't is easier to just check `this._text === undefined`?

> Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:451
> +            this._initialState.ownerStyle = this._ownerStyle.initialState;

NIT: should we be resetting this each time we call `_markModified`, or should this only happen the first time we create `this._initialState`?

> Source/WebInspectorUI/UserInterface/Models/CSSRule.js:54
> +        else

Style: remove the else.

> Source/WebInspectorUI/UserInterface/Models/CSSRule.js:151
> +    get initialState()

NIT: simple getters like this can be one-lined at the beginning of the "Public" section.

> Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js:70
> +                [], // Passing CSS properties here would change their ownerStyle.

Interesting.  This seems like a weird edge-case.  I'd imagine that we'd want `set properties` to also have this effect, or neither of them.

> Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js:285
> +    get initialState()

Ditto (>CSSRule.js:151).

> Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.js:87
> +        if (WI.settings.indentWithTabs.value)
> +            indent = "\t";
> +        else
> +            indent = " ".repeat(WI.settings.indentUnit.value);

You can just use `WI.indentString()` for this.

> Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.js:105
> +                if (action == 0) {

Style: should be `===`.

Also, you could use a `switch` here instead of a bunch of `if`s.

> Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.js:111
> +

Style: extra newline.

> Source/WebInspectorUI/UserInterface/Views/ElementsTabContentView.js:35
> +

Style: I'd remove this newline.

[Nikita Vasilyev]

Comment on attachment 360274 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:451
>> +            this._initialState.ownerStyle = this._ownerStyle.initialState;
> 
> NIT: should we be resetting this each time we call `_markModified`, or should this only happen the first time we create `this._initialState`?

Seems like I could simply add

        if (this.modified)
            return;

to the top of this method.

>> Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js:70
>> +                [], // Passing CSS properties here would change their ownerStyle.
> 
> Interesting.  This seems like a weird edge-case.  I'd imagine that we'd want `set properties` to also have this effect, or neither of them.

Initially, I was passing `properties` here. It set `ownerStyle` of modified properties to this._initialState, which caused very bizarre bugs.

>> Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.js:87
>> +            indent = " ".repeat(WI.settings.indentUnit.value);
> 
> You can just use `WI.indentString()` for this.

I knew there must be something like this :)

[Nikita Vasilyev]

Comment on attachment 360274 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:103
>> +        if (typeof this._text !== "undefined" && this._text !== text)
> 
> I'm still not sure why we need to check for `undefined`.  Isn't is easier to just check `this._text === undefined`?

This doesn't seem to be needed anymore.

[Nikita Vasilyev]

Created attachment 360324 [details]
Patch

[Nikita Vasilyev]

(In reply to Devin Rousso from comment #22)
> Comment on attachment 360274 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=360274&action=review
> 
> r=me, nice test!
> 
> One thing I thought of is maybe we should add a "Show All"-style checkbox to
> the top of the changes panel that shows only rules associated with the
> current node (e.g. those in the Rules panel) when unchecked.  I could then
> check the checkbox to see everything as it currently is now.  Being able to
> see specifically what changed for a given node is quite helpful sometimes.

I'm experimenting with:

1. Highlighting matching rules in the Changes panel.

2. Providing a way to see initial property values and removed properties right in the Styles panel.

[WebKit Commit Bot]

Comment on attachment 360324 [details]
Patch

Clearing flags on attachment: 360324

Committed r240559: <https://trac.webkit.org/changeset/240559>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.