8185831: Pseudo selectors do not appear to work in Node.lookupAll()

SaiPradeepDandem · Andy Goryachev · commit bb06b64c9e1c · 2023-11-01T22:46:05.000Z
Reviewed-by: kcr, angorya, jhendrikx
diff --git a/modules/javafx.graphics/src/main/java/javafx/scene/Node.java b/modules/javafx.graphics/src/main/java/javafx/scene/Node.java
@@ -1950,9 +1950,15 @@ private void updateDisabled() {
      * into the branch until it finds a match. If more than one sub-node matches the
      * specified selector, this function returns the first of them.
      * <p>
-     *     For example, if a Node is given the id of "myId", then the lookup method can
-     *     be used to find this node as follows: <code>scene.lookup("#myId");</code>.
-     * </p>
+     * If the lookup selector does not specify a pseudo class, the lookup will ignore pseudo class states;
+     * it will return the first matching node whether or not it contains pseudo classes.
+     * <p>
+     * For example, if a Node is given the id of "myId", then the lookup method can
+     * be used to find this node as follows: {@code scene.lookup("#myId");}.
+     * <p>
+     * For example, if two nodes, NodeA and NodeB, have the same style class "myStyle" and NodeA has
+     * a pseudo state "myPseudo", then to find NodeA, the lookup method can be used as follows:
+     * {@code scene.lookup(".myStyle:myPseudo");} or {@code scene.lookup(":myPseudo");}.
      *
      * @param selector The css selector of the node to find
      * @return The first node, starting from this {@code Node}, which matches
@@ -1961,13 +1967,23 @@ private void updateDisabled() {
     public Node lookup(String selector) {
         if (selector == null) return null;
         Selector s = Selector.createSelector(selector);
-        return s != null && s.applies(this) ? this : null;
+        return selectorMatches(s) ? this : null;
     }
 
     /**
      * Finds all {@code Node}s, including this one and any children, which match
      * the given CSS selector. If no matches are found, an empty unmodifiable set is
      * returned. The set is explicitly unordered.
+     * <p>
+     * If the lookupAll selector does not specify a pseudo class, the lookupAll will ignore pseudo class states;
+     * it will return all matching nodes whether or not the nodes contain pseudo classes.
+     * <p>
+     * For example, if there are multiple nodes with same style class "myStyle", then the lookupAll method can
+     * be used to find all these nodes as follows: {@code scene.lookupAll(".myStyle");}.
+     * <p>
+     * For example, if multiple nodes have same style class "myStyle" and few nodes have
+     * a pseudo state "myPseudo", then to find all nodes with "myPseudo" state, the lookupAll method can be used as follows:
+     * {@code scene.lookupAll(".myStyle:myPseudo");} or {@code scene.lookupAll(":myPseudo");}.
      *
      * @param selector The css selector of the nodes to find
      * @return All nodes, starting from and including this {@code Node}, which match
@@ -1986,11 +2002,12 @@ public Set<Node> lookupAll(String selector) {
      * Used by Node and Parent for traversing the tree and adding all nodes which
      * match the given selector.
      *
-     * @param selector The Selector. This will never be null.
-     * @param results The results. This will never be null.
+     * @param selector the css selector of the nodes to find
+     * @param results the results
+     * @return list of matching nodes
      */
     List<Node> lookupAll(Selector selector, List<Node> results) {
-        if (selector.applies(this)) {
+        if (selectorMatches(selector)) {
             // Lazily create the set to reduce some trash.
             if (results == null) {
                 results = new LinkedList<>();
@@ -2024,6 +2041,19 @@ public void toFront() {
         }
     }
 
+    /**
+     * Checks whether the provided selector matches the node with both styles and pseudo states.
+     * @param s selector to match
+     * @return {@code true} if the selector matches
+     */
+    private boolean selectorMatches(Selector s) {
+        boolean matches = s != null && s.applies(this);
+        if (matches && !s.createMatch().getPseudoClasses().isEmpty()) {
+            matches = s.stateMatches(this, this.getPseudoClassStates());
+        }
+        return matches;
+    }
+
     // TODO: need to verify whether this is OK to do starting from a node in
     // the scene graph other than the root.
     private void doCSSPass() {
diff --git a/modules/javafx.graphics/src/test/java/test/javafx/scene/Node_lookup_Test.java b/modules/javafx.graphics/src/test/java/test/javafx/scene/Node_lookup_Test.java
@@ -30,6 +30,8 @@
 import static org.junit.Assert.assertTrue;
 
 import java.util.Set;
+
+import javafx.css.PseudoClass;
 import javafx.scene.Group;
 import javafx.scene.Node;
 import javafx.scene.ParentShim;
@@ -39,11 +41,11 @@
 
 public class Node_lookup_Test {
     //                  Group & #root
-    //                /      \
-    //              #a      .b.c
-    //             /   \        \
-    //           .d    #e        .d
-    private Group root, a, bc, d, e, d2;
+    //                /      \        \
+    //              #a      .b.c       .f:testPseudo
+    //             /   \        \         /           \
+    //           .d    #e        .d   .g:testPseudo1   .h.g:testPseudo1:testPseudo2
+    private Group root, a, bc, d, e, d2, f, g, hg;
 
     @Before public void setup() {
         root = new Group();
@@ -58,12 +60,23 @@ public class Node_lookup_Test {
         bc.getStyleClass().addAll("b", "c");
         d2 = new Group();
         d2.getStyleClass().add("d");
-        ParentShim.getChildren(root).addAll(a, bc);
+        f = new Group();
+        f.getStyleClass().add("f");
+        f.pseudoClassStateChanged(PseudoClass.getPseudoClass("testPseudo"), true);
+        g = new Group();
+        g.getStyleClass().add("g");
+        g.pseudoClassStateChanged(PseudoClass.getPseudoClass("testPseudo1"), true);
+        hg = new Group();
+        hg.getStyleClass().addAll("h", "g");
+        hg.pseudoClassStateChanged(PseudoClass.getPseudoClass("testPseudo1"), true);
+        hg.pseudoClassStateChanged(PseudoClass.getPseudoClass("testPseudo2"), true);
+        ParentShim.getChildren(root).addAll(a, bc, f);
         ParentShim.getChildren(a).addAll(d, e);
         ParentShim.getChildren(bc).addAll(d2);
+        ParentShim.getChildren(f).addAll(g, hg);
     }
 
-    @Test public void quickTest() {
+    @Test public void test_lookup_on_nodes_without_pseudo_classes() {
         Node found = root.lookup("Group");
         assertSame(root, found);
 
@@ -86,7 +99,7 @@ public class Node_lookup_Test {
         assertSame(bc, found);
     }
 
-    @Test public void lookupAllTest() {
+    @Test public void test_lookupAll_on_nodes_without_pseudo_classes() {
         Set<Node> nodes = root.lookupAll("#a");
         assertEquals(1, nodes.size());
         assertTrue(nodes.contains(a));
@@ -96,4 +109,131 @@ public class Node_lookup_Test {
         assertTrue(nodes.contains(d));
         assertTrue(nodes.contains(d2));
     }
+
+    @Test
+    public void test_lookup_and_lookupAll_on_nodes_with_pseudo_classes() {
+        Set<Node> nodes = root.lookupAll(".h:testPseudo2");
+        assertEquals(1, nodes.size());
+        assertTrue(nodes.contains(hg));
+
+        Node found = root.lookup(".h:testPseudo2");
+        assertSame(hg, found);
+
+        found = root.lookup(":testPseudo2");
+        assertSame(hg, found);
+
+        found = root.lookup(".h:testPseudo1:testPseudo2");
+        assertSame(hg, found);
+
+        nodes = root.lookupAll(".g:testPseudo1");
+        assertEquals(2, nodes.size());
+        assertTrue(nodes.contains(g));
+        assertTrue(nodes.contains(hg));
+
+        nodes = root.lookupAll(":testPseudo1");
+        assertEquals(2, nodes.size());
+        assertTrue(nodes.contains(g));
+        assertTrue(nodes.contains(hg));
+
+        nodes = root.lookupAll(".f > .h:testPseudo1");
+        assertEquals(1, nodes.size());
+        assertTrue(nodes.contains(hg));
+
+        nodes = root.lookupAll(".f:testPseudo > .h:testPseudo1");
+        assertEquals(1, nodes.size());
+        assertTrue(nodes.contains(hg));
+
+        nodes = root.lookupAll(".f:randomPseudo");
+        assertEquals(0, nodes.size());
+    }
+
+    /**
+     * Verifies that the lookup ignores pseudo classes when selector contains no explicit pseudo class, but all the nodes have pseudo classes set to them.
+     */
+    @Test
+    public void test_lookupAll_on_nodes_with_pseudo_classes_ignoring_pseudo_classes_in_selector() {
+        // Except root node all the other nodes (f, g, hg) have pseudo classes set to them
+        Set<Node> nodes = root.lookupAll(".g");
+        assertEquals(2, nodes.size());
+        assertTrue(nodes.contains(g));
+        assertTrue(nodes.contains(hg));
+
+        nodes = root.lookupAll("#root .g");
+        assertEquals(2, nodes.size());
+        assertTrue(nodes.contains(g));
+        assertTrue(nodes.contains(hg));
+
+        nodes = root.lookupAll(".f .g");
+        assertEquals(2, nodes.size());
+        assertTrue(nodes.contains(g));
+        assertTrue(nodes.contains(hg));
+
+        nodes = root.lookupAll(".f .h");
+        assertEquals(1, nodes.size());
+        assertTrue(nodes.contains(hg));
+
+        nodes = root.lookupAll(".f > .h");
+        assertEquals(1, nodes.size());
+        assertTrue(nodes.contains(hg));
+
+        nodes = root.lookupAll(".h");
+        assertEquals(1, nodes.size());
+        assertTrue(nodes.contains(hg));
+
+        nodes = root.lookupAll("#root > .f");
+        assertEquals(1, nodes.size());
+        assertTrue(nodes.contains(f));
+
+        nodes = root.lookupAll("#root .f");
+        assertEquals(1, nodes.size());
+        assertTrue(nodes.contains(f));
+
+        nodes = root.lookupAll(".random");
+        assertEquals(0, nodes.size());
+
+        nodes = root.lookupAll(".random .h");
+        assertEquals(0, nodes.size());
+    }
+
+    /**
+     * Verifies that the lookup ignores pseudo classes when selector contains no explicit pseudo class.
+     */
+    @Test
+    public void test_lookupAll_on_nodes_with_same_style_and_different_pseudo_classes() {
+        Group root = new Group();
+        root.setId("root");
+
+        Group xy = new Group();
+        xy.getStyleClass().addAll("x", "y");
+
+        Group x = new Group();
+        x.getStyleClass().addAll("x");
+
+        Group x1 = new Group();
+        x1.getStyleClass().addAll("x");
+        x1.pseudoClassStateChanged(PseudoClass.getPseudoClass("pseudo"), true);
+
+        ParentShim.getChildren(root).addAll(x, x1, xy);
+
+        Set<Node> nodes = root.lookupAll(".x");
+        assertEquals(3, nodes.size());
+        assertTrue(nodes.contains(x));
+        assertTrue(nodes.contains(x1));
+        assertTrue(nodes.contains(xy));
+
+        nodes = root.lookupAll(".x:pseudo");
+        assertTrue(nodes.contains(x1));
+
+        nodes = root.lookupAll("#root .x");
+        assertEquals(3, nodes.size());
+        assertTrue(nodes.contains(x));
+        assertTrue(nodes.contains(x1));
+        assertTrue(nodes.contains(xy));
+
+        nodes = root.lookupAll("#root .x:pseudo");
+        assertTrue(nodes.contains(x1));
+
+        nodes = root.lookupAll(".x:random");
+        assertEquals(0, nodes.size());
+    }
 }
