8245145: Spinner: throws IllegalArgumentException when replacing skin

Reviewed-by: aghaisas, kcr

Author: Andy Goryachev

modules/javafx.controls/src/main/java/javafx/scene/control/skin/SpinnerSkin.java

@@ -24,30 +24,31 @@
  */
 package javafx.scene.control.skin;
 
-import com.sun.javafx.scene.ParentHelper;
-import com.sun.javafx.scene.traversal.Algorithm;
-import com.sun.javafx.scene.control.FakeFocusTextField;
-import com.sun.javafx.scene.traversal.Direction;
-import com.sun.javafx.scene.traversal.ParentTraversalEngine;
-import javafx.scene.control.Control;
-import javafx.scene.control.SkinBase;
-import com.sun.javafx.scene.control.behavior.SpinnerBehavior;
-import com.sun.javafx.scene.traversal.TraversalContext;
-import javafx.collections.ListChangeListener;
+import java.util.List;
+
 import javafx.css.PseudoClass;
 import javafx.geometry.HPos;
 import javafx.geometry.VPos;
 import javafx.scene.AccessibleAction;
 import javafx.scene.AccessibleRole;
 import javafx.scene.Node;
+import javafx.scene.control.Control;
+import javafx.scene.control.SkinBase;
 import javafx.scene.control.Spinner;
 import javafx.scene.control.TextField;
 import javafx.scene.input.KeyCode;
 import javafx.scene.input.KeyEvent;
 import javafx.scene.layout.Region;
 import javafx.scene.layout.StackPane;
 
-import java.util.List;
+import com.sun.javafx.scene.ParentHelper;
+import com.sun.javafx.scene.control.FakeFocusTextField;
+import com.sun.javafx.scene.control.ListenerHelper;
+import com.sun.javafx.scene.control.behavior.SpinnerBehavior;
+import com.sun.javafx.scene.traversal.Algorithm;
+import com.sun.javafx.scene.traversal.Direction;
+import com.sun.javafx.scene.traversal.ParentTraversalEngine;
+import com.sun.javafx.scene.traversal.TraversalContext;
 
 /**
  * Default skin implementation for the {@link Spinner} control.
@@ -80,11 +81,9 @@ public class SpinnerSkin<T> extends SkinBase<Spinner<T>> {
     private static final int SPLIT_ARROWS_HORIZONTAL    = 5;
 
     private int layoutMode = 0;
-
     private final SpinnerBehavior behavior;
 
 
-
     /* *************************************************************************
      *                                                                         *
      * Constructors                                                            *
@@ -103,13 +102,15 @@ public SpinnerSkin(Spinner<T> control) {
 
         // install default input map for the Button control
         behavior = new SpinnerBehavior<>(control);
-//        control.setInputMap(behavior.getInputMap());
 
         textField = control.getEditor();
-        getChildren().add(textField);
+
+        ListenerHelper lh = ListenerHelper.get(this);
 
         updateStyleClass();
-        control.getStyleClass().addListener((ListChangeListener<String>) c -> updateStyleClass());
+        lh.addListChangeListener(control.getStyleClass(), (ch) -> {
+            updateStyleClass();
+        });
 
         // increment / decrement arrows
         incrementArrow = new Region();
@@ -169,12 +170,12 @@ public void executeAccessibleAction(AccessibleAction action, Object... parameter
         // Fixes in the same vein as ComboBoxListViewSkin
 
         // move fake focus in to the textfield if the spinner is editable
-        control.focusedProperty().addListener((ov, t, hasFocus) -> {
+        lh.addChangeListener(control.focusedProperty(), (op) -> {
             // Fix for the regression noted in a comment in RT-29885.
-            ((FakeFocusTextField)textField).setFakeFocus(hasFocus);
+            ((FakeFocusTextField)textField).setFakeFocus(control.isFocused());
         });
 
-        control.addEventFilter(KeyEvent.ANY, ke -> {
+        lh.addEventFilter(control, KeyEvent.ANY, (ke) -> {
             if (control.isEditable()) {
                 // This prevents a stack overflow from our rebroadcasting of the
                 // event to the textfield that occurs in the final else statement
@@ -204,14 +205,15 @@ public void executeAccessibleAction(AccessibleAction action, Object... parameter
         // Spinner control. Without this the up/down/left/right arrow keys don't
         // work when you click inside the TextField area (but they do in the case
         // of tabbing in).
-        textField.addEventFilter(KeyEvent.ANY, ke -> {
+        lh.addEventFilter(textField, KeyEvent.ANY, (ke) -> {
             if (! control.isEditable() || isIncDecKeyEvent(ke)) {
                 control.fireEvent(ke.copyFor(control, control));
                 ke.consume();
             }
         });
 
-        textField.focusedProperty().addListener((ov, t, hasFocus) -> {
+        lh.addChangeListener(textField.focusedProperty(), (op) -> {
+            boolean hasFocus = textField.isFocused();
             // Fix for RT-29885
             control.getProperties().put("FOCUSED", hasFocus);
             // --- end of RT-29885
@@ -229,7 +231,6 @@ public void executeAccessibleAction(AccessibleAction action, Object... parameter
 
         textField.focusTraversableProperty().bind(control.editableProperty());
 
-
         // Following code borrowed from ComboBoxPopupControl, to resolve the
         // issue initially identified in RT-36902, but specifically (for Spinner)
         // identified in RT-40625
@@ -261,13 +262,27 @@ private boolean isIncDecKeyEvent(KeyEvent ke) {
      *                                                                         *
      **************************************************************************/
 
+    @Override
+    public void install() {
+        // when replacing the skin, the textField (which comes from the control), must first be uninstalled
+        // by the old skin in its dispose(), followed by (re-)adding it here.
+        getChildren().add(textField);
+    }
+
     /** {@inheritDoc} */
-    @Override public void dispose() {
-        super.dispose();
+    @Override
+    public void dispose() {
+        if (getSkinnable() == null) {
+            return;
+        }
+
+        getChildren().removeAll(textField, incrementArrowButton, decrementArrowButton);
 
         if (behavior != null) {
             behavior.dispose();
         }
+
+        super.dispose();
     }
 
     /** {@inheritDoc} */

modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/SkinMemoryLeakTest.java

@@ -235,7 +235,7 @@ public static Collection<Object[]> data() {
                 PasswordField.class,
 
                 //
-                Spinner.class,
+                //Spinner.class,
 
                 //
                 //SplitPane.class,

modules/javafx.controls/src/test/java/test/javafx/scene/control/skin/SpinnerSkinTest.java

@@ -141,4 +141,12 @@ public void shouldPositionArrowsSplitTopAndBottom() {
         assertEquals(new BoundingBox(PADDING_LEFT, PADDING_TOP + HEIGHT - tallest, WIDTH, tallest), decrementArrowButton.getBoundsInParent());
         assertEquals(new BoundingBox(PADDING_LEFT, PADDING_TOP, WIDTH, tallest), incrementArrowButton.getBoundsInParent());
     }
+
+    /** Tests JDK-8245145: IAE when replacing skins */
+    @Test
+    public void testSpinnerSkin() {
+        Spinner<?> spinner = new Spinner<>();
+        spinner.setSkin(new SpinnerSkin<>(spinner));
+        spinner.setSkin(new SpinnerSkin<>(spinner));
+    }
 }