Bug 178498 – Web Inspector: Styles Redesign: Typing semicolon at the end of value should move to the next property

• Component: Web Inspector
• Status: RESOLVED FIXED

[Chris Chiera]

In the past few versions enabling spreadsheet experiment didn't really work, but looks like in latest version its starting working to wanted to provide some feedback.

In Chrome if you enter a ";" it closes that line and moves to the next. With Webkit/Safari it does not. In fact it lets you add multiple ";" or add one and still has a faded out gray one. Should always only show one, and if you enter a ";" should close that rule and move to next line.

[Nikita Vasilyev]

Thanks for the feedback.

I haven't yet implemented special handling of semicolons in values, but I'm planning to.

[Radar WebKit Bug Importer]

<rdar://problem/35065995>

[Devin Rousso]

Created attachment 328330 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 328330 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetTextField.js:262
> +            const quoteRegex = /["']/g;
> +            let value = this.value;
> +            let start = -1;
> +            let end = value.length;
> +            let match = null;
> +            while (match = quoteRegex.exec(value)) {
> +                if (start < 0)
> +                    start = match.index;
> +                end = match.index + 1;
> +            }
> +
> +            if (value.substring(start, end).hasMatchingEscapedQuotes()) {
> +                commit();
> +                return;
> +            }

I see what this is trying to do but I don't think it is doing it correctly. Right now it is just scanning for quotes, and doesn't detect if those quotes are within quotes.

I think the algorithm you want is something like this:

    if (event.key === ";") {
        // We only want to commit on semicolon if the semicolon was at the end of the value.
        let semicolonInsertionIndex = window.getSelection().getRangeAt(0).startOffset;
        if (semicolonInsertionIndex !== this.value.length)
            return;

        // Determine if the semicolon is inside of an unfinished string.
        // First gather all complete quoted strings. Then check at the
        // end for an unfinished string.
        let range;
        let startIndex = 0;
        while (range = findQuotedStringRange(value, startIndex))
            startIndex = range.endIndex + 1;
        range = findUnterminatedStringRange(value, startIndex);
        if (range) {
            // Assert that the unterminated string range does go to the end of the string (where the semicolon is).
            console.assert(range.endIndex === value.length - 1);
            return;
        }

        // We have now determined this is a semicolon, not inside of an unterminated string
        // and at the end of input. Lets go!
        commit();
        return;
    }

With findQuotedStringRange / findUnterminatedStringRange left as an exercise for the reader.

That make sense?

[Nikita Vasilyev]

Comment on attachment 328330 [details]
Patch

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

> Source/WebInspectorUI/ChangeLog:12
> +        semicolon after `url("foo` or `url(bar` would not move to the next property.

`url(bar` doesn't have any quotes. I don't see in your code handling of parenthesis.

[Devin Rousso]

Comment on attachment 328330 [details]
Patch

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

>> Source/WebInspectorUI/ChangeLog:12
>> +        semicolon after `url("foo` or `url(bar` would not move to the next property.
> 
> `url(bar` doesn't have any quotes. I don't see in your code handling of parenthesis.

I don't think we have to worry about parenthesis.  I was only worried about quotes for situations like `content: ";";`, as the first semicolon is a string, not a property "terminator" (not sure what the word is).

>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetTextField.js:262
>> +            }
> 
> I see what this is trying to do but I don't think it is doing it correctly. Right now it is just scanning for quotes, and doesn't detect if those quotes are within quotes.
> 
> I think the algorithm you want is something like this:
> 
>     if (event.key === ";") {
>         // We only want to commit on semicolon if the semicolon was at the end of the value.
>         let semicolonInsertionIndex = window.getSelection().getRangeAt(0).startOffset;
>         if (semicolonInsertionIndex !== this.value.length)
>             return;
> 
>         // Determine if the semicolon is inside of an unfinished string.
>         // First gather all complete quoted strings. Then check at the
>         // end for an unfinished string.
>         let range;
>         let startIndex = 0;
>         while (range = findQuotedStringRange(value, startIndex))
>             startIndex = range.endIndex + 1;
>         range = findUnterminatedStringRange(value, startIndex);
>         if (range) {
>             // Assert that the unterminated string range does go to the end of the string (where the semicolon is).
>             console.assert(range.endIndex === value.length - 1);
>             return;
>         }
> 
>         // We have now determined this is a semicolon, not inside of an unterminated string
>         // and at the end of input. Lets go!
>         commit();
>         return;
>     }
> 
> With findQuotedStringRange / findUnterminatedStringRange left as an exercise for the reader.
> 
> That make sense?

I don't think we need to detect quotes within quotes.  All I really care about is that there are no unbalanced quotes, as in that case the semicolon could be part of the value instead of the property "terminator".

// OK
url(abc);
url("abc");
url('abc');
url("a'b'c");
url('a"b"c');
url("a\"b\"c");
url('a\'b\'c');

// Bad
url("ab
url('ab
url("a"b"
url('a'b'
url("a"b'
url('a'b"
url("a"'b'
url('a'"b"

The purpose of the while loop is to find the index (if any) of the first and last quote character (" or ') so that I can then check that substring to ensure that it is properly "wrapped".  If it is, then there's no way that the semicolon could be part of a string, so we can assume that it's the property "terminator".

[Joseph Pecoraro]

Comment on attachment 328330 [details]
Patch

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

>>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetTextField.js:262
>>> +            }
>> 
>> I see what this is trying to do but I don't think it is doing it correctly. Right now it is just scanning for quotes, and doesn't detect if those quotes are within quotes.
>> 
>> I think the algorithm you want is something like this:
>> 
>>     if (event.key === ";") {
>>         // We only want to commit on semicolon if the semicolon was at the end of the value.
>>         let semicolonInsertionIndex = window.getSelection().getRangeAt(0).startOffset;
>>         if (semicolonInsertionIndex !== this.value.length)
>>             return;
>> 
>>         // Determine if the semicolon is inside of an unfinished string.
>>         // First gather all complete quoted strings. Then check at the
>>         // end for an unfinished string.
>>         let range;
>>         let startIndex = 0;
>>         while (range = findQuotedStringRange(value, startIndex))
>>             startIndex = range.endIndex + 1;
>>         range = findUnterminatedStringRange(value, startIndex);
>>         if (range) {
>>             // Assert that the unterminated string range does go to the end of the string (where the semicolon is).
>>             console.assert(range.endIndex === value.length - 1);
>>             return;
>>         }
>> 
>>         // We have now determined this is a semicolon, not inside of an unterminated string
>>         // and at the end of input. Lets go!
>>         commit();
>>         return;
>>     }
>> 
>> With findQuotedStringRange / findUnterminatedStringRange left as an exercise for the reader.
>> 
>> That make sense?
> 
> I don't think we need to detect quotes within quotes.  All I really care about is that there are no unbalanced quotes, as in that case the semicolon could be part of the value instead of the property "terminator".
> 
> // OK
> url(abc);
> url("abc");
> url('abc');
> url("a'b'c");
> url('a"b"c');
> url("a\"b\"c");
> url('a\'b\'c');
> 
> // Bad
> url("ab
> url('ab
> url("a"b"
> url('a'b'
> url("a"b'
> url('a'b"
> url("a"'b'
> url('a'"b"
> 
> The purpose of the while loop is to find the index (if any) of the first and last quote character (" or ') so that I can then check that substring to ensure that it is properly "wrapped".  If it is, then there's no way that the semicolon could be part of a string, so we can assume that it's the property "terminator".

Hmm. Then this deserves a comment in the code at this point to inform the reader about what it is trying to do. Since I saw this, assumed it was trying to do something else, and came to different conclusions.

In the end I do think a simple sanity check makes sense. As NVI pointed out, top level parenthesis throw a wrench in really parsing (and I hadn't even considered them).

I'm fine with a simple check as long as there is a comment describing what it is doing, because it isn't intended to be a 100% perfect solution and should say so.

[Nikita Vasilyev]

Comment on attachment 328330 [details]
Patch

r- because this patch doesn't work for values without any quotes, e.g.:

    color: red;

I think typing ; only in property value should have a special case.
Typing ; in property name shouldn't navigate to property value.

I implemented the special case for ":" (https://trac.webkit.org/changeset/224906/webkit)
in SpreadsheetStyleProperty and not SpreadsheetTextField because I didn't want to add any
CSS property logic to SpreadsheetTextField, so I could (hopefully) merge SpreadsheetTextField
with SpreadsheetSelectorField later.

[Devin Rousso]

Created attachment 329164 [details]
Patch

[Nikita Vasilyev]

Comment on attachment 329164 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:513
> +        let text = this._valueElement.textContent;
> +        if (window.getSelection().getRangeAt(0).startOffset !== text.length)
> +            return;

Typing ; should move to the next property even if it's before the suggestion hint.

Consider:

  float: r[ight]

[irht] is a suggestion hint here. Typing ; should move the focus to the next property.

SpreadsheetTextField.prototype._getPrefix returns text without the suggestion hint. You can make it public and possibly rename to something more meaningful.

[Devin Rousso]

Created attachment 329433 [details]
Patch

[Nikita Vasilyev]

Comment on attachment 329433 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:511
> +        if (window.getSelection().getRangeAt(0).startOffset !== text.length)

1. When the whole value is selected, I'd expect typing ";" to move to the next field. This can be done by replacing startOffset with endOffset.

2. Usually, you want to check getSelection().rangeCount before calling getRangeAt. When nothing is selected, getRangeAt throws an exception:

    (DOM Exception 1): The index is not in the allowed range.

I don't know if it ever happens here.

[Devin Rousso]

Created attachment 329677 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 329677 [details]
Patch

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

r=me

> Source/WebInspectorUI/ChangeLog:13
> +        semicolon after `url("foo` or `url(bar` would not move to the next property.

I don't see how it would make `url(bar;` not move to the next property.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:528
> +        if (!quoteRegex.test(text) || text.substring(start, end).hasMatchingEscapedQuotes()) {

No need to run the regex again, just check if we found a match:

    if (start === -1 || ...)

Better yet make this an early return:

    if (start !== -1 && !text.substring(start, end).hasMatchingEscapedQuotes())
        return;

[Devin Rousso]

Comment on attachment 329677 [details]
Patch

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

>> Source/WebInspectorUI/ChangeLog:13
>> +        semicolon after `url("foo` or `url(bar` would not move to the next property.
> 
> I don't see how it would make `url(bar;` not move to the next property.

Oops.  This was supposed to be `url('bar;`.

[Devin Rousso]

Created attachment 329831 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 329831 [details]
Patch

Clearing flags on attachment: 329831

Committed r226149: <https://trac.webkit.org/changeset/226149>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.