Bug 44620 – Web Inspector: reflect changes to styles when they happen outside inspector.

• Component: Web Inspector (Deprecated)
• Status: RESOLVED FIXED

[Aaron Boodman]

When you have a node selected in the inspector, the computed style should change as the node changes. At worst, clicking again on the node should update the values. Currently, you have to select away from the node, then reselect it to get updated values.

This makes it hard to debug whether something is changing.

[Ojan Vafai]

Saw this in Chrome trunk. Not sure if it happens in Safari.

[Pavel Feldman]

It tries to update, but dies trying. Could you please share the url you are seeing it on? Or open inspector on inspector and dump console errors here please!

[Aaron Boodman]

I saw it on the chrome NTP.

[Ojan Vafai]

Created attachment 65443 [details]
before

Before and after shots of updating the style at http://plexode.com/eval3/#ht=%3Cdiv%20id%3Dfoo%3Efoo%3C%2Fdiv%3E&ohh=1&ohj=1&jt=foo.style.border%20%3D%20%221px%20solid%22&ojh=0&ojj=1&ms=100&oth=0&otj=0&cex=1

[Ojan Vafai]

Created attachment 65444 [details]
after

[Pavel Feldman]

Investigation note: inspector does not detect changes to the style of the currently selected node when they are done programmatically by someone other than inspector. We should detect style recalculation on the selected node and update inspector.

[Alexander Pavlov (apavlov)]

Created attachment 80194 [details]
[PATCH] Suggested solution

[Pavel Feldman]

Comment on attachment 80194 [details]
[PATCH] Suggested solution

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

> Source/WebCore/css/CSSMutableStyleDeclaration.cpp:590
> +    setNeedsStyleRecalc(true);

I'd add #if ENABLED(INSPECTOR) here and do all the work here instead of changing the method signature.

> Source/WebCore/inspector/InspectorStyleSheet.cpp:1197
> +inline CSSStyleDeclaration* InspectorStyleSheetForInlineStyle::inlineStyle() const

Did this help? Any stats?

> Source/WebCore/inspector/front-end/ElementsPanel.js:465
> +        if (WebInspector.isEditingAnyField())

Is this synchronous?

> Source/WebCore/inspector/front-end/StylesSidebarPane.js:1772
> +            WebInspector.panels.elements.modifyingStyle = true;

You should define this on sidebar.

[Dave Hyatt]

Comment on attachment 80194 [details]
[PATCH] Suggested solution

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

> Source/WebCore/css/CSSMutableStyleDeclaration.cpp:464
> +void CSSMutableStyleDeclaration::setNeedsStyleRecalc(bool viaStyleAttribute)

Might be cleaner to reuse the enum for the node setNeedsStyleRecalc.  Then you could use InlineStyleChange etc.

[Alexander Pavlov (apavlov)]

(In reply to comment #8)
> (From update of attachment 80194 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=80194&action=review
> 
> > Source/WebCore/css/CSSMutableStyleDeclaration.cpp:590
> > +    setNeedsStyleRecalc(true);
> 
> I'd add #if ENABLED(INSPECTOR) here and do all the work here instead of changing the method signature.

The point is that the notification should occur when the value is _false_, i.e. in the remaining 7 cases. I will follow up with dhyatt's suggestion.

> > Source/WebCore/inspector/InspectorStyleSheet.cpp:1197
> > +inline CSSStyleDeclaration* InspectorStyleSheetForInlineStyle::inlineStyle() const
> 
> Did this help? Any stats?

Around 0.8-1.0% - perhaps is not worth it.

> > Source/WebCore/inspector/front-end/ElementsPanel.js:465
> > +        if (WebInspector.isEditingAnyField())
> 
> Is this synchronous?

Yes, this is synchronous. It just checks a flag set by WebInspector.startEditing(). But for this, every time we increment/decrement a numeric property value in the Styles pane, the style change would eventually reach this method and update the Styles pane itself, thereby killing the active editor. This check only forbids "killing" the active property editor, no other editing use cases are affected, in the frontend or backend.

> > Source/WebCore/inspector/front-end/StylesSidebarPane.js:1772
> > +            WebInspector.panels.elements.modifyingStyle = true;
> 
> You should define this on sidebar.

Done.

[Alexander Pavlov (apavlov)]

Created attachment 81199 [details]
[PATCH] Comments addressed

[Pavel Feldman]

Comment on attachment 81199 [details]
[PATCH] Comments addressed

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

> Source/WebCore/css/CSSMutableStyleDeclaration.cpp:474
> +            if (styleAttributeChange == FullStyleChange && m_node->document())

I think there has been misunderstanding: your boolean values had different meaning than the ones in StyleChangeType. What you are fighting was dupe in the didModifyAttr notification. You should either find out what negative implications this dupe has or come up with a separate  InspectorInstrumentation::inlineStyleNeedsRecalc(m_node) signal that you would fire here unconditionally.

> Source/WebCore/inspector/front-end/StylesSidebarPane.js:1770
> +            WebInspector.panels.elements.sidebarPanes.styles.isModifyingStyle = true;

Please pass styles pane into section instead.

[Pavel Feldman]

*** Bug 51118 has been marked as a duplicate of this bug. ***

[Alexander Pavlov (apavlov)]

@pfeldman: as a lot of complications have risen from the delayed notification approach, I suggest landing the current solution as-is (with proper fixes applied) and think about something different afterwards, as users are asking for this issue to be fixed.

[Alexander Pavlov (apavlov)]

Created attachment 82797 [details]
[PATCH] The timer-based attribute revalidation approach

[Pavel Feldman]

Comment on attachment 82797 [details]
[PATCH] The timer-based attribute revalidation approach

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

> LayoutTests/inspector/styles/styles-update-from-js.html:21
> +        InspectorTest.evaluateInPage("modifyStyleAttribute()", f);

You should not rely upon pending dispatches - use sniffers instead.

> LayoutTests/inspector/styles/styles-update-from-js.html:28
> +            InspectorTest.runAfterPendingDispatches(setParsedAttribute);

ditto

> LayoutTests/inspector/styles/styles-update-from-js.html:41
> +            InspectorTest.runAfterPendingDispatches(done);

ditto

> LayoutTests/inspector/styles/styles-update-from-js.html:70
> +        var innerMapping = WebInspector.domAgent._idToDOMNode;

treeOutline.findTreeElement(InspectorTest.expandedNodeWithId("container"))

> LayoutTests/inspector/styles/styles-update-from-js.html:80
> +    InspectorTest.expandElementsTree(selectContainerElementContinuation);

1) Tests now flow top->bottom.
2) No need to expand tree, start with selectNode

> LayoutTests/inspector/styles/styles-update-from-js.html:83
> +function modifyStyleAttribute()

test functions go before test()

> LayoutTests/inspector/styles/styles-update-from-js.html:110
> +<div id="other">

Remove this div.

> Source/WebCore/css/CSSMutableStyleDeclaration.cpp:475
> +                InspectorInstrumentation::didInvalidateStyleAttr(m_node->document(), static_cast<StyledElement*>(m_node));

Simply pass node here, do all the casts and gets in the instrumentation code.

> Source/WebCore/dom/StyledElement.cpp:255
> +        setNeedsStyleRecalc(InlineStyleChange);

Please revert or ask for Dave's blessing.

> Source/WebCore/inspector/InspectorDOMAgent.cpp:238
> +    , m_revalidateStyleAttrTask(this)

You should only create this timer when needed.

> Source/WebCore/inspector/InspectorDOMAgent.h:148
> +    class RevalidateStyleAttrTask {

No abbreviations in WebCore. You should also do a forward declaration here with private implementation in cpp.

> Source/WebCore/inspector/InspectorStyleSheet.cpp:1202
> +const String& InspectorStyleSheetForInlineStyle::styleTextFromElement() const

elementStyleText ?

[Alexander Pavlov (apavlov)]

Created attachment 82827 [details]
[PATCH] Comments addressed

All comments have been addressed in this patch

[Alexander Pavlov (apavlov)]

Committing to http://svn.webkit.org/repository/webkit/trunk ...
        M       LayoutTests/ChangeLog
        A       LayoutTests/inspector/styles/styles-update-from-js-expected.txt
        A       LayoutTests/inspector/styles/styles-update-from-js.html
        M       Source/WebCore/ChangeLog
        M       Source/WebCore/css/CSSMutableStyleDeclaration.cpp
        M       Source/WebCore/inspector/InspectorDOMAgent.cpp
        M       Source/WebCore/inspector/InspectorDOMAgent.h
        M       Source/WebCore/inspector/InspectorInstrumentation.cpp
        M       Source/WebCore/inspector/InspectorInstrumentation.h
        M       Source/WebCore/inspector/InspectorStyleSheet.cpp
        M       Source/WebCore/inspector/InspectorStyleSheet.h
        M       Source/WebCore/inspector/front-end/ElementsPanel.js
        M       Source/WebCore/inspector/front-end/StylesSidebarPane.js
Committed r79009