Bug 154082 - Web Inspector: In the styles sidebar, Option-clicking on --css-variable should jump to its definition
Summary: Web Inspector: In the styles sidebar, Option-clicking on --css-variable shoul...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Devin Rousso
URL:
Keywords: InRadar
Depends on: 154215
Blocks:
  Show dependency treegraph
 
Reported: 2016-02-10 12:48 PST by Nikita Vasilyev
Modified: 2016-02-17 22:37 PST (History)
10 users (show)

See Also:


Attachments
[WIP] Patch (5.88 KB, patch)
2016-02-13 00:12 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (5.38 KB, patch)
2016-02-15 16:28 PST, Devin Rousso
timothy: review-
Details | Formatted Diff | Diff
Patch (4.54 KB, patch)
2016-02-16 11:03 PST, Devin Rousso
timothy: review+
Details | Formatted Diff | Diff
Patch (4.64 KB, patch)
2016-02-17 22:12 PST, Devin Rousso
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nikita Vasilyev 2016-02-10 12:48:16 PST
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)
Comment 1 Radar WebKit Bug Importer 2016-02-10 12:48:41 PST
<rdar://problem/24593361>
Comment 2 Devin Rousso 2016-02-13 00:12:05 PST
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 >.> )
Comment 3 Joseph Pecoraro 2016-02-13 01:52:11 PST
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.
Comment 4 Timothy Hatcher 2016-02-13 05:31:45 PST
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.
Comment 5 Timothy Hatcher 2016-02-13 05:33:33 PST
FWIW, Chrome just made them show up in the rules sidebar. https://twitter.com/ChromeDevTools/status/696742554402852865
Comment 6 Timothy Hatcher 2016-02-13 09:55:20 PST
That actually does not require a backend change. We control what counts as inherited on the front-end! I filed bug 154215.
Comment 7 Timothy Hatcher 2016-02-13 10:45:07 PST
With bug 154215 you should be able to jump to the right variable, just like computed style jumps.
Comment 8 Devin Rousso 2016-02-14 03:00:12 PST
(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
Comment 9 Devin Rousso 2016-02-15 16:28:17 PST
Created attachment 271389 [details]
Patch
Comment 10 Timothy Hatcher 2016-02-15 18:28:45 PST
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.
Comment 11 Devin Rousso 2016-02-16 11:03:22 PST
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!
Comment 12 Timothy Hatcher 2016-02-17 19:29:46 PST
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.
Comment 13 Devin Rousso 2016-02-17 22:12:08 PST
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!
Comment 14 WebKit Commit Bot 2016-02-17 22:37:44 PST
Comment on attachment 271631 [details]
Patch

Clearing flags on attachment: 271631

Committed r196746: <http://trac.webkit.org/changeset/196746>
Comment 15 WebKit Commit Bot 2016-02-17 22:37:50 PST
All reviewed patches have been landed.  Closing bug.