Bug 177208 - Web Inspector: Styles Redesign: support editing of properties and property values
Summary: Web Inspector: Styles Redesign: support editing of properties and property va...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Nikita Vasilyev
URL:
Keywords: InRadar
Depends on:
Blocks: 177313 177314
  Show dependency treegraph
 
Reported: 2017-09-19 16:07 PDT by Nikita Vasilyev
Modified: 2017-09-22 14:37 PDT (History)
5 users (show)

See Also:


Attachments
Patch (7.11 KB, patch)
2017-09-21 17:36 PDT, Nikita Vasilyev
no flags Details | Formatted Diff | Diff
[Animated GIF] With patch applied (198.25 KB, image/gif)
2017-09-21 17:43 PDT, Nikita Vasilyev
no flags Details
Patch (7.11 KB, patch)
2017-09-21 17:47 PDT, Nikita Vasilyev
joepeck: review+
Details | Formatted Diff | Diff
Patch (8.32 KB, patch)
2017-09-22 13:57 PDT, Nikita Vasilyev
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nikita Vasilyev 2017-09-19 16:07:45 PDT
Make it possible to edit property names and values.

<rdar://problem/33525247>
Comment 1 Nikita Vasilyev 2017-09-21 17:36:31 PDT
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
Comment 2 Build Bot 2017-09-21 17:37:34 PDT
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.
Comment 3 Nikita Vasilyev 2017-09-21 17:43:51 PDT
Created attachment 321500 [details]
[Animated GIF] With patch applied
Comment 4 Nikita Vasilyev 2017-09-21 17:47:31 PDT
Created attachment 321501 [details]
Patch
Comment 5 Joseph Pecoraro 2017-09-21 19:20:05 PDT
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.
Comment 6 Nikita Vasilyev 2017-09-22 13:56:43 PDT
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.
Comment 7 Nikita Vasilyev 2017-09-22 13:57:07 PDT
Created attachment 321592 [details]
Patch
Comment 8 WebKit Commit Bot 2017-09-22 14:37:19 PDT
Comment on attachment 321592 [details]
Patch

Clearing flags on attachment: 321592

Committed r222408: <http://trac.webkit.org/changeset/222408>
Comment 9 WebKit Commit Bot 2017-09-22 14:37:20 PDT
All reviewed patches have been landed.  Closing bug.