JDK-8189677 : RadioMenuItem fires extra NULL value in property
  • Type: Bug
  • Component: javafx
  • Sub-Component: controls
  • Affected Version: 8,9,10
  • Priority: P3
  • Status: Resolved
  • Resolution: Fixed
  • OS: generic
  • CPU: generic
  • Submitted: 2017-10-18
  • Updated: 2020-01-31
  • Resolved: 2017-11-03
The Version table provides details related to the release that this issue/RFE will be addressed.

Unresolved : Release in which this issue/RFE will be addressed.
Resolved: Release in which this issue/RFE has been resolved.
Fixed : Release in which this issue/RFE has been fixed. The release containing this fix may be available for download as an Early Access Release or a General Availability Release.

To download the current JDK release, click here.
JDK 10 JDK 8
10Fixed 8u192Fixed
Description
FULL PRODUCT VERSION :
java version "1.8.0_152"
Java(TM) SE Runtime Environment (build 1.8.0_152-b16)
Java HotSpot(TM) 64-Bit Server VM (build 25.152-b16, mixed mode)

ADDITIONAL OS VERSION INFORMATION :
Microsoft Windows [Version 10.0.15063]

A DESCRIPTION OF THE PROBLEM :
The RadioButton class and the RadioMenuItem class are very similar in that they are toggle buttons in toggle groups.  However, they interact with the ToggleGroup differently, in a way that I believe is likely unintentional.  ToggleButtons call ToggleGroup.clearSelectedToggle(), but RadioMenuItem calls ToggleGroup.selectToggle(null).  The latter creates an extra event that affects the contract of the ToggleGroups "selectedToggleProperty"

If someone is trying to use the oldValue / newValue in a ChangeListener, this will really complicate the code since the actual old value would be lost when the new value was set.  I found someone asking about this on Stack Overflow:
https://stackoverflow.com/questions/44403887/togglegroup-with-a-changelistenertoggle-always-throws-a-null-value-to-the-oldv

I believe ToggleButton uses ToggleGroup.clearSelectedToggle() as intended, while RadioMenuItem uses ToggleGroup.selectToggle(null) which causes the null.

From ToggleButton.java
    public final BooleanProperty selectedProperty() {
        if (selected == null) {
            selected = new BooleanPropertyBase() {
                @Override protected void invalidated() {
                    final boolean selected = get();
                    final ToggleGroup tg = getToggleGroup();
                    // Note: these changes need to be done before selectToggle/clearSelectedToggle since
                    // those operations change properties and can execute user code, possibly modifying selected property again
                    pseudoClassStateChanged(PSEUDO_CLASS_SELECTED, selected);
                    notifyAccessibleAttributeChanged(AccessibleAttribute.SELECTED);
                    if (tg != null) {
                        if (selected) {
                            tg.selectToggle(ToggleButton.this);
                        } else if (tg.getSelectedToggle() == ToggleButton.this) {
                            tg.clearSelectedToggle();
                        }
                    }
                }

                @Override
                public Object getBean() {
                    return ToggleButton.this;
                }

                @Override
                public String getName() {
                    return "selected";
                }
            };
        }
        return selected;
    }

From RadioMenuItem.java
@Override public final BooleanProperty selectedProperty() {
        if (selected == null) {
            selected = new BooleanPropertyBase() {
                @Override protected void invalidated() {
                    if (getToggleGroup() != null) {
                        if (get()) {
                            getToggleGroup().selectToggle(RadioMenuItem.this);
                        } else if (getToggleGroup().getSelectedToggle() == RadioMenuItem.this) {
                            getToggleGroup().selectToggle(null);
                        }
                    }

                    if (isSelected()) {
                        getStyleClass().add(STYLE_CLASS_SELECTED);
                    } else {
                        getStyleClass().remove(STYLE_CLASS_SELECTED);
                    }
                }

                @Override
                public Object getBean() {
                    return RadioMenuItem.this;
                }

                @Override
                public String getName() {
                    return "selected";
                }
            };
        }
        return selected;
    }



STEPS TO FOLLOW TO REPRODUCE THE PROBLEM :
Use the controls in the application.  Changing the RadioButtons fires property change events that proceed from one state to the other, while the RadioMenuItems pass through an extra NULL state.

EXPECTED VERSUS ACTUAL BEHAVIOR :
EXPECTED -
That the toggle group would work the same in both of these cases
ACTUAL -
The RadioMenuItem would behave the same as the RadioButton and not fire the extra NULL event.

REPRODUCIBILITY :
This bug can be reproduced always.

---------- BEGIN SOURCE ----------

import javafx.application.Application;
import javafx.beans.binding.Bindings;
import javafx.beans.binding.StringBinding;
import javafx.scene.Scene;
import javafx.scene.control.Label;
import javafx.scene.control.MenuButton;
import javafx.scene.control.RadioButton;
import javafx.scene.control.RadioMenuItem;
import javafx.scene.control.Toggle;
import javafx.scene.control.ToggleGroup;
import javafx.scene.layout.VBox;
import javafx.stage.Stage;

public class ToggleMenuItem extends Application {
	public static void main(String[] args) {
		Application.launch(ToggleMenuItem.class, args);
	}

	@Override
	public void start(Stage stage) {
		stage.setTitle("Toggle Test");
		VBox box = new VBox();
		MenuButton menuButton = new MenuButton();
		RadioMenuItem b1 = new RadioMenuItem("BUTTON ONE");
		RadioMenuItem b2 = new RadioMenuItem("BUTTON TWO");

		menuButton.getItems().addAll(b1, b2);
		ToggleGroup group = new ToggleGroup();
		group.getToggles().addAll(b1, b2);
		group.selectToggle(b1);

		Label label = new Label("BASE");
		box.getChildren().add(menuButton);
		box.getChildren().add(label);

		Label radioLabel = new Label("RADIO BASE");
		RadioButton rb1 = new RadioButton("RADIO ONE");
		RadioButton rb2 = new RadioButton("RADIO TWO");
		ToggleGroup radioGroup = new ToggleGroup();
		radioGroup.getToggles().addAll(rb1, rb2);
		radioGroup.selectToggle(rb2);

		box.getChildren().add(rb1);
		box.getChildren().add(rb2);
		box.getChildren().add(radioLabel);

		createBinding("RadioMenuItem", group, label);
		createBinding("RadioButton", radioGroup, radioLabel);

		System.out.println(System.getProperty("java.runtime.version"));

		final Scene scene = new Scene(box, 400, 400);
		stage.setScene(scene);
		stage.show();
	}
	private void createBinding(String groupName, ToggleGroup group, Label label) {
		StringBinding binding = Bindings.createStringBinding(() -> {
			Toggle selectedToggle = group.getSelectedToggle();
			if (selectedToggle == null) {
				System.out.println(String.format("--- %15s is NULL ---", groupName));
				return "NULL";
			}
			else {
				String text = (selectedToggle instanceof RadioMenuItem) ? ((RadioMenuItem) selectedToggle).getText() : ((RadioButton) selectedToggle).getText();
				System.out.println(String.format("%15s is not NULL (%s)", groupName, text));
				return text;
			}
		}, group.selectedToggleProperty());
		label.textProperty().bind(binding);
	}
}

---------- END SOURCE ----------


Comments
Thanks for such a finer eye for review. I have taken care of this while pushing the changeset.
13-04-2018

Look good, although I see you started from the webrev for 10 rather than the pushed changeset, meaning that your 8u webrev also has the "Result8189677" as the instance variable name rather than "result8189677" (which you fixed when you pushed the changeset to 10). You might want to also fix this when pushing to 8u-dev. +1 Approved to push to 8u-dev for 8u192.
12-04-2018

Here is 8u-backport review request : http://cr.openjdk.java.net/~aghaisas/fx/8189677/8u/webrev.0/
11-04-2018

Ajit, can you please back port it to 8u-dev?
10-04-2018

Thanks for the review. I have renamed the field name in test as suggested and pushed the fix.
03-11-2017

Fix looks good. One recommendation for the test is that we use initial lower case for field names, so "Result8189677" should be "result8189677" -- no need for a new webrev if you want to fix this when you push. +1
03-11-2017

Here is the fix : http://cr.openjdk.java.net/~aghaisas/fx/8189677/webrev.0/ Request you to review.
30-10-2017

Issue reproducible on 8GA, 8u152, 9GA and 10ea26
20-10-2017

This is an issue reproducible in both 8 and 9 == RadioMenuItem is not NULL (BUTTON ONE) RadioButton is not NULL (RADIO TWO) 9+181 RadioButton is not NULL (RADIO ONE) RadioButton is not NULL (RADIO TWO) RadioButton is not NULL (RADIO ONE) RadioButton is not NULL (RADIO TWO) --- RadioMenuItem is NULL --- RadioMenuItem is not NULL (BUTTON TWO) RadioButton is not NULL (RADIO ONE) RadioButton is not NULL (RADIO TWO) --- RadioMenuItem is NULL --- RadioMenuItem is not NULL (BUTTON ONE)
19-10-2017