Bug 194607 – Web Inspector: CSS Changes: resetting value still shows in the diff

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

[Devin Rousso]

Commenting out a property and then uncommenting it back in still shows the property in the changes view.  The same is true for changing a value to something different and then changing it back to the original value.

[Radar WebKit Bug Importer]

<rdar://problem/48050248>

[Devin Rousso]

Created attachment 363482 [details]
[Patch] WIP

This was sort of the idea I was thinking of.  What I did is quite buggy, and can reach these very odd states where changed things are considered initial content.

[Devin Rousso]

Comment on attachment 363482 [details]
[Patch] WIP

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

> Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js:366
> +            let properties = this._initialState ? this._initialState.properties : this._properties;

I think the buggy-ness may come from this point, as it uses the `initialState`s `properties` instead of whatever's current (this is why the override on (>384) is there).  I get that we don't want to use `this._properties` as new properties may have been added to that after the modification, but in order to determine whether something has been "un-modified" we'd need to be able to look at the current state of things and compare it to the "true" initial one.

[Nikita Vasilyev]

I have a WIP for this as well but I’m away from the keyboard right. I suggest not to invest time into this issue right now.

[Nikita Vasilyev]

Comment on attachment 363482 [details]
[Patch] WIP

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

> Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:390
> +            if (this._ownerStyle)
> +                this._ownerStyle.checkIfModified();

This happens on every CSS value change. Some of this work can be done when Changes panel is shown. I'm about to upload a patch.

> Source/WebInspectorUI/UserInterface/Models/CSSRule.js:163
> +            if (this._initialState.style.__original && !this._initialState.style.__original.modified) {

I don't think adding `__original` is necessary.

> Source/WebInspectorUI/UserInterface/Models/CSSRule.js:182
> +        this._initialState.__original = this;

Ditton.

> Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js:365
> +        if (!this._initialState || this._initialState.properties.some((property) => property.__original && property.__original.modified)) {

Ditto.

> Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js:379
> +                this._initialState.__original = this;

Ditto.

> Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js:384
> +                if (property.__original)
> +                    property = property.__original;

Ditto.

[Nikita Vasilyev]

Created attachment 365764 [details]
Patch

[Devin Rousso]

Comment on attachment 365764 [details]
Patch

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

Nice work!

I'm encountering a few edge cases where this doesn't seem to work exactly right, and I'm not sure if this is an existing bug or something new, but otherwise it looks great :)

# STEPS TO REPRODUCE:
1. inspect any page
2. open the Elements tab
3. select any element that has at least one rule with at least one property
4. delete the last property of any style that has at least one property (e.g. select the name, press ⌫, and press ↵)
5. add a new property that matches the exact text of the removed property in the same position/index in the same style
6. show the Changes panel
 => the property is shown as both added and removed even though the rendered text appears the same

# STEPS TO REPRODUCE:
1. inspect any page
2. open the Elements tab
3. select any element that has at least one rule with at least two properties
4. disable (e.g. comment out) the first property of the rule with at least two properties
5. show the Changes panel
 => all properties after the disabled one appear as changed

# STEPS TO REPRODUCE:
1. inspect any page
2. open the Elements tab
3. select any element that has at least one rule with at least two properties
4. disable (e.g. comment out) the first property of the rule with at least two properties
5. add a new property immediately after the first property
 => the green "modified" border/highlight next to the first property disappears

All of these are reproducible on <https://devinrousso.com/demo/WebKit/test.html>.

> Source/WebInspectorUI/UserInterface/Controllers/CSSManager.js:433
> +            let hasModified = style.visibleProperties.some((property) => property.modified);
> +            if (hasModified) {
> +                // Update properties array.
> +                style.markModified();
> +            } else
> +                style.unmarkModifiedIfNeeded();

Rather than have this work be done in `WI.CSSManager`, could we move this to `WI.CSSStyleDeclaration` as an `updateModifiedState`?  That way `WI.CSSManager` doesn't need to know about "Update properties array.", as that is "internal" functionality of `WI.CSSStyleDeclaration`.

> Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js:393
> +        let visibleProperties = this.visibleProperties;

Why save this to a local?  If you prefer it as a local, please make `this._initialState.visibleProperties` into a local as well for consistency (like `initialVisibleProperties`).

[Nikita Vasilyev]

(In reply to Devin Rousso from comment #7)
> Comment on attachment 365764 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=365764&action=review
> 
> Nice work!
> 
> I'm encountering a few edge cases where this doesn't seem to work exactly
> right, and I'm not sure if this is an existing bug or something new, but
> otherwise it looks great :)
> 
> # STEPS TO REPRODUCE:
> 1. inspect any page
> 2. open the Elements tab
> 3. select any element that has at least one rule with at least one property
> 4. delete the last property of any style that has at least one property
> (e.g. select the name, press ⌫, and press ↵)
> 5. add a new property that matches the exact text of the removed property in
> the same position/index in the same style
> 6. show the Changes panel
>  => the property is shown as both added and removed even though the rendered
> text appears the same

This is tricky since I use Array.diffArrays, which does a strict equality check of CSSproperty models and doesn't do the full-text comparison. I think, for this particular case (a removed property followed by an added property) I could do a full-text comparison.

> # STEPS TO REPRODUCE:
> 1. inspect any page
> 2. open the Elements tab
> 3. select any element that has at least one rule with at least two properties
> 4. disable (e.g. comment out) the first property of the rule with at least
> two properties
> 5. show the Changes panel
>  => all properties after the disabled one appear as changed

This should be resolved by https://bugs.webkit.org/show_bug.cgi?id=196038.

> 
> # STEPS TO REPRODUCE:
> 1. inspect any page
> 2. open the Elements tab
> 3. select any element that has at least one rule with at least two properties
> 4. disable (e.g. comment out) the first property of the rule with at least
> two properties
> 5. add a new property immediately after the first property
>  => the green "modified" border/highlight next to the first property
> disappears

This broke with https://bugs.webkit.org/show_bug.cgi?id=196038 :(

[Nikita Vasilyev]

Created attachment 365897 [details]
Patch

[Nikita Vasilyev]

(In reply to Nikita Vasilyev from comment #8)
> (In reply to Devin Rousso from comment #7)
> > Comment on attachment 365764 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=365764&action=review
> > 
> > Nice work!
> > 
> > I'm encountering a few edge cases where this doesn't seem to work exactly
> > right, and I'm not sure if this is an existing bug or something new, but
> > otherwise it looks great :)
> > 
> > # STEPS TO REPRODUCE:
> > 1. inspect any page
> > 2. open the Elements tab
> > 3. select any element that has at least one rule with at least one property
> > 4. delete the last property of any style that has at least one property
> > (e.g. select the name, press ⌫, and press ↵)
> > 5. add a new property that matches the exact text of the removed property in
> > the same position/index in the same style
> > 6. show the Changes panel
> >  => the property is shown as both added and removed even though the rendered
> > text appears the same
> 
> This is tricky since I use Array.diffArrays, which does a strict equality
> check of CSSproperty models and doesn't do the full-text comparison. I
> think, for this particular case (a removed property followed by an added
> property) I could do a full-text comparison.

I fixed it in the latest patch.

> > 
> > # STEPS TO REPRODUCE:
> > 1. inspect any page
> > 2. open the Elements tab
> > 3. select any element that has at least one rule with at least two properties
> > 4. disable (e.g. comment out) the first property of the rule with at least
> > two properties
> > 5. add a new property immediately after the first property
> >  => the green "modified" border/highlight next to the first property
> > disappears
> 
> This broke with https://bugs.webkit.org/show_bug.cgi?id=196038 :(

I filed Bug 196215 - Web Inspector: Green highlight disappears from edited properties when adding a new property.

[Devin Rousso]

Comment on attachment 365897 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js:397
> +        } else {

NIT: use an early return rather than an `else`.

> Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.js:159
> +                    propertiesDiff.push({cssProperty: cssProperty.initialState, action: -1});

Why not just use the string?  Having the "conversion" be done in the loop at the bottom seems "unnecessary" when you could also be using it here (e.g. `action: "remove"`).

> Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.js:164
> +                if (propertiesDiff.lastValue && propertiesDiff.lastValue.action === -1 && cssProperty.equals(propertiesDiff.lastValue.cssProperty)) {

It seems like this check could be done (possibly as a form of sanity check) for every property that's added, not just removed->added.  If any property is equal to the previous property, we shouldn't show it twice and it should only be shown as "unchanged".

> Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.js:170
> +                propertiesDiff.push({cssProperty, action});

Is it possible that a we would have an added->removed situation (the reverse of the one on >164)?

[Nikita Vasilyev]

Comment on attachment 365897 [details]
Patch

There are a couple more CSS Changes bugs:
- Bug 195264 - Web Inspector: CSS Changes: modifications aren't shared for rules that match multiple elements
- Bug 196215 - Web Inspector: Green highlight disappears from edited properties when adding a new property

To fix them, I'm going to try a different approach:

- Check property equality based on their content. Unfortunately, CSSProperty instances sometimes get re-created with the same content and strict equality checks of models doesn't work.
- Copy all properties of a style declaration on the first edit. Updating `this._initialState.properties` on each edit turned out to be unnecessary confusing and error-prone.

This different approach would make this patch above obsolete.

[Nikita Vasilyev]

(In reply to Nikita Vasilyev from comment #12)
> Comment on attachment 365897 [details]
> Patch
> 
> There are a couple more CSS Changes bugs:
> - Bug 195264 - Web Inspector: CSS Changes: modifications aren't shared for
> rules that match multiple elements
> - Bug 196215 - Web Inspector: Green highlight disappears from edited
> properties when adding a new property
> 
> To fix them, I'm going to try a different approach:
> 
> - Check property equality based on their content. Unfortunately, CSSProperty
> instances sometimes get re-created with the same content and strict equality
> checks of models doesn't work.
> - Copy all properties of a style declaration on the first edit. Updating
> `this._initialState.properties` on each edit turned out to be unnecessary
> confusing and error-prone.
> 
> This different approach would make this patch above obsolete.

The patch is in https://bugs.webkit.org/show_bug.cgi?id=195264#c17.

[Devin Rousso]

*** This bug has been marked as a duplicate of bug 195264 ***