Changeset 239251 in webkit


Ignore:
Timestamp:
Dec 15, 2018 2:03:19 AM (5 years ago)
Author:
Nikita Vasilyev
Message:

Web Inspector: Styles: toggling selected properties may cause data corruption
​https://bugs.webkit.org/show_bug.cgi?id=192396
<rdar://problem/46478383>

Reviewed by Devin Rousso.

Source/WebInspectorUI:

Uncommenting a property after a commented out property used to insert an unnecessary semicolon,
and not updating ranges of the following properties.

For example:

/* color: red; */
/* font-size: 12px */

Uncommenting font-size would result in something like this:

/* color: red; */; font-size: 12px


unnecessary semicolon

Now the semicolon doesn't get inserted and the white space is preserved better:

/* color: red; */
font-size: 12px

  • UserInterface/Models/CSSProperty.js:

(WI.CSSProperty.prototype._updateOwnerStyleText):
(WI.CSSProperty.prototype._appendSemicolonIfNeeded): Removed.
(WI.CSSProperty.prototype._prependSemicolonIfNeeded): Added.

  • UserInterface/Views/SpreadsheetStyleProperty.js:

(WI.SpreadsheetStyleProperty.prototype.remove):
(WI.SpreadsheetStyleProperty.prototype.update):
(WI.SpreadsheetStyleProperty.prototype._handleNameChange):
(WI.SpreadsheetStyleProperty.prototype._handleValueChange):
Style declaration should be locked while editing. Add asserts to ensure this.

LayoutTests:

  • inspector/css/add-css-property-expected.txt: Added.
  • inspector/css/add-css-property.html: Added.

Test adding new properties.

  • inspector/css/modify-css-property-expected.txt:
  • inspector/css/modify-css-property.html:

Test commenting out and uncommenting CSS properties.

Location:
trunk
Files:
2 added
6 edited

Legend:

Unmodified
Added
Removed

  • trunk/LayoutTests/ChangeLog

    r239243 r239251  
     12018-12-15  Nikita Vasilyev  <nvasilyev@apple.com>
     2
     3        Web Inspector: Styles: toggling selected properties may cause data corruption
     4        https://bugs.webkit.org/show_bug.cgi?id=192396
     5        <rdar://problem/46478383>
     6
     7        Reviewed by Devin Rousso.
     8
     9        * inspector/css/add-css-property-expected.txt: Added.
     10        * inspector/css/add-css-property.html: Added.
     11        Test adding new properties.
     12
     13        * inspector/css/modify-css-property-expected.txt:
     14        * inspector/css/modify-css-property.html:
     15        Test commenting out and uncommenting CSS properties.
     16
    1172018-12-14  Youenn Fablet  <youenn@apple.com>
    218

  • trunk/LayoutTests/inspector/css/modify-css-property-expected.txt

    r227370 r239251  
    1717PASS: Inline style declaration text should update when not locked.
    1818
     19-- Running test case: ModifyCSSProperty.CommentOutAndUncommentPropertyWithNewlines
     20PASS: Commented out property should be disabled.
     21PASS: Style declaration text should update immediately with uncommented property.
     22PASS: Uncommented property should be enabled.
     23PASS: Style declaration text should update immediately with commented out property.
     24PASS: Commented out property should be disabled.
     25
     26-- Running test case: ModifyCSSProperty.CommentOutAndUncommentPropertyWithoutNewlines
     27PASS: Commented out property should be disabled.
     28PASS: Style declaration text should update immediately with uncommented property.
     29PASS: Uncommented property should be enabled.
     30PASS: Commented out property should be disabled.
     31PASS: Style declaration text should update immediately with commented out property.
     32PASS: Uncommented property should be enabled.
     33

  • trunk/LayoutTests/inspector/css/modify-css-property.html

    r236766 r239251  
    2424                        return rule.style;
    2525                }
     26                InspectorTest.fail("No declaration found.");
     27                resolve();
    2628            };
    2729
    … …  
    3234                        return property;
    3335                }
     36                InspectorTest.fail("No property found.");
     37                resolve();
    3438            };
    3539
    … …  
    5559                        return rule.style;
    5660                }
     61                InspectorTest.fail("No declaration found.");
     62                resolve();
    5763            };
    5864
    … …  
    6369                        return property;
    6470                }
     71                InspectorTest.fail("No property found.");
     72                resolve();
    6573            };
    6674
    … …  
    94102                        return styleDeclaration;
    95103                }
     104                InspectorTest.fail("No declaration found.");
     105                resolve();
    96106            };
    97107
    … …  
    102112                        return property;
    103113                }
     114                InspectorTest.fail("No property found.");
     115                resolve();
    104116            };
    105117
    … …  
    125137
    126138            InspectorTest.evaluateInPage(`makeWide()`);
     139        }
     140    });
     141
     142    suite.addTestCase({
     143        name: "ModifyCSSProperty.CommentOutAndUncommentPropertyWithNewlines",
     144        test(resolve, reject) {
     145            let getMatchedStyleDeclaration = () => {
     146                for (let rule of nodeStyles.matchedRules) {
     147                    if (rule.selectorText === ".rule-c")
     148                        return rule.style;
     149                }
     150                InspectorTest.fail("No declaration found.");
     151                resolve();
     152            };
     153
     154            let getProperty = (propertyName) => {
     155                let styleDeclaration = getMatchedStyleDeclaration();
     156                for (let property of styleDeclaration.allProperties) {
     157                    if (property.name === propertyName)
     158                        return property;
     159                }
     160                InspectorTest.fail("No property found.");
     161                resolve();
     162            };
     163
     164            let styleDeclaration = getMatchedStyleDeclaration();
     165            styleDeclaration.locked = true;
     166
     167            InspectorTest.expectThat(!getProperty("padding-right").enabled, `Commented out property should be disabled.`);
     168
     169            let disabled = false;
     170            getProperty("padding-right").commentOut(disabled);
     171
     172            let expectedStyleText = `\n        /* padding-left: 2em; */\n        padding-right: 0px;\n    `;
     173            InspectorTest.expectEqual(styleDeclaration.text, expectedStyleText, `Style declaration text should update immediately with uncommented property.`);
     174
     175            InspectorTest.expectThat(getProperty("padding-right").enabled, `Uncommented property should be enabled.`);
     176
     177            disabled = true;
     178            getProperty("padding-right").commentOut(disabled);
     179
     180            expectedStyleText = `\n        /* padding-left: 2em; */\n        /* padding-right: 0px; */\n    `;
     181            InspectorTest.expectEqual(styleDeclaration.text, expectedStyleText, `Style declaration text should update immediately with commented out property.`);
     182
     183            InspectorTest.expectThat(!getProperty("padding-right").enabled, `Commented out property should be disabled.`);
     184
     185            resolve();
     186        }
     187    });
     188
     189    suite.addTestCase({
     190        name: "ModifyCSSProperty.CommentOutAndUncommentPropertyWithoutNewlines",
     191        test(resolve, reject) {
     192            let getMatchedStyleDeclaration = () => {
     193                for (let rule of nodeStyles.matchedRules) {
     194                    if (rule.selectorText === ".rule-d")
     195                        return rule.style;
     196                }
     197                InspectorTest.fail("No declaration found.");
     198                resolve();
     199            };
     200
     201            let getProperty = (propertyName) => {
     202                let styleDeclaration = getMatchedStyleDeclaration();
     203                for (let property of styleDeclaration.allProperties) {
     204                    if (property.name === propertyName)
     205                        return property;
     206                }
     207                InspectorTest.fail("No property found.");
     208                resolve();
     209            };
     210
     211            let styleDeclaration = getMatchedStyleDeclaration();
     212            styleDeclaration.locked = true;
     213
     214            InspectorTest.expectThat(!getProperty("font-size").enabled, `Commented out property should be disabled.`);
     215
     216            let disabled = false;
     217            getProperty("font-size").commentOut(disabled);
     218
     219            let expectedStyleText = `font-size: 13px;/*border: 2px solid brown*/`;
     220            InspectorTest.expectEqual(styleDeclaration.text, expectedStyleText, `Style declaration text should update immediately with uncommented property.`);
     221
     222            InspectorTest.expectThat(getProperty("font-size").enabled, `Uncommented property should be enabled.`);
     223            InspectorTest.expectThat(!getProperty("border").enabled, `Commented out property should be disabled.`);
     224
     225            disabled = false;
     226            getProperty("border").commentOut(disabled);
     227
     228            expectedStyleText = `font-size: 13px;border: 2px solid brown`;
     229            InspectorTest.expectEqual(styleDeclaration.text, expectedStyleText, `Style declaration text should update immediately with commented out property.`);
     230
     231            InspectorTest.expectThat(getProperty("border").enabled, `Uncommented property should be enabled.`);
     232
     233            resolve();
    127234        }
    128235    });
    … …  
    161268    }
    162269    .rule-b {font-size: 12px; color: antiquewhite}
     270    .rule-c {
     271        /* padding-left: 2em; */
     272        /* padding-right: 0px; */
     273    }
     274    .rule-d {/*font-size: 13px;*//*border: 2px solid brown*/}
    163275    </style>
    164     <div id="x" class="test-node rule-a rule-b" style="width: 100px"></div>
     276    <div id="x" class="test-node rule-a rule-b rule-c rule-d" style="width: 100px"></div>
    165277</body>
    166278</html>

  • trunk/Source/WebInspectorUI/ChangeLog

    r239246 r239251  
     12018-12-15  Nikita Vasilyev  <nvasilyev@apple.com>
     2
     3        Web Inspector: Styles: toggling selected properties may cause data corruption
     4        https://bugs.webkit.org/show_bug.cgi?id=192396
     5        <rdar://problem/46478383>
     6
     7        Reviewed by Devin Rousso.
     8
     9        Uncommenting a property after a commented out property used to insert an unnecessary semicolon,
     10        and not updating ranges of the following properties.
     11
     12        For example:
     13
     14            /* color: red; */
     15            /* font-size: 12px */
     16
     17        Uncommenting `font-size` would result in something like this:
     18
     19            /* color: red; */; font-size: 12px
     20                             ^
     21                             unnecessary semicolon
     22
     23        Now the semicolon doesn't get inserted and the white space is preserved better:
     24
     25            /* color: red; */
     26            font-size: 12px
     27
     28        * UserInterface/Models/CSSProperty.js:
     29        (WI.CSSProperty.prototype._updateOwnerStyleText):
     30        (WI.CSSProperty.prototype._appendSemicolonIfNeeded): Removed.
     31        (WI.CSSProperty.prototype._prependSemicolonIfNeeded): Added.
     32
     33        * UserInterface/Views/SpreadsheetStyleProperty.js:
     34        (WI.SpreadsheetStyleProperty.prototype.remove):
     35        (WI.SpreadsheetStyleProperty.prototype.update):
     36        (WI.SpreadsheetStyleProperty.prototype._handleNameChange):
     37        (WI.SpreadsheetStyleProperty.prototype._handleValueChange):
     38        Style declaration should be locked while editing. Add asserts to ensure this.
     39
    1402018-12-14  Matt Baker  <mattbaker@apple.com>
    241

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

    r238882 r239251  
    359359        }
    360360
     361        this._prependSemicolonIfNeeded();
     362
    361363        let styleText = this._ownerStyle.text || "";
    362364
    … …  
    376378        }
    377379
    378         let newStyleText = this._appendSemicolonIfNeeded(styleText.slice(0, range.startOffset)) + newText + styleText.slice(range.endOffset);
     380        let newStyleText = styleText.slice(0, range.startOffset) + newText + styleText.slice(range.endOffset);
    379381
    380382        let lineDelta = newText.lineCount - oldText.lineCount;
    … …  
    388390    }
    389391
    390     _appendSemicolonIfNeeded(styleText)
    391     {
    392         if (/[^;\s]\s*$/.test(styleText))
    393             return styleText.trimRight() + "; ";
    394 
    395         return styleText;
     392    _prependSemicolonIfNeeded()
     393    {
     394        for (let i = this.index - 1; i >= 0; --i) {
     395            let property = this._ownerStyle.allProperties[i];
     396            if (!property.enabled)
     397                continue;
     398
     399            let match = property.text.match(/[^;\s](\s*)$/);
     400            if (match)
     401                property.text = property.text.trimRight() + ";" + match[1];
     402
     403            break;
     404        }
    396405    }
    397406};

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

    r238813 r239251  
    130130    remove(replacement = null)
    131131    {
     132        console.assert(this._property.ownerStyle.locked, `Removed property was unlocked (${this._property.name})`);
    132133        this.element.remove();
    133134
    … …  
    154155            this._checkboxElement.tabIndex = -1;
    155156            this._checkboxElement.addEventListener("click", (event) => {
     157                console.assert(this._property.ownerStyle.locked, `Toggled property was unlocked (${this._property.name})`);
    156158                event.stopPropagation();
    157159                let disabled = !this._checkboxElement.checked;
    … …  
    652654    _handleNameChange()
    653655    {
     656        console.assert(this._property.ownerStyle.locked, `Modified property was unlocked (${this._property.name})`);
     657
    654658        this._property.name = this._nameElement.textContent.trim();
    655659    }
    … …  
    657661    _handleValueChange()
    658662    {
     663        console.assert(this._property.ownerStyle.locked, `Modified property was unlocked (${this._property.name})`);
     664
    659665        this._property.rawValue = this._valueElement.textContent.trim();
    660666    }
Note: See TracChangeset for help on using the changeset viewer.