WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
193803
Web Inspector: Add Changes panel to Elements tab
https://bugs.webkit.org/show_bug.cgi?id=193803
Summary
Web Inspector: Add Changes panel to Elements tab
Nikita Vasilyev
Reported
2019-01-24 17:13:56 PST
Introduce a new panel that shows a list of CSS changes made via Web Inspector.
rdar://problem/47524618
Attachments
Patch
(33.89 KB, patch)
2019-01-24 17:21 PST
,
Nikita Vasilyev
ews-watchlist
: commit-queue-
Details
Formatted Diff
Diff
Archive of layout-test-results from ews101 for mac-highsierra
(2.54 MB, application/zip)
2019-01-24 18:25 PST
,
EWS Watchlist
no flags
Details
Archive of layout-test-results from ews104 for mac-highsierra-wk2
(3.34 MB, application/zip)
2019-01-24 19:05 PST
,
EWS Watchlist
no flags
Details
Archive of layout-test-results from ews112 for mac-highsierra
(2.15 MB, application/zip)
2019-01-24 19:25 PST
,
EWS Watchlist
no flags
Details
Patch
(33.92 KB, patch)
2019-01-25 01:16 PST
,
Nikita Vasilyev
no flags
Details
Formatted Diff
Diff
Patch
(34.17 KB, patch)
2019-01-25 16:56 PST
,
Nikita Vasilyev
hi
: review-
Details
Formatted Diff
Diff
[Video] Changes panel not updating
(433.44 KB, video/mp4)
2019-01-25 17:43 PST
,
Matt Baker
no flags
Details
[Image] Changes panel - alignment issues
(40.21 KB, image/png)
2019-01-25 18:40 PST
,
Matt Baker
no flags
Details
Patch
(37.57 KB, patch)
2019-01-27 00:14 PST
,
Nikita Vasilyev
hi
: review+
Details
Formatted Diff
Diff
Patch
(36.85 KB, patch)
2019-01-28 00:52 PST
,
Nikita Vasilyev
no flags
Details
Formatted Diff
Diff
Show Obsolete
(7)
View All
Add attachment
proposed patch, testcase, etc.
Nikita Vasilyev
Comment 1
2019-01-24 17:21:24 PST
Comment hidden (obsolete)
Created
attachment 360059
[details]
Patch
EWS Watchlist
Comment 2
2019-01-24 18:25:40 PST
Comment hidden (obsolete)
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
Comment 3
2019-01-24 18:25:42 PST
Comment hidden (obsolete)
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 4
2019-01-24 19:05:01 PST
Comment hidden (obsolete)
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
Comment 5
2019-01-24 19:05:03 PST
Comment hidden (obsolete)
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 6
2019-01-24 19:25:24 PST
Comment hidden (obsolete)
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
Comment 7
2019-01-24 19:25:25 PST
Comment hidden (obsolete)
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
Comment 8
2019-01-25 01:16:07 PST
Comment hidden (obsolete)
Created
attachment 360091
[details]
Patch
Nikita Vasilyev
Comment 9
2019-01-25 16:56:24 PST
Created
attachment 360192
[details]
Patch Clear Changes panel when the inspected page reloads. The rest is the same as the previous patch.
Matt Baker
Comment 10
2019-01-25 17:43:52 PST
Created
attachment 360200
[details]
[Video] Changes panel not updating
Matt Baker
Comment 11
2019-01-25 17:47:26 PST
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
Comment 12
2019-01-25 17:51:33 PST
(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
Comment 13
2019-01-25 18:10:37 PST
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
Comment 14
2019-01-25 18:40:20 PST
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
Comment 15
2019-01-26 17:10:17 PST
(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 16
2019-01-26 17:11:46 PST
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 17
2019-01-26 17:53:53 PST
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 18
2019-01-26 18:46:45 PST
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 19
2019-01-26 19:03:28 PST
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 20
2019-01-26 19:25:37 PST
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
Comment 21
2019-01-27 00:14:59 PST
Created
attachment 360274
[details]
Patch
Devin Rousso
Comment 22
2019-01-27 14:08:31 PST
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 23
2019-01-28 00:43:25 PST
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 24
2019-01-28 00:45:38 PST
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
Comment 25
2019-01-28 00:52:04 PST
Created
attachment 360324
[details]
Patch
Nikita Vasilyev
Comment 26
2019-01-28 00:55:28 PST
(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 27
2019-01-28 01:29:26 PST
Comment on
attachment 360324
[details]
Patch Clearing flags on attachment: 360324 Committed
r240559
: <
https://trac.webkit.org/changeset/240559
>
WebKit Commit Bot
Comment 28
2019-01-28 01:29:28 PST
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug