Bug 182523

Summary: Web Inspector: Styles: close unbalanced quotes and parenthesis when editing values
Product: WebKit Reporter: Nikita Vasilyev <nvasilyev>
Component: Web InspectorAssignee: Nikita Vasilyev <nvasilyev>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, hi, inspector-bugzilla-changes, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Attachments:
Description Flags
[Animated GIF] Bug
none
WIP
nvasilyev: review-, nvasilyev: commit-queue-
[animated GIF] With patch applied
none
Patch
hi: review+, hi: commit-queue-
[Animated GIF] With patch applied
none
Patch for landing none

Description Nikita Vasilyev 2018-02-05 19:37:23 PST
Created attachment 333149 [details]
[Animated GIF] Bug

Steps:
1. Open http://nv.github.io/webkit-inspector-bugs/styles-redesign/tests/commenting-rules.html
2. Inspect one of the div elements.
3. Add `font-family: 'Helvetica` CSS property to one of the rules.

Expected:
`font-family: 'Helvetica'` gets added. Notice the added closing quote at the end.

Actual:
Something like `font-family: 'Helvetica; display: inline-block` shows as unsupported property.

Notes:
This doesn't work very well in Chrome DevTools either.
Comment 1 Radar WebKit Bug Importer 2018-02-05 19:38:11 PST
<rdar://problem/37260209>
Comment 2 Nikita Vasilyev 2019-02-04 18:05:09 PST
http://nv.github.io/CSSOM/docs/fix-value/

I made a demo page for the function that fixes unbalanced quotes (' and "), parenthesis, and comments.
Comment 3 Nikita Vasilyev 2019-02-04 18:15:05 PST
Created attachment 361144 [details]
WIP

I haven't added tests yet, but you're free to apply the patch and try it out.
Comment 4 Nikita Vasilyev 2019-02-04 18:19:30 PST
Created attachment 361146 [details]
[animated GIF] With patch applied

Try to break it :)
Comment 5 Nikita Vasilyev 2019-02-06 19:03:26 PST
Created attachment 361363 [details]
Patch
Comment 6 Nikita Vasilyev 2019-02-06 19:13:16 PST
I want to split this into two patches:

Part 1: make it impossible to commit CSS values with unbalanced characters. The patch for this is above.

Part 2: provide good autocompletion UX, as shown on https://bugs.webkit.org/attachment.cgi?id=361146&action=edit. This is going to be a follow-up patch.
Comment 7 Nikita Vasilyev 2019-02-06 19:13:51 PST
Created attachment 361367 [details]
[Animated GIF] With patch applied
Comment 8 Devin Rousso 2019-02-07 10:53:29 PST
Comment on attachment 361363 [details]
Patch

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

r=me, awesome work.  I'd love it if we could adapt this to work with any type of string (<https://webkit.org/b/192102> would benefit from that).

> LayoutTests/inspector/unit-tests/css-completion.html:8
> +    let suite = InspectorTest.createSyncSuite("CSSCompletion");

NIT: this should probably be `CSSCompletions` to match the model class name.

> LayoutTests/inspector/unit-tests/css-completion.html:11
> +        name: "fixUnbalancedCharacters",

NIT: please prefix the test case name with the suite name.

    CSSCompletions.fixUnbalancedCharacters.

> LayoutTests/inspector/unit-tests/css-completion.html:43
> +            log(`('foo"`);
> +            log(`('foo")`);
> +            log(`("bar"')`);
> +            log(`("bar")`);

As a sanity check, we should add tests for when a parenthesis is inside quotes.

    log (`"(`);
    log (`"(foo`);
    log (`'(`);
    log (`'(bar`);

> LayoutTests/inspector/unit-tests/css-completion.html:51
> +            log(`"\\"`);
> +            log(`'\\'`);
> +            log(`(\\)`);

We should also test when there are multiple sequences of quotes/parenthesis in a single value (e.g. `font-family`, `calc(...)`, `linear-gradient(...)`/`radial-gradient(...)` with some color/`var`/`calc` inside it).

> Source/WebInspectorUI/UserInterface/Models/CSSCompletions.js:174
> +    static fixUnbalancedCharacters(value)

I'm not a huge fan of this name.  Using `fix` implies to me that it will modify `value` (which isn't possible if it's a string, as you'd create a new value that wouldn't be reflected back on the caller argument).

How about `determineUnbalancedCharacters`?

> Source/WebInspectorUI/UserInterface/Models/CSSCompletions.js:177
> +            Data: 0,

Is there a reason you skipped `1`?

> Source/WebInspectorUI/UserInterface/Models/CSSCompletions.js:180
> +            SingleQuoteString: 2,
> +            DoubleQuoteString: 3,
> +            Comment: 4

Rather than keep these values as numbers, why not make them the actual character value (e.g. a token), and replace `State.Data` with a null value?

    const Token = {
        SingleQuote: `'`,
        DoubleQuote: `"`,
        OpenParenthesis: `(`,
        ClosedParenthesis: `)`,
        Comment:  `/`,
        Asterisk: `*`,
        Backslash: `\\`,
    };

    let currentToken = null;

    ...

    switch (value[i]) {
    case Token.SingleQuote:
        if (!currentToken)
            currentToken = Token.SingleQuote;
        else if (currentToken === Token.SingleQuote)
            currentToken = null;
        ...

    case Token.DoubleQuote:
        ...

    case Token.OpenParenthesis:
        ...

    case Token.ClosedParenthesis:
        ...

    case Token.Comment:
        ...

    case Token.Asterisk:
        ...

    case Token.Backslash:
        ...
    }

> Source/WebInspectorUI/UserInterface/Models/CSSCompletions.js:184
> +        let parenthesisUnclosedCount = 0;

NIT: I'd flip this name to be `unclosedParenthesisCount`.

> Source/WebInspectorUI/UserInterface/Models/CSSCompletions.js:210
> +                if (state === State.Data && parenthesisUnclosedCount)

Aside: we'd still have an issue if I type `display: );`, so maybe we'd want to add a version of this function that tries to calculate a prefix (AFAICT, it would only add a leading open parenthesis or open comment, as everything else can be handled by a suffix).  Just a thought :)

> Source/WebInspectorUI/UserInterface/Models/CSSCompletions.js:230
> +                    if (i + 1 < length && value[i + 1] === "/")

NIT: since you're just checking the value (e.g. not trying to call any functions on the value), you can drop the `i + 1 < length` check.

> Source/WebInspectorUI/UserInterface/Models/CSSCompletions.js:240
> +            suffix = "\\";

NIT: I get that the string is empty until this point, but you're using `+=` later, so I think we should also use `+=` here to be consistent.

> Source/WebInspectorUI/UserInterface/Models/CSSCompletions.js:254
> +        if (parenthesisUnclosedCount)

NIT: this `if` is unnecessary, as a `")".repeat(0)` will just return an empty string.
Comment 9 Nikita Vasilyev 2019-02-07 16:35:05 PST
Comment on attachment 361363 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Models/CSSCompletions.js:174
>> +    static fixUnbalancedCharacters(value)
> 
> I'm not a huge fan of this name.  Using `fix` implies to me that it will modify `value` (which isn't possible if it's a string, as you'd create a new value that wouldn't be reflected back on the caller argument).
> 
> How about `determineUnbalancedCharacters`?

I don't like fixUnbalancedCharacters, but I'm not a huge fan of determineUnbalancedCharacters either. I'd like to stress out that this function returns a suffix string, such as `*/`.

completeUnbalancedCharacters?
completeUnbalancedValue?

I'm open to hearing more options.
Comment 10 Nikita Vasilyev 2019-02-08 11:43:45 PST
Comment on attachment 361363 [details]
Patch

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

>> LayoutTests/inspector/unit-tests/css-completion.html:8
>> +    let suite = InspectorTest.createSyncSuite("CSSCompletion");
> 
> NIT: this should probably be `CSSCompletions` to match the model class name.

Good point.

>> Source/WebInspectorUI/UserInterface/Models/CSSCompletions.js:177
>> +            Data: 0,
> 
> Is there a reason you skipped `1`?

I skipped it by mistake.

>> Source/WebInspectorUI/UserInterface/Models/CSSCompletions.js:180
>> +            Comment: 4
> 
> Rather than keep these values as numbers, why not make them the actual character value (e.g. a token), and replace `State.Data` with a null value?
> 
>     const Token = {
>         SingleQuote: `'`,
>         DoubleQuote: `"`,
>         OpenParenthesis: `(`,
>         ClosedParenthesis: `)`,
>         Comment:  `/`,
>         Asterisk: `*`,
>         Backslash: `\\`,
>     };
> 
>     let currentToken = null;
> 
>     ...
> 
>     switch (value[i]) {
>     case Token.SingleQuote:
>         if (!currentToken)
>             currentToken = Token.SingleQuote;
>         else if (currentToken === Token.SingleQuote)
>             currentToken = null;
>         ...
> 
>     case Token.DoubleQuote:
>         ...
> 
>     case Token.OpenParenthesis:
>         ...
> 
>     case Token.ClosedParenthesis:
>         ...
> 
>     case Token.Comment:
>         ...
> 
>     case Token.Asterisk:
>         ...
> 
>     case Token.Backslash:
>         ...
>     }

I'd rather keep State to have a clear indication of how many states there are.

>> Source/WebInspectorUI/UserInterface/Models/CSSCompletions.js:184
>> +        let parenthesisUnclosedCount = 0;
> 
> NIT: I'd flip this name to be `unclosedParenthesisCount`.

Sounds better!

>> Source/WebInspectorUI/UserInterface/Models/CSSCompletions.js:210
>> +                if (state === State.Data && parenthesisUnclosedCount)
> 
> Aside: we'd still have an issue if I type `display: );`, so maybe we'd want to add a version of this function that tries to calculate a prefix (AFAICT, it would only add a leading open parenthesis or open comment, as everything else can be handled by a suffix).  Just a thought :)

I'm not sure what's the intention behind typing `dispaly: );`.
Both `display: );` and `display: ();` would be invalid values.

If I replace this example with "color", I wouldn't know where to add an opening parenthesis to make it valid: `color: rgb1,2,3);`.
Comment 11 Nikita Vasilyev 2019-02-08 11:53:55 PST
Created attachment 361521 [details]
Patch for landing
Comment 12 Devin Rousso 2019-02-08 13:29:17 PST
Comment on attachment 361363 [details]
Patch

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

>>> Source/WebInspectorUI/UserInterface/Models/CSSCompletions.js:210
>>> +                if (state === State.Data && parenthesisUnclosedCount)
>> 
>> Aside: we'd still have an issue if I type `display: );`, so maybe we'd want to add a version of this function that tries to calculate a prefix (AFAICT, it would only add a leading open parenthesis or open comment, as everything else can be handled by a suffix).  Just a thought :)
> 
> I'm not sure what's the intention behind typing `dispaly: );`.
> Both `display: );` and `display: ();` would be invalid values.
> 
> If I replace this example with "color", I wouldn't know where to add an opening parenthesis to make it valid: `color: rgb1,2,3);`.

I mentioned this more to make sure we didn't wreck the stylesheet, but I don't think that a lone closing parenthesis will cause an issue like a lone quote would.  It wasn't about trying to insert it in the right place as much as it was trying to not completely wreck the whole stylesheet (e.g. adding a lone `}`).
Comment 13 Nikita Vasilyev 2019-02-08 13:51:55 PST
Comment on attachment 361521 [details]
Patch for landing

(In reply to Devin Rousso from comment #12)
> I mentioned this more to make sure we didn't wreck the stylesheet, but I
> don't think that a lone closing parenthesis will cause an issue like a lone
> quote would.  It wasn't about trying to insert it in the right place as much
> as it was trying to not completely wreck the whole stylesheet (e.g. adding a
> lone `}`).

Perhaps we should escape `}` as well.
Comment 14 WebKit Commit Bot 2019-02-08 14:18:01 PST
Comment on attachment 361521 [details]
Patch for landing

Clearing flags on attachment: 361521

Committed r241209: <https://trac.webkit.org/changeset/241209>
Comment 15 WebKit Commit Bot 2019-02-08 14:18:03 PST
All reviewed patches have been landed.  Closing bug.
Comment 16 Nikita Vasilyev 2019-02-10 20:23:03 PST
*** Bug 154492 has been marked as a duplicate of this bug. ***
Comment 17 Nikita Vasilyev 2019-02-15 17:35:18 PST
*** Bug 194662 has been marked as a duplicate of this bug. ***