Bug 227811 – Web Inspector: Regression (r278607) Jump to CSS variable declaration from Computed panel not working

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

[Razvan Caliman]

Created attachment 433157 [details]
Video of bug (with workaround)

Regressed by bug 225972.

If a CSS variable was not used and is hidden in the Styles panel, clicking on the "jump to" arrow in the Computed panel does not reveal and highlight the CSS variable declaration in the Styles panel.


Steps to reproduce:
-Run this in a new tab: 

data:text/html,<style>html{--unused:red}</style><p>target

- Open Web Inspector, inspect the `<p>`
- In the Computed details sidebar panel, click on the "jump to" arrow next to the `--unused` CSS variable.

Expected:
Reveal and highlight the unused CSS variable in the Styles panel.

Actual:
Nothing happens.

Workaround: 
Click the button to reveal the unused CSS variables in the Styles panel. Afterwards, clicking the "jump to" arrow from the Computed panel highlights the correct item.

[Radar WebKit Bug Importer]

<rdar://problem/80339360>

[Razvan Caliman]

Created attachment 433499 [details]
Patch 1.0

[Razvan Caliman]

Created attachment 433500 [details]
Video with patch applied

Correctly revealing and highlighting a hidden unused CSS variable when clicking the button from the Computed panel to jump to its definition in the Styles panel.

[Patrick Angle]

Comment on attachment 433499 [details]
Patch 1.0

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

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:321
> +        function propertiesMatch(cssProperty)
> +        {

Did you change this from a `let ...` to a `function ...` to enable its use before the declaration here? If so, instead of changing these signature, I'd move the let propertiesMatch to be above its first use on :314.

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:331
> +        function hasMatchingLonghandProperty(cssProperty)
> +        {

This should really go above `propertiesMatch`, since it is used inside that function. Then it wouldn't be necessary to change this to a `function ...` either, unless I'm missing some other difference you are relying on here.

[Razvan Caliman]

Comment on attachment 433499 [details]
Patch 1.0

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

>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:321
>> +        {
> 
> Did you change this from a `let ...` to a `function ...` to enable its use before the declaration here? If so, instead of changing these signature, I'd move the let propertiesMatch to be above its first use on :314.

Yes, I changed it to an explicitly named function declaration so it benefits from hoisting and the code block above can call it without erroring.

Two reasons:
1) seemed like the least invasive change to make and preserve a clean blame history (moving it around muddies that);
2) I don't see the need for these _not_ to be explicitly named function declaration. The indirection is unnecessary in my opinion. 

Open to discussion if there is a reason for the keeping the functions as expression assigned to variables/consts.

>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:331
>> +        {
> 
> This should really go above `propertiesMatch`, since it is used inside that function. Then it wouldn't be necessary to change this to a `function ...` either, unless I'm missing some other difference you are relying on here.

This wasn't necessary for this patch to work. I renamed it just to keep consistency with the above change to a function declaration which was necessary for hoisting.

[Devin Rousso]

Comment on attachment 433499 [details]
Patch 1.0

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

r=me

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:314
> +        if (!property.overridden && this._hiddenUnusedVariables.find(propertiesMatch)) {

Is it actually desirable to have `_hiddenUnusedVariables` be a `Set` anymore?  Is it only for deduplication?

> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:316
>              this.propertyVisibilityMode = WI.SpreadsheetCSSStyleDeclarationEditor.PropertyVisibilityMode.ShowAll;

It's a little unfortunate that we show _all_ variables when trying to find one, but I suppose that's not the end of the world :/

>>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:321
>>> +        {
>> 
>> Did you change this from a `let ...` to a `function ...` to enable its use before the declaration here? If so, instead of changing these signature, I'd move the let propertiesMatch to be above its first use on :314.
> 
> Yes, I changed it to an explicitly named function declaration so it benefits from hoisting and the code block above can call it without erroring.
> 
> Two reasons:
> 1) seemed like the least invasive change to make and preserve a clean blame history (moving it around muddies that);
> 2) I don't see the need for these _not_ to be explicitly named function declaration. The indirection is unnecessary in my opinion. 
> 
> Open to discussion if there is a reason for the keeping the functions as expression assigned to variables/consts.

Our typical style is to only save a function to a `let` if it's using an arrow function, as there's no way of creating a named arrow function.

What you have here is fine, but we do usually prefer to declare things before they're used.  In this case it's also not possible to completely do that as both functions refer to each other, so one of them will have to use and be declared before the other 😅

Style: we put the `{` on the same line for inline functions

[Razvan Caliman]

Created attachment 433582 [details]
Patch 1.1

Carry over R+; Address nits

[Razvan Caliman]

Comment on attachment 433499 [details]
Patch 1.0

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

>> Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:314
>> +        if (!property.overridden && this._hiddenUnusedVariables.find(propertiesMatch)) {
> 
> Is it actually desirable to have `_hiddenUnusedVariables` be a `Set` anymore?  Is it only for deduplication?

Yes, it's a Set just to ensure all items within are unique.

[EWS]

Committed r279946 (239690@main): <https://commits.webkit.org/239690@main>

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