8242489: ChoiceBox: initially toggle not sync'ed to selection

Reviewed-by: aghaisas, arapte
openjdk · Apr 20, 2020 · 69e4266 · 69e4266
1 parent 1d88180
commit 69e4266
Show file tree

Hide file tree

Showing 5 changed files with 354 additions and 28 deletions.
diff --git a/modules/javafx.controls/src/main/java/javafx/scene/control/ChoiceBox.java b/modules/javafx.controls/src/main/java/javafx/scene/control/ChoiceBox.java
@@ -182,6 +182,7 @@ public ChoiceBox(ObservableList<T> items) {
             oldSM = sm;
             if (sm != null) {
                 sm.selectedItemProperty().addListener(selectedItemListener);
+                // FIXME JDK-8242001 - must sync to model state always
                 if (sm.getSelectedItem() != null && ! valueProperty().isBound()) {
                     ChoiceBox.this.setValue(sm.getSelectedItem());
                 }

diff --git a/modules/javafx.controls/src/main/java/javafx/scene/control/skin/ChoiceBoxSkin.java b/modules/javafx.controls/src/main/java/javafx/scene/control/skin/ChoiceBoxSkin.java
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2010, 2016, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2010, 2020, 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
@@ -28,9 +28,6 @@
 import com.sun.javafx.scene.control.ContextMenuContent;
 import com.sun.javafx.scene.control.behavior.BehaviorBase;
 import javafx.beans.WeakInvalidationListener;
-import javafx.scene.Node;
-import javafx.scene.control.Accordion;
-import javafx.scene.control.Button;
 import javafx.scene.control.Control;
 import javafx.scene.control.SkinBase;
 import javafx.util.StringConverter;
@@ -156,20 +153,11 @@ public ChoiceBoxSkin(ChoiceBox<T> control) {
         registerChangeListener(control.selectionModelProperty(), e -> updateSelectionModel());
         registerChangeListener(control.showingProperty(), e -> {
             if (getSkinnable().isShowing()) {
-                MenuItem item = null;
 
-                SelectionModel sm = getSkinnable().getSelectionModel();
+                SelectionModel<T> sm = getSkinnable().getSelectionModel();
                 if (sm == null) return;
 
                 long currentSelectedIndex = sm.getSelectedIndex();
-                int itemInControlCount = choiceBoxItems.size();
-                boolean hasSelection = currentSelectedIndex >= 0 && currentSelectedIndex < itemInControlCount;
-                if (hasSelection) {
-                    item = popup.getItems().get((int) currentSelectedIndex);
-                    if (item != null && item instanceof RadioMenuItem) ((RadioMenuItem)item).setSelected(true);
-                } else {
-                    if (itemInControlCount > 0) item = popup.getItems().get(0);
-                }
 
                 // This is a fix for RT-9071. Ideally this won't be necessary in
                 // the long-run, but for now at least this resolves the
@@ -201,15 +189,6 @@ public ChoiceBoxSkin(ChoiceBox<T> control) {
                 label.setText(""); // clear label text when selectedIndex is -1
             }
         });
-        registerChangeListener(control.getSelectionModel().selectedItemProperty(), e -> {
-            if (getSkinnable().getSelectionModel() != null) {
-                int index = getSkinnable().getSelectionModel().getSelectedIndex();
-                if (index != -1) {
-                    MenuItem item = popup.getItems().get(index);
-                    if (item instanceof RadioMenuItem) ((RadioMenuItem)item).setSelected(true);
-                }
-            }
-        });
         registerChangeListener(control.converterProperty(), e -> {
             updateChoiceBoxItems();
             updatePopupItems();
@@ -364,6 +343,11 @@ String getChoiceBoxSelectedText() {
         return label.getText();
     }
 
+    // Test only purpose
+    ContextMenu getChoiceBoxPopup() {
+        return popup;
+    }
+
     private void addPopupItem(final T o, int i) {
         MenuItem popupItem = null;
         if (o instanceof Separator) {
@@ -428,6 +412,9 @@ private void updateSelection() {
                 MenuItem selectedItem = popup.getItems().get(selectedIndex);
                 if (selectedItem instanceof RadioMenuItem) {
                     ((RadioMenuItem) selectedItem).setSelected(true);
+                } else {
+                    // need to unselect toggles if selectionModel allows a Separator/MenuItem
+                    // to be selected
                     toggleGroup.selectToggle(null);
                 }
                 // update the label

diff --git a/modules/javafx.controls/src/shims/java/javafx/scene/control/skin/ChoiceBoxSkinNodesShim.java b/modules/javafx.controls/src/shims/java/javafx/scene/control/skin/ChoiceBoxSkinNodesShim.java
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2011, 2015, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2011, 2020, 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
@@ -25,11 +25,17 @@
 
 package javafx.scene.control.skin;
 
+import javafx.scene.control.ContextMenu;
+
 public class ChoiceBoxSkinNodesShim {
 
     // can only access the getChoiceBoxSelectedText method in ChoiceBoxSkin
     // from this package.
     public static String getChoiceBoxSelectedText(ChoiceBoxSkin skin) {
         return skin.getChoiceBoxSelectedText();
     }
+
+    public static ContextMenu getChoiceBoxPopup(ChoiceBoxSkin skin) {
+        return skin.getChoiceBoxPopup();
+    }
 }