Bug 177314 – Web Inspector: Styles Redesign: support undo/redo of manual edits

• Component: Web Inspector
• Status: RESOLVED FIXED

[Nikita Vasilyev]

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>

[Nikita Vasilyev]

Created attachment 322220 [details]
Patch

[Nikita Vasilyev]

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.

[Nikita Vasilyev]

Created attachment 322222 [details]
[Animated GIF] With patch applied

[Joseph Pecoraro]

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()

[Nikita Vasilyev]

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.

[Nikita Vasilyev]

Created attachment 322254 [details]
Patch

[Joseph Pecoraro]

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.

[Nikita Vasilyev]

Created attachment 322276 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 322276 [details]
Patch

Clearing flags on attachment: 322276

Committed r222678: <http://trac.webkit.org/changeset/222678>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.