RFR: 8185831: Psuedo selectors do not appear to work in Node.lookupAll() [v4]

[Andy Goryachev]

On Thu, 19 Oct 2023 00:23:18 GMT, Sai Pradeep Dandem <duke at openjdk.org> wrote:

>> **Issue:**
>> Using pseudo classes in programmatic query using Node.lookupAll() or Node.lookup() gives unexpected results.
>> 
>> **Cause:**
>> There is no check for checking the psuedo states matching in the applies() method of SimpleSelector.java. So checking for "applies()" alone is not sufficient in lookup() method.
>> 
>> **Fix:**
>> Included an extra check for the psuedo states to match.
>
> Sai Pradeep Dandem has updated the pull request incrementally with one additional commit since the last revision:
> 
>   8185831: Fixed code review comments

On second though, I think this change will not cause regression: when no pseudo class is specified in the lookup selector, the lookup ignores the pseudo classes as before.

What I strongly recommend is to clarify the behavior of lookup() and lookupAll() in their javadoc (which might require a CSR) with respect to pseudo classes.

modules/javafx.graphics/src/test/java/test/javafx/scene/Node_lookup_Test.java line 114:

> 112: 
> 113:     @Test
> 114:     public void lookupPseudoTest(){

please insert space

public void lookupPseudoTest() {

modules/javafx.graphics/src/test/java/test/javafx/scene/Node_lookup_Test.java line 136:

> 134:         assertEquals(2, nodes.size());
> 135:         assertTrue(nodes.contains(g));
> 136:         assertTrue(nodes.contains(hg));

I'd add one more test case to verify lack of regression when no pseudo class is specified.  May be something like this (feel free to expand, add negative cases etc.):


    /**
     * Verifies that the lookup ignores pseudo classes when selector contains no explicit pseudo class.
     */
    @Test
    public void lookupPseudoTest2() {
        Group root = new Group();
        
        Group ab = new Group();
        ab.getStyleClass().addAll("a", "b");
        
        Group a = new Group();
        a.getStyleClass().addAll("a");
        
        Group a1 = new Group();
        a1.getStyleClass().addAll("a");
        a1.pseudoClassStateChanged(PseudoClass.getPseudoClass("P1"), true);
        
        ParentShim.getChildren(root).addAll(a, a1, ab);
        
        Set<Node> rv;
        
        rv = root.lookupAll(".a");
        assertTrue(rv.contains(a));
        assertTrue(rv.contains(a1));
        
        rv = root.lookupAll(".a:P1");
        assertTrue(rv.contains(a1));
    }

-------------

PR Review: https://git.openjdk.org/jfx/pull/1245#pullrequestreview-1688051352
PR Review Comment: https://git.openjdk.org/jfx/pull/1245#discussion_r1365737158
PR Review Comment: https://git.openjdk.org/jfx/pull/1245#discussion_r1365741530