Changeset 252682 in webkit

Timestamp:

Nov 19, 2019 8:31:20 PM (4 years ago)

Author:

Devin Rousso

Message:

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.

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):

Location:

trunk

Files:

• ChangeLog (modified) (1 diff)
• ManualTests/inspector/overlay-selectors.html (added)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/inspector/agents/InspectorDOMAgent.cpp (modified) (4 diffs)

trunk/ChangeLog

@@ +1 @@
+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>
 

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+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>
 

trunk/Source/WebCore/inspector/agents/InspectorDOMAgent.cpp

@@ -1261 +1261 @@
     SelectorChecker selectorChecker(*document);
 
-    Vector<Ref<Node>> nodes;
+    Vector<Ref<Node>> nodeList;
+    HashSet<Node*> seenNodes;
 
     for (auto& descendant : composedTreeDescendants(*document)) {
@@ -1282 +1283 @@
             unsigned ignoredSpecificity;
             if (selectorChecker.match(*selector, descendantElement, context, ignoredSpecificity)) {
-                nodes.append(descendantElement);
-                break;
+                if (seenNodes.add(&descendantElement))
+                    nodeList.append(descendantElement);
             }
 
@@ -1291 +1292 @@
                 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);
                 }
             }
@@ -1309 +1314 @@
     }
 
-    m_overlay->highlightNodeList(StaticNodeList::create(WTFMove(nodes)), *highlightConfig);
+    m_overlay->highlightNodeList(StaticNodeList::create(WTFMove(nodeList)), *highlightConfig);
 }