Bug 182523 – Web Inspector: Styles: close unbalanced quotes and parenthesis when editing values

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

[Nikita Vasilyev]

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.

[Radar WebKit Bug Importer]

<rdar://problem/37260209>

[Nikita Vasilyev]

http://nv.github.io/CSSOM/docs/fix-value/

I made a demo page for the function that fixes unbalanced quotes (' and "), parenthesis, and comments.

[Nikita Vasilyev]

Created attachment 361144 [details]
WIP

I haven't added tests yet, but you're free to apply the patch and try it out.

[Nikita Vasilyev]

Created attachment 361146 [details]
[animated GIF] With patch applied

Try to break it :)

[Nikita Vasilyev]

Created attachment 361363 [details]
Patch

[Nikita Vasilyev]

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.

[Nikita Vasilyev]

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

[Devin Rousso]

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.

[Nikita Vasilyev]

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.

[Nikita Vasilyev]

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);`.

[Nikita Vasilyev]

Created attachment 361521 [details]
Patch for landing

[Devin Rousso]

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 `}`).

[Nikita Vasilyev]

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.

[WebKit Commit Bot]

Comment on attachment 361521 [details]
Patch for landing

Clearing flags on attachment: 361521

Committed r241209: <https://trac.webkit.org/changeset/241209>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Nikita Vasilyev]

*** Bug 154492 has been marked as a duplicate of this bug. ***

[Nikita Vasilyev]

*** Bug 194662 has been marked as a duplicate of this bug. ***