Bug 199143 – REGRESSION(r246621): Web Inspector: Styles: property may get removed when editing after deleting value

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Nikita Vasilyev]

Created attachment 372732 [details]
[Animated GIF] Bug

Steps:
1. Add property "color: red"
2. Press Enter
3. Press Shift-Tab
4. Press Delete to delete "red"

Actual:
The entire property gets removed without giving a chance to enter a new value.

Expected:
Editing continues.

Notes:
The attached animated GIF runs Web Inspector with Style Editing Debug Mode (Debug -> Styles: Enable style editing debug mode).
The red border on the right side of a style rule indicates that the style declaration is locked. After step 3,
you can see it is no longer the case and that's the problem.

This is a regression from Bug 198505 - REGRESSION(r240946): Web Inspector: Styles: Pasting multiple properties has issues.

[Radar WebKit Bug Importer]

<rdar://problem/52042815>

[Nikita Vasilyev]

Created attachment 372734 [details]
Patch

[Nikita Vasilyev]

Created attachment 372736 [details]
[Animated GIF] Bug

[Matt Baker]

Interestingly this isn't a problem when there is more than one property.

Steps to Reproduce:

1. Add "color: red"
2. Press Enter
3. Add "text-align: center"
4. Press Enter
5. Shift-Tab three times
  => Property value "red" gets the focus
6. Press Delete
  => Carat positioned at empty property value, autocomplete menu shown

[Matt Baker]

Comment on attachment 372734 [details]
Patch

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

This fixes the problem, I'd just like to have a better understanding of the underlying issue before r+ing.

> Source/WebInspectorUI/ChangeLog:11
> +        incorrectly set to false.

This explanation is a bit confusing. Based on the change log for r246621, it looks like we now dispatch this event in cases where previously we did not:

"When WI.CSSStyleDeclaration.prototype.update gets called after setting text, it exits early without firing WI.CSSStyleDeclaration.Event.PropertiesChanged."

If I understand the code, the problem is that a handler which blurs the element is now being called, causing the property to be deleted. Why doesn't this happen in the case mentioned here: https://bugs.webkit.org/show_bug.cgi?id=199143#c4?

[Nikita Vasilyev]

(In reply to Matt Baker from comment #4)
> Interestingly this isn't a problem when there is more than one property.
> 
> Steps to Reproduce:
> 
> 1. Add "color: red"
> 2. Press Enter
> 3. Add "text-align: center"
> 4. Press Enter
> 5. Shift-Tab three times
>   => Property value "red" gets the focus
> 6. Press Delete
>   => Carat positioned at empty property value, autocomplete menu shown

Correct. Let's dig into step 5.

1st Shift-Tab:
  focused "center" -> focused=true
  blank property removed -> focused=false
2nd Shift-Tab:
  focused "text-align" -> focused=true
3rd Shift-Tab:
  focused "red" -> focused=true

My patch makes it so on the 1st Shift-Tab unfocused property removal doesn't set focus to false.

[Matt Baker]

Comment on attachment 372734 [details]
Patch

r=me, thanks for the detailed explanation. I'll leave it up to you whether to update the change log.

[Nikita Vasilyev]

> If I understand the code, the problem is that a handler which blurs the
> element is now being called, causing the property to be deleted. 

Not quite. The property is being deleted because of the full layout. When a style is being edited, it shouldn't do a full layout. It should call _updatePropertiesStatus instead.

WI.CSSStyleDeclaration.Event.PropertiesChanged calls WI.SpreadsheetCSSStyleDeclarationEditor.prototype._propertiesChanged
_propertiesChanged

    _propertiesChanged(event)
    {
        if (this.editing && isNaN(this._pendingAddBlankPropertyIndexOffset))
            this._updatePropertiesStatus();
        else
            this.needsLayout();
    }

Because `focused` is incorrectly false, `this.editing` returns false, too:

    get editing()
    {
        return this._focused || this._inlineSwatchActive;
    }

> Why doesn't this happen in the case mentioned here:
> https://bugs.webkit.org/show_bug.cgi?id=199143#c4?

Let me know if answered it above!

[WebKit Commit Bot]

Comment on attachment 372734 [details]
Patch

Clearing flags on attachment: 372734

Committed r246816: <https://trac.webkit.org/changeset/246816>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Matt Baker]

(In reply to Nikita Vasilyev from comment #8)
> > If I understand the code, the problem is that a handler which blurs the
> > element is now being called, causing the property to be deleted. 
> 
> Not quite. The property is being deleted because of the full layout. When a
> style is being edited, it shouldn't do a full layout. It should call
> _updatePropertiesStatus instead.
> 
> WI.CSSStyleDeclaration.Event.PropertiesChanged calls
> WI.SpreadsheetCSSStyleDeclarationEditor.prototype._propertiesChanged
> _propertiesChanged
> 
>     _propertiesChanged(event)
>     {
>         if (this.editing && isNaN(this._pendingAddBlankPropertyIndexOffset))
>             this._updatePropertiesStatus();
>         else
>             this.needsLayout();
>     }
> 
> Because `focused` is incorrectly false, `this.editing` returns false, too:
> 
>     get editing()
>     {
>         return this._focused || this._inlineSwatchActive;
>     }

Ahh okay. I totally missed this.

> 
> > Why doesn't this happen in the case mentioned here:
> > https://bugs.webkit.org/show_bug.cgi?id=199143#c4?
> 
> Let me know if answered it above!