Bug 65918 - Web Inspector: [REGRESSION] Editor lost after committing a CSS property value for inline style
Summary: Web Inspector: [REGRESSION] Editor lost after committing a CSS property value...
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-08-09 06:47 PDT by Alexander Pavlov (apavlov)
Modified: 2011-08-10 06:38 PDT (History)
10 users (show)

See Also:


Attachments
[PATCH] Suggested fix (7.71 KB, patch)
2011-08-09 09:19 PDT, Alexander Pavlov (apavlov)
no flags Details | Formatted Diff | Diff
[PATCH] Comments addressed (7.54 KB, patch)
2011-08-10 05:17 PDT, Alexander Pavlov (apavlov)
pfeldman: 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-08-09 06:47:07 PDT
1. Bring up Web Inspector for any element
2. Start editing new/existing CSS property value in the Styles pane (Elements panel)
3. Press Enter or Tab to commit the property.

What is the expected output?
A new property editor appears

What do you see instead?
Oftentimes, no property editor appears. This works fine for non-inline styles and seems to be a regression introduced in http://trac.webkit.org/changeset/91070 (essentially, a race condition mostly seen with large stylesheets and/or Debug builds).

Upstreaming http://code.google.com/p/chromium/issues/detail?id=91822
Comment 1 Alexander Pavlov (apavlov) 2011-08-09 09:19:00 PDT
Created attachment 103366 [details]
[PATCH] Suggested fix
Comment 2 Pavel Feldman 2011-08-10 03:08:55 PDT
Comment on attachment 103366 [details]
[PATCH] Suggested fix

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

Looks promising!

> Source/WebCore/inspector/front-end/StylesSidebarPane.js:936
> +    update: function(full, callback)

Is this change needed?

> Source/WebCore/inspector/front-end/StylesSidebarPane.js:1623
> +    _updateAll: function(userCallback)

What does updateAll actually do?

> Source/WebCore/inspector/front-end/StylesSidebarPane.js:2100
> +            --parentPane._userOperation;

You should keep userOperation's logging + invent a new flag for this "transitioning between the values".
Comment 3 Alexander Pavlov (apavlov) 2011-08-10 05:04:21 PDT
(In reply to comment #2)
> (From update of attachment 103366 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=103366&action=review
> 
> Looks promising!
> 
> > Source/WebCore/inspector/front-end/StylesSidebarPane.js:936
> > +    update: function(full, callback)
> 
> Is this change needed?

Good point, rewrote the call site.

> > Source/WebCore/inspector/front-end/StylesSidebarPane.js:1623
> > +    _updateAll: function(userCallback)
> 
> What does updateAll actually do?

It attempts to update the biggest possible scope accessible from this tree element (i.e., the Styles pane (the parent section has the "pane" field set), the parent section (no "pane" field), or the element's title (no parent section)). After some experimenting, I failed to hit the second and the third cases, so I've cleaned them up and renamed the method to _updatePane().

> > Source/WebCore/inspector/front-end/StylesSidebarPane.js:2100
> > +            --parentPane._userOperation;
> 
> You should keep userOperation's logging + invent a new flag for this "transitioning between the values".

logging -> logic. I've managed to get rid of this counting merely by avoiding the flag modification in the !updateInterface case (since in the !updateInterface case the _isEditingStyle flag is true, and we are safe).

Attaching an updated patch shortly.
Comment 4 Alexander Pavlov (apavlov) 2011-08-10 05:17:56 PDT
Created attachment 103466 [details]
[PATCH] Comments addressed
Comment 5 Alexander Pavlov (apavlov) 2011-08-10 06:38:47 PDT
Committed r92764: <http://trac.webkit.org/changeset/92764>