Bug 64283 - Web Inspector: Focusing on another node while editing CSS property does not update styles
Summary: Web Inspector: Focusing on another node while editing CSS property does not u...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (Deprecated) (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Alexander Pavlov (apavlov)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-07-11 08:12 PDT by Alexander Pavlov (apavlov)
Modified: 2011-07-15 08:34 PDT (History)
10 users (show)

See Also:


Attachments
[PATCH] Suggested fix (7.15 KB, patch)
2011-07-11 09:04 PDT, Alexander Pavlov (apavlov)
pfeldman: review-
Details | Formatted Diff | Diff
[PATCH] Introduced a kind of an "editing session" for a CSS style (21.74 KB, patch)
2011-07-14 03:40 PDT, Alexander Pavlov (apavlov)
no flags Details | Formatted Diff | Diff
[PATCH] Removed unused method (21.90 KB, patch)
2011-07-14 03:55 PDT, Alexander Pavlov (apavlov)
no flags Details | Formatted Diff | Diff
Patch (16.36 KB, patch)
2011-07-14 07:38 PDT, Pavel Feldman
no flags Details | Formatted Diff | Diff
Patch (20.00 KB, patch)
2011-07-14 09:14 PDT, Pavel Feldman
no flags Details | Formatted Diff | Diff
Patch (20.62 KB, patch)
2011-07-15 08:08 PDT, Pavel Feldman
no flags Details | Formatted Diff | Diff
Patch (24.58 KB, patch)
2011-07-15 08:22 PDT, Pavel Feldman
no flags Details | Formatted Diff | Diff
Patch (24.49 KB, patch)
2011-07-15 08:25 PDT, Pavel Feldman
yurys: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexander Pavlov (apavlov) 2011-07-11 08:12:10 PDT
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
Comment 1 Alexander Pavlov (apavlov) 2011-07-11 09:04:40 PDT
Created attachment 100309 [details]
[PATCH] Suggested fix
Comment 2 Pavel Feldman 2011-07-12 05:19:35 PDT
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?
Comment 3 Alexander Pavlov (apavlov) 2011-07-14 03:40:45 PDT
Created attachment 100795 [details]
[PATCH] Introduced a kind of an "editing session" for a CSS style
Comment 4 Alexander Pavlov (apavlov) 2011-07-14 03:55:13 PDT
Created attachment 100796 [details]
[PATCH] Removed unused method
Comment 5 Pavel Feldman 2011-07-14 06:02:26 PDT
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?
Comment 6 Pavel Feldman 2011-07-14 07:38:45 PDT
Created attachment 100806 [details]
Patch
Comment 7 Alexander Pavlov (apavlov) 2011-07-14 07:56:27 PDT
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
Comment 8 Pavel Feldman 2011-07-14 09:14:51 PDT
Created attachment 100814 [details]
Patch
Comment 9 Yury Semikhatsky 2011-07-15 06:25:26 PDT
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?
Comment 10 Pavel Feldman 2011-07-15 08:05:18 PDT
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.
Comment 11 Pavel Feldman 2011-07-15 08:08:02 PDT
Created attachment 100981 [details]
Patch
Comment 12 Pavel Feldman 2011-07-15 08:22:03 PDT
Created attachment 100985 [details]
Patch
Comment 13 Pavel Feldman 2011-07-15 08:25:58 PDT
Created attachment 100986 [details]
Patch
Comment 14 Pavel Feldman 2011-07-15 08:34:43 PDT
Committed r91070: <http://trac.webkit.org/changeset/91070>