Bug 218295

Summary: Web Inspector: Extra closing parenthesis added after var in styles panel
Product: WebKit Reporter: Zach Bjornson <zbbjornson>
Component: Web InspectorAssignee: Nikita Vasilyev <nvasilyev>
Status: RESOLVED FIXED    
Severity: Normal CC: drousso, ews-watchlist, inspector-bugzilla-changes, nvasilyev, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: Safari 13   
Hardware: All   
OS: All   
See Also: https://bugs.webkit.org/show_bug.cgi?id=218357
Attachments:
Description Flags
WIP
nvasilyev: commit-queue-
Patch none

Description Zach Bjornson 2020-10-28 11:00:56 PDT
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.
Comment 1 Nikita Vasilyev 2020-10-28 11:02:21 PDT
Thank you for reporting, I'll look into this.
Comment 2 Radar WebKit Bug Importer 2020-10-28 11:02:58 PDT
<rdar://problem/70771314>
Comment 3 Nikita Vasilyev 2020-10-28 16:13:54 PDT
WI.SpreadsheetStyleProperty.prototype._addVariableTokens(tokens) returns an array of tokens with an extra closed parenthesis.
Comment 4 Nikita Vasilyev 2020-10-28 17:58:52 PDT
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.
Comment 5 Devin Rousso 2020-10-29 12:02:24 PDT
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).
Comment 6 Nikita Vasilyev 2020-10-29 14:08:23 PDT
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.
Comment 7 Nikita Vasilyev 2020-10-29 15:37:39 PDT
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 :/
Comment 8 Nikita Vasilyev 2020-10-29 15:38:53 PDT
Created attachment 412694 [details]
Patch

Now it's a really tiny patch ☺️
Comment 9 Devin Rousso 2020-10-30 10:15:43 PDT
Comment on attachment 412694 [details]
Patch

r=me
Comment 10 EWS 2020-10-30 11:06:33 PDT
Committed r269201: <https://trac.webkit.org/changeset/269201>

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