Web Inspector: DOM.highlightSelector should work for "div, div::before"
authordrousso@apple.com <drousso@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 20 Nov 2019 04:31:20 +0000 (04:31 +0000)
committerdrousso@apple.com <drousso@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 20 Nov 2019 04:31:20 +0000 (04:31 +0000)
https://bugs.webkit.org/show_bug.cgi?id=204306

Reviewed by Brian Burg.

.:

* ManualTests/inspector/overlay-selectors.html: Added.

Source/WebCore:

In r252436, the implementation of `DOM.highlightSelector` was changed from just calling
`document.querySelectorAll` to actually attempting to mimic what the CSS selector matching
engine does. Basically, this meant adding logic to walk the entire DOM tree and for each
node test each `CSSSelector` of the given `selector` string to see if it matched.

At the time, I had incorrectly assumed that once a selector was found that matched the
current node, it wouldn't need to be checked against ever again. This would be a fine
assumption if we didn't care about `:before`/`:after`, but since `DOM.highlightSelector`
also wants to match those, it is necessary to test every `CSSSelector` in case a later one
in the given `selector` string matches a pseudo-element (e.g. `div, div:before`).

The fix is simply to change `break` to `continue` and to ensure that every item in the
generated `NodeList` is unique (otherwise the overlay for a node may be drawn twice).

* inspector/agents/InspectorDOMAgent.cpp:
(WebCore::InspectorDOMAgent::highlightSelector):

git-svn-id: http://svn.webkit.org/repository/webkit/trunk@252682 268f45cc-cd09-0410-ab3c-d52691b4dbfc

ChangeLog
ManualTests/inspector/overlay-selectors.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/inspector/agents/InspectorDOMAgent.cpp

index eda5d7d..4f5188f 100644 (file)
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,12 @@
+2019-11-19  Devin Rousso  <drousso@apple.com>
+
+        Web Inspector: DOM.highlightSelector should work for "div, div::before"
+        https://bugs.webkit.org/show_bug.cgi?id=204306
+
+        Reviewed by Brian Burg.
+
+        * ManualTests/inspector/overlay-selectors.html: Added.
+
 2019-11-12  Carlos Alberto Lopez Perez  <clopez@igalia.com>
 
         [GTK][WPE] Support Pointer Events
diff --git a/ManualTests/inspector/overlay-selectors.html b/ManualTests/inspector/overlay-selectors.html
new file mode 100644 (file)
index 0000000..1fd9cb2
--- /dev/null
@@ -0,0 +1,44 @@
+<style>
+p {
+    position: fixed;
+    right: 20px;
+    bottom: 20px;
+    margin: 0;
+}
+
+.expected {
+    position: absolute;
+    width: 300px;
+    height: 100px;
+    background-image: linear-gradient(to right, rgba(111, 168, 220, 0.66) 33.3%, transparent 33.3%, transparent 66.6%, rgba(111, 168, 220, 0.66) 66.6%);
+}
+.actual, .actual::before {
+    display: inline-block;
+    width: 100px;
+    height: 100px;
+}
+.actual {
+    position: relative;
+}
+.actual::before {
+    position: absolute;
+    left: 200px;
+    content: "";
+}
+</style>
+<body>
+    <div id="test1">
+        <div class="expected" hidden></div>
+        <div class="actual"></div>
+    </div>
+
+    <p>Inspect this page and hover the `<code>.actual, .actual::before</code>` CSS selector in the Styles panel of the details sidebar in the Elements tab.  Click <button>Show Expected</button> to compare with the expected result.</p>
+    <script>
+let showingExpected = false;
+document.querySelector("button").addEventListener("click", (event) => {
+    showingExpected = !showingExpected;
+    for (let node of document.querySelectorAll(".expected"))
+        node.hidden = !showingExpected;
+});
+    </script>
+</body>
index 39ceebe..8d44968 100644 (file)
@@ -1,3 +1,27 @@
+2019-11-19  Devin Rousso  <drousso@apple.com>
+
+        Web Inspector: DOM.highlightSelector should work for "div, div::before"
+        https://bugs.webkit.org/show_bug.cgi?id=204306
+
+        Reviewed by Brian Burg.
+
+        In r252436, the implementation of `DOM.highlightSelector` was changed from just calling
+        `document.querySelectorAll` to actually attempting to mimic what the CSS selector matching
+        engine does. Basically, this meant adding logic to walk the entire DOM tree and for each
+        node test each `CSSSelector` of the given `selector` string to see if it matched.
+
+        At the time, I had incorrectly assumed that once a selector was found that matched the
+        current node, it wouldn't need to be checked against ever again. This would be a fine
+        assumption if we didn't care about `:before`/`:after`, but since `DOM.highlightSelector`
+        also wants to match those, it is necessary to test every `CSSSelector` in case a later one
+        in the given `selector` string matches a pseudo-element (e.g. `div, div:before`).
+
+        The fix is simply to change `break` to `continue` and to ensure that every item in the
+        generated `NodeList` is unique (otherwise the overlay for a node may be drawn twice).
+
+        * inspector/agents/InspectorDOMAgent.cpp:
+        (WebCore::InspectorDOMAgent::highlightSelector):
+
 2019-11-19  Youenn Fablet  <youenn@apple.com>
 
         getUserMedia echoCancellation constraint has no affect
index c23edb6..aecc79b 100644 (file)
@@ -1260,7 +1260,8 @@ void InspectorDOMAgent::highlightSelector(ErrorString& errorString, const JSON::
 
     SelectorChecker selectorChecker(*document);
 
-    Vector<Ref<Node>> nodes;
+    Vector<Ref<Node>> nodeList;
+    HashSet<Node*> seenNodes;
 
     for (auto& descendant : composedTreeDescendants(*document)) {
         if (!is<Element>(descendant))
@@ -1281,8 +1282,8 @@ void InspectorDOMAgent::highlightSelector(ErrorString& errorString, const JSON::
 
             unsigned ignoredSpecificity;
             if (selectorChecker.match(*selector, descendantElement, context, ignoredSpecificity)) {
-                nodes.append(descendantElement);
-                break;
+                if (seenNodes.add(&descendantElement))
+                    nodeList.append(descendantElement);
             }
 
             if (context.pseudoIDSet) {
@@ -1290,25 +1291,29 @@ void InspectorDOMAgent::highlightSelector(ErrorString& errorString, const JSON::
 
                 if (pseudoIDs.has(PseudoId::Before)) {
                     pseudoIDs.remove(PseudoId::Before);
-                    if (auto* beforePseudoElement = descendantElement.beforePseudoElement())
-                        nodes.append(*beforePseudoElement);
+                    if (auto* beforePseudoElement = descendantElement.beforePseudoElement()) {
+                        if (seenNodes.add(beforePseudoElement))
+                            nodeList.append(*beforePseudoElement);
+                    }
                 }
 
                 if (pseudoIDs.has(PseudoId::After)) {
                     pseudoIDs.remove(PseudoId::After);
-                    if (auto* afterPseudoElement = descendantElement.afterPseudoElement())
-                        nodes.append(*afterPseudoElement);
+                    if (auto* afterPseudoElement = descendantElement.afterPseudoElement()) {
+                        if (seenNodes.add(afterPseudoElement))
+                            nodeList.append(*afterPseudoElement);
+                    }
                 }
 
                 if (pseudoIDs) {
-                    nodes.append(descendantElement);
-                    break;
+                    if (seenNodes.add(&descendantElement))
+                        nodeList.append(descendantElement);
                 }
             }
         }
     }
 
-    m_overlay->highlightNodeList(StaticNodeList::create(WTFMove(nodes)), *highlightConfig);
+    m_overlay->highlightNodeList(StaticNodeList::create(WTFMove(nodeList)), *highlightConfig);
 }
 
 void InspectorDOMAgent::highlightNode(ErrorString& errorString, const JSON::Object& highlightInspectorObject, const int* nodeId, const String* objectId)