Bug 229153 – Web Inspector: CSS Changes: changes are not updated live

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

[Razvan Caliman]

Created attachment 435619 [details]
Video of issue

Steps to reproduce:
- Open Web Inspector
- Enable independent Styles sidebar: Settings > Elements > Show independent Styles sidebar
- Switch to Elements Tab
- Select the Changes sidebar
- Make a change to the CSS in the Styles sidebar

Expected result:
The change is reflected live in the Changes sidebar.

Actual result:
The Changes sidebar doesn't update until selecting another sidebar, then selecting Changes again.

[Radar WebKit Bug Importer]

<rdar://problem/81989328>

[Razvan Caliman]

Created attachment 435621 [details]
Patch v1.0

Dispatch an event whenever the list of modified styles changes and re-layout the Changes sidebar in reaction to this event.

[Devin Rousso]

Comment on attachment 435621 [details]
Patch v1.0

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

> Source/WebInspectorUI/UserInterface/Controllers/CSSManager.js:357
> +        this.dispatchEventToListeners(WI.CSSManager.Event.ModifiedStylesChanged, {modifiedStyles: this.modifiedStyles});

Why do we include `modifiedStyles` in `event.data` if it's not used?

Also, does this also handle when a CSS rule set is modified more than once?

> Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.js:48
> +        WI.CSSManager.addEventListener(WI.CSSManager.Event.ModifiedStylesChanged, this.needsLayout, this);

We usually only like to have event listener handlers be functions defined on the same object that's adding the event listener (i.e. create a `_handleModifiedStylesChanged` and call `this.needsLayout()` inside it), but I don't think that's actually a "rule" anywhere so it's probably fine (I personally am a fan of this too).

Along those lines tho, there is `WI.settings.cssChangesPerNode`, so maybe we should do some minimal amount of checking to see if the added/removed `style.stringId` was actually being displayed (assuming we currently save a list of things being displayed)?  Not a big deal and likely wouldn't be too huge of an issue tho, so if we don't want/have that then that's fine.

[Razvan Caliman]

Comment on attachment 435621 [details]
Patch v1.0

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

>> Source/WebInspectorUI/UserInterface/Controllers/CSSManager.js:357
>> +        this.dispatchEventToListeners(WI.CSSManager.Event.ModifiedStylesChanged, {modifiedStyles: this.modifiedStyles});
> 
> Why do we include `modifiedStyles` in `event.data` if it's not used?
> 
> Also, does this also handle when a CSS rule set is modified more than once?

Indeed, `modifiedStyles` isn't used by event handlers. I thought to add it to match the signature of most events dispatched from this file. 
I'll remove it now. It there are consumers for it in the future, it can be added then.

> Also, does this also handle when a CSS rule set is modified more than once?
Yes, the event is fired whenever a CSS declaration is changed, even repeated changes in the same CSS rule: see `WI.CSSProperty._markModified()` which ends up calling `WI.cssManager.addModifiedStyle()`

>> Source/WebInspectorUI/UserInterface/Views/ChangesDetailsSidebarPanel.js:48
>> +        WI.CSSManager.addEventListener(WI.CSSManager.Event.ModifiedStylesChanged, this.needsLayout, this);
> 
> We usually only like to have event listener handlers be functions defined on the same object that's adding the event listener (i.e. create a `_handleModifiedStylesChanged` and call `this.needsLayout()` inside it), but I don't think that's actually a "rule" anywhere so it's probably fine (I personally am a fan of this too).
> 
> Along those lines tho, there is `WI.settings.cssChangesPerNode`, so maybe we should do some minimal amount of checking to see if the added/removed `style.stringId` was actually being displayed (assuming we currently save a list of things being displayed)?  Not a big deal and likely wouldn't be too huge of an issue tho, so if we don't want/have that then that's fine.

> I don't think that's actually a "rule" anywhere so it's probably fine (I personally am a fan of this too).
Yes, it seemed needless to create a wrapper method just to call `this.needsLayout()`.

> Along those lines tho, there is `WI.settings.cssChangesPerNode`, so maybe we should do some minimal amount of checking
I checked the feature with and without the setting enabled. Seems to work as intended: the list of changes gets filtered by the selected node. It is still updated live. The setting value is handled within `WI.ChangesDetailsSidebarPanel.layout()`.

I also see there's a follow-up to move this option straight into the Changes panel: https://bugs.webkit.org/show_bug.cgi?id=195365 
Any reason why this hasn't been attempted yet? Can I try?

[Razvan Caliman]

Created attachment 435863 [details]
Patch v1.1

Remove unused payload from  event data

[Devin Rousso]

Comment on attachment 435863 [details]
Patch v1.1

r=me

[Razvan Caliman]

Created attachment 435984 [details]
[fast-cq] Patch v1.1

Carry over R+

[Razvan Caliman]

Created attachment 436188 [details]
Patch v1.1

Carry over R+; Solve merge conflict on Changelog

[EWS]

/Volumes/Data/worker/Commit-Queue/build/Source/WebInspectorUI/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

[Razvan Caliman]

Created attachment 436189 [details]
[fast-cq] Patch v1.1

Carry over R+; Add reviewer to Changelog. Dio mio!

[EWS]

Committed r281441 (240824@main): <https://commits.webkit.org/240824@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 436189 [details].