Bug 218295 – Web Inspector: Extra closing parenthesis added after var in styles panel

• Component: Web Inspector
• Status: RESOLVED FIXED

[Zach Bjornson]

Given a rule like:

.class {
  --my-var: calc(var(--other-var) * 1em);
}

The Styles panel adds an extra closing parenthesis, making it look like:

  --my-var: calc(var(--other-var)) * 1em);  

Tested in Safari 13.1.2.

[Nikita Vasilyev]

Thank you for reporting, I'll look into this.

[Radar WebKit Bug Importer]

<rdar://problem/70771314>

[Nikita Vasilyev]

WI.SpreadsheetStyleProperty.prototype._addVariableTokens(tokens) returns an array of tokens with an extra closed parenthesis.

[Nikita Vasilyev]

Created attachment 412596 [details]
WIP

This fixes the bug. However, this method is pretty error prone and could really benefit from unit tests. I'm planning to move it outside of the Views.

[Devin Rousso]

Comment on attachment 412596 [details]
WIP

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

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:-826
> -                        contents.pushAll(rawTokens.slice(variableNameIndex + 1, i));

We also probably should remove the `, i` above too.  Using `i` is incorrect because `rawTokens` is not the same length as `tokens` :/

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:-827
> -                    contents.push(token);

I think this was ultimately the issue (i.e. you could leave the `i + 1` as is above).

[Nikita Vasilyev]

Comment on attachment 412596 [details]
WIP

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

>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:-826
>> -                        contents.pushAll(rawTokens.slice(variableNameIndex + 1, i));
> 
> We also probably should remove the `, i` above too.  Using `i` is incorrect because `rawTokens` is not the same length as `tokens` :/

You mean on the line 817, right? Yes, it shouldn't be used there.
And yes, that's why I'm removing `, i`.

>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:-827
>> -                    contents.push(token);
> 
> I think this was ultimately the issue (i.e. you could leave the `i + 1` as is above).

My change on line 793 alone fixes this bug. At the same time, all the changes in this patch but on the line 793 fixes the bug, too :)
However, keeping the ")" at the end of rawTokens isn't used for anything, and is a footgun.

[Nikita Vasilyev]

Comment on attachment 412596 [details]
WIP

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

>>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js:-827
>>> -                    contents.push(token);
>> 
>> I think this was ultimately the issue (i.e. you could leave the `i + 1` as is above).
> 
> My change on line 793 alone fixes this bug. At the same time, all the changes in this patch but on the line 793 fixes the bug, too :)
> However, keeping the ")" at the end of rawTokens isn't used for anything, and is a footgun.

Actually, I see rawTokens is being used in fallbackTokens, which is passed to the rest of _add*Tokens function. I'll take your suggestion and leave it as-is.
"At the same time, all the changes in this patch but on the line 793 fixes the bug, too :)" — this statement was incorrect :/

[Nikita Vasilyev]

Created attachment 412694 [details]
Patch

Now it's a really tiny patch ☺️

[Devin Rousso]

Comment on attachment 412694 [details]
Patch

r=me

[EWS]

Committed r269201: <https://trac.webkit.org/changeset/269201>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 412694 [details].