Bug 51679 – Web Inspector: A disabled style property should get enabled when edited

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

[Alexander Pavlov (apavlov)]

In the Styles sidebar, a changed disabled CSS property remains disabled, while Firebug enables it when the new value is applied.

[Alexander Pavlov (apavlov)]

Created attachment 77562 [details]
[PATCH] Suggested solution

[Pavel Feldman]

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

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

> WebCore/inspector/front-end/CSSStyleModel.js:474
> +                if (newProperty && this.disabled && !propertyText.match(/^\s*$/) && (!this.property || newProperty.propertyText !== this.property.propertyText)) {

What is "this.property" and where does it get defined?

[Alexander Pavlov (apavlov)]

(In reply to comment #2)
> (From update of attachment 77562 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=77562&action=review
> 
> > WebCore/inspector/front-end/CSSStyleModel.js:474
> > +                if (newProperty && this.disabled && !propertyText.match(/^\s*$/) && (!this.property || newProperty.propertyText !== this.property.propertyText)) {
> 
> What is "this.property" and where does it get defined?

It is the WebInspector.CSSProperty that lives in this WebInspector.StylePropertyTreeElement.

[Alexander Pavlov (apavlov)]

Created attachment 77610 [details]
[PATCH] propertyText change computation fixed

[Pavel Feldman]

Comment on attachment 77610 [details]
[PATCH] propertyText change computation fixed

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

> WebCore/inspector/front-end/CSSStyleModel.js:475
> +                if (newProperty && this.disabled && !propertyText.match(/^\s*$/) && textChanged) {

No need to check for textChanged.

[Alexander Pavlov (apavlov)]

Landed with the nit fixed.

Committing to http://svn.webkit.org/repository/webkit/trunk ...
        M       LayoutTests/ChangeLog
        A       LayoutTests/inspector/styles-disable-then-change-expected.txt
        A       LayoutTests/inspector/styles-disable-then-change.html
        M       WebCore/ChangeLog
        M       WebCore/inspector/front-end/CSSStyleModel.js
Committed r74740

[Joseph Pecoraro]

Comment on attachment 77610 [details]
[PATCH] propertyText change computation fixed

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

> WebCore/inspector/front-end/CSSStyleModel.js:477
> +                    return;

I have no objections to this patch, I was just curious why this branch early returns,
and doesn't invoke the possible userCallback function like the other branches.
I didn't investigate this deeply, but maybe that could use a comment?

[Alexander Pavlov (apavlov)]

(In reply to comment #7)
> (From update of attachment 77610 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=77610&action=review
> 
> > WebCore/inspector/front-end/CSSStyleModel.js:477
> > +                    return;
> 
> I have no objections to this patch, I was just curious why this branch early returns,
> and doesn't invoke the possible userCallback function like the other branches.
> I didn't investigate this deeply, but maybe that could use a comment?

Ugh, sorry for not commenting that, as it does looks strange to an untrained eye :) The point is that we need to run setPropertyText2+setDisabled(false) as an atomic operation, and hence the callback should be called only after setDisabled() returns (see enabledCallback()).

[Joseph Pecoraro]

> Ugh, sorry for not commenting that, as it does looks strange to an untrained eye :)
> The point is that we need to run setPropertyText2+setDisabled(false) as an atomic
> operation, and hence the callback should be called only after setDisabled() returns
> (see enabledCallback()).

I see. That makes perfect sense, and its really my fault for overlooking that. Thanks!