Bug 203754 – Web Inspector: Elements: Styles: support multiline CSS property values

• Component: Web Inspector
• Status: RESOLVED FIXED

[Devin Rousso]

Right now,

```
    grid-template-areas: "header header header header"
                         "main main .sidebar"
                         "footer footer footer footer";
```

appears as

```
    grid-template-areas: "header header header header" "main main .sidebar" "footer footer footer footer";
```

in the Styles details sidebar.

[Devin Rousso]

<rdar://problem/56819044>

[Devin Rousso]

Created attachment 382614 [details]
Patch

[Nikita Vasilyev]

Created attachment 382628 [details]
[GIF] With patch applied

This value jumps between inline and inline-block.

When the value is inline-block, property name, ":" and ";" are vertically aligned to the bottom. It should be aligned to the top.

[Devin Rousso]

Created attachment 382633 [details]
Patch

[Nikita Vasilyev]

Comment on attachment 382633 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:127
> +        this._rawValueNewlineIndent = "";
> +        if (this._rawValue) {
> +            let match = this._rawValue.match(/^[^\n]+\n(\s*)/);
> +            if (match)
> +                this._rawValueNewlineIndent = match[1];
> +        }
> +        this._rawValue = this._rawValue.replace(/\n\s*/g, "\n");

Can this be handled by the view? I'd extend SpreadsheetTextField to maintain indentation for new lines.

> Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:467
> +            let value = this._rawValue.replace(/\n/g, "\n" + this._rawValueNewlineIndent);

Ditto.

[Devin Rousso]

Comment on attachment 382633 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:127
>> +        this._rawValue = this._rawValue.replace(/\n\s*/g, "\n");
> 
> Can this be handled by the view? I'd extend SpreadsheetTextField to maintain indentation for new lines.

We actually don't want the view to maintain the indentation, as that can lead to very weird results.

In the example I gave in the description, the `_rawValue` is actually

```
"header header header header"
                         "main main .sidebar"
                         "footer footer footer footer"
```

because there is no indentation, instead it's the property name.  We don't want to show anything remotely like that in the styles sidebar, so really we just want things to align based on the block of the multiline value.

If we can, however, we should try to re-insert the indentation when updating the underlying style sheet, so that if the developer goes to the Sources tab they can see it (hopefully) as they'd written it originally.  That's really the only reason we're trying to preserve the indentation at all.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.css:65
> +    vertical-align: bottom;

This cause have one slightly weird result, in that if the last line of the multiline value is _not_ the longest line, the ";" doesn't show up as the next character since the value is `display: inline-block;`.  All things considered, I think this is acceptable, given that we're providing a super useful piece of functionality, and it helps convey the idea of the value being a "block" of code (which I think is how most developers think when they have multiline CSS property values).

[Nikita Vasilyev]

Comment on attachment 382633 [details]
Patch

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

This could use a test. I'd add it to LayoutTests/inspector/css/modify-css-property.html.

>>> Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:127
>>> +        this._rawValue = this._rawValue.replace(/\n\s*/g, "\n");
>> 
>> Can this be handled by the view? I'd extend SpreadsheetTextField to maintain indentation for new lines.
> 
> We actually don't want the view to maintain the indentation, as that can lead to very weird results.
> 
> In the example I gave in the description, the `_rawValue` is actually
> 
> ```
> "header header header header"
>                          "main main .sidebar"
>                          "footer footer footer footer"
> ```
> 
> because there is no indentation, instead it's the property name.  We don't want to show anything remotely like that in the styles sidebar, so really we just want things to align based on the block of the multiline value.
> 
> If we can, however, we should try to re-insert the indentation when updating the underlying style sheet, so that if the developer goes to the Sources tab they can see it (hopefully) as they'd written it originally.  That's really the only reason we're trying to preserve the indentation at all.

I see. That makes sense.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetTextField.js:238
> +            if (this._delegate && this._delegate.spreadsheetTextFieldAllowsNewlines(this))

CSS value fields always allow new lines, and CSS name fields always don't. I'd add a constructor option to SpreadsheetTextField instead of a delegate.

[Nikita Vasilyev]

Comment on attachment 382633 [details]
Patch

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

>>>> Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:127
>>>> +        this._rawValue = this._rawValue.replace(/\n\s*/g, "\n");
>>> 
>>> Can this be handled by the view? I'd extend SpreadsheetTextField to maintain indentation for new lines.
>> 
>> We actually don't want the view to maintain the indentation, as that can lead to very weird results.
>> 
>> In the example I gave in the description, the `_rawValue` is actually
>> 
>> ```
>> "header header header header"
>>                          "main main .sidebar"
>>                          "footer footer footer footer"
>> ```
>> 
>> because there is no indentation, instead it's the property name.  We don't want to show anything remotely like that in the styles sidebar, so really we just want things to align based on the block of the multiline value.
>> 
>> If we can, however, we should try to re-insert the indentation when updating the underlying style sheet, so that if the developer goes to the Sources tab they can see it (hopefully) as they'd written it originally.  That's really the only reason we're trying to preserve the indentation at all.
> 
> I see. That makes sense.

Please add a comment. Otherwise it's hard to tell why this code is needed.

Nit: Looks like we can use /\n\s+/ instead of /\n\s*/

[Devin Rousso]

Comment on attachment 382633 [details]
Patch

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

>>>>> Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:127
>>>>> +        this._rawValue = this._rawValue.replace(/\n\s*/g, "\n");
>>>> 
>>>> Can this be handled by the view? I'd extend SpreadsheetTextField to maintain indentation for new lines.
>>> 
>>> We actually don't want the view to maintain the indentation, as that can lead to very weird results.
>>> 
>>> In the example I gave in the description, the `_rawValue` is actually
>>> 
>>> ```
>>> "header header header header"
>>>                          "main main .sidebar"
>>>                          "footer footer footer footer"
>>> ```
>>> 
>>> because there is no indentation, instead it's the property name.  We don't want to show anything remotely like that in the styles sidebar, so really we just want things to align based on the block of the multiline value.
>>> 
>>> If we can, however, we should try to re-insert the indentation when updating the underlying style sheet, so that if the developer goes to the Sources tab they can see it (hopefully) as they'd written it originally.  That's really the only reason we're trying to preserve the indentation at all.
>> 
>> I see. That makes sense.
> 
> Please add a comment. Otherwise it's hard to tell why this code is needed.
> 
> Nit: Looks like we can use /\n\s+/ instead of /\n\s*/

I intentionally wanted to use `/\n\s*/` because I want to match the first newline, regardless of whether it has any whitespace after it.

For example,

```
ab
cd
    ef
```

shouldn't use the indentation before "ef".

>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetTextField.js:238
>> +            if (this._delegate && this._delegate.spreadsheetTextFieldAllowsNewlines(this))
> 
> CSS value fields always allow new lines, and CSS name fields always don't. I'd add a constructor option to SpreadsheetTextField instead of a delegate.

We actually could expand this to, for example, not allow newlines when editing a CSS function (e.g. `rgb(...)`), as that isn't valid, and could `InspectorFrontendHost.beep()` in that case.

I'm fine either way though.

[Nikita Vasilyev]

Comment on attachment 382633 [details]
Patch

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

>>>>>> Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:127
>>>>>> +        this._rawValue = this._rawValue.replace(/\n\s*/g, "\n");
>>>>> 
>>>>> Can this be handled by the view? I'd extend SpreadsheetTextField to maintain indentation for new lines.
>>>> 
>>>> We actually don't want the view to maintain the indentation, as that can lead to very weird results.
>>>> 
>>>> In the example I gave in the description, the `_rawValue` is actually
>>>> 
>>>> ```
>>>> "header header header header"
>>>>                          "main main .sidebar"
>>>>                          "footer footer footer footer"
>>>> ```
>>>> 
>>>> because there is no indentation, instead it's the property name.  We don't want to show anything remotely like that in the styles sidebar, so really we just want things to align based on the block of the multiline value.
>>>> 
>>>> If we can, however, we should try to re-insert the indentation when updating the underlying style sheet, so that if the developer goes to the Sources tab they can see it (hopefully) as they'd written it originally.  That's really the only reason we're trying to preserve the indentation at all.
>>> 
>>> I see. That makes sense.
>> 
>> Please add a comment. Otherwise it's hard to tell why this code is needed.
>> 
>> Nit: Looks like we can use /\n\s+/ instead of /\n\s*/
> 
> I intentionally wanted to use `/\n\s*/` because I want to match the first newline, regardless of whether it has any whitespace after it.
> 
> For example,
> 
> ```
> ab
> cd
>     ef
> ```
> 
> shouldn't use the indentation before "ef".

I meant only the second regexp (line 127).

>>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetTextField.js:238
>>> +            if (this._delegate && this._delegate.spreadsheetTextFieldAllowsNewlines(this))
>> 
>> CSS value fields always allow new lines, and CSS name fields always don't. I'd add a constructor option to SpreadsheetTextField instead of a delegate.
> 
> We actually could expand this to, for example, not allow newlines when editing a CSS function (e.g. `rgb(...)`), as that isn't valid, and could `InspectorFrontendHost.beep()` in that case.
> 
> I'm fine either way though.

I see. By the way, new lines inside of rgb(...) are actually valid :p

[Devin Rousso]

Created attachment 382780 [details]
Patch

[Blaze Burg]

Comment on attachment 382780 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 382780 [details]
Patch

Clearing flags on attachment: 382780

Committed r252523: <https://trac.webkit.org/changeset/252523>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.