Bug 120640 – Web Inspector: Jump from a computed style to the rule it came from

• Component: Web Inspector
• Status: RESOLVED FIXED

[Dan Wood]

I've frequently found myself in a situation where I'm looking at a computed property, wondering how that property got that value.  Was it a specific rule?  If so, which one?  Or is it inherited from some containing element?  If so, which, and what rule?

It would be very useful if one could, say, double-click on any property/value line in the properties list, and have the inspector jump over to the Rules tab, highlighting the specific rule that gives it that value.  Perhaps even changing the selected element so that the rule is properly highlighted for the relevant element.

Perhaps even some sort of popover display to help explain even more.  For instance, if a computed value in in pixels and the rule declares a value in em, it could explain how it came to that calculation.

Anything that might help track down *why* a property has a particular value would be extremely helpful.  Easy for WebKit to know, and hard for a human to try to figure out.

[Radar WebKit Bug Importer]

<rdar://problem/14898027>

[Dan Wood]

Additionally, it might be useful to be able to get to an explanation from the Metrics tab; e.g. option-clicking on a particular margin, border, padding, or dimension and go from there to the rule that generated that value.

[Joseph Pecoraro]

Yes, it would be nice to have some way to filter / highlight a property name in the Styles sidebar Rules tab to see just how a single property cascades. All the data is already there right now.

[Devin Rousso]

Created attachment 253717 [details]
Patch

[Devin Rousso]

Created attachment 253718 [details]
Patch

[Timothy Hatcher]

Comment on attachment 253718 [details]
Patch

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

Very good first patch! I commented most about style nits. But there is some changes you can make to make this more integrated and not have as many changes.

> Source/WebInspectorUI/ChangeLog:9
> +        * UserInterface/Views/CSSStyleDeclarationSection.js:
> +        (WebInspector.CSSStyleDeclarationSection.prototype.refreshPropertiesTextEditor):

You should add comments under each function or file group about what you did and why.

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationSection.js:263
> +    refreshPropertiesTextEditor: function() {

{ on newline.

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationSection.js:264
> +        this._propertiesTextEditor.refresh();

Might not need this in the end, see below.

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:440
> +        } else if(this._delegate instanceof WebInspector.ComputedStyleDetailsPanel) {

Add a space after if.

This check, while correct, is a layering violation. This code should not know about ComputedStyleDetailsPanel.

What this should do is check for a function on the delegate (like "this._delegate.cssStyleDeclarationTextEditorShouldAddPropertyGoToArrows") then call that function to see if it returns true. if it returns true, then you can do the code like you have it. And you should change WebInspector.cssStyleDetailsSidebarPanel.switchViewToStyleInRulesStyleDetailsPanel to another delegate call.

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:470
> +        if(property.highlighted) {

Space after if.

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDetailsSidebarPanel.js:159
> +        var rsdp = this._rulesStyleDetailsPanel;

We don't abbreviate things like this. Just use "rulesStyleDetailsPanel" or "rulesPanel".

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDetailsSidebarPanel.js:160
> +        if(rsdp.sections.length == 0) {

Space after if.

!rsdp.sections.length instead of "== 0" (should have been === too)

Should add a comment why this code is needed. Why the timeout?

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDetailsSidebarPanel.js:161
> +            var interval = setInterval(function() {

No need to save the interval.

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDetailsSidebarPanel.js:162
> +                if(rsdp.sections.length > 0) {

if (rsdp.sections.length)

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDetailsSidebarPanel.js:164
> +                    clearInterval(interval);

No need to clearInterval for a timeout, it is already single fire.

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDetailsSidebarPanel.js:169
> +        } else {
> +            rsdp.scrollToSectionAndHighlightProperty(property);
> +        }

No { } for single line blocks.

> Source/WebInspectorUI/UserInterface/Views/RulesStyleDetailsPanel.js:248
> +        for(var i = 0; i < this._sections.length; ++i) {

Space after for.

> Source/WebInspectorUI/UserInterface/Views/RulesStyleDetailsPanel.js:249
> +            var section = this._sections[i];

Use for (var section of this._sections) and you don't need the i or this line.

> Source/WebInspectorUI/UserInterface/Views/RulesStyleDetailsPanel.js:251
> +            for(var j = 0; j < section.style.properties.length; ++j) {

Use for (...of...)

> Source/WebInspectorUI/UserInterface/Views/RulesStyleDetailsPanel.js:253
> +                    var elements = section.element.querySelectorAll(".properties .css-style-text-editor .CodeMirror-scroll .CodeMirror-code pre");

This is fragile. It could easily break with a CodeMirror change and we wouldn't catch it.

This should be contained in a helper on CSSStyleDeclarationTextEditor, as a revealAndHighlightProperty function. CSSProperty.js has styleDeclarationTextRange and CSSStyleDeclarationTextEditor can use it to scroll to a property.

> Source/WebInspectorUI/UserInterface/Views/RulesStyleDetailsPanel.js:255
> +                    for(var k = 0; k < elements.length; ++k) {

Use for (...of...)

> Source/WebInspectorUI/UserInterface/Views/RulesStyleDetailsPanel.js:260
> +                            section.style.properties[j].highlighted = true;
> +                            section.refreshPropertiesTextEditor();
> +                            section.needsToUpdate = true;

Doing a refresh to get the highlight works, but it is heavy. _updateTextMarkers on CSSStyleDeclarationTextEditor is all you should need to do. That is a private function, so moving this logic to CSSStyleDeclarationTextEditor will make sense.

> Source/WebInspectorUI/UserInterface/Views/RulesStyleDetailsPanel.js:270
> +            if(propertyName) {
> +                break;
> +            }

Space after if. No need for { }

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

Newline before {.

> Source/WebInspectorUI/UserInterface/Views/RulesStyleDetailsPanel.js:274
> +            if(cssProperty.enabled && !cssProperty.overridden) {

Space before if.

> Source/WebInspectorUI/UserInterface/Views/RulesStyleDetailsPanel.js:275
> +                if(cssProperty.canonicalName == property.canonicalName || hasMatchingLonghandProperty(cssProperty)) {

Ditto.

> Source/WebInspectorUI/UserInterface/Views/RulesStyleDetailsPanel.js:280
> +            }
> +            return false;

Newline between.

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

Newline before {.

> Source/WebInspectorUI/UserInterface/Views/RulesStyleDetailsPanel.js:286
> +            if(cssProperties.length === 0)

Space after if. !cssProperties.length

> Source/WebInspectorUI/UserInterface/Views/RulesStyleDetailsPanel.js:289
> +            for(var i = 0; i < cssProperties.length; ++i) {

for (...of...)

> Source/WebInspectorUI/UserInterface/Views/RulesStyleDetailsPanel.js:290
> +                if(propertiesMatch(cssProperties[i])) {

No need for {}.

> Source/WebInspectorUI/UserInterface/Views/RulesStyleDetailsPanel.js:294
> +            }
> +            return false;

Newline between.

> Source/WebInspectorUI/UserInterface/Views/RulesStyleDetailsPanel.js:317
> +            if(section.needsToUpdate) {
> +                section.refreshPropertiesTextEditor();
> +                delete section.needsToUpdate;
> +            }

Might not need this if you move things into CSSStyleDeclarationTextEditor and don't do a full refresh.

[Devin Rousso]

Created attachment 253769 [details]
Patch

[Devin Rousso]

Created attachment 253770 [details]
Patch

[Devin Rousso]

Created attachment 253824 [details]
Patch

[Timothy Hatcher]

Comment on attachment 253824 [details]
Patch

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

Looking a lot better. Some more work and this will be nice and clean.

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:488
> +                delegate.switchToPropertyInRulesPanel(property);

This should be something more generic than mentioning "rules panel". The delegate could be/do anything. The point of a delegate is to be generic and not know about the other parts of the code.

Something like: delegate.cssStyleDeclarationTextEditorShowProperty

You will need change the typeof check above too.

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDetailsSidebarPanel.js:146
> +            this._navigationBar.selectedNavigationItem = this._lastSelectedSectionSetting.value;

I would move this to _switchPanels.

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDetailsSidebarPanel.js:147
> +            this._navigationBar.updateLayout();

Not sure this is needed.

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDetailsSidebarPanel.js:156
> +        if (this._rulesStyleDetailsPanel.contentsHaveChanged(this.domNode)) {
> +            this._rulesStyleDetailsPanel.savedProperty = property;
> +            switchToRulesStyleDetailsPanel.call(this);
> +        } else {
> +            switchToRulesStyleDetailsPanel.call(this);
> +            this._rulesStyleDetailsPanel.scrollToSectionAndHighlightProperty(property);
> +        }

This should just be:

this._switchPanels(this._rulesStyleDetailsPanel);
this._rulesStyleDetailsPanel.scrollToSectionAndHighlightProperty(property);

Then scrollToSectionAndHighlightProperty should have the check for contentsHaveChanged, and internally save the property. The property saving should not be the job of the caller, it should be transparent to the caller. You wouldn't want to have to repeat this code if two or more classes needed to do it.

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDetailsSidebarPanel.js:-204
> -        this._lastSelectedSectionSetting.value = selectedNavigationItem.identifier;

This could stay, then all the callers to _switchPanels won't need to do it.

> Source/WebInspectorUI/UserInterface/Views/ComputedStyleDetailsPanel.js:149
> +        if (typeof this._delegate.switchViewToPropertyInRulesStyleDetailsPanel === "function" )
> +            this._delegate.switchViewToPropertyInRulesStyleDetailsPanel(property);

This should be generic, like "computedStyleDetailsPanelShowProperty".

> Source/WebInspectorUI/UserInterface/Views/RulesStyleDetailsPanel.js:34
> +        this._savedProperty = null;

_propertyToSelectAndHighlight would be a better name.

> Source/WebInspectorUI/UserInterface/Views/RulesStyleDetailsPanel.js:250
> +    scrollToSectionAndHighlightProperty(property)
> +    {
> +        for (var section of this._sections) {

I think this should save to _propertyToSelectAndHighlight and return early if (!this.visible). Then nodeStylesRefreshed will be called when it becomes visible, which will call scrollToSectionAndHighlightProperty again. Should eliminate the need for contentsHaveChanged and _currentDOMNode.

> Source/WebInspectorUI/UserInterface/Views/RulesStyleDetailsPanel.js:266
> +    get sections()
> +    {
> +        return this._sections;
> +    }
> +
> +    set savedProperty(cssProperty)
> +    {
> +        this._savedProperty = cssProperty;
>      }

Shouldn't need these.

> Source/WebInspectorUI/UserInterface/Views/StyleDetailsPanel.js:117
> +    contentsHaveChanged(domNode)
> +    {
> +        if (domNode && this._currentDOMNode && this._currentDOMNode === domNode)
> +            return false;
> +
> +        this._currentDOMNode = domNode;
> +
> +        return true;
> +    }

I don't think you need this. See scrollToSectionAndHighlightProperty.

[Devin Rousso]

Created attachment 253869 [details]
Patch

[Timothy Hatcher]

Comment on attachment 253869 [details]
Patch

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

Almost there!

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:214
> +                this._codeMirror.setSelection(selection.from, selection.to);

You might want to do this which we use in TextEditor to scroll:

    _scrollIntoViewCentered(position)
    {
        var scrollInfo = this._codeMirror.getScrollInfo();
        var lineHeight = Math.ceil(this._codeMirror.defaultTextHeight());
        var margin = Math.floor((scrollInfo.clientHeight - lineHeight) / 2);
        this._codeMirror.scrollIntoView(position, margin);
    }

Might not need to make it as complex, it would be good to share that code. Maybe in CodeMirrorAdditions.js?

You can remove the scrollIntoView from CSSStyleDeclarationSection then.

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:482
> +                && !property.implicit
> +                && typeof this._delegate.cssStyleDeclarationTextEditorShowProperty === "function") {

I'd put these on the same line.

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationTextEditor.js:491
> +            var arrowMarker = this._codeMirror.setUniqueBookmark(to, arrowElement);

You never use arrowMarker, you can drop the var.

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDetailsSidebarPanel.js:146
> +        this._navigationBar.updateLayout();

Why is this needed? It should happen automatically.

> Source/WebInspectorUI/UserInterface/Views/RulesStyleDetailsPanel.js:298
> +            delete this._propertyToSelectAndHighlight;

We are trying to avoid delete for performance reasons. This can just be:

this._propertyToSelectAndHighlight = null;

[Timothy Hatcher]

Comment on attachment 253869 [details]
Patch

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

> Source/WebInspectorUI/UserInterface/Views/CSSStyleDeclarationSection.js:266
> +            this._element.scrollIntoView();

This would be better as: this._element.scrollIntoViewIfNeeded(true)

[Devin Rousso]

Created attachment 253884 [details]
Patch

[Timothy Hatcher]

Comment on attachment 253884 [details]
Patch

Nice job!

[WebKit Commit Bot]

Comment on attachment 253884 [details]
Patch

Clearing flags on attachment: 253884

Committed r184977: <http://trac.webkit.org/changeset/184977>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.