Summary: | Web Inspector: Focusing on another node while editing CSS property does not update styles | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Alexander Pavlov (apavlov) <apavlov> | ||||||||||||||||||
Component: | Web Inspector (Deprecated) | Assignee: | Alexander Pavlov (apavlov) <apavlov> | ||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||
Severity: | Normal | CC: | apavlov, bweinstein, joepeck, keishi, loislo, pfeldman, pmuellr, rik, timothy, yurys | ||||||||||||||||||
Priority: | P2 | ||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||
Hardware: | All | ||||||||||||||||||||
OS: | All | ||||||||||||||||||||
Attachments: |
|
Description
Alexander Pavlov (apavlov)
2011-07-11 08:12:10 PDT
Created attachment 100309 [details]
[PATCH] Suggested fix
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? Created attachment 100795 [details]
[PATCH] Introduced a kind of an "editing session" for a CSS style
Created attachment 100796 [details]
[PATCH] Removed unused method
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? Created attachment 100806 [details]
Patch
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 Created attachment 100814 [details]
Patch
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 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. Created attachment 100981 [details]
Patch
Created attachment 100985 [details]
Patch
Created attachment 100986 [details]
Patch
Committed r91070: <http://trac.webkit.org/changeset/91070> |