On Thu, 19 May 2022 04:03:46 GMT, Michael Strauß <mstra...@openjdk.org> wrote:
>> This PR adds the `Node.focusVisible` and `Node.focusWithin` properties, as >> well as the corresponding `:focus-visible` and `:focus-within` CSS >> pseudo-classes. > > Michael Strauß has updated the pull request with a new target base due to a > merge or a rebase. The incremental webrev excludes the unrelated changes > brought in by the merge/rebase. The pull request contains 23 additional > commits since the last revision: > > - Merge branch 'master' into feature/focusvisible > - Updated since tag > - Update copyright headers > - Merge branch 'master' into feature/focusvisible > - fixed undeterministic test failures > - minor wording change > - restart github actions > - Merge branch 'master' of https://github.com/mstr2/jfx into > feature/focusvisible > - changes per discussion, added test > - wording > - ... and 13 more: https://git.openjdk.org/jfx/compare/b608ace8...0c7d6e72 The API looks good. I also reviewed the implementation, and it seems fine. I left a few comments inline. If you still want to get this into JavaFX 19, can you update the CSR as follows? 1. Change the `@since` tags from 18 to 19 2. For the two new properties, please add the two public methods for each property, replacing the "private" field (if you prefer, you can leave the private field as well). 3. The diffs for `cssref.html` need to be added to the Specification section. Once you do that, I will "Review" the CSR, since I am done reviewing the API changes in this PR. You can then Finalize it. modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/behavior/ButtonBehavior.java line 181: > 179: */ > 180: protected void mousePressed(MouseEvent e) { > 181: if (getNode().isFocusTraversable()) { The old code used to skip the call to `requestFocus()` if the node was already `focused` (here and in a couple other cases), in order to correctly update the `focusVisible` state. This is probably fine; can you think of any possible side effects of this? modules/javafx.graphics/src/main/java/javafx/scene/Node.java line 8191: > 8189: */ > 8190: private void updateRemovedParentFocus(Node oldParent) { > 8191: if (oldParent != null && focusWithin.get()) { I see that you aren't calling `this.focusWithin.set(false)`. Is that because in the case of the node that was removed from its parent, it will have its `focused` property set to false (as a result of it being removed from the scene), which will then set its `focusWithin` to false? modules/javafx.graphics/src/main/java/javafx/scene/Scene.java line 4056: > 4054: > 4055: private void setFocusVisible(Node node, boolean focusVisible) { > 4056: Node.FocusPropertyBase property = > (Node.FocusPropertyBase)node.focusVisibleProperty(); Did you consider using a similar pattern to `ReadOnlyBooleanWrapper` to avoid the cast? Given that an application can't call the set method (either by casting or by using reflection), I don't see a problem with this, so it is just a question. modules/javafx.graphics/src/test/java/test/javafx/scene/FocusTest.java line 844: > 842: > 843: node2.requestFocus(); > 844: You might also check that node2 is focused and not focusVisible. ------------- PR: https://git.openjdk.org/jfx/pull/475
