Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
8300013: Node.focusWithin doesn't account for nested focused nodes
Reviewed-by: aghaisas, kcr
  • Loading branch information
Michael Strauß committed Jan 31, 2023
1 parent 05a67df commit a4bc9d1
Show file tree
Hide file tree
Showing 2 changed files with 89 additions and 16 deletions.
33 changes: 26 additions & 7 deletions modules/javafx.graphics/src/main/java/javafx/scene/Node.java
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2010, 2022, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2010, 2023, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
Expand Down Expand Up @@ -8189,13 +8189,13 @@ final void notifyFocusListeners() {
* If the current node has the focusWithin bit, we also need to clear the focusWithin bits of this
* node's parents. Note that a scene graph can have more than a single focused node, for example when
* a PopupWindow is used to present a branch of the scene graph. Since we need to preserve multi-level
* focus, we need to stop clearing the focusWithin bits if we encounter a parent node that is focused.
* focus, we need to adjust the focus-within count on all parents of the node.
*/
private void clearParentsFocusWithin(Node oldParent) {
if (oldParent != null && focusWithin.get()) {
Node node = oldParent;
while (node != null && !node.focused.get()) {
node.focusWithin.set(false);
while (node != null) {
node.focusWithin.adjust(-focusWithin.count);
node = node.getParent();
}

Expand Down Expand Up @@ -8237,11 +8237,13 @@ public void set(boolean value) {
if (get() != value) {
super.set(value);

int change = value ? 1 : -1;
Node node = Node.this;

do {
node.focusWithin.set(value);
node.focusWithin.adjust(change);
node = node.getParent();
} while (node != null && !node.focused.get());
} while (node != null);
}
}
};
Expand Down Expand Up @@ -8295,7 +8297,11 @@ public final ReadOnlyBooleanProperty focusVisibleProperty() {
* @defaultValue false
* @since 19
*/
private final FocusPropertyBase focusWithin = new FocusPropertyBase() {
private final FocusWithinProperty focusWithin = new FocusWithinProperty();

private class FocusWithinProperty extends FocusPropertyBase {
int count;

@Override
protected PseudoClass getPseudoClass() {
return FOCUS_WITHIN_PSEUDOCLASS_STATE;
Expand All @@ -8305,6 +8311,19 @@ protected PseudoClass getPseudoClass() {
public String getName() {
return "focusWithin";
}

/**
* Adjusts the number of focused nodes within this node.
*/
void adjust(int change) {
count += change;

if (count == 1) {
set(true);
} else if (count == 0) {
set(false);
}
}
};

public final boolean isFocusWithin() {
Expand Down
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2010, 2022, Oracle and/or its affiliates. All rights reserved.
* Copyright (c) 2010, 2023, Oracle and/or its affiliates. All rights reserved.
* DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
*
* This code is free software; you can redistribute it and/or modify it
Expand Down Expand Up @@ -1011,7 +1011,7 @@ private void fireTouchPressedEvent(EventTarget target) {
* cleared when a focused node is removed must only be cleared as long as we don't encounter
* another focused node up the tree.
*/
@Test public void testMultiLevelFocusWithinIsPreserved() {
@Test public void testMultiLevelFocusWithinIsPreservedWhenFocusedNodeIsRemoved() {
class N extends Group {
N(Node... children) {
super(children);
Expand All @@ -1023,32 +1023,86 @@ void setFocused() {
}
}

N node1, node2, node3, node4;
N node1, node2, node3, node4, node5;

scene.setRoot(
node1 = new N(
node2 = new N(
node3 = new N(
node4 = new N()
node1 = new N( // focusWithin=3
node2 = new N( // focusWithin=3, focused
node3 = new N( // focusWithin=2
node4 = new N( // focusWithin=2, focused
node5 = new N() // focusWithin=1, focused
)
)
)
));

node2.setFocused();
node4.setFocused();

// Remove node4 from the scene graph
node5.setFocused();

// Detach node4 from the scene graph:
// node1 focusWithin=1
// |--> node2 focusWithin=1, focused
// |--> node3
//
// node4 focusWithin=2, focused
// |--> node5 focusWithin=1, focused
//
node3.getChildren().clear();

assertIsFocusWithin(node1);
assertIsFocusWithin(node2);
assertNotFocusWithin(node3);
assertIsFocusWithin(node4);
assertIsFocusWithin(node5);

assertNotFocused(node1);
assertIsFocused(node2);
assertNotFocused(node3);
assertIsFocused(node4);
assertIsFocused(node5);
}

/**
* When a scene graph contains multiple nested focused nodes, the focusWithin bits that would
* be cleared when a focused node is de-focused must not be cleared when we have another
* focused downstream node.
*/
@Test public void testMultiLevelFocusWithinIsPreservedWhenIntermediateFocusedNodeIsDefocused() {
class N extends Group {
N(Node... children) { super(children); }
void _setFocused(boolean value) { setFocused(value); }
}

N node1, node2, node3, node4;

scene.setRoot(
node1 = new N(
node2 = new N(
node3 = new N(
node4 = new N()
)
)
));

node2._setFocused(true);
node4._setFocused(true);
assertIsFocusWithin(node1);
assertIsFocusWithin(node2);
assertIsFocusWithin(node3);
assertIsFocusWithin(node4);

node2._setFocused(false);
assertIsFocusWithin(node1);
assertIsFocusWithin(node2);
assertIsFocusWithin(node3);
assertIsFocusWithin(node4);

node4._setFocused(false);
assertNotFocusWithin(node1);
assertNotFocusWithin(node2);
assertNotFocusWithin(node3);
assertNotFocusWithin(node4);
}

}

1 comment on commit a4bc9d1

@openjdk-notifier
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please sign in to comment.