On Thu, 19 Oct 2023 00:23:18 GMT, Sai Pradeep Dandem <d...@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
