Bug 177208 – Web Inspector: Styles Redesign: support editing of properties and property values

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Nikita Vasilyev]

Make it possible to edit property names and values.

<rdar://problem/33525247>

[Nikita Vasilyev]

Created attachment 321498 [details]
Patch

Goals of this patch: 
  - Provide a primitive UI to make sure editing works, changes apply live, and CSS doesn’t get corrupted.
  - Changes in CSSProperty model don’t break the current styles sidebar.

Non-goals:
  - Make a polished UI for editing names and values. I’ll work on it in:
    - Bug 177314 Web Inspector: Styles Redesign: support undo/redo of manual edits
    - Bug 177313 Web Inspector: Styles Redesign: hook up autocompletion to property names and values

[Build Bot]

Attachment 321498 [details] did not pass style-queue:


ERROR: Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:196:  Line contains single-quote character.  [js/syntax] [5]
ERROR: Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:322:  Line contains single-quote character.  [js/syntax] [5]
ERROR: Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:328:  Line contains single-quote character.  [js/syntax] [5]
Total errors found: 3 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Nikita Vasilyev]

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

[Nikita Vasilyev]

Created attachment 321501 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 321501 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=321501&action=review

> Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:164
> +    set name(newName)

Nit: You can just use `name`.

> Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:166
> +        this._name = newName;

Nit: Lets bail if things did not change:

    if (this._name === name)
        return;

    this._name = name;
    ...

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:138
> +            nameElement.addEventListener("input", () => {
> +                this._property.name = nameElement.textContent.trim();
> +            });
> +
> +            valueElement.contentEditable = "plaintext-only";
> +            valueElement.addEventListener("input", () => {
> +                this._property.rawValue = valueElement.textContent.trim();
> +            });

As far as I can, by using the "input" event that means we are going to replace the CSS every single keystroke. For our model objects that may be fine, but it does look like that synchronously triggers a backend update. That is probably is not as performant as we want. If I'm typing "color: red" I'm able to type that pretty quickly, all of the intermediate "c", "co", "col", "colo" etc inputs don't need to be sent to the backend because they are about to be obsoleted very very quickly.

We probably want to limit/throttle/debounce backend updates. Example strategies could be:

    • Maximum 1 update every 100ms.
        => If the user types super fast this would only update at most 10 times a second

    • Only update if no user change in 100ms.
        => As long as the user is typing fast there are no updates until they stop typing.

The second strategy seems to be what the sidebar does right now. That said, the first strategy seems pretty reasonable.

For now this seems fine, but lets add a FIXME somewhere in order to throttle updates.

[Nikita Vasilyev]

Comment on attachment 321501 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=321501&action=review

>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:138
>> +            });
> 
> As far as I can, by using the "input" event that means we are going to replace the CSS every single keystroke. For our model objects that may be fine, but it does look like that synchronously triggers a backend update. That is probably is not as performant as we want. If I'm typing "color: red" I'm able to type that pretty quickly, all of the intermediate "c", "co", "col", "colo" etc inputs don't need to be sent to the backend because they are about to be obsoleted very very quickly.
> 
> We probably want to limit/throttle/debounce backend updates. Example strategies could be:
> 
>     • Maximum 1 update every 100ms.
>         => If the user types super fast this would only update at most 10 times a second
> 
>     • Only update if no user change in 100ms.
>         => As long as the user is typing fast there are no updates until they stop typing.
> 
> The second strategy seems to be what the sidebar does right now. That said, the first strategy seems pretty reasonable.
> 
> For now this seems fine, but lets add a FIXME somewhere in order to throttle updates.

The existing delay is 250ms.

    WI.CSSStyleDeclarationTextEditor.CommitCoalesceDelay = 250

I used 250ms debounce for a bit, and it didn't feel too slow.

For incrementing/decrementing numeric values using arrow keys and changing colors via color picker I plan to use a shorter delay (~100ms) and throttle instead of debounce.

[Nikita Vasilyev]

Created attachment 321592 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 321592 [details]
Patch

Clearing flags on attachment: 321592

Committed r222408: <http://trac.webkit.org/changeset/222408>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.