Bug 229153 - Web Inspector: CSS Changes: changes are not updated live
Summary: Web Inspector: CSS Changes: changes are not updated live
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Razvan Caliman
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-08-16 11:04 PDT by Razvan Caliman
Modified: 2021-08-23 07:20 PDT (History)
5 users (show)

See Also:


Attachments
Video of issue (1.68 MB, video/quicktime)
2021-08-16 11:04 PDT, Razvan Caliman
no flags Details
Patch v1.0 (3.61 KB, patch)
2021-08-16 11:16 PDT, Razvan Caliman
no flags Details | Formatted Diff | Diff
Patch v1.1 (3.47 KB, patch)
2021-08-19 08:45 PDT, Razvan Caliman
no flags Details | Formatted Diff | Diff
[fast-cq] Patch v1.1 (3.53 KB, patch)
2021-08-20 07:57 PDT, Razvan Caliman
no flags Details | Formatted Diff | Diff
Patch v1.1 (3.49 KB, patch)
2021-08-23 06:04 PDT, Razvan Caliman
no flags Details | Formatted Diff | Diff
[fast-cq] Patch v1.1 (3.53 KB, patch)
2021-08-23 06:47 PDT, Razvan Caliman
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Razvan Caliman 2021-08-16 11:04:55 PDT
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.
Comment 1 Radar WebKit Bug Importer 2021-08-16 11:05:46 PDT
<rdar://problem/81989328>
Comment 2 Razvan Caliman 2021-08-16 11:16:10 PDT
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.
Comment 3 Devin Rousso 2021-08-16 12:09:06 PDT
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.
Comment 4 Razvan Caliman 2021-08-19 08:43:43 PDT
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?
Comment 5 Razvan Caliman 2021-08-19 08:45:22 PDT
Created attachment 435863 [details]
Patch v1.1

Remove unused payload from  event data
Comment 6 Devin Rousso 2021-08-19 13:12:30 PDT
Comment on attachment 435863 [details]
Patch v1.1

r=me
Comment 7 Razvan Caliman 2021-08-20 07:57:09 PDT
Created attachment 435984 [details]
[fast-cq] Patch v1.1

Carry over R+
Comment 8 Razvan Caliman 2021-08-23 06:04:04 PDT
Created attachment 436188 [details]
Patch v1.1

Carry over R+; Solve merge conflict on Changelog
Comment 9 EWS 2021-08-23 06:44:38 PDT
/Volumes/Data/worker/Commit-Queue/build/Source/WebInspectorUI/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).
Comment 10 Razvan Caliman 2021-08-23 06:47:55 PDT
Created attachment 436189 [details]
[fast-cq] Patch v1.1

Carry over R+; Add reviewer to Changelog. Dio mio!
Comment 11 EWS 2021-08-23 07:20:57 PDT
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].