Bug 120640 - Web Inspector: Jump from a computed style to the rule it came from
Summary: Web Inspector: Jump from a computed style to the rule it came from
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Enhancement
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2013-09-03 10:37 PDT by Dan Wood
Modified: 2015-05-28 21:48 PDT (History)
4 users (show)

See Also:


Attachments
Patch (10.64 KB, patch)
2015-05-26 12:49 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (10.64 KB, patch)
2015-05-26 12:49 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (14.78 KB, patch)
2015-05-26 19:05 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (14.78 KB, patch)
2015-05-26 19:06 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (17.32 KB, patch)
2015-05-27 18:55 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (16.34 KB, patch)
2015-05-28 15:18 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (16.22 KB, patch)
2015-05-28 18:11 PDT, 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 Dan Wood 2013-09-03 10:37:24 PDT
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.
Comment 1 Radar WebKit Bug Importer 2013-09-03 10:38:12 PDT
<rdar://problem/14898027>
Comment 2 Dan Wood 2013-09-03 10:39:58 PDT
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.
Comment 3 Joseph Pecoraro 2013-09-03 10:52:25 PDT
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.
Comment 4 Devin Rousso 2015-05-26 12:49:08 PDT
Created attachment 253717 [details]
Patch
Comment 5 Devin Rousso 2015-05-26 12:49:56 PDT
Created attachment 253718 [details]
Patch
Comment 6 Timothy Hatcher 2015-05-26 13:56:06 PDT
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.
Comment 7 Devin Rousso 2015-05-26 19:05:44 PDT
Created attachment 253769 [details]
Patch
Comment 8 Devin Rousso 2015-05-26 19:06:59 PDT
Created attachment 253770 [details]
Patch
Comment 9 Devin Rousso 2015-05-27 18:55:52 PDT
Created attachment 253824 [details]
Patch
Comment 10 Timothy Hatcher 2015-05-28 14:17:41 PDT
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.
Comment 11 Devin Rousso 2015-05-28 15:18:05 PDT
Created attachment 253869 [details]
Patch
Comment 12 Timothy Hatcher 2015-05-28 16:06:49 PDT
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;
Comment 13 Timothy Hatcher 2015-05-28 18:03:51 PDT
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)
Comment 14 Devin Rousso 2015-05-28 18:11:54 PDT
Created attachment 253884 [details]
Patch
Comment 15 Timothy Hatcher 2015-05-28 20:58:23 PDT
Comment on attachment 253884 [details]
Patch

Nice job!
Comment 16 WebKit Commit Bot 2015-05-28 21:47:58 PDT
Comment on attachment 253884 [details]
Patch

Clearing flags on attachment: 253884

Committed r184977: <http://trac.webkit.org/changeset/184977>
Comment 17 WebKit Commit Bot 2015-05-28 21:48:02 PDT
All reviewed patches have been landed.  Closing bug.