Attachment 358003 Details for Bug 178489 – Patch

[patch] Patch

bug-178489-20181221180134.patch (text/plain), 20.95 KB, created by Devin Rousso on 2018-12-21 17:01:35 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Devin Rousso

Created: 2018-12-21 17:01:35 PST

Size: 20.95 KB

patch

obsolete

>diff --git a/Source/WebInspectorUI/ChangeLog b/Source/WebInspectorUI/ChangeLog
>index 41b0ed9502a871ecbe08e2832064a86781c28040..8f04b0a9eb8cd625331fc6614ea11525dac2d409 100644
>--- a/Source/WebInspectorUI/ChangeLog
>+++ b/Source/WebInspectorUI/ChangeLog
>@@ -1,3 +1,43 @@
>+2018-12-21  Devin Rousso  <drousso@apple.com>
>+
>+        Web Inspector: Styles Redesign: Editing selector should not hide the rule
>+        https://bugs.webkit.org/show_bug.cgi?id=178489
>+        <rdar://problem/35062434>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Extracts the selector payload parsing logic inside `WI.DOMNodeStyles` into static functions
>+        so that when the user changes the selector of a `WI.CSSRule`, it's able to process and
>+        update itself with the new selector. This is mainly useful in the case where the `WI.CSSRule`
>+        no longer applies to the selected node (meaning it won't be part of that node's `WI.DOMNodeStyles`)
>+        in that it allows the `WI.SpreadsheetCSSStyleDeclarationSection` to display the new selector
>+        text and the owner `WI.SpreadsheetRulesStyleDetailsPanel` to keep that section visible even
>+        though it isn't applicable to the current node anymore.
>+
>+        * UserInterface/Models/CSSRule.js:
>+        (WI.CSSRule.prototype.update):
>+        (WI.CSSRule.prototype._selectorResolved):
>+        Drive-by: remove unused event.
>+
>+        * UserInterface/Models/DOMNodeStyles.js:
>+        (WI.DOMNodeStyles.parseSelectorListPayload): Added.
>+        (WI.DOMNodeStyles.createSourceCodeLocation): Added.
>+        (WI.DOMNodeStyles.prototype._parseRulePayload):
>+        (WI.DOMNodeStyles.prototype._createSourceCodeLocation): Deleted.
>+        (WI.DOMNodeStyles.prototype._parseSelectorListPayload): Deleted.
>+
>+        * UserInterface/Views/SpreadsheetRulesStyleDetailsPanel.js:
>+        (WI.SpreadsheetRulesStyleDetailsPanel.prototype.layout):
>+        (WI.SpreadsheetRulesStyleDetailsPanel.prototype._handleSectionFilterApplied):
>+        (WI.SpreadsheetRulesStyleDetailsPanel.prototype._handleSectionSelectorWillChange): Added.
>+        Attempt to preserve the position of any sections that are changed and no longer apply to the
>+        current node.
>+
>+        * UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.js:
>+        (WI.SpreadsheetCSSStyleDeclarationSection.prototype.spreadsheetSelectorFieldDidChange):
>+        (WI.SpreadsheetCSSStyleDeclarationSection.prototype._renderSelector):
>+        Drive-by: remove unused CSS classes.
>+
> 2018-12-21  Devin Rousso  <drousso@apple.com>
> 
>         Web Inspector: Settings: experimental setting editors are misaligned in other locales
>diff --git a/Source/WebInspectorUI/UserInterface/Models/CSSRule.js b/Source/WebInspectorUI/UserInterface/Models/CSSRule.js
>index fc5dd5859ddc2c8d006df02c4a43bee163351af0..d56bb78688a8524fc14c0cabf9c7be15cad91b43 100644
>--- a/Source/WebInspectorUI/UserInterface/Models/CSSRule.js
>+++ b/Source/WebInspectorUI/UserInterface/Models/CSSRule.js
>@@ -56,7 +56,7 @@ WI.CSSRule = class CSSRule extends WI.Object
>         return !!this._id && (this._type === WI.CSSStyleSheet.Type.Author || this._type === WI.CSSStyleSheet.Type.Inspector);
>     }
> 
>-    update(sourceCodeLocation, selectorText, selectors, matchedSelectorIndices, style, mediaList, dontFireEvents)
>+    update(sourceCodeLocation, selectorText, selectors, matchedSelectorIndices, style, mediaList)
>     {
>         sourceCodeLocation = sourceCodeLocation || null;
>         selectorText = selectorText || "";
>@@ -65,14 +65,6 @@ WI.CSSRule = class CSSRule extends WI.Object
>         style = style || null;
>         mediaList = mediaList || [];
> 
>-        var changed = false;
>-        if (!dontFireEvents) {
>-            changed = this._selectorText !== selectorText || !Array.shallowEqual(this._selectors, selectors) ||
>-                !Array.shallowEqual(this._matchedSelectorIndices, matchedSelectorIndices) || this._style !== style ||
>-                !!this._sourceCodeLocation !== !!sourceCodeLocation || this._mediaList.length !== mediaList.length;
>-            // FIXME: Look for differences in the media list arrays.
>-        }
>-
>         if (this._style)
>             this._style.ownerRule = null;
> 
>@@ -85,9 +77,6 @@ WI.CSSRule = class CSSRule extends WI.Object
> 
>         if (this._style)
>             this._style.ownerRule = this;
>-
>-        if (changed)
>-            this.dispatchEventToListeners(WI.CSSRule.Event.Changed);
>     }
> 
>     get type()
>@@ -163,11 +152,36 @@ WI.CSSRule = class CSSRule extends WI.Object
> 
>     _selectorResolved(rulePayload)
>     {
>+        if (rulePayload) {
>+            let selectorText = rulePayload.selectorList.text;
>+            if (selectorText !== this._selectorText) {
>+                let selectors = WI.DOMNodeStyles.parseSelectorListPayload(rulePayload.selectorList);
>+
>+                let sourceCodeLocation = null;
>+                let sourceRange = rulePayload.selectorList.range;
>+                if (sourceRange) {
>+                    sourceCodeLocation = WI.DOMNodeStyles.createSourceCodeLocation(rulePayload.sourceURL, {
>+                        line: sourceRange.startLine,
>+                        column: sourceRange.startColumn,
>+                        documentNode: this._nodeStyles.node.ownerDocument,
>+                    });
>+                }
>+
>+                if (this._ownerStyleSheet) {
>+                    if (!sourceCodeLocation && this._ownerStyleSheet.isInspectorStyleSheet())
>+                        sourceCodeLocation = this._ownerStyleSheet.createSourceCodeLocation(sourceRange.startLine, sourceRange.startColumn);
>+
>+                    sourceCodeLocation = this._ownerStyleSheet.offsetSourceCodeLocation(sourceCodeLocation);
>+                }
>+
>+                this.update(sourceCodeLocation, selectorText, selectors, [], this._style, this._mediaList);
>+            }
>+        }
>+
>         this.dispatchEventToListeners(WI.CSSRule.Event.SelectorChanged, {valid: !!rulePayload});
>     }
> };
> 
> WI.CSSRule.Event = {
>-    Changed: "css-rule-changed",
>     SelectorChanged: "css-rule-invalid-selector"
> };
>diff --git a/Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js b/Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js
>index f44b05628a1e4a95316454911b65cef3e5b353d8..fe1b18889cc6bfb41f8a22d5f9cc1e6d41519e21 100644
>--- a/Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js
>+++ b/Source/WebInspectorUI/UserInterface/Models/DOMNodeStyles.js
>@@ -49,6 +49,53 @@ WI.DOMNodeStyles = class DOMNodeStyles extends WI.Object
>         this.refresh();
>     }
> 
>+    // Static
>+
>+    static parseSelectorListPayload(selectorList)
>+    {
>+        let selectors = selectorList.selectors;
>+        if (!selectors.length)
>+            return [];
>+
>+        // COMPATIBILITY (iOS 8): The selectorList payload was an array of selector text strings.
>+        // Now they are CSSSelector objects with multiple properties.
>+        if (typeof selectors[0] === "string") {
>+            return selectors.map(function(selectorText) {
>+                return new WI.CSSSelector(selectorText);
>+            });
>+        }
>+
>+        return selectors.map(function(selectorPayload) {
>+            return new WI.CSSSelector(selectorPayload.text, selectorPayload.specificity, selectorPayload.dynamic);
>+        });
>+    }
>+
>+    static createSourceCodeLocation(sourceURL, {line, column, documentNode} = {})
>+    {
>+        if (!sourceURL)
>+            return null;
>+
>+        var sourceCode;
>+
>+        // Try to use the node to find the frame which has the correct resource first.
>+        if (documentNode) {
>+            var mainResource = WI.networkManager.resourceForURL(documentNode.documentURL);
>+            if (mainResource) {
>+                var parentFrame = mainResource.parentFrame;
>+                sourceCode = parentFrame.resourceForURL(sourceURL);
>+            }
>+        }
>+
>+        // If that didn't find the resource, then search all frames.
>+        if (!sourceCode)
>+            sourceCode = WI.networkManager.resourceForURL(sourceURL);
>+
>+        if (!sourceCode)
>+            return null;
>+
>+        return sourceCode.createSourceCodeLocation(line || 0, column || 0);
>+    }
>+
>     // Public
> 
>     get node()
>@@ -468,32 +515,6 @@ WI.DOMNodeStyles = class DOMNodeStyles extends WI.Object
> 
>     // Private
> 
>-    _createSourceCodeLocation(sourceURL, sourceLine, sourceColumn)
>-    {
>-        if (!sourceURL)
>-            return null;
>-
>-        var sourceCode;
>-
>-        // Try to use the node to find the frame which has the correct resource first.
>-        if (this._node.ownerDocument) {
>-            var mainResource = WI.networkManager.resourceForURL(this._node.ownerDocument.documentURL);
>-            if (mainResource) {
>-                var parentFrame = mainResource.parentFrame;
>-                sourceCode = parentFrame.resourceForURL(sourceURL);
>-            }
>-        }
>-
>-        // If that didn't find the resource, then search all frames.
>-        if (!sourceCode)
>-            sourceCode = WI.networkManager.resourceForURL(sourceURL);
>-
>-        if (!sourceCode)
>-            return null;
>-
>-        return sourceCode.createSourceCodeLocation(sourceLine || 0, sourceColumn || 0);
>-    }
>-
>     _parseSourceRangePayload(payload)
>     {
>         if (!payload)
>@@ -677,25 +698,6 @@ WI.DOMNodeStyles = class DOMNodeStyles extends WI.Object
>         return styleDeclaration;
>     }
> 
>-    _parseSelectorListPayload(selectorList)
>-    {
>-        var selectors = selectorList.selectors;
>-        if (!selectors.length)
>-            return [];
>-
>-        // COMPATIBILITY (iOS 8): The selectorList payload was an array of selector text strings.
>-        // Now they are CSSSelector objects with multiple properties.
>-        if (typeof selectors[0] === "string") {
>-            return selectors.map(function(selectorText) {
>-                return new WI.CSSSelector(selectorText);
>-            });
>-        }
>-
>-        return selectors.map(function(selectorPayload) {
>-            return new WI.CSSSelector(selectorPayload.text, selectorPayload.specificity, selectorPayload.dynamic);
>-        });
>-    }
>-
>     _parseRulePayload(payload, matchedSelectorIndices, node, inherited, ruleOccurrences)
>     {
>         if (!payload)
>@@ -742,16 +744,23 @@ WI.DOMNodeStyles = class DOMNodeStyles extends WI.Object
>         var styleSheet = id ? WI.cssManager.styleSheetForIdentifier(id.styleSheetId) : null;
> 
>         var selectorText = payload.selectorList.text;
>-        var selectors = this._parseSelectorListPayload(payload.selectorList);
>+        var selectors = WI.DOMNodeStyles.parseSelectorListPayload(payload.selectorList);
>         var type = WI.CSSManager.protocolStyleSheetOriginToEnum(payload.origin);
> 
>         var sourceCodeLocation = null;
>         var sourceRange = payload.selectorList.range;
>-        if (sourceRange)
>-            sourceCodeLocation = this._createSourceCodeLocation(payload.sourceURL, sourceRange.startLine, sourceRange.startColumn);
>-        else {
>+        if (sourceRange) {
>+            sourceCodeLocation = WI.DOMNodeStyles.createSourceCodeLocation(payload.sourceURL, {
>+                line: sourceRange.startLine,
>+                column: sourceRange.startColumn,
>+                documentNode: this._node.ownerDocument,
>+            });
>+        } else {
>             // FIXME: Is it possible for a CSSRule to have a sourceLine without its selectorList having a sourceRange? Fall back just in case.
>-            sourceCodeLocation = this._createSourceCodeLocation(payload.sourceURL, payload.sourceLine);
>+            sourceCodeLocation = WI.DOMNodeStyles.createSourceCodeLocation(payload.sourceURL, {
>+                line: payload.sourceLine,
>+                documentNode: this._node.ownerDocument,
>+            });
>         }
> 
>         if (styleSheet) {
>@@ -766,7 +775,7 @@ WI.DOMNodeStyles = class DOMNodeStyles extends WI.Object
>             var mediaItem = payload.media[i];
>             var mediaType = WI.CSSManager.protocolMediaSourceToEnum(mediaItem.source);
>             var mediaText = mediaItem.text;
>-            var mediaSourceCodeLocation = this._createSourceCodeLocation(mediaItem.sourceURL, mediaItem.sourceLine);
>+            var mediaSourceCodeLocation = WI.DOMNodeStyles.createSourceCodeLocation(mediaItem.sourceURL, {line: mediaItem.sourceLine});
>             if (styleSheet)
>                 mediaSourceCodeLocation = styleSheet.offsetSourceCodeLocation(mediaSourceCodeLocation);
> 
>diff --git a/Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.js b/Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.js
>index d4f7dbc4eefb8e4e71a85ef55710a344056caf95..238ebe323e845b618a7d4d1af19e36ceaad4f60a 100644
>--- a/Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.js
>+++ b/Source/WebInspectorUI/UserInterface/Views/SpreadsheetCSSStyleDeclarationSection.js
>@@ -167,6 +167,7 @@ WI.SpreadsheetCSSStyleDeclarationSection = class SpreadsheetCSSStyleDeclarationS
>         if (!selectorText || selectorText === this._style.ownerRule.selectorText)
>             this._discardSelectorChange();
>         else {
>+            this.dispatchEventToListeners(WI.SpreadsheetCSSStyleDeclarationSection.Event.SelectorWillChange);
>             this._style.ownerRule.singleFireEventListener(WI.CSSRule.Event.SelectorChanged, this._renderSelector, this);
>             this._style.ownerRule.selectorText = selectorText;
>         }
>@@ -289,18 +290,12 @@ WI.SpreadsheetCSSStyleDeclarationSection = class SpreadsheetCSSStyleDeclarationS
> 
>             var selectors = this._style.ownerRule.selectors;
>             var matchedSelectorIndices = this._style.ownerRule.matchedSelectorIndices;
>-            var alwaysMatch = !matchedSelectorIndices.length;
>             if (selectors.length) {
>-                let hasMatchingPseudoElementSelector = false;
>                 for (let i = 0; i < selectors.length; ++i) {
>-                    appendSelector(selectors[i], alwaysMatch || matchedSelectorIndices.includes(i));
>+                    appendSelector(selectors[i], matchedSelectorIndices.includes(i));
>                     if (i < selectors.length - 1)
>                         this._selectorElement.append(", ");
>-
>-                    if (matchedSelectorIndices.includes(i) && selectors[i].isPseudoElementSelector())
>-                        hasMatchingPseudoElementSelector = true;
>                 }
>-                this._element.classList.toggle("pseudo-element-selector", hasMatchingPseudoElementSelector);
>             } else
>                 appendSelectorTextKnownToMatch(this._style.ownerRule.selectorText);
> 
>@@ -499,6 +494,7 @@ WI.SpreadsheetCSSStyleDeclarationSection = class SpreadsheetCSSStyleDeclarationS
> 
> WI.SpreadsheetCSSStyleDeclarationSection.Event = {
>     FilterApplied: "spreadsheet-css-style-declaration-section-filter-applied",
>+    SelectorWillChange: "spreadsheet-css-style-declaration-section-selector-will-change",
> };
> 
> WI.SpreadsheetCSSStyleDeclarationSection.MatchedSelectorElementStyleClassName = "matched";
>diff --git a/Source/WebInspectorUI/UserInterface/Views/SpreadsheetRulesStyleDetailsPanel.js b/Source/WebInspectorUI/UserInterface/Views/SpreadsheetRulesStyleDetailsPanel.js
>index d8f0a5a4d259807ab9e8a3b93fbccdf8e46deaad..ad98bfa0164b42215c246ba42f7fb9e391077461 100644
>--- a/Source/WebInspectorUI/UserInterface/Views/SpreadsheetRulesStyleDetailsPanel.js
>+++ b/Source/WebInspectorUI/UserInterface/Views/SpreadsheetRulesStyleDetailsPanel.js
>@@ -209,22 +209,40 @@ WI.SpreadsheetRulesStyleDetailsPanel = class SpreadsheetRulesStyleDetailsPanel e
> 
>         this._shouldRefreshSubviews = false;
> 
>-        this.removeAllSubviews();
>+        let currentNode = this.nodeStyles.node;
>+        let oldSections = this._sections.slice();
>+        let preservedSections = oldSections.filter((section) => {
>+            if (section.__showingForNode !== currentNode) {
>+                delete section.__showingForNode;
>+                delete section.__index;
>+                return false;
>+            }
>+            return section.__index >= 0;
>+        });
>+
>+        if (preservedSections.length) {
>+            for (let section of oldSections) {
>+                if (!preservedSections.includes(section))
>+                    this.removeSubview(section);
>+            }
>+            for (let header of this._headerMap.values())
>+                header.remove();
>+        } else
>+            this.removeAllSubviews();
> 
>         let previousStyle = null;
>+        let currentHeader = null;
>         this._headerMap.clear();
>         this._sections = [];
> 
>         let createHeader = (text, node) => {
>-            let header = this.element.appendChild(document.createElement("h2"));
>-            header.classList.add("section-header");
>-            header.append(text);
>-            header.append(" ", WI.linkifyNodeReference(node, {
>+            currentHeader = this.element.appendChild(document.createElement("h2"));
>+            currentHeader.classList.add("section-header");
>+            currentHeader.append(text);
>+            currentHeader.append(" ", WI.linkifyNodeReference(node, {
>                 maxLength: 100,
>                 excludeRevealElement: true,
>             }));
>-
>-            this._headerMap.set(node, header);
>         };
> 
>         let createSection = (style) => {
>@@ -235,30 +253,58 @@ WI.SpreadsheetRulesStyleDetailsPanel = class SpreadsheetRulesStyleDetailsPanel e
>             }
> 
>             section.addEventListener(WI.SpreadsheetCSSStyleDeclarationSection.Event.FilterApplied, this._handleSectionFilterApplied, this);
>+            section.addEventListener(WI.SpreadsheetCSSStyleDeclarationSection.Event.SelectorWillChange, this._handleSectionSelectorWillChange, this);
> 
>             if (this._newRuleSelector === style.selectorText && !style.hasProperties())
>                 section.startEditingRuleSelector();
> 
>-            this.addSubview(section);
>-            section.needsLayout();
>-            this._sections.push(section);
>+            addSection(section);
> 
>-            previousStyle = style;
>+            let preservedSection = preservedSections.find((sectionToPreserve) => sectionToPreserve.__index === this._sections.length - 1);
>+            if (preservedSection)
>+                addSection(preservedSection);
>         };
> 
>-        for (let style of this.nodeStyles.uniqueOrderedStyles) {
>-            if (style.inherited && (!previousStyle || previousStyle.node !== style.node))
>-                createHeader(WI.UIString("Inherited From"), style.node);
>+        let addSection = (section) => {
>+            if (this._sections.some((existingSection) => Object.shallowEqual(section.style.id, existingSection.style.id))) {
>+                this.removeSubview(section);
>+                return;
>+            }
> 
>+            if (!previousStyle || previousStyle.node !== section.style.node) {
>+                if (section.style.node.isPseudoElement())
>+                    createHeader(WI.UIString("Pseudo Element"), section.style.node);
>+                else if (section.style.inherited)
>+                    createHeader(WI.UIString("Inherited From"), section.style.node);
>+            }
>+
>+            if (!section.isDescendantOf(this)) {
>+                let referenceView = this.subviews[this._sections.length];
>+                if (!referenceView || referenceView.__index === this._sections.length)
>+                    this.addSubview(section);
>+                else
>+                    this.insertSubviewBefore(section, referenceView);
>+            }
>+
>+            this._sections.push(section);
>+
>+            previousStyle = section.style;
>+            if (currentHeader)
>+                this._headerMap.set(section.style, currentHeader);
>+
>+            section.needsLayout();
>+        };
>+
>+        for (let style of this.nodeStyles.uniqueOrderedStyles)
>             createSection(style);
>-        }
> 
>-        let pseudoElements = Array.from(this.nodeStyles.node.pseudoElements().values());
>+        let pseudoElements = Array.from(currentNode.pseudoElements().values());
>         Promise.all(pseudoElements.map((pseudoElement) => WI.cssManager.stylesForNode(pseudoElement).refreshIfNeeded()))
>         .then((pseudoNodeStyles) => {
>-            for (let pseudoNodeStyle of pseudoNodeStyles) {
>-                createHeader(WI.UIString("Pseudo Element"), pseudoNodeStyle.node);
>+            if (this.nodeStyles.node !== currentNode)
>+                return;
> 
>+            for (let pseudoNodeStyle of pseudoNodeStyles) {
>                 for (let style of pseudoNodeStyle.uniqueOrderedStyles)
>                     createSection(style);
>             }
>@@ -288,12 +334,18 @@ WI.SpreadsheetRulesStyleDetailsPanel = class SpreadsheetRulesStyleDetailsPanel e
> 
>         this.element.classList.remove("filter-non-matching");
> 
>-        let header = this._headerMap.get(event.target.style.node);
>+        let header = this._headerMap.get(event.target.style);
>         if (header)
>             header.classList.remove(WI.GeneralStyleDetailsSidebarPanel.NoFilterMatchInSectionClassName);
>     }
> 
>-    // Private
>+    _handleSectionSelectorWillChange(event)
>+    {
>+        let section = event.target;
>+        section.__showingForNode = this.nodeStyles.node;
>+        section.__index = this._sections.indexOf(section);
>+        console.assert(section.__index >= 0);
>+    }
> 
>     _addNewRule(stylesheetId)
>     {

Actions: View | Formatted Diff | Diff

Attachments on bug 178489: 329468 | 335759 | 335764 | 340634 | 340679 | 356749 | 357995 | 358002 | 358003 | 360915 | 365415 | 365434 | 365436