Bug 199143

Summary: REGRESSION(r246621): Web Inspector: Styles: property may get removed when editing after deleting value
Product: WebKit Reporter: Nikita Vasilyev <nvasilyev>
Component: Web InspectorAssignee: Nikita Vasilyev <nvasilyev>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, inspector-bugzilla-changes, mattbaker, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Attachments:
Description Flags
[Animated GIF] Bug
none
Patch
none
[Animated GIF] Bug none

Description Nikita Vasilyev 2019-06-23 23:29:07 PDT
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.
Comment 1 Radar WebKit Bug Importer 2019-06-23 23:29:23 PDT
<rdar://problem/52042815>
Comment 2 Nikita Vasilyev 2019-06-23 23:45:23 PDT
Created attachment 372734 [details]
Patch
Comment 3 Nikita Vasilyev 2019-06-23 23:50:13 PDT
Created attachment 372736 [details]
[Animated GIF] Bug
Comment 4 Matt Baker 2019-06-25 12:56:29 PDT
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
Comment 5 Matt Baker 2019-06-25 14:00:18 PDT
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?
Comment 6 Nikita Vasilyev 2019-06-25 15:57:24 PDT
(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.
Comment 7 Matt Baker 2019-06-25 16:05:04 PDT
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.
Comment 8 Nikita Vasilyev 2019-06-25 16:08:18 PDT
> 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!
Comment 9 WebKit Commit Bot 2019-06-25 16:40:21 PDT
Comment on attachment 372734 [details]
Patch

Clearing flags on attachment: 372734

Committed r246816: <https://trac.webkit.org/changeset/246816>
Comment 10 WebKit Commit Bot 2019-06-25 16:40:22 PDT
All reviewed patches have been landed.  Closing bug.
Comment 11 Matt Baker 2019-06-25 16:56:37 PDT
(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!