Bug 179587

Summary: Web Inspector: Styles Redesign: can't add new property after property without trailing semicolon
Product: WebKit Reporter: Nikita Vasilyev <nvasilyev>
Component: Web InspectorAssignee: Nikita Vasilyev <nvasilyev>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, inspector-bugzilla-changes, timothy, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Attachments:
Description Flags
[Animated GIF] Bug
none
Patch
timothy: review-
[Animated GIF] With patch applied
none
Patch
timothy: review+
Patch none

Description Nikita Vasilyev 2017-11-11 19:26:56 PST 
Created attachment 326703 [details]
[Animated GIF] Bug

Steps:
1. Open http://nv.github.io/webkit-inspector-bugs/styles-redesign/tests/no-trailing-semicolon.html
2. Inspect <body>.
3. Click on the white space after "color: #333".
4. Add any property, such as "font-size: 12px".

Expected:
"font-size: 12px" property gets added.

Actual:
"color: #333" no longer works and the newly added property doesn't work either.

Notes:
When there's no ";" after the last property, it doesn't get added, resulting in this:

    color: #333font-size: 12px;
Comment 1 Radar WebKit Bug Importer 2017-11-11 19:27:17 PST 
<rdar://problem/35490858>
Comment 2 Nikita Vasilyev 2017-11-27 15:00:23 PST 
Created attachment 327689 [details]
Patch
Comment 3 Nikita Vasilyev 2017-11-27 15:04:37 PST 
Created attachment 327691 [details]
[Animated GIF] With patch applied
Comment 4 Timothy Hatcher 2017-11-28 14:58:46 PST 
You should add a space after the semicolon too.
Comment 5 Timothy Hatcher 2017-11-28 15:05:33 PST 
Comment on attachment 327689 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:382
> +        let trimmedText = styleText.trimRight();
> +        if (!trimmedText)
> +            return styleText;
> +
> +        if (trimmedText.slice(-1) === ";")

Trimming and slicing just to look for ";" seems expensive to me. Consider using `lastIndexOf` instead.

> Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:384
> +        else

No need for this else after a return.

> Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:385
> +            return styleText + ";";

Add a space with the semicolon too?
Comment 6 Nikita Vasilyev 2017-11-28 15:22:27 PST 
Comment on attachment 327689 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:382
>> +        if (trimmedText.slice(-1) === ";")
> 
> Trimming and slicing just to look for ";" seems expensive to me. Consider using `lastIndexOf` instead.

If I use lastIndexOf I'd have to check if the text after ";" is a white space. Consider:

    font-size:12px;
    color: red

Maybe I should just use a simple RegExp?

    if (/;\s*$/.test(styleText))

>> Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:385
>> +            return styleText + ";";
> 
> Add a space with the semicolon too?

I'll add a space in this patch, and I'll add proper indentation in Bug 178835 - Web Inspector: [PARITY] Styles Redesign: Add indentation set in settings for newly added properties.
Comment 7 Nikita Vasilyev 2017-11-28 18:54:24 PST 
(In reply to Nikita Vasilyev from comment #6)
> Comment on attachment 327689 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=327689&action=review
> 
> >> Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:382
> >> +        if (trimmedText.slice(-1) === ";")
> > 
> > Trimming and slicing just to look for ";" seems expensive to me. Consider using `lastIndexOf` instead.
> 
> If I use lastIndexOf I'd have to check if the text after ";" is a white
> space. Consider:
> 
>     font-size:12px;
>     color: red
> 
> Maybe I should just use a simple RegExp?
> 
>     if (/;\s*$/.test(styleText))

After looking at it more, I couldn't make this function any simpler by using lastIndexOf. Consider the following cases:

    "\n  " -> "\n  "
    "color: red" -> "color: red; "
    "color: red;" -> "color: red;"
    "font-size: 12px; color: red" -> "font-size: 12px; color: red; "

trimmedText.slice(-1) returns the last character. It shouldn't be an expensive operation.
Comment 8 Timothy Hatcher 2017-11-29 10:20:51 PST 
Comment on attachment 327689 [details]
Patch

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

>>>> Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:382
>>>> +        if (trimmedText.slice(-1) === ";")
>>> 
>>> Trimming and slicing just to look for ";" seems expensive to me. Consider using `lastIndexOf` instead.
>> 
>> If I use lastIndexOf I'd have to check if the text after ";" is a white space. Consider:
>> 
>>     font-size:12px;
>>     color: red
>> 
>> Maybe I should just use a simple RegExp?
>> 
>>     if (/;\s*$/.test(styleText))
> 
> After looking at it more, I couldn't make this function any simpler by using lastIndexOf. Consider the following cases:
> 
>     "\n  " -> "\n  "
>     "color: red" -> "color: red; "
>     "color: red;" -> "color: red;"
>     "font-size: 12px; color: red" -> "font-size: 12px; color: red; "
> 
> trimmedText.slice(-1) returns the last character. It shouldn't be an expensive operation.

You are right, I wasn't thinking about whitespace. I think regex would be best then. The initial trimRight() is what I was wanting to avoid.
Comment 9 Nikita Vasilyev 2017-11-29 14:00:36 PST 
Created attachment 327901 [details]
Patch
Comment 10 Nikita Vasilyev 2017-11-29 14:04:54 PST 
Comment on attachment 327901 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:378
> +        if (/[^;\s]\s*$/.test(styleText))

I inverted the logic in the RegExp, since

    if (!/;\s*$/.test(styleText)

this would add a semicolon to "\n  ".
Comment 11 Timothy Hatcher 2017-11-29 14:19:18 PST 
Comment on attachment 327901 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Models/CSSProperty.js:379
> +            return styleText + "; ";

In this case I think return styleText.trimRight() + "; "; would be better then. And it only trims if needed.
Comment 12 Nikita Vasilyev 2017-11-29 14:43:10 PST 
Created attachment 327910 [details]
Patch
Comment 13 WebKit Commit Bot 2017-11-29 15:16:23 PST 
Comment on attachment 327910 [details]
Patch

Clearing flags on attachment: 327910

Committed r225299: <https://trac.webkit.org/changeset/225299>
Comment 14 WebKit Commit Bot 2017-11-29 15:16:25 PST 
All reviewed patches have been landed.  Closing bug.