Introduce a new panel that shows a list of CSS changes made via Web Inspector. rdar://problem/47524618
Created attachment 360059 [details] Patch
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
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
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
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
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
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
Created attachment 360091 [details] Patch
Created attachment 360192 [details] Patch Clear Changes panel when the inspected page reloads. The rest is the same as the previous patch.
Created attachment 360200 [details] [Video] Changes panel not updating
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..
(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.
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
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.
(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.
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.
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`? :(
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.
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?
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`.
Created attachment 360274 [details] Patch
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.
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 :)
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.
Created attachment 360324 [details] Patch
(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.
Comment on attachment 360324 [details] Patch Clearing flags on attachment: 360324 Committed r240559: <https://trac.webkit.org/changeset/240559>
All reviewed patches have been landed. Closing bug.