Undo/redo in the new styles sidebar should work similarly to the current one. When not focused on a selector, CSS name or value field, Command-Z should undo an edit, and Command-Shift-Z should redo it. When focused on a selector, CSS name or value field, Command-Z/Command-Shift-Z should only undo/redo edits in that field. This behavior matches our current styles sidebar and Chrome DevTools as well. <rdar://problem/33526468>
Created attachment 322220 [details] Patch
Created attachment 322221 [details] [Animated GIF] With patch applied Please note of Bug 177676 - Web Inspector: Styles: Undo reverts all changes at once. My patch doesn't address it.
Created attachment 322222 [details] [Animated GIF] With patch applied
Comment on attachment 322220 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=322220&action=review > Source/WebInspectorUI/ChangeLog:13 > + Make sure WI._undoKeyboardShortcut don't call WI.undo() when editing inside of a contentEditable element. Grammar: "Make sure ... don't" => "Make sure ... doesn't" > Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:68 > + if (this._style) > + this._style.removeEventListener(WI.CSSStyleDeclaration.Event.PropertiesChanged, this._propertiesChanged, this); > + > + if (style) > + style.addEventListener(WI.CSSStyleDeclaration.Event.PropertiesChanged, this._propertiesChanged, this); > > + this._style = style || null; Nice. We normally order this: if (this._style) this._style.removeEventListener(...); this._style = style || null; if (this._style) this._style.addEventListener(...); Given this is a View that is adding an event listener someone should probably be removing it when the view goes away. WI.SpreadsheetRulesStyleDetailsPanel * - WI.SpreadsheetCSSStyleDeclarationSection 1 - WI.SpreadsheetCSSStyleDeclarationEditor When the Sections are destroyed, do they clear the style on the editor? If not, then we may be abandoning a bunch of editors that are event listeners of a WI.CSSStyleDeclaration. I didn't see anywhere where this could be cleared up. r- for the potential leak, but if you can explain how it goes away then set back to r? and I'll re-review. > Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:91 > + let isFocused = false; > + > + let focusedElement = document.activeElement; > + if (focusedElement && focusedElement.isSelfOrDescendant(this.element)) > + isFocused = true; > + > + if (!isFocused) > + this.needsLayout(); You can simplify the boolean initialization and it'll read better: let focusedElement = document.activeElement; let isFocused = focusedElement && focusedElement.isSelfOrDescendant(this.element); if (!isFocused) this.needsLayout()
Comment on attachment 322220 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=322220&action=review >> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:68 >> + this._style = style || null; > > Nice. We normally order this: > > if (this._style) > this._style.removeEventListener(...); > > this._style = style || null; > > if (this._style) > this._style.addEventListener(...); > > Given this is a View that is adding an event listener someone should probably be removing it when the view goes away. > > WI.SpreadsheetRulesStyleDetailsPanel > * - WI.SpreadsheetCSSStyleDeclarationSection > 1 - WI.SpreadsheetCSSStyleDeclarationEditor > > When the Sections are destroyed, do they clear the style on the editor? If not, then we may be abandoning a bunch of editors that are event listeners of a WI.CSSStyleDeclaration. I didn't see anywhere where this could be cleared up. > > r- for the potential leak, but if you can explain how it goes away then set back to r? and I'll re-review. This does look like a leak and it's the same as in the old styles sidebar. It probably wasn't significant because WI.CSSStyleDeclaration.Event.PropertiesChanged is usually called when CSSStyleDeclarationTextEditor is visible. I'll add attache/detach methods to be safe.
Created attachment 322254 [details] Patch
Comment on attachment 322254 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=322254&action=review r=me > Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:85 > + attached() > + { > + if (this._style) > + this._style.addEventListener(WI.CSSStyleDeclaration.Event.PropertiesChanged, this._propertiesChanged, this); > + } > + > + detached() > + { > + if (this._style) > + this._style.removeEventListener(WI.CSSStyleDeclaration.Event.PropertiesChanged, this._propertiesChanged, this); > + } I think after our discussion we might be fine not doing this. Given the style -> section -> editor relationship.
Created attachment 322276 [details] Patch
Comment on attachment 322276 [details] Patch Clearing flags on attachment: 322276 Committed r222678: <http://trac.webkit.org/changeset/222678>
All reviewed patches have been landed. Closing bug.