8244075: Accelerator of ContextMenu's MenuItem is not removed when ContextMenu is removed from Scene

Reviewed-by: kcr, aghaisas

Author: arapte

modules/javafx.controls/src/main/java/com/sun/javafx/scene/control/ControlAcceleratorSupport.java

@@ -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);
     }
 

modules/javafx.controls/src/test/java/test/javafx/scene/control/AcceleratorParameterizedTest.java

@@ -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);
+    }
 }