JDK-8337246 : SpinnerSkin does not consume ENTER KeyEvent when editor ActionEvent is consumed
  • Type: Bug
  • Component: javafx
  • Sub-Component: controls
  • Affected Version: jfx22
  • Priority: P4
  • Status: Open
  • Resolution: Unresolved
  • OS: generic
  • CPU: generic
  • Submitted: 2024-07-20
  • Updated: 2024-11-07
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.
Other
tbdUnresolved
Related Reports
Relates :  
Relates :  
Description
A DESCRIPTION OF THE PROBLEM :
The Spinner control has an editor TextField. This field is the only way to notice when a user is done editing the Spinner, and the value can be committed. Indeed, by default, the Spinner wires a listener to the editor's action listener to do so. However, unlike a normal TextField, whose ActionEvent can be consumed to stop handling of the KeyEvent that triggered it; the SpinnerSkin class does not properly consume the originating KeyEvent when the propagated one is consumed. This can result in unwanted handling of this event by parent nodes.

STEPS TO FOLLOW TO REPRODUCE THE PROBLEM :
1. Start the class provided (SpinnerNonConsuming).
2. Click into the "Normal" TextField, hit ENTER, and notice that it prints out an action, but does not log a KeyEvent from the VBox listener.
3. Click into the Spinner's TextField, hit ENTER, and notice that it both prints out the action, and also logs a KeyEvent from the VBox listener.

EXPECTED VERSUS ACTUAL BEHAVIOR :
EXPECTED -
Performing (3) should not log a KeyEvent from the VBox listener.
ACTUAL -
The KeyEvent event is logged.

---------- BEGIN SOURCE ----------
public class SpinnerNonConsuming extends Application {
    public static void main(String[] args) {
        Application.launch(args);
    }

    @Override
    public void start(Stage primaryStage) throws Exception {
        TextField textField = new TextField("Normal text field");
        textField.setOnAction(event -> {
            System.out.println("Normal action event: " + textField.getText());
            event.consume();
        });
        Spinner<Integer> integerSpinner = new Spinner<>(new SpinnerValueFactory.IntegerSpinnerValueFactory(0, 100, 50));
        integerSpinner.getEditor().setOnAction(event -> {
            System.out.println("Spinner action event: " + integerSpinner.getValue());
            event.consume();
        });
        integerSpinner.setEditable(true);
        VBox root = new VBox(textField, integerSpinner);
        root.setOnKeyPressed(event -> System.out.println("Key pressed: " + event.getCode()));
        primaryStage.setScene(new Scene(root, 300, 300));
        primaryStage.show();
    }
}

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

CUSTOMER SUBMITTED WORKAROUND :
Attach the following listener to the Spinner, which is over-eager as it suppresses even if the action event isn't consumed:
            spinner.setOnKeyPressed(t -> {
                if (t.getCode() == KeyCode.ENTER) {
                    t.consume();
                }
            });

FREQUENCY : always



Comments
There seems to be misunderstanding why events are copied. They are copied because the target needs to change (from control to the internal text field). The control is the original target as it has the focus (and controls are supposed to be black boxes, ie. the control has the focus, and its internal structure supplied by the Skin should not be leaking out). The target field of events is used to allow using a single event handler that is shared among many controls (view cells are a good example where this makes sense).
07-11-2024

A pull request was submitted for review. Branch: master URL: https://git.openjdk.org/jfx/pull/1629 Date: 2024-11-07 14:08:09 +0000
07-11-2024

Thank you [~fastegal] for pointing to JDK-8229467. Let me take a look if the same solution can be applied there...
07-08-2024

A pull request was submitted for review. Branch: master URL: https://git.openjdk.org/jfx/pull/1523 Date: 2024-07-30 18:26:21 +0000
07-08-2024

yeah, there are problems with copying events, see another variant JDK-8229467 - it all is deeply and solidly buried
03-08-2024

John created an enhancement a while ago regarding the event system and to better find out if an event was consumed or not. See JDK-8303209. Maybe John gathered more insights here already.
03-08-2024

JDK-8088897 is basically a dup of JDK-8087863. They are both related to the Mac system menu bar handling an event that was already consumed.
02-08-2024

Thank you for mentioning JDK-8087863. Do you know of any other bugs that might have a similar root cause? It looks like the copying of events is indeed a design feature (why, I ask, why? but it's too late now). I still think that consuming a cloned event should consume the original one, but adding this logic blindly in Event.copyFor() does not work, as it is being used to create legitimately separate events. I am going to try to see if these two different code paths can be separated.
02-08-2024

I recently ran into a similar issue while investigating the fix for JDK-8087863. Getting rid of the event copy in the SpinnerSkin code won't help, the event will just get copied somewhere else. The EventUtil code might make a copy and any event dispatcher in the dispatch chain can make one also. I think a new copy is made for each potential target but I don't remember the details. What I'm doing is calling EventUtil.fireEvent(event, target) and checking for a null return value which indicates that the event or one of its copies was consumed. This leverages the way event dispatcher's work; when an event is consumed the dispatchEvent() call stack unwinds with null return values. It would be nice if there was a public version of EventUtil.fireEvent(event, target). I don't think there's any public API that lets a client fire an event and find out if was consumed or not.
02-08-2024

I wonder why the copy is needed in the first place? The event should go to the TextField even without copy, no? Otherwise the bug might be somewhere else. I would not expect a copy of the event for a normal case of an action event fired to a control with children.
31-07-2024

[~mhanl]: tried propagating the consume() action to the ancestors of the cloned events. unfortunately, there are many failures of existing tests (headless and headful), even though I think the idea to propagate is a correct one. either the code relies on the current behavior, in which case it will be very difficult to fix this particular issue, or the tests needs to be fixed. Please see https://github.com/openjdk/jfx/pull/1523
30-07-2024

Preliminary testing shows that propagating 'consume()' flag does indeed fix the issue in this ticket. I suppose we could try investigating, my concern is that it creates substantial regression risk, including all the code that was added to work around this design problem. We don't have such problem in swing because it does not need and does not create multiple copies of events.
26-07-2024

One thing that immediately came into my mind when reading this is, that TextFields suffer from the same problem. This is especially noticable in a Dialog, where ENTER will trigger the onAction event AND also close the dialog (default button) as it is not consumed. Maybe something to looks as well.
26-07-2024

https://bugs.openjdk.org/browse/JDK-8115009 KeyEvent.copyFor() in SpinnerSkin:197 An interesting point about Event.copyFor() is that it creates a copy of an event, but does not propagate setting of consumed() flag to the original event. Even worse, an ENTER key press in the Spinner invokes copyFor() **17** times for the same KeyEvent. At this point it's unclear what a possible fix might be, for the Spinner specifically and for Events in general. Should any event created by copyFor() propagate consume() operation to its parent? Would that create massive regressions elsewhere?
26-07-2024

The issue is reproducible. I checked on Ubuntu 22 x64. When we click into the "Normal" TextField and hit ENTER, it prints out an action but does not log a KeyEvent from the VBox listener. Below is the output of the same: ------------------------------------------- Normal action event: Normal text ffield Spinner action event: 50 Key pressed: ENTER
26-07-2024