Bug 64283 – Web Inspector: Focusing on another node while editing CSS property does not update styles

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

[Alexander Pavlov (apavlov)]

1. Select an element in the Elements panel, start editing one of its applicable styles' CSS properties
2. Commit the edit by clicking on another element in the DOM tree
3. Observe that the Styles pane does not update to reflect the new node styles

[Alexander Pavlov (apavlov)]

Created attachment 100309 [details]
[PATCH] Suggested fix

[Pavel Feldman]

Comment on attachment 100309 [details]
[PATCH] Suggested fix

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

> Source/WebCore/inspector/front-end/StylesSidebarPane.js:1969
> +                WebInspector.panels.elements.startEditingStyle();

There is no guarantee that endEditingStyle is going to be called. Can we add an "editing" state that we enter once upon start editing and exit upon canceling / navigating out of the rule?

[Alexander Pavlov (apavlov)]

Created attachment 100795 [details]
[PATCH] Introduced a kind of an "editing session" for a CSS style

[Alexander Pavlov (apavlov)]

Created attachment 100796 [details]
[PATCH] Removed unused method

[Pavel Feldman]

Comment on attachment 100796 [details]
[PATCH] Removed unused method

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

Lets see how stylesheet update affects standard update routines.

> LayoutTests/inspector/styles/commit-with-selection-change.html:9
> +    WebInspector.showPanel("elements");

selectNodeAndWaitForStyles does select elements panel.

> Source/WebCore/inspector/front-end/StylesSidebarPane.js:1935
> +    //   "selector" - edit selector (|target|: section whose selector to edit next).

We need constants for these.

> Source/WebCore/inspector/front-end/StylesSidebarPane.js:1999
> +        var newTarget = this._newEditorTarget(userInput, previousContent, context, moveDirection);

var nextTarget = this._nextEditorTarget...

> Source/WebCore/inspector/front-end/StylesSidebarPane.js:2000
> +        this.editingEnded(context, !!newTarget);

this.editingEnded(context, newTarget) ?

> Source/WebCore/inspector/front-end/StylesSidebarPane.js:2027
> +        function editTargetCallback(newTarget)

Make this a top level function _startEditingTarget() ?

> Source/WebCore/inspector/front-end/StylesSidebarPane.js:2038
> +                var subtarget = newTarget.subtarget;

name, value?

[Pavel Feldman]

Created attachment 100806 [details]
Patch

[Alexander Pavlov (apavlov)]

Comment on attachment 100806 [details]
Patch

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

> Source/WebCore/inspector/front-end/MetricsSidebarPane.js:42
> +        this._innerUpdate();

Please add a FIXME somewhere around here to avoid updates of a collapsed pane.

> Source/WebCore/inspector/front-end/MetricsSidebarPane.js:81
> +        // "style" attribute might have changed. update styles unless they are being edited.

"update" should probably be capitalized to stick to the WebKit comment style.
"styles" -> "metrics"

> Source/WebCore/inspector/front-end/StylesSidebarPane.js:216
> +    update: function(node, forceUpdate, callback)

Please add a FIXME somewhere around here to avoid updates of a collapsed pane.

> Source/WebCore/inspector/front-end/StylesSidebarPane.js:287
> +        // "style" attribute might have changed. update styles unless they are being edited.

"update" should probably be capitalized.

> Source/WebCore/inspector/front-end/StylesSidebarPane.js:-1909
> -

Should we keep this blank line?

> Source/WebCore/inspector/front-end/StylesSidebarPane.js:2098
> +        section.pane._userOperation = true;

I'm a bit wary of this hacky access to private fields of other objects. Or do we allow this in the scope of a single file?

> Source/WebCore/inspector/front-end/StylesSidebarPane.js:2125
> +

odd blank line

[Pavel Feldman]

Created attachment 100814 [details]
Patch

[Yury Semikhatsky]

Comment on attachment 100814 [details]
Patch

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

Is there a test that covers this change?

> LayoutTests/http/tests/inspector/elements-test.js:87
> +        InspectorTest.addSniffer(WebInspector.StylesSidebarPane.prototype, "_nodeStylesUpdatedForTest", sniff);

Since the method is for tests only it makes sense to convert it into a property and don't use addSniffer.

> Source/WebCore/inspector/front-end/CSSStyleModel.js:516
> +                WebInspector.cssModel._fireStyleSheetChanged(style.id.styleSheetId, majorChange, userCallback ? userCallback.bind(this, style) : null);

It is not clear which object is going to be 'this' at the moment of binding.

> Source/WebCore/inspector/front-end/DOMAgent.js:432
> +    _inlineStyleInvalidated: function(nodeIds)

Inline this?

> Source/WebCore/inspector/front-end/StylesSidebarPane.js:2106
> +            this._parentPane._userOperation = true;

Is intentional that _userOperation is never reset to false?

[Pavel Feldman]

Comment on attachment 100814 [details]
Patch

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

>> LayoutTests/http/tests/inspector/elements-test.js:87
>> +        InspectorTest.addSniffer(WebInspector.StylesSidebarPane.prototype, "_nodeStylesUpdatedForTest", sniff);
> 
> Since the method is for tests only it makes sense to convert it into a property and don't use addSniffer.

I only want to react on it once, sniffer is ideal for that.

>> Source/WebCore/inspector/front-end/CSSStyleModel.js:516

> 
> It is not clear which object is going to be 'this' at the moment of binding.

Done.

>> Source/WebCore/inspector/front-end/DOMAgent.js:432
>> +    _inlineStyleInvalidated: function(nodeIds)
> 
> Inline this?

I'll add fixme.

>> Source/WebCore/inspector/front-end/StylesSidebarPane.js:2106
>> +            this._parentPane._userOperation = true;
> 
> Is intentional that _userOperation is never reset to false?

Good catch, thanks.

[Pavel Feldman]

Created attachment 100981 [details]
Patch

[Pavel Feldman]

Created attachment 100985 [details]
Patch

[Pavel Feldman]

Created attachment 100986 [details]
Patch

[Pavel Feldman]

Committed r91070: <http://trac.webkit.org/changeset/91070>