Changeset 227370 in webkit


Ignore:
Timestamp:
Jan 22, 2018 4:52:32 PM (6 years ago)
Author:
Nikita Vasilyev
Message:

Web Inspector: Styles Redesign: data corruption when updating values quickly
​https://bugs.webkit.org/show_bug.cgi?id=179461
<rdar://problem/35431882>

Reviewed by Joseph Pecoraro.

Source/WebInspectorUI:

Data corruption used to happen because CSSStyleDeclaration.prototype.text didn't
update synchronously. Making two or more quick changes resulted in corrupted data.

Imagine we modify a CSS value 3 times:

Front-end: (1)-(2)---(3)
Back-end: (1)-----(2)-(3)

The first response from the backend could happen after the 2nd edit. In this patch,
CSSStyleDeclaration is locked when its view is being edited.

To correctly display invalid and overridden properties, the backend is allowed to update
CSSStyleDeclaration and CSSProperty when they're locked if the text from the backend
matches the model's text. This should happen when the backend is caught up with the
front-end changes.

  • UserInterface/Models/CSSProperty.js:

(WI.CSSProperty.prototype.update):

  • UserInterface/Models/CSSStyleDeclaration.js:

(WI.CSSStyleDeclaration):
(WI.CSSStyleDeclaration.prototype.get locked):
(WI.CSSStyleDeclaration.prototype.set locked):
(WI.CSSStyleDeclaration.prototype.set text):

  • UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:

(WI.SpreadsheetCSSStyleDeclarationEditor):
(WI.SpreadsheetCSSStyleDeclarationEditor.prototype.initialLayout):
(WI.SpreadsheetCSSStyleDeclarationEditor.prototype.detached):
(WI.SpreadsheetCSSStyleDeclarationEditor.prototype.get editing):
(WI.SpreadsheetCSSStyleDeclarationEditor.prototype.set focused):
(WI.SpreadsheetCSSStyleDeclarationEditor.prototype.set inlineSwatchActive):
(WI.SpreadsheetCSSStyleDeclarationEditor.prototype.stylePropertyInlineSwatchActivated):
(WI.SpreadsheetCSSStyleDeclarationEditor.prototype.stylePropertyInlineSwatchDeactivated):
(WI.SpreadsheetCSSStyleDeclarationEditor.prototype._propertiesChanged):
(WI.SpreadsheetCSSStyleDeclarationEditor.prototype._updateStyleLock):
Lock CSSStyleDeclaration when a CSS property name or value is focused or
an inline widget is active.

  • UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.js:

(WI.SpreadsheetCSSStyleDeclarationSection):
(WI.SpreadsheetCSSStyleDeclarationSection.prototype._handleMouseDown):
(WI.SpreadsheetCSSStyleDeclarationSection.prototype._handleClick):

  • UserInterface/Views/SpreadsheetStyleProperty.js:

(WI.SpreadsheetStyleProperty):
(WI.SpreadsheetStyleProperty.prototype._createInlineSwatch):
When selector is focused, clicking on the white-space should not add a new blank property.

LayoutTests:

  • inspector/css/modify-css-property-expected.txt: Added.
  • inspector/css/modify-css-property.html: Added.
Location:
trunk
Files:
2 added
7 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r227369 r227370  
     12018-01-22  Nikita Vasilyev  <nvasilyev@apple.com>
     2
     3        Web Inspector: Styles Redesign: data corruption when updating values quickly
     4        https://bugs.webkit.org/show_bug.cgi?id=179461
     5        <rdar://problem/35431882>
     6
     7        Reviewed by Joseph Pecoraro.
     8
     9        * inspector/css/modify-css-property-expected.txt: Added.
     10        * inspector/css/modify-css-property.html: Added.
     11
    1122018-01-22  Matt Lewis  <jlewis3@apple.com>
    213

  • trunk/Source/WebInspectorUI/ChangeLog

    r227244 r227370  
     12018-01-22  Nikita Vasilyev  <nvasilyev@apple.com>
     2
     3        Web Inspector: Styles Redesign: data corruption when updating values quickly
     4        https://bugs.webkit.org/show_bug.cgi?id=179461
     5        <rdar://problem/35431882>
     6
     7        Reviewed by Joseph Pecoraro.
     8
     9        Data corruption used to happen because CSSStyleDeclaration.prototype.text didn't
     10        update synchronously. Making two or more quick changes resulted in corrupted data.
     11
     12        Imagine we modify a CSS value 3 times:
     13
     14        Front-end:  (1)-(2)---(3)
     15        Back-end:          (1)-----(2)-(3)
     16
     17        The first response from the backend could happen after the 2nd edit. In this patch,
     18        CSSStyleDeclaration is locked when its view is being edited.
     19
     20        To correctly display invalid and overridden properties, the backend is allowed to update
     21        CSSStyleDeclaration and CSSProperty when they're locked if the text from the backend
     22        matches the model's text. This should happen when the backend is caught up with the
     23        front-end changes.
     24
     25        * UserInterface/Models/CSSProperty.js:
     26        (WI.CSSProperty.prototype.update):
     27        * UserInterface/Models/CSSStyleDeclaration.js:
     28        (WI.CSSStyleDeclaration):
     29        (WI.CSSStyleDeclaration.prototype.get locked):
     30        (WI.CSSStyleDeclaration.prototype.set locked):
     31        (WI.CSSStyleDeclaration.prototype.set text):
     32
     33        * UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js:
     34        (WI.SpreadsheetCSSStyleDeclarationEditor):
     35        (WI.SpreadsheetCSSStyleDeclarationEditor.prototype.initialLayout):
     36        (WI.SpreadsheetCSSStyleDeclarationEditor.prototype.detached):
     37        (WI.SpreadsheetCSSStyleDeclarationEditor.prototype.get editing):
     38        (WI.SpreadsheetCSSStyleDeclarationEditor.prototype.set focused):
     39        (WI.SpreadsheetCSSStyleDeclarationEditor.prototype.set inlineSwatchActive):
     40        (WI.SpreadsheetCSSStyleDeclarationEditor.prototype.stylePropertyInlineSwatchActivated):
     41        (WI.SpreadsheetCSSStyleDeclarationEditor.prototype.stylePropertyInlineSwatchDeactivated):
     42        (WI.SpreadsheetCSSStyleDeclarationEditor.prototype._propertiesChanged):
     43        (WI.SpreadsheetCSSStyleDeclarationEditor.prototype._updateStyleLock):
     44        Lock CSSStyleDeclaration when a CSS property name or value is focused or
     45        an inline widget is active.
     46
     47        * UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.js:
     48        (WI.SpreadsheetCSSStyleDeclarationSection):
     49        (WI.SpreadsheetCSSStyleDeclarationSection.prototype._handleMouseDown):
     50        (WI.SpreadsheetCSSStyleDeclarationSection.prototype._handleClick):
     51        * UserInterface/Views/SpreadsheetStyleProperty.js:
     52        (WI.SpreadsheetStyleProperty):
     53        (WI.SpreadsheetStyleProperty.prototype._createInlineSwatch):
     54        When selector is focused, clicking on the white-space should not add a new blank property.
     55
    1562018-01-19  Ross Kirsling  <ross.kirsling@sony.com>
    257

  • trunk/Source/WebInspectorUI/UserInterface/Models/CSSProperty.js

    r225299 r227370  
    7171    update(text, name, value, priority, enabled, overridden, implicit, anonymous, valid, styleSheetTextRange, dontFireEvents)
    7272    {
     73        // Locked CSSProperty can still be updated from the back-end when the text matches.
     74        // We need to do this to keep attributes such as valid and overridden up to date.
     75        if (this._ownerStyle && this._ownerStyle.locked && text !== this._text)
     76            return;
     77
    7378        text = text || "";
    7479        name = name || "";

  • trunk/Source/WebInspectorUI/UserInterface/Models/CSSStyleDeclaration.js

    r225568 r227370  
    4141        this._inherited = inherited || false;
    4242
     43        this._locked = false;
    4344        this._pendingProperties = [];
    4445        this._propertyNameMap = {};
    … …  
    5051        this._allVisibleProperties = null;
    5152
    52         this.update(text, properties, styleSheetTextRange, true);
     53        this.update(text, properties, styleSheetTextRange, {dontFireEvents: true});
    5354    }
    5455
    … …  
    99100    }
    100101
    101     update(text, properties, styleSheetTextRange, dontFireEvents)
    102     {
     102    get locked() { return this._locked; }
     103    set locked(value) { this._locked = value; }
     104
     105    update(text, properties, styleSheetTextRange, options = {})
     106    {
     107        let dontFireEvents = options.dontFireEvents || false;
     108        let suppressLock = options.suppressLock || false;
     109
     110        if (this._locked && !suppressLock && text !== this._text)
     111            return;
     112
    103113        text = text || "";
    104114        properties = properties || [];
    … …  
    162172        // Don't fire the event if there is text and it hasn't changed.
    163173        if (oldText && this._text && oldText === this._text) {
    164             // We shouldn't have any added or removed properties in this case.
    165             console.assert(!addedProperties.length && !removedProperties.length);
     174            if (!this._locked || suppressLock) {
     175                // We shouldn't have any added or removed properties in this case.
     176                console.assert(!addedProperties.length && !removedProperties.length);
     177            }
     178
    166179            if (!addedProperties.length && !removedProperties.length)
    167180                return;
    … …  
    209222            this.dispatchEventToListeners(WI.CSSStyleDeclaration.Event.InitialTextModified);
    210223        }
     224
     225        // Update text immediately when it was modified via the styles sidebar.
     226        if (this._locked)
     227            this._text = text;
    211228
    212229        this._nodeStyles.changeStyleText(this, text);
    … …  
    371388            this._allProperties[index].index = index;
    372389
    373         const suppressEvents = true;
    374         this.update(this._text, this._allProperties, this._styleSheetTextRange, suppressEvents);
     390        this.update(this._text, this._allProperties, this._styleSheetTextRange, {dontFireEvents: true, suppressLock: true});
    375391
    376392        return property;

  • trunk/Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationEditor.js

    r227232 r227370  
    3535        this.style = style;
    3636        this._propertyViews = [];
     37
     38        this._inlineSwatchActive = false;
     39        this._focused = false;
     40
    3741        this._propertyPendingStartEditing = null;
    3842        this._filterText = null;
    … …  
    4044
    4145    // Public
     46
     47    initialLayout()
     48    {
     49        if (!this.style.editable)
     50            return;
     51
     52        this.element.addEventListener("focus", () => { this.focused = true; }, true);
     53        this.element.addEventListener("blur", (event) => {
     54            let focusedElement = event.relatedTarget;
     55            if (focusedElement && focusedElement.isDescendant(this.element))
     56                return;
     57
     58            this.focused = false;
     59        }, true);
     60    }
    4261
    4362    layout()
    … …  
    7594    detached()
    7695    {
     96        this._inlineSwatchActive = false;
     97        this.focused = false;
     98
    7799        for (let propertyView of this._propertyViews)
    78100            propertyView.detached();
    … …  
    98120
    99121        this.needsLayout();
     122    }
     123
     124    get editing()
     125    {
     126        return this._focused || this._inlineSwatchActive;
     127    }
     128
     129    set focused(value)
     130    {
     131        this._focused = value;
     132        this._updateStyleLock();
     133    }
     134
     135    set inlineSwatchActive(value)
     136    {
     137        this._inlineSwatchActive = value;
     138        this._updateStyleLock();
    100139    }
    101140
    … …  
    161200
    162201        return false;
    163     }
    164 
    165     isFocused()
    166     {
    167         let focusedElement = document.activeElement;
    168 
    169         if (!focusedElement || focusedElement.tagName === "BODY")
    170             return false;
    171 
    172         return focusedElement.isSelfOrDescendant(this.element);
    173202    }
    174203
    … …  
    223252    }
    224253
     254    // SpreadsheetStyleProperty delegate
     255
    225256    spreadsheetStylePropertyRemoved(propertyView)
    226257    {
    227258        this._propertyViews.remove(propertyView);
    228259        this.updateLayout();
     260    }
     261
     262    stylePropertyInlineSwatchActivated()
     263    {
     264        this.inlineSwatchActive = true;
     265    }
     266
     267    stylePropertyInlineSwatchDeactivated()
     268    {
     269        this.inlineSwatchActive = false;
    229270    }
    230271
    … …  
    279320    _propertiesChanged(event)
    280321    {
    281         if (this.isFocused()) {
     322        if (this.editing) {
    282323            for (let propertyView of this._propertyViews)
    283324                propertyView.updateStatus();
    … …  
    285326            this.needsLayout();
    286327    }
     328
     329    _updateStyleLock()
     330    {
     331        this.style.locked = this._focused || this._inlineSwatchActive;
     332    }
    287333};
    288334

  • trunk/Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.js

    r226082 r227370  
    4141        this._filterText = null;
    4242        this._shouldFocusSelectorElement = false;
    43         this._wasFocused = false;
     43        this._wasEditing = false;
    4444    }
    4545
    … …  
    4747
    4848    get style() { return this._style; }
    49 
    50     get propertiesEditor() { return this._propertiesEditor; }
    5149
    5250    get editable()
    … …  
    153151    }
    154152
     153    // SpreadsheetSelectorField delegate
     154
    155155    spreadsheetSelectorFieldDidChange(direction)
    156156    {
    … …  
    181181        this._discardSelectorChange();
    182182    }
     183
     184    // SpreadsheetCSSStyleDeclarationEditor delegate
    183185
    184186    cssStyleDeclarationEditorStartEditingAdjacentRule(toPreviousRule)
    … …  
    419421    _handleMouseDown(event)
    420422    {
    421         this._wasFocused = this._propertiesEditor.isFocused();
     423        this._wasEditing = this._propertiesEditor.editing || document.activeElement === this._selectorElement;
    422424    }
    423425
    424426    _handleClick(event)
    425427    {
    426         if (this._wasFocused)
     428        if (this._wasEditing)
    427429            return;
    428430

  • trunk/Source/WebInspectorUI/UserInterface/Views/SpreadsheetStyleProperty.js

    r227232 r227370  
    356356        }, this);
    357357
     358        if (typeof this._delegate.stylePropertyInlineSwatchActivated === "function") {
     359            swatch.addEventListener(WI.InlineSwatch.Event.Activated, () => {
     360                this._delegate.stylePropertyInlineSwatchActivated();
     361            });
     362        }
     363
     364        if (typeof this._delegate.stylePropertyInlineSwatchDeactivated === "function") {
     365            swatch.addEventListener(WI.InlineSwatch.Event.Deactivated, () => {
     366                this._delegate.stylePropertyInlineSwatchDeactivated();
     367            });
     368        }
     369
    358370        tokenElement.append(swatch.element, innerElement);
    359371
Note: See TracChangeset for help on using the changeset viewer.