Skip to content
Permalink
Browse files
8244075: Accelerator of ContextMenu's MenuItem is not removed when Co…
…ntextMenu is removed from Scene

Reviewed-by: kcr, aghaisas
  • Loading branch information
arapte committed Jun 11, 2021
1 parent e6cf1df commit 0ffa8e2824d64b585363ae7a1a51890e1f5b9000
@@ -75,21 +75,19 @@ public static void addAcceleratorsIntoScene(ObservableList<MenuItem> items, Node
}

final Scene scene = anchor.getScene();
if (scene == null) {
// listen to the scene property on the anchor until it is set, and
// then install the accelerators
anchor.sceneProperty().addListener(new InvalidationListener() {
@Override public void invalidated(Observable observable) {
Scene scene = anchor.getScene();
if (scene != null) {
anchor.sceneProperty().removeListener(this);
doAcceleratorInstall(items, scene);
}
}
});
} else {
if (scene != null) {
doAcceleratorInstall(items, scene);
}
// Scene change listener is added to the anchor for scenarios like,
// 1. Installing accelerators when Control is added to Scene
// 2. Removing accelerators when Control is removed from Scene
// Remove previously added listener if any
if (sceneChangeListenerMap.containsKey(anchor)) {
anchor.sceneProperty().removeListener(sceneChangeListenerMap.get(anchor));
sceneChangeListenerMap.remove(anchor);
}
// Add a new listener
anchor.sceneProperty().addListener(getSceneChangeListener(anchor, items));
}

private static void addAcceleratorsIntoScene(ObservableList<MenuItem> items, Object anchor) {
@@ -115,6 +113,24 @@ private static void addAcceleratorsIntoScene(ObservableList<MenuItem> items, Obj
}
}

private static Map<Object, ChangeListener<Scene>> sceneChangeListenerMap = new WeakHashMap<>();

private static ChangeListener<Scene> getSceneChangeListener(Object anchor, ObservableList<MenuItem> items) {
ChangeListener<Scene> sceneChangeListener = sceneChangeListenerMap.get(anchor);
if (sceneChangeListener == null) {
sceneChangeListener = (ov, oldScene, newScene) -> {
if (oldScene != null) {
removeAcceleratorsFromScene(items, oldScene);
}
if (newScene != null) {
doAcceleratorInstall(items, newScene);
}
};
sceneChangeListenerMap.put(anchor, sceneChangeListener);
}
return sceneChangeListener;
}

private static void doAcceleratorInstall(final ObservableList<MenuItem> items, final Scene scene) {
// we're given an observable list of menu items, which we will add an observer to
// so that when menu items are added or removed we can properly handle
@@ -222,6 +238,14 @@ public static void removeAcceleratorsFromScene(List<? extends MenuItem> items, T

public static void removeAcceleratorsFromScene(List<? extends MenuItem> items, Node anchor) {
Scene scene = anchor.getScene();
if (scene == null) {
// The Node is not part of a Scene: Remove the Scene listener that was added
// at the time of installing the accelerators.
if (sceneChangeListenerMap.containsKey(anchor)) {
anchor.sceneProperty().removeListener(sceneChangeListenerMap.get(anchor));
sceneChangeListenerMap.remove(anchor);
}
}
removeAcceleratorsFromScene(items, scene);
}

@@ -25,6 +25,7 @@

package test.javafx.scene.control;

import static test.com.sun.javafx.scene.control.infrastructure.ControlTestUtils.*;
import test.com.sun.javafx.scene.control.infrastructure.KeyEventFirer;
import test.com.sun.javafx.scene.control.infrastructure.KeyModifier;
import test.com.sun.javafx.scene.control.infrastructure.StageLoader;
@@ -34,12 +35,14 @@
import javafx.scene.input.KeyCombination;
import org.junit.After;
import org.junit.Before;
import org.junit.Ignore;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.Parameterized;

import java.util.Arrays;
import java.util.Collection;
import javafx.scene.Group;
import javafx.scene.control.Button;
import javafx.scene.control.ContextMenu;
import javafx.scene.control.Menu;
@@ -297,4 +300,77 @@ private void assertSceneDoesNotContainKeyCombination(KeyCombination keyCombinati
keyboard.doKeyPress(KeyCode.DIGIT1, KeyModifier.ALT);
assertEquals(0, eventCounter);
}

@Test public void testAcceleratorShouldNotGetFiredWhenMenuItemRemovedFromScene() {
KeyEventFirer kb = new KeyEventFirer(item1, scene);

kb.doKeyPress(KeyCode.DIGIT1, KeyModifier.ALT);
assertEquals(1, eventCounter);
assertEquals(1, getListenerCount(item1.acceleratorProperty()));

// Remove all children from the scene
Group root = (Group)scene.getRoot();
root.getChildren().clear();

assertEquals(0, getListenerCount(item1.acceleratorProperty()));
kb.doKeyPress(KeyCode.DIGIT1, KeyModifier.ALT);
assertEquals(1, eventCounter);
}

@Test public void testAcceleratorShouldGetFiredWhenMenuItemRemovedAndAddedBackToScene() {
KeyEventFirer kb = new KeyEventFirer(item1, scene);

kb.doKeyPress(KeyCode.DIGIT1, KeyModifier.ALT);
assertEquals(1, eventCounter);

Group root = (Group)scene.getRoot();
scene.setRoot(new Group()); // Remove all children from the scene
scene.setRoot(root); // Add the children to the same scene

assertEquals(1, getListenerCount(item1.acceleratorProperty()));
kb.doKeyPress(KeyCode.DIGIT1, KeyModifier.ALT);
assertEquals(2, eventCounter);
}

@Test public void testAcceleratorShouldGetFiredWhenMenuItemRemovedAndAddedToDifferentScene() {
KeyEventFirer kb = new KeyEventFirer(item1, scene);

kb.doKeyPress(KeyCode.DIGIT1, KeyModifier.ALT);
assertEquals(1, eventCounter);

Group root = (Group)scene.getRoot();
scene.setRoot(new Group()); // Remove all children from the scene
Scene diffScene = new Scene(root); // Add the children to a different scene
sl.getStage().setScene(diffScene);
kb = new KeyEventFirer(item1, diffScene);

assertEquals(1, getListenerCount(item1.acceleratorProperty()));
kb.doKeyPress(KeyCode.DIGIT1, KeyModifier.ALT);
assertEquals(2, eventCounter);
}

@Ignore("JDK-8268374")
@Test public void testAcceleratorShouldNotGetFiredWhenControlsIsRemovedFromSceneThenContextMenuIsSetToNullAndControlIsAddedBackToScene() {
KeyEventFirer kb = new KeyEventFirer(item1, scene);
kb.doKeyPress(KeyCode.DIGIT1, KeyModifier.ALT);
assertEquals(1, eventCounter);

Group root = (Group)scene.getRoot();
scene.setRoot(new Group()); // Remove all children from the scene

if (testClass == Button.class) {
btn.setContextMenu(null);
} else if (testClass == Tab.class) {
tab.setContextMenu(null);
} else if (testClass == TableColumn.class) {
tableColumn.setContextMenu(null);
} else if (testClass == TreeTableColumn.class) {
treeTableColumn.setContextMenu(null);
}
scene.setRoot(root); // Add the children to a different scene

assertEquals(0, getListenerCount(item1.acceleratorProperty()));
kb.doKeyPress(KeyCode.DIGIT1, KeyModifier.ALT);
assertEquals(1, eventCounter);
}
}

1 comment on commit 0ffa8e2

@openjdk-notifier

This comment has been minimized.

Copy link

@openjdk-notifier openjdk-notifier bot commented on 0ffa8e2 Jun 11, 2021

Please sign in to comment.