Attachment 371613 Details for Bug 198629 – Patch

[patch] Patch

patch.txt (text/plain), 14.53 KB, created by Nikita Vasilyev on 2019-06-07 15:00:55 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Nikita Vasilyev

Created: 2019-06-07 15:00:55 PDT

Size: 14.53 KB

patch

obsolete

>diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
>index d6e8c8af05a..60b9e7b49df 100644
>--- a/LayoutTests/ChangeLog
>+++ b/LayoutTests/ChangeLog
>@@ -1,3 +1,14 @@
>+2019-06-07  Nikita Vasilyev  <nvasilyev@apple.com>
>+
>+        Web Inspector: longhand CSS properties overridden by shorthands miss strikethrough
>+        https://bugs.webkit.org/show_bug.cgi?id=198629
>+        <rdar://problem/51504160>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        * inspector/css/overridden-property-expected.txt:
>+        * inspector/css/overridden-property.html:
>+
> 2019-05-23  Saam barati  <sbarati@apple.com>
> 
>         [WHLSL] Property resolver needs to recurse to handle the base when simplifying rvalues
>diff --git a/LayoutTests/inspector/css/overridden-property-expected.txt b/LayoutTests/inspector/css/overridden-property-expected.txt
>index 6c762cbe2b1..cf76385ce43 100644
>--- a/LayoutTests/inspector/css/overridden-property-expected.txt
>+++ b/LayoutTests/inspector/css/overridden-property-expected.txt
>@@ -1,4 +1,4 @@
>-Test that CSSProperty.prototype.overridden is cleared when the overriding (effective) property is removed.
>+Test that CSSProperty.prototype.overridden is set correctly.
> 
> 
> == Running test suite: OverriddenProperty
>@@ -11,3 +11,19 @@ REMOVED
> color: red;
> 
> 
>+-- Running test case: OverriddenByShorthand
>+PASS: border-top-color is overridden.
>+PASS: border-color is NOT overridden.
>+
>+-- Running test case: OverriddenByImportantShorthand
>+PASS: border-color is NOT overridden.
>+PASS: border-top-color is overridden.
>+
>+-- Running test case: NotOverriddenByImportantLonghand
>+PASS: border-top-color is NOT overridden.
>+PASS: border-color is NOT overridden.
>+
>+-- Running test case: NotOverriddenByLonghand
>+PASS: border-color is NOT overridden.
>+PASS: border-top-color is NOT overridden.
>+
>diff --git a/LayoutTests/inspector/css/overridden-property.html b/LayoutTests/inspector/css/overridden-property.html
>index f19ea1dec3c..096479f1e21 100644
>--- a/LayoutTests/inspector/css/overridden-property.html
>+++ b/LayoutTests/inspector/css/overridden-property.html
>@@ -4,8 +4,6 @@
> <script src="../../http/tests/inspector/resources/inspector-test.js"></script>
> <script>
> function test() {
>-    let nodeStyles = null;
>-
>     let suite = InspectorTest.createAsyncSuite("OverriddenProperty");
> 
>     function logProperty(property) {
>@@ -15,65 +13,161 @@ function test() {
>         InspectorTest.log(text);
>     }
> 
>+    function getNodeStyles(selector, callback) {
>+        WI.domManager.requestDocument((documentNode) => {
>+            WI.domManager.querySelector(documentNode.id, selector, (contentNodeId) => {
>+                if (!contentNodeId) {
>+                    InspectorTest.fail("DOM node not found.");
>+                    InspectorTest.completeTest();
>+                    return;
>+                }
>+
>+                let domNode = WI.domManager.nodeForId(contentNodeId);
>+                let nodeStyles = WI.cssManager.stylesForNode(domNode);
>+
>+                if (nodeStyles.needsRefresh) {
>+                    nodeStyles.singleFireEventListener(WI.DOMNodeStyles.Event.Refreshed, (event) => {
>+                        callback(nodeStyles);
>+                    });
>+                } else
>+                    callback(nodeStyles);
>+            });
>+        });
>+    }
>+
>+    function getStyleDeclaration(selector, callback, resolve) {
>+        getNodeStyles(selector, (nodeStyles) => {
>+            let matchedRule = null;
>+            for (let rule of nodeStyles.matchedRules) {
>+                if (rule.selectorText === selector) {
>+                    matchedRule = rule;
>+                    break;
>+                }
>+            }
>+
>+            if (!matchedRule) {
>+                InspectorTest.fail(`Couldn't find ${selector}`);
>+                resolve();
>+                return;
>+            }
>+
>+            callback(matchedRule.style)
>+        });
>+    }
>+
>     suite.addTestCase({
>         name: "OverriddenProperty.effectivePropertyRemoved",
>+        description: "Test that CSSProperty.prototype.overridden is cleared when the overriding (effective) property is removed.",
>         test(resolve, reject) {
>-            let inlineStyle = nodeStyles.inlineStyle;
>-            let styleRule = nodeStyles.matchedRules[0];
>-            let inlineStyleProperty = inlineStyle.enabledProperties[0];
>-            let styleRuleProperty = styleRule.style.enabledProperties[0];
>-
>-            function log() {
>-                logProperty(inlineStyleProperty);
>-                logProperty(styleRuleProperty);
>-                InspectorTest.log("");
>-            }
>+            getNodeStyles("#x", (nodeStyles) => {
>+                let inlineStyle = nodeStyles.inlineStyle;
>+                let styleRule = nodeStyles.matchedRules[0];
>+                let inlineStyleProperty = inlineStyle.enabledProperties[0];
>+                let styleRuleProperty = styleRule.style.enabledProperties[0];
>+
>+                function log() {
>+                    logProperty(inlineStyleProperty);
>+                    logProperty(styleRuleProperty);
>+                    InspectorTest.log("");
>+                }
> 
>-            log();
>+                log();
> 
>-            inlineStyleProperty.remove();
>+                inlineStyleProperty.remove();
> 
>-            styleRuleProperty.addEventListener(WI.CSSProperty.Event.OverriddenStatusChanged, (event) => {
>-                // FIXME: <https://webkit.org/b/195651> OverriddenStatusChanged may fire more than once.
>-                if (styleRuleProperty.overridden)
>-                    return;
>+                styleRuleProperty.addEventListener(WI.CSSProperty.Event.OverriddenStatusChanged, (event) => {
>+                    // FIXME: <https://webkit.org/b/195651> OverriddenStatusChanged may fire more than once.
>+                    if (styleRuleProperty.overridden)
>+                        return;
> 
>-                InspectorTest.log("OverriddenStatusChanged event fired.");
>-                log();
>-                resolve();
>+                    InspectorTest.log("OverriddenStatusChanged event fired.");
>+                    log();
>+                    resolve();
>+                });
>             });
>         }
>     });
> 
>-    WI.domManager.requestDocument((documentNode) => {
>-        WI.domManager.querySelector(documentNode.id, "div#x", (contentNodeId) => {
>-            if (!contentNodeId) {
>-                InspectorTest.fail("DOM node not found.");
>-                InspectorTest.completeTest();
>-                return;
>-            }
>+    suite.addTestCase({
>+        name: "OverriddenByShorthand",
>+        test(resolve, reject) {
>+            getStyleDeclaration(".shorthand", (style) => {
>+                InspectorTest.expectTrue(style.visibleProperties[0].overridden, "border-top-color is overridden.");
>+                InspectorTest.expectFalse(style.visibleProperties[1].overridden, "border-color is NOT overridden.");
>+                resolve();
>+            }, resolve);
>+        }
>+    });
> 
>-            let domNode = WI.domManager.nodeForId(contentNodeId);
>-            nodeStyles = WI.cssManager.stylesForNode(domNode);
>+    suite.addTestCase({
>+        name: "OverriddenByImportantShorthand",
>+        test(resolve, reject) {
>+            getStyleDeclaration(".longhand-overridden-by-important-shorthand", (style) => {
>+                InspectorTest.expectFalse(style.visibleProperties[0].overridden, "border-color is NOT overridden.");
>+                InspectorTest.expectTrue(style.visibleProperties[1].overridden, "border-top-color is overridden.");
>+                resolve();
>+            }, resolve);
>+        }
>+    });
> 
>-            if (nodeStyles.needsRefresh) {
>-                nodeStyles.singleFireEventListener(WI.DOMNodeStyles.Event.Refreshed, (event) => {
>-                    suite.runTestCasesAndFinish();
>-                });
>-            } else
>-                suite.runTestCasesAndFinish();
>-        });
>+    suite.addTestCase({
>+        name: "NotOverriddenByImportantLonghand",
>+        test(resolve, reject) {
>+            getStyleDeclaration(".shorthand-overridden-by-important-longhand", (style) => {
>+                InspectorTest.expectFalse(style.visibleProperties[0].overridden, "border-top-color is NOT overridden.");
>+                InspectorTest.expectFalse(style.visibleProperties[1].overridden, "border-color is NOT overridden.");
>+                resolve();
>+            });
>+        }
>     });
>+
>+    suite.addTestCase({
>+        name: "NotOverriddenByLonghand",
>+        test(resolve, reject) {
>+            getStyleDeclaration(".shorthand-not-overridden-longhand", (style) => {
>+                InspectorTest.expectFalse(style.visibleProperties[0].overridden, "border-color is NOT overridden.");
>+                InspectorTest.expectFalse(style.visibleProperties[1].overridden, "border-top-color is NOT overridden.");
>+                resolve();
>+            });
>+        }
>+    });
>+
>+    suite.runTestCasesAndFinish();
> }
> </script>
> </head>
> <body onload="runTest()">
>-    <p>Test that CSSProperty.prototype.overridden is cleared when the overriding (effective) property is removed.</p>
>+    <p>Test that CSSProperty.prototype.overridden is set correctly.</p>
>     <style>
>     #x {
>         color: red;
>     }
>+
>+    .shorthand {
>+        border-top-color: red;
>+        border-color: green;
>+    }
>+
>+    .longhand-overridden-by-important-shorthand {
>+        border-color: green !important;
>+        border-top-color: red;
>+    }
>+
>+    .shorthand-overridden-by-important-longhand {
>+        border-top-color: red !important;
>+        border-color: green;
>+    }
>+
>+    .shorthand-not-overridden-longhand {
>+        border-color: green;
>+        border-top-color: red;
>+    }
>     </style>
>     <div id="x" style="color: green"></div>
>+    <div class="shorthand"></div>
>+    <div class="longhand-overridden-by-important-shorthand"></div>
>+    <div class="shorthand-overridden-by-important-longhand"></div>
>+    <div class="shorthand-overridden-by-important-longhand"></div>
>+    <div class="shorthand-not-overridden-longhand"></div>
> </body>
> </html>
>diff --git a/Source/WebInspectorUI/ChangeLog b/Source/WebInspectorUI/ChangeLog
>index 8205b91b694..d14712a172a 100644
>--- a/Source/WebInspectorUI/ChangeLog
>+++ b/Source/WebInspectorUI/ChangeLog
>@@ -1,3 +1,24 @@
>+2019-06-07  Nikita Vasilyev  <nvasilyev@apple.com>
>+
>+        Web Inspector: longhand CSS properties overridden by shorthands miss strikethrough
>+        https://bugs.webkit.org/show_bug.cgi?id=198629
>+        <rdar://problem/51504160>
>+
>+        Longhand CSS properties (e.g. "font-size") overriden by shorthands (e.g. "font") now have strikethroughs.
>+
>+        * UserInterface/Models/CSSProperty.js:
>+        (WI.CSSProperty.prototype.set overridingProperty):
>+        (WI.CSSProperty):
>+
>+        * UserInterface/Models/DOMNodeStyles.js:
>+        (WI.DOMNodeStyles.prototype._updateStyleCascade):
>+        Call _associateRelatedProperties before _markOverriddenProperties because
>+        _associateRelatedProperties sets relatedShorthandProperty property, which
>+        is now used by _markOverriddenProperties.
>+
>+        (WI.DOMNodeStyles.prototype._markOverriddenProperties.isOverriddenBy):
>+        (WI.DOMNodeStyles.prototype._markOverriddenProperties):
>+
> 2019-05-23  Matt Baker  <mattbaker@apple.com>
> 
>         Web Inspector: Remove unused CSS class "offset-sections"
>diff --git a/Source/WebInspectorUI/UserInterface/Models/CSSProperty.js b/Source/WebInspectorUI/UserInterface/Models/CSSProperty.js
>index 2c10895cc78..0c055c9ba63 100644
>--- a/Source/WebInspectorUI/UserInterface/Models/CSSProperty.js
>+++ b/Source/WebInspectorUI/UserInterface/Models/CSSProperty.js
>@@ -299,6 +299,7 @@ WI.CSSProperty = class CSSProperty extends WI.Object
>         if (!WI.settings.experimentalEnableStylesJumpToEffective.value)
>             return;
> 
>+        console.assert(this !== effectiveProperty, `Property "${this.formattedText}" can't override itself.`, this);
>         this._overridingProperty = effectiveProperty || null;
>     }
> 
>diff --git a/Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js b/Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js
>index a57b5201e35..15e39c9e28a 100644
>--- a/Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js
>+++ b/Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js
>@@ -795,13 +795,13 @@ WI.DOMNodeStyles = class DOMNodeStyles extends WI.Object
> 
>         this._propertyNameToEffectivePropertyMap = {};
> 
>-        this._markOverriddenProperties(cascadeOrderedStyleDeclarations, this._propertyNameToEffectivePropertyMap);
>         this._associateRelatedProperties(cascadeOrderedStyleDeclarations, this._propertyNameToEffectivePropertyMap);
>+        this._markOverriddenProperties(cascadeOrderedStyleDeclarations, this._propertyNameToEffectivePropertyMap);
> 
>         for (let pseudoElementInfo of this._pseudoElements.values()) {
>             pseudoElementInfo.orderedStyles = this._collectStylesInCascadeOrder(pseudoElementInfo.matchedRules, null, null);
>-            this._markOverriddenProperties(pseudoElementInfo.orderedStyles);
>             this._associateRelatedProperties(pseudoElementInfo.orderedStyles);
>+            this._markOverriddenProperties(pseudoElementInfo.orderedStyles);
>         }
>     }
> 
>@@ -847,6 +847,25 @@ WI.DOMNodeStyles = class DOMNodeStyles extends WI.Object
>     {
>         propertyNameToEffectiveProperty = propertyNameToEffectiveProperty || {};
> 
>+        function isOverriddenByRelatedShorthand(property) {
>+            let shorthand = property.relatedShorthandProperty;
>+            if (!shorthand)
>+                return false;
>+
>+            if (property.important && !shorthand.important)
>+                return false;
>+
>+            if (!property.important && shorthand.important)
>+                return true;
>+
>+            if (property.ownerStyle === shorthand.ownerStyle)
>+                return shorthand.index > property.index;
>+
>+            let propertyStyleIndex = styles.indexOf(property.ownerStyle);
>+            let shorthandStyleIndex = styles.indexOf(shorthand.ownerStyle);
>+            return shorthandStyleIndex > propertyStyleIndex;
>+        }
>+
>         for (var i = 0; i < styles.length; ++i) {
>             var style = styles[i];
>             var properties = style.enabledProperties;
>@@ -885,7 +904,11 @@ WI.DOMNodeStyles = class DOMNodeStyles extends WI.Object
>                     }
>                 }
> 
>-                property.overridden = false;
>+                if (isOverriddenByRelatedShorthand(property)) {
>+                    property.overridden = true;
>+                    property.overridingProperty = property.relatedShorthandProperty;
>+                } else
>+                    property.overridden = false;
> 
>                 propertyNameToEffectiveProperty[canonicalName] = property;
>             }

Flags:

hi: review+
hi: commit-queue-

Actions: View | Formatted Diff | Diff

Attachments on bug 198629: 371536 | 371613 | 371618