Bug 154082 – Web Inspector: In the styles sidebar, Option-clicking on --css-variable should jump to its definition

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

[Nikita Vasilyev]

In "Styles — Rules", Option-clicking on a property name/value isn't
a very common action. CSS rule's source links (on the right
to CSS selectors) allow navigating to the corresponding line
in CSS resource already.

With CSS variables gaining popularity, I suggest repurposing Option-Click.
In the following example,

    .title {
      color: var(--theme-text-color)
    }

Option-clicking on "--theme-text-color" would jump to the variable's
definition, e.g.:

    body {
      --theme-text-color: black
    }

(via Bug 149521)

[Radar WebKit Bug Importer]

<rdar://problem/24593361>

[Devin Rousso]

Created attachment 271274 [details]
[WIP] Patch

So I was playing around with this for a bit, and I ran into something of a roadblock.  It looks like CSSStyleDeclarations are only created for each instance of DOMNodeStyles, which means that CSS variables declared on a different tag won't have a style associated with them until the node is selected on which they are applied (look at UserInterface/Views/Variables.css for an example).  I only spent a little time on this, so I may be missing an extremely obvious case for how to work around this, but what I have written so far works if the node which matches the rule containing the CSS variable is selected (thus creating the DOMNodeStyles) before trying to Option-click a CSS variable.  If anyone else wants to take over, feel free as I am not sure how much time I have in the next few weeks (horray midterms >.> )

[Joseph Pecoraro]

Comment on attachment 271274 [details]
[WIP] Patch

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

> Source/WebInspectorUI/ChangeLog:3
> +        Web Inspector: In the styles sidebar, Option-clicking on --css-variable should jump to its definition

This will require some thought.

Technically, CSS variables, being properties, have an overriding cascade. For example:

    <style>
    :root { --my-color: blue; }
    div { --my-color: red; }
    .foo { color: var(--my-color); }
    </style>
    <div class="foo">Red</div>
    <p class="foo">Blue</p>

However, I think in practice most variables will be global.

This patch doesn't handle re-definitions properly. Perhaps that isn't required for a first take on the feature though. But ideally we would know and jump to precisely the appropriate --var declaration that applies.

[Timothy Hatcher]

I think we need a backend change to make this easy and correct.

CSS variables should show up as a matched rule in the sidebar (at least inherited rules), but they don't. They do show up in the computed styles list, if you "show all". If they where in the matched rules cascade, this problem would be as simple as jump to property was for the computed styles goto arrow.

[Timothy Hatcher]

FWIW, Chrome just made them show up in the rules sidebar. https://twitter.com/ChromeDevTools/status/696742554402852865

[Timothy Hatcher]

That actually does not require a backend change. We control what counts as inherited on the front-end! I filed bug 154215.

[Timothy Hatcher]

With bug 154215 you should be able to jump to the right variable, just like computed style jumps.

[Devin Rousso]

(In reply to comment #7)
> With bug 154215 you should be able to jump to the right variable, just like
> computed style jumps.

This was a very smart way of tackling this issue.  Awesome catch! XD

[Devin Rousso]

Created attachment 271389 [details]
Patch

[Timothy Hatcher]

Comment on attachment 271389 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:1670
> +        // Special case option clicking CSS variables.
> +        if (token && /\bvariable-2\b/.test(token.type)) {
> +            for (let rule of this._style.nodeStyles.matchedRules) {
> +                for (let property of rule.style.properties) {
> +                    if (property.name === token.string && showSourceCode(rule.sourceCodeLocation, property.styleSheetTextRange))
> +                        return;
> +                }
> +            }
> +            for (let inherited of this._style.nodeStyles.inheritedRules) {
> +                for (let rule of inherited.matchedRules) {
> +                    for (let property of rule.style.properties) {
> +                        if (property.name === token.string && showSourceCode(rule.sourceCodeLocation, property.styleSheetTextRange))
> +                            return;
> +                    }
> +                }
>              }
>          }

You should not need to do this iteration. You should be able to use DOMNodeStyle's effectivePropertyForName. That will give you the property that is active.

[Devin Rousso]

Created attachment 271440 [details]
Patch

(In reply to comment #10)
> Comment on attachment 271389 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=271389&action=review
> 
> > Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:1670
> > +        // Special case option clicking CSS variables.
> > +        if (token && /\bvariable-2\b/.test(token.type)) {
> > +            for (let rule of this._style.nodeStyles.matchedRules) {
> > +                for (let property of rule.style.properties) {
> > +                    if (property.name === token.string && showSourceCode(rule.sourceCodeLocation, property.styleSheetTextRange))
> > +                        return;
> > +                }
> > +            }
> > +            for (let inherited of this._style.nodeStyles.inheritedRules) {
> > +                for (let rule of inherited.matchedRules) {
> > +                    for (let property of rule.style.properties) {
> > +                        if (property.name === token.string && showSourceCode(rule.sourceCodeLocation, property.styleSheetTextRange))
> > +                            return;
> > +                    }
> > +                }
> >              }
> >          }
> 
> You should not need to do this iteration. You should be able to use
> DOMNodeStyle's effectivePropertyForName. That will give you the property
> that is active.

Totally forgot that that function existed.  Thanks for the heads up!

[Timothy Hatcher]

Comment on attachment 271440 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:1645
> +        function showSourceCode(location, range)

It is weird that you take a SourceCodeLocation but only use it for the sourceCode. You should change location to sourceCode.

[Devin Rousso]

Created attachment 271631 [details]
Patch

(In reply to comment #12)
> Comment on attachment 271440 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=271440&action=review
> 
> > Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:1645
> > +        function showSourceCode(location, range)
> 
> It is weird that you take a SourceCodeLocation but only use it for the
> sourceCode. You should change location to sourceCode.

I had only done that because I knew that both of the calls of showSourceCode (which I renamed to showRangeInSourceCode) had a ".sourceCode" member.  This is also somewhat of a mixing of styleguides with one of my current classes, so it's my bad.  Good catch though!

[WebKit Commit Bot]

Comment on attachment 271631 [details]
Patch

Clearing flags on attachment: 271631

Committed r196746: <http://trac.webkit.org/changeset/196746>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.