218295 2020-10-28 11:00:56 -0700 Web Inspector: Extra closing parenthesis added after var in styles panel 2020-10-30 11:06:35 -0700 1 1 1 Unclassified WebKit Web Inspector Safari 13 All All RESOLVED FIXED https://bugs.webkit.org/show_bug.cgi?id=218357 InRadar P2 Normal --- 1 zbbjornson nvasilyev ews-watchlist hi inspector-bugzilla-changes nvasilyev webkit-bug-importer oldest_to_newest 1702385 0 zbbjornson 2020-10-28 11:00:56 -0700 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. 1702387 1 nvasilyev 2020-10-28 11:02:21 -0700 Thank you for reporting, I'll look into this. 1702390 2 webkit-bug-importer 2020-10-28 11:02:58 -0700 <rdar://problem/70771314> 1702544 3 nvasilyev 2020-10-28 16:13:54 -0700 WI.SpreadsheetStyleProperty.prototype._addVariableTokens(tokens) returns an array of tokens with an extra closed parenthesis. 1702583 4 412596 nvasilyev 2020-10-28 17:58:52 -0700 Created attachment 412596 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. 1702810 5 412596 hi 2020-10-29 12:02:24 -0700 Comment on attachment 412596 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). 1702857 6 412596 nvasilyev 2020-10-29 14:08:23 -0700 Comment on attachment 412596 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. 1702899 7 412596 nvasilyev 2020-10-29 15:37:39 -0700 Comment on attachment 412596 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 :/ 1702900 8 412694 nvasilyev 2020-10-29 15:38:53 -0700 Created attachment 412694 Patch Now it's a really tiny patch ☺️ 1703113 9 412694 hi 2020-10-30 10:15:43 -0700 Comment on attachment 412694 Patch r=me 1703130 10 ews-feeder 2020-10-30 11:06:33 -0700 Committed r269201: <https://trac.webkit.org/changeset/269201> All reviewed patches have been landed. Closing bug and clearing flags on attachment 412694. 412596 2020-10-28 17:58:52 -0700 2020-10-29 15:38:53 -0700 WIP patch.txt text/plain 2824 nvasilyev ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJJbnNwZWN0b3JVSS9DaGFuZ2VMb2cgYi9Tb3VyY2UvV2Vi SW5zcGVjdG9yVUkvQ2hhbmdlTG9nCmluZGV4IDFlMmZjNDNkNjMzMC4uZGVkMTljZTU0MzU0IDEw MDY0NAotLS0gYS9Tb3VyY2UvV2ViSW5zcGVjdG9yVUkvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9X ZWJJbnNwZWN0b3JVSS9DaGFuZ2VMb2cKQEAgLTEsMyArMSwxNCBAQAorMjAyMC0xMC0yOCAgTmlr aXRhIFZhc2lseWV2ICA8bnZhc2lseWV2QGFwcGxlLmNvbT4KKworICAgICAgICBXZWIgSW5zcGVj dG9yOiBFeHRyYSBjbG9zaW5nIHBhcmVudGhlc2lzIGFkZGVkIGFmdGVyIHZhciBpbiBzdHlsZXMg cGFuZWwKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTIx ODI5NQorICAgICAgICA8cmRhcjovL3Byb2JsZW0vNzA3NzEzMTQ+CisKKyAgICAgICAgUmV2aWV3 ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgKiBVc2VySW50ZXJmYWNlL1ZpZXdzL1Nw cmVhZHNoZWV0U3R5bGVQcm9wZXJ0eS5qczoKKyAgICAgICAgKFdJLlNwcmVhZHNoZWV0U3R5bGVQ cm9wZXJ0eS5wcm90b3R5cGUuX2FkZFZhcmlhYmxlVG9rZW5zKToKKwogMjAyMC0xMC0yMCAgRGV2 aW4gUm91c3NvICA8ZHJvdXNzb0BhcHBsZS5jb20+CiAKICAgICAgICAgV2ViIEluc3BlY3Rvcjog UkVHUkVTU0lPTihyMjY2MDc0KTogVW5jYXVnaHQgRXhjZXB0aW9uOiB1bmRlZmluZWQgaXMgbm90 IGFuIG9iamVjdCAoZXZhbHVhdGluZyAndGhpcy5fYWxsTGlzdGVuZXJzQnJlYWtwb2ludC5kaXNh YmxlZCcpCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViSW5zcGVjdG9yVUkvVXNlckludGVyZmFjZS9W aWV3cy9TcHJlYWRzaGVldFN0eWxlUHJvcGVydHkuanMgYi9Tb3VyY2UvV2ViSW5zcGVjdG9yVUkv VXNlckludGVyZmFjZS9WaWV3cy9TcHJlYWRzaGVldFN0eWxlUHJvcGVydHkuanMKaW5kZXggZTI0 ZTYxYjg3NzUxLi5lNWEwNzMxNTEyMzggMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJJbnNwZWN0b3JV SS9Vc2VySW50ZXJmYWNlL1ZpZXdzL1NwcmVhZHNoZWV0U3R5bGVQcm9wZXJ0eS5qcworKysgYi9T b3VyY2UvV2ViSW5zcGVjdG9yVUkvVXNlckludGVyZmFjZS9WaWV3cy9TcHJlYWRzaGVldFN0eWxl UHJvcGVydHkuanMKQEAgLTc5MCw3ICs3OTAsNyBAQCBXSS5TcHJlYWRzaGVldFN0eWxlUHJvcGVy dHkgPSBjbGFzcyBTcHJlYWRzaGVldFN0eWxlUHJvcGVydHkgZXh0ZW5kcyBXSS5PYmplY3QKICAg ICAgICAgICAgICAgICBpZiAob3BlblBhcmVudGhlc2lzID4gMCkKICAgICAgICAgICAgICAgICAg ICAgY29udGludWU7CiAKLSAgICAgICAgICAgICAgICBsZXQgcmF3VG9rZW5zID0gdG9rZW5zLnNs aWNlKHN0YXJ0SW5kZXgsIGkgKyAxKTsKKyAgICAgICAgICAgICAgICBsZXQgcmF3VG9rZW5zID0g dG9rZW5zLnNsaWNlKHN0YXJ0SW5kZXgsIGkpOyAvLyBEb24ndCBpbmNsdWRlICIpIi4KICAgICAg ICAgICAgICAgICBsZXQgdmFyaWFibGVOYW1lSW5kZXggPSByYXdUb2tlbnMuZmluZEluZGV4KCh0 b2tlbikgPT4gdG9rZW4udmFsdWUuc3RhcnRzV2l0aCgiLS0iKSAmJiAvXGJ2YXJpYWJsZS0yXGIv LnRlc3QodG9rZW4udHlwZSkpOwogICAgICAgICAgICAgICAgIGlmICh2YXJpYWJsZU5hbWVJbmRl eCAhPT0gLTEpIHsKICAgICAgICAgICAgICAgICAgICAgbGV0IGNvbnRlbnRzID0gcmF3VG9rZW5z LnNsaWNlKDAsIHZhcmlhYmxlTmFtZUluZGV4ICsgMSk7CkBAIC04MjMsOCArODIzLDcgQEAgV0ku U3ByZWFkc2hlZXRTdHlsZVByb3BlcnR5ID0gY2xhc3MgU3ByZWFkc2hlZXRTdHlsZVByb3BlcnR5 IGV4dGVuZHMgV0kuT2JqZWN0CiAgICAgICAgICAgICAgICAgICAgICAgICBmYWxsYmFja1Rva2Vu cyA9IHRoaXMuX2FkZFZhcmlhYmxlVG9rZW5zKGZhbGxiYWNrVG9rZW5zKTsKICAgICAgICAgICAg ICAgICAgICAgICAgIGNvbnRlbnRzLnB1c2hBbGwoZmFsbGJhY2tUb2tlbnMpOwogICAgICAgICAg ICAgICAgICAgICB9IGVsc2UKLSAgICAgICAgICAgICAgICAgICAgICAgIGNvbnRlbnRzLnB1c2hB bGwocmF3VG9rZW5zLnNsaWNlKHZhcmlhYmxlTmFtZUluZGV4ICsgMSwgaSkpOwotICAgICAgICAg ICAgICAgICAgICBjb250ZW50cy5wdXNoKHRva2VuKTsKKyAgICAgICAgICAgICAgICAgICAgICAg IGNvbnRlbnRzLnB1c2hBbGwocmF3VG9rZW5zLnNsaWNlKHZhcmlhYmxlTmFtZUluZGV4ICsgMSkp OwogCiAgICAgICAgICAgICAgICAgICAgIGxldCB0ZXh0ID0gcmF3VG9rZW5zLnJlZHVjZSgoYWNj dW11bGF0b3IsIHRva2VuKSA9PiBhY2N1bXVsYXRvciArIHRva2VuLnZhbHVlLCAiIik7CiAgICAg ICAgICAgICAgICAgICAgIGlmICh0aGlzLl9wcm9wZXJ0eS5vd25lclN0eWxlLm5vZGVTdHlsZXMu Y29tcHV0ZWRTdHlsZS5yZXNvbHZlVmFyaWFibGVWYWx1ZSh0ZXh0KSkKQEAgLTgzNSw2ICs4MzQs OCBAQCBXSS5TcHJlYWRzaGVldFN0eWxlUHJvcGVydHkgPSBjbGFzcyBTcHJlYWRzaGVldFN0eWxl UHJvcGVydHkgZXh0ZW5kcyBXSS5PYmplY3QKICAgICAgICAgICAgICAgICAgICAgdGhpcy5faGFz SW52YWxpZFZhcmlhYmxlVmFsdWUgPSB0cnVlOwogICAgICAgICAgICAgICAgICAgICBuZXdUb2tl bnMucHVzaEFsbChyYXdUb2tlbnMpOwogICAgICAgICAgICAgICAgIH0KKyAgICAgICAgICAgICAg ICAvLyBBbHdheXMgaW5jbHVkZSAiKSIuCisgICAgICAgICAgICAgICAgbmV3VG9rZW5zLnB1c2go dG9rZW4pOwogCiAgICAgICAgICAgICAgICAgc3RhcnRJbmRleCA9IE5hTjsKICAgICAgICAgICAg IH0gZWxzZSBpZiAoaXNOYU4oc3RhcnRJbmRleCkpCg== 412694 2020-10-29 15:38:53 -0700 2020-10-30 11:06:34 -0700 Patch patch.txt text/plain 2814 nvasilyev ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJJbnNwZWN0b3JVSS9DaGFuZ2VMb2cgYi9Tb3VyY2UvV2Vi SW5zcGVjdG9yVUkvQ2hhbmdlTG9nCmluZGV4IDFlMmZjNDNkNjMzMC4uMjBjNmU0MWFiZTU1IDEw MDY0NAotLS0gYS9Tb3VyY2UvV2ViSW5zcGVjdG9yVUkvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9X ZWJJbnNwZWN0b3JVSS9DaGFuZ2VMb2cKQEAgLTEsMyArMSwxOCBAQAorMjAyMC0xMC0yOCAgTmlr aXRhIFZhc2lseWV2ICA8bnZhc2lseWV2QGFwcGxlLmNvbT4KKworICAgICAgICBXZWIgSW5zcGVj dG9yOiBFeHRyYSBjbG9zaW5nIHBhcmVudGhlc2lzIGFkZGVkIGFmdGVyIHZhciBpbiBzdHlsZXMg cGFuZWwKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTIx ODI5NQorICAgICAgICA8cmRhcjovL3Byb2JsZW0vNzA3NzEzMTQ+CisKKyAgICAgICAgUmV2aWV3 ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgKiBVc2VySW50ZXJmYWNlL1ZpZXdzL1Nw cmVhZHNoZWV0U3R5bGVQcm9wZXJ0eS5qczoKKyAgICAgICAgKFdJLlNwcmVhZHNoZWV0U3R5bGVQ cm9wZXJ0eS5wcm90b3R5cGUuX2FkZFZhcmlhYmxlVG9rZW5zKToKKyAgICAgICAgUmVtb3ZlIGBj b250ZW50cy5wdXNoKHRva2VuKWAgd2hpY2ggd2FzIGFkZGluZyBgKWAuIFRoZSBjbG9zaW5nIHBh cmVudGhlc2lzIHRoYXQgd2FzIGFscmVhZHkKKyAgICAgICAgaW5jbHVkZWQgaW4gcmF3VG9rZW5z IGFib3ZlLgorICAgICAgICBEcml2ZS1ieTogcmF3VG9rZW5zLnNsaWNlKCkgc2hvdWxkIG5ldmVy IGluY2x1ZGUgYGlgIGFzIHRoZSAybmQgYXJndW1lbnQuIGBpYCBpcyBhbiBpbmRleCBvZiBhbiBp dGVtCisgICAgICAgIGluIHRoZSBgdG9rZW5zYCAtIGEgZGlmZmVyZW50IGFycmF5LgorCiAyMDIw LTEwLTIwICBEZXZpbiBSb3Vzc28gIDxkcm91c3NvQGFwcGxlLmNvbT4KIAogICAgICAgICBXZWIg SW5zcGVjdG9yOiBSRUdSRVNTSU9OKHIyNjYwNzQpOiBVbmNhdWdodCBFeGNlcHRpb246IHVuZGVm aW5lZCBpcyBub3QgYW4gb2JqZWN0IChldmFsdWF0aW5nICd0aGlzLl9hbGxMaXN0ZW5lcnNCcmVh a3BvaW50LmRpc2FibGVkJykKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJJbnNwZWN0b3JVSS9Vc2Vy SW50ZXJmYWNlL1ZpZXdzL1NwcmVhZHNoZWV0U3R5bGVQcm9wZXJ0eS5qcyBiL1NvdXJjZS9XZWJJ bnNwZWN0b3JVSS9Vc2VySW50ZXJmYWNlL1ZpZXdzL1NwcmVhZHNoZWV0U3R5bGVQcm9wZXJ0eS5q cwppbmRleCBlMjRlNjFiODc3NTEuLmY5ZDFkYzQ2MjNkOSAxMDA2NDQKLS0tIGEvU291cmNlL1dl Ykluc3BlY3RvclVJL1VzZXJJbnRlcmZhY2UvVmlld3MvU3ByZWFkc2hlZXRTdHlsZVByb3BlcnR5 LmpzCisrKyBiL1NvdXJjZS9XZWJJbnNwZWN0b3JVSS9Vc2VySW50ZXJmYWNlL1ZpZXdzL1NwcmVh ZHNoZWV0U3R5bGVQcm9wZXJ0eS5qcwpAQCAtODE0LDcgKzgxNCw3IEBAIFdJLlNwcmVhZHNoZWV0 U3R5bGVQcm9wZXJ0eSA9IGNsYXNzIFNwcmVhZHNoZWV0U3R5bGVQcm9wZXJ0eSBleHRlbmRzIFdJ Lk9iamVjdAogICAgICAgICAgICAgICAgICAgICBpZiAoZmFsbGJhY2tTdGFydEluZGV4ICE9PSAt MSkgewogICAgICAgICAgICAgICAgICAgICAgICAgY29udGVudHMucHVzaEFsbChyYXdUb2tlbnMu c2xpY2UodmFyaWFibGVOYW1lSW5kZXggKyAxLCBmYWxsYmFja1N0YXJ0SW5kZXgpKTsKIAotICAg ICAgICAgICAgICAgICAgICAgICAgbGV0IGZhbGxiYWNrVG9rZW5zID0gcmF3VG9rZW5zLnNsaWNl KGZhbGxiYWNrU3RhcnRJbmRleCwgaSk7CisgICAgICAgICAgICAgICAgICAgICAgICBsZXQgZmFs bGJhY2tUb2tlbnMgPSByYXdUb2tlbnMuc2xpY2UoZmFsbGJhY2tTdGFydEluZGV4KTsKICAgICAg ICAgICAgICAgICAgICAgICAgIGZhbGxiYWNrVG9rZW5zID0gdGhpcy5fYWRkQm94U2hhZG93VG9r ZW5zKGZhbGxiYWNrVG9rZW5zKTsKICAgICAgICAgICAgICAgICAgICAgICAgIGZhbGxiYWNrVG9r ZW5zID0gdGhpcy5fYWRkR3JhZGllbnRUb2tlbnMoZmFsbGJhY2tUb2tlbnMpOwogICAgICAgICAg ICAgICAgICAgICAgICAgZmFsbGJhY2tUb2tlbnMgPSB0aGlzLl9hZGRDb2xvclRva2VucyhmYWxs YmFja1Rva2Vucyk7CkBAIC04MjMsOCArODIzLDcgQEAgV0kuU3ByZWFkc2hlZXRTdHlsZVByb3Bl cnR5ID0gY2xhc3MgU3ByZWFkc2hlZXRTdHlsZVByb3BlcnR5IGV4dGVuZHMgV0kuT2JqZWN0CiAg ICAgICAgICAgICAgICAgICAgICAgICBmYWxsYmFja1Rva2VucyA9IHRoaXMuX2FkZFZhcmlhYmxl VG9rZW5zKGZhbGxiYWNrVG9rZW5zKTsKICAgICAgICAgICAgICAgICAgICAgICAgIGNvbnRlbnRz LnB1c2hBbGwoZmFsbGJhY2tUb2tlbnMpOwogICAgICAgICAgICAgICAgICAgICB9IGVsc2UKLSAg ICAgICAgICAgICAgICAgICAgICAgIGNvbnRlbnRzLnB1c2hBbGwocmF3VG9rZW5zLnNsaWNlKHZh cmlhYmxlTmFtZUluZGV4ICsgMSwgaSkpOwotICAgICAgICAgICAgICAgICAgICBjb250ZW50cy5w dXNoKHRva2VuKTsKKyAgICAgICAgICAgICAgICAgICAgICAgIGNvbnRlbnRzLnB1c2hBbGwocmF3 VG9rZW5zLnNsaWNlKHZhcmlhYmxlTmFtZUluZGV4ICsgMSkpOwogCiAgICAgICAgICAgICAgICAg ICAgIGxldCB0ZXh0ID0gcmF3VG9rZW5zLnJlZHVjZSgoYWNjdW11bGF0b3IsIHRva2VuKSA9PiBh Y2N1bXVsYXRvciArIHRva2VuLnZhbHVlLCAiIik7CiAgICAgICAgICAgICAgICAgICAgIGlmICh0 aGlzLl9wcm9wZXJ0eS5vd25lclN0eWxlLm5vZGVTdHlsZXMuY29tcHV0ZWRTdHlsZS5yZXNvbHZl VmFyaWFibGVWYWx1ZSh0ZXh0KSkK