On Wed, 6 Jul 2022 22:17:52 GMT, Kevin Rushforth <k...@openjdk.org> wrote:
>> 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/7cf8f2d0...0c7d6e72 > > 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? Yes, that's correct. A node that is removed from a scene graph cannot be focused, which also means that it can't have focusWithin. > 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. I removed the cast by making the `Node.focusVisible` field package-private. That should make it clearer that the property is actually set from outside the `Node` class. ------------- PR: https://git.openjdk.org/jfx/pull/475
