On Wed, 18 Oct 2023 03:45:32 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 test failing issues and code review comments Another question: this PR changes the behavior of Node.lookup() and Node.lookupAll() in respect to pseudo classes. Now, wouldn't that pose a regression risk for applications? Should we create a new method with the new semantics instead (+ boolean considerPseudoClasses or some such)? modules/javafx.graphics/src/main/java/javafx/scene/Node.java line 1993: > 1991: */ > 1992: List<Node> lookupAll(Selector selector, List<Node> results) { > 1993: if (selectorMatches(selector)) { line 1990 might be incorrect: the return value **can** be null. We probably should just correct the javadoc, and since it's a private API no CSR is needed. All callers of this method do check for null. modules/javafx.graphics/src/test/java/test/javafx/scene/Node_lookup_Test.java line 108: > 106: > 107: @Test > 108: public void lookupPsuedoTest(){ spelling: lookupPseudoTest (also in the bug and PR title) ------------- PR Comment: https://git.openjdk.org/jfx/pull/1245#issuecomment-1769133936 PR Review Comment: https://git.openjdk.org/jfx/pull/1245#discussion_r1364317319 PR Review Comment: https://git.openjdk.org/jfx/pull/1245#discussion_r1364312507