Bug 51390 – Web Inspector: After scrolling new property value and cancelling, removed property remains active in page style

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

[Alexander Pavlov (apavlov)]

1.

[Alexander Pavlov (apavlov)]

Somehow the blank report got committed.

1. In any page, add a numeric property, say, "height" for the "body" element
2. Type in 20% and press Up or Down to have the value inc/decremented.
3. Hit Esc to cancel the property addition

Observe that the effect of the property (e.g. height: 21%) remains active in the page.

[Alexander Pavlov (apavlov)]

Created attachment 77119 [details]
[PATCH] Suggested fix

[Joseph Pecoraro]

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

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

> WebCore/inspector/front-end/StylesSidebarPane.js:1857
> +        if (!styleTextLength && updateInterface && this._newProperty && !("originalPropertyText" in this)) {

How can something be in the state where this._newProperty is true and it has "originalPropertyText"?

[Alexander Pavlov (apavlov)]

(In reply to comment #3)
> (From update of attachment 77119 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=77119&action=review
> 
> > WebCore/inspector/front-end/StylesSidebarPane.js:1857
> > +        if (!styleTextLength && updateInterface && this._newProperty && !("originalPropertyText" in this)) {
> 
> How can something be in the state where this._newProperty is true and it has "originalPropertyText"?

It happens exactly after the steps 1-3 are executed. In this case, originalPropertyText === "" (it is remembered after the Up or Down keys are hit to increment/decrement the property value). Setting the property text back to "" effectively removes the property from the style.

[Joseph Pecoraro]

I see, you explained this to me on IRC:

  • "originalPropertyText" 
  • Before patch: Up/Down + Esc = the property gets removed from UI (sidebar) but kept in CSSStyleDeclarations (page)
  • After patch: Up/Down + Esc = the property gets removed both from UI (sidebar) and CSSStyleDeclarations (page)

That sounds good to me.

For the sake of understanding this, moving the ("originalPropertyText" in this) into a
helper function under a more descriptive name. Something like:

  _hasBeenAppliedToPageViaUpDown: { return ("originalPropertyText" in this); }

In doing so, the reading becomes:

  if (!styleTextLength && updateInterface && this._newProperty && !this._hasBeenAppliedToPageViaUpDown()) {
    // Delete from UI (sidebar)
  }

So the code inside this block doesn't run! Now I see what you mean when you said:

  Setting the property text back to "" effectively removes the property from the style.

That is the part I wasn't seeing. I think that part should be commented

  // New properties applied via up/down have an originalPropertyText and will be deleted later
  // on, if cancelled, when the empty string gets applied as their style text.

If that sounds correct then r=me. I applied the patch and it worked nicely. However, up/down
seemed broken in other ways. I think I just need to update for some recent fixes.

[Joseph Pecoraro]

>   • "originalPropertyText" is used with up/down only, to save the original value

[Alexander Pavlov (apavlov)]

Created attachment 77210 [details]
[PATCH] Comments addressed

[Alexander Pavlov (apavlov)]

(In reply to comment #5)
> I see, you explained this to me on IRC:
> ...
> If that sounds correct then r=me. I applied the patch and it worked nicely. However, up/down
> seemed broken in other ways. I think I just need to update for some recent fixes.

Yes, all your statements are valid, I have fixed the nits in the latest patch.
Just in case, can you say how up/down is broken for you? Some changes around it are coming in the CSS value autocompletion (bug 41414).

[Joseph Pecoraro]

(In reply to comment #8)
> Just in case, can you say how up/down is broken for you? Some changes around
> it are coming in the CSS value autocompletion (bug 41414).

I just tested with a fresh ToT build (and with your patch applied). Things were working
better then when I made my comment.

Dabbling with the inline style attribute I found some issues:
<http://webkit.org/b/51477> Web Inspector: ASSERT removing inline HTML style property
<http://webkit.org/b/51478> Web Inspector: Inline HTML style property out of sync with element.style in Sidebar

[Joseph Pecoraro]

Comment on attachment 77210 [details]
[PATCH] Comments addressed

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

> WebCore/inspector/front-end/StylesSidebarPane.js:1854
> +        // New properties applied via up/down have an originalPropertyText and will be deleted later
> +        // on, if cancelled, when the empty string gets applied as their style text.

This comment would be better suited down below, in `applyStyleText`, next to the usage of
the _hasBeenAppliedToPageViaUpDown function.

[Alexander Pavlov (apavlov)]

Comment on attachment 77210 [details]
[PATCH] Comments addressed

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

>> WebCore/inspector/front-end/StylesSidebarPane.js:1854
>> +        // on, if cancelled, when the empty string gets applied as their style text.
> 
> This comment would be better suited down below, in `applyStyleText`, next to the usage of
> the _hasBeenAppliedToPageViaUpDown function.

There is a very similar comment down below (which had been there for ages, I just augmented it), so I decided to keep all things clear :)

[Alexander Pavlov (apavlov)]

Comment on attachment 77210 [details]
[PATCH] Comments addressed

Landing as is, since there is a very similar comment in the snippet you are pointing to. Please speak up if you think both comments should be changed, and I can drive-by fix them in one of subsequent changes (you submitted a good many of bugs yesterday :))

[WebKit Commit Bot]

Comment on attachment 77210 [details]
[PATCH] Comments addressed

Clearing flags on attachment: 77210

Committed r74548: <http://trac.webkit.org/changeset/74548>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.