JDK-8088068; accelerators should be MenuItem specific

[Groostav]

This is my interpretation of https://bugs.openjdk.java.net/browse/JDK-8088068

code to reproduce is in a comment below

The accelerator API appears to assume a kind of singleton menubar. The binding logic for accelerators uses the scene and is keyed only by the key combination. This is likely suitable for a large portion of users.

This has a surprising ramification for contextMenus using accelerators: if you create multiple instances of a MenuItem with an accelerator, one will overwrite the other, causing instance confusion.

I don't believe there is an obvious fix for this: the existing implementation is on the scene, and is keyed only by the key-combination (not the instance or any extra meta about who registered that key combination). As such there are certain to be implementations that rely on this behavior for functionality; for them the lack of instance-specificity is a convenience.

---

my .02:

I'm wondering if the accelerator system could be updated to include "local" vs "global" accelerators, with the existing API being an alias for global accelerators. I would then change MenuItem to include localAccelerator (is there a system for me to rename accelerator to globalAccelerator with backwards compatibility?) The binding logic could then traverse the node graph (specifically, the instigating event target's parent lineage) to find the most local accelerator, defaulting to the global accelerator.

[Groostav]

OP's code to reproduce

import javafx.application.Application;
import javafx.scene.Scene;
import javafx.scene.control.ContextMenu;
import javafx.scene.control.MenuItem;
import javafx.scene.control.TextArea;
import javafx.scene.input.KeyCombination;
import javafx.scene.layout.VBox;
import javafx.stage.Stage;

public class AcceleratorTest extends Application {

    public void start(Stage primaryStage) {
        TextArea ta1 = new TextArea();
        TextArea ta2 = new TextArea();

        MenuItem item1 = new MenuItem("Insert \"Hello\"");
        item1.setOnAction((ActionEvent) -> ta1.setText("Hello"));
        item1.setAccelerator(KeyCombination.keyCombination("Ctrl+I"));

        MenuItem item2 = new MenuItem("Insert \"World!\"");
        item2.setOnAction((ActionEvent) -> ta2.setText("World!"));
        item2.setAccelerator(KeyCombination.keyCombination("Ctrl+I"));

        ContextMenu context1 = new ContextMenu();
        context1.getItems().add(item1);

        ContextMenu context2 = new ContextMenu();
        context2.getItems().add(item2);

        ta1.setContextMenu(context1);
        ta2.setContextMenu(context2);

        VBox vBox = new VBox();
        vBox.getChildren().add(ta1);
        vBox.getChildren().add(ta2);

        Scene scene = new Scene(vBox, 300, 300);

        primaryStage.setTitle("Accelerator Test");
        primaryStage.setScene(scene);
        primaryStage.show();
    }

    public static void main(String[] args) {
        launch(args);
    }

}

If you start this application, but the text cursor into the first TextArea and press Ctrl+I, the text of the second textarea is set.

This is quite unexpected, as the shortcuts live within a contextmenu of their respective textareas.

[kleopatra]

note that this repo has been moved (unfortunately the new repo has the issues disabled ..) - added a reference to the bug report

[effad]

For what it's worth: I (the OP) have since used a subclass of Scene as a workaround that will install an event filter on key events, an check each keypress for a matching menuitem (only works with context menus).
It's a bit clumsy as you have to explicetly register the Control together with its parent but it works around the situation for me.

import javafx.event.EventTarget;
import javafx.scene.Node;
import javafx.scene.Parent;
import javafx.scene.Scene;
import javafx.scene.control.Control;
import javafx.scene.control.MenuItem;
import javafx.scene.input.KeyEvent;

/**
 * MultiAcceleratorEnabledScene works around
 * https://bugs.openjdk.java.net/browse/JDK-8088068
 * 
 * @author r.lichtenberger@synedra.com
 */
public class MultiAcceleratorEnabledScene extends Scene {

	private static final Object ANCHOR = new Object();

	public MultiAcceleratorEnabledScene(Parent root, double width, double height) {
		super(root, width, height);
		addEventFilter(KeyEvent.KEY_PRESSED, this::handleShortcut);
	}
	
	/** Prepares a node as anchor for multi accelerator handling.
	 * @param parent The parent node to prepare as anchor for multi accelerator handling. If a key event happens in a focused node below parent, the context menu of handler will be used to handle the accelerator.
	 * @param handler The node to handle key events
	 */
	public static void setHandler(Parent parent, Control handler) {
		parent.getProperties().put(ANCHOR, handler);
	}
	
	private void handleShortcut(KeyEvent e) {
		Control anchor = findAnchor(e.getTarget());
		if (anchor != null && anchor.getContextMenu() != null) {
			for (MenuItem item : anchor.getContextMenu().getItems()) {
				if (item.getAccelerator() != null && item.getAccelerator().match(e) && !item.isDisable()) {
					item.fire();
					e.consume();
				}
			}
		}
	}

	private Control findAnchor(EventTarget target) {
		Control anchor = null;
		if (target instanceof Node) {
			Node current = (Node) target;
			while (current != null && anchor == null) {
				if (current.hasProperties()) {
					anchor = (Control) current.getProperties().get(ANCHOR);					
				}
				current = current.getParent();
			}
		}
		return anchor;
	}
}