Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

8337246: SpinnerSkin does not consume ENTER KeyEvent when editor ActionEvent is consumed #1629

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

8337246: SpinnerSkin does not consume ENTER KeyEvent when editor ActionEvent is consumed #1629

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
afef515
Proof of concept fix for Spinner event handling
hjohn Nov 7, 2024
4e6dafc
Change default button tests to use non-editable spinner
hjohn Nov 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 21 additions & 18 deletions .../javafx.controls/src/main/java/com/sun/javafx/scene/control/behavior/SpinnerBehavior.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,14 +31,12 @@
import javafx.scene.control.Spinner;
import javafx.scene.control.SpinnerValueFactory;
import com.sun.javafx.scene.control.inputmap.InputMap;
import javafx.scene.input.KeyCode;
import javafx.scene.input.KeyEvent;
import javafx.util.Duration;

import java.util.List;

import static javafx.scene.input.KeyCode.*;
import static com.sun.javafx.scene.control.inputmap.InputMap.KeyMapping;

public class SpinnerBehavior<T> extends BehaviorBase<Spinner<T>> {

// this specifies how long the mouse has to be pressed on a button
Expand Down Expand Up @@ -69,7 +67,20 @@ public class SpinnerBehavior<T> extends BehaviorBase<Spinner<T>> {
}
};

private final EventHandler<KeyEvent> spinnerKeyHandler = e -> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sold on this solution.

The issue, as I see it, stems from the fact that copies of events are being sent and their isConsumed() is not checked. So even though the secondary event is consumed, the code that fires the copy does not check the flag, and as a result this information is lost.

This is currently being discussed in the mailing list, me advocating fixing maybe by removing Event.copyFor() and/or linking the isConsumed() flags for events created via Event.copyFor().

The alternative is illustrated here - adding what I think are band-aids in the form of custom event dispatchers, which I think is wrong (I think there should not be a public EventDispatcher interface and no need for Event.copyFor()).

The other alternative, using only internal hacks can be seen in #1523

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently the system treats events as immutable objects. In order to update either the target or source of an event the system needs to make a copy with the new target or source. This means a copy might be made when an event is fired at a target OR when an event is handed off to a filter or handler. To get rid of all copies would require the target and source fields in the event to become mutable which is a very big and problematic change.

There are other reasons why an event dispatcher might not send along the original event to a handler. See EnteredExitHandler.java to see how new mouse events are created during dispatch to satisfy the demands of the documented MouseEvent API.

There's an existing protocol for determining whether an event was consumed or not, namely checking to see if dispatchEvent returns null. It is not well documented but part of the external API. And it works fine even in the face of events being copied or modified during dispatch which happens all over the place.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mailing list might be a better place for this discussion, but for some reason a lot of discussion happens in the PRs.

I agree events should be immutable. What I am saying there should be no 'target' field - then one does not need to do copyFor(). One can still fire a new event if so desired.

The current FX event mechanism is just not a good one, causes more problem than it solves (I presume it solves some problems, though I can't see how - in Swing, neither EventDispatcher is public, nor the Events have target fields and the need for copyFor). Yes, FX is not Swing, this is just an example of a better design (from the event handling point of view).

Copy link
Collaborator Author

@hjohn hjohn Nov 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@andy-goryachev-oracle

The issue, as I see it, stems from the fact that copies of events are being sent and their isConsumed() is not checked. So even though the secondary event is consumed, the code that fires the copy does not check the flag, and as a result this information is lost.

What do you mean? The code that does the copy is here below:

               Event event = textField.getEventDispatcher().dispatchEvent(e.copyFor(textField, textField), new EventDispatchChainImpl());

                if (event == null || event.isConsumed()) {
                    e.consume();  // consume original trigger event
                }

It clearly does check the flag, and then consumes the original event?

This solution is much cleaner than the current implementation, does not require any new semantics and can all be achieved with the existing system.

This is currently being discussed in the mailing list, me advocating fixing maybe by removing Event.copyFor() and/or linking the isConsumed() flags for events created via Event.copyFor().

You may want to check if this is backwards compatible. Take into account that often events are converted (ie. a KeyEvent converts to an ActionEvent) and that it is possible currently for one event to trigger multiple other events.

I think the fix I do here is far better; the whole problem was that the event that needed forwarding was re-fired across the whole scenegraph, confusing every handler that may be seeing that copy. That's because fireEvent will always build a full dispatch chain (as that's usually what you want, but not always). In cases like this, where the Skin wants to forward an event into its internals (which neither the Control, nor Scene, nor the Behavior knows about), the event should be confined to that location only. The "copy" that must be made to do this is irrelevant, performance and memory wise (events are things that occur with millisecond frequency, and so are not worth optimizing for).

Furthermore, as I already explained on the mailing list, the target field is essential to be able to re-use handlers across controls. You can't just remove it.

The other alternative, using only internal hacks can be seen in #1523

This PR still requires the Skin to be aware of what keys the Behavior needs. It still needs to deal with the stack overflow problems as it will see its own event twice -- it still fires what should be an internal only event across the entire scene graph. For example, a KeyEvent listener located at some in between Node will therefore see things like the pressed ENTER KeyEvent pass by twice(*), even though it was only pressed once.... this is not a good solution, and never was. It was done this way because the implementor didn't see a better way.

(*) I could file a bug for that if you so wish.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want to step back and discuss the idea of re-thinking of the event dispatching where Event has no target, no need for Event.copyFor() and possibly removing public EventDispatcher. This PR is not a good place for such a discussion.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current FX event mechanism is just not a good one, causes more problem than it solves (I presume it solves some problems, though I can't see how - in Swing, neither EventDispatcher is public, nor the Events have target fields and the need for copyFor). Yes, FX is not Swing, this is just an example of a better design (from the event handling point of view).

You can only say things like this when you fully understand the current design.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can only say things like this when you fully understand the current design.

this is offensive and not very productive.

boolean arrowsAreVertical = arrowsAreVertical();
KeyCode increment = arrowsAreVertical ? KeyCode.UP : KeyCode.RIGHT;
KeyCode decrement = arrowsAreVertical ? KeyCode.DOWN : KeyCode.LEFT;

if (e.getCode() == increment) {
increment(1);
e.consume();
}
else if (e.getCode() == decrement) {
decrement(1);
e.consume();
}
};

/***************************************************************************
* *
Expand All @@ -84,24 +95,16 @@ public SpinnerBehavior(Spinner<T> spinner) {
// InputMap installed on the control, if it is non-null, allowing us to pick up any user-specified mappings)
spinnerInputMap = createInputMap();

// then spinner-specific mappings for key and mouse input
addDefaultMapping(spinnerInputMap,
new KeyMapping(UP, KeyEvent.KEY_PRESSED, e -> {
if (arrowsAreVertical()) increment(1); else FocusTraversalInputMap.traverseUp(e);
}),
new KeyMapping(RIGHT, KeyEvent.KEY_PRESSED, e -> {
if (! arrowsAreVertical()) increment(1); else FocusTraversalInputMap.traverseRight(e);
}),
new KeyMapping(LEFT, KeyEvent.KEY_PRESSED, e -> {
if (! arrowsAreVertical()) decrement(1); else FocusTraversalInputMap.traverseLeft(e);
}),
new KeyMapping(DOWN, KeyEvent.KEY_PRESSED, e -> {
if (arrowsAreVertical()) decrement(1); else FocusTraversalInputMap.traverseDown(e);
})
);
spinner.addEventFilter(KeyEvent.KEY_PRESSED, spinnerKeyHandler);
}


@Override
public void dispose() {
getNode().removeEventFilter(KeyEvent.KEY_PRESSED, spinnerKeyHandler);

super.dispose();
}

/***************************************************************************
* *
Expand Down
52 changes: 18 additions & 34 deletions modules/javafx.controls/src/main/java/javafx/scene/control/skin/SpinnerSkin.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
import java.util.List;

import javafx.css.PseudoClass;
import javafx.event.Event;
import javafx.geometry.HPos;
import javafx.geometry.VPos;
import javafx.scene.AccessibleAction;
Expand All @@ -41,6 +42,7 @@
import javafx.scene.layout.Region;
import javafx.scene.layout.StackPane;

import com.sun.javafx.event.EventDispatchChainImpl;
import com.sun.javafx.scene.ParentHelper;
import com.sun.javafx.scene.control.FakeFocusTextField;
import com.sun.javafx.scene.control.ListenerHelper;
Expand Down Expand Up @@ -177,40 +179,22 @@ public void executeAccessibleAction(AccessibleAction action, Object... parameter
((FakeFocusTextField)textField).setFakeFocus(control.isFocused());
});

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
// of the conditions below.
if (ke.getTarget().equals(textField)) return;

// Fix for RT-38527 which led to a stack overflow
if (ke.getCode() == KeyCode.ESCAPE) return;

// This and the additional check of isIncDecKeyEvent in
// textField's event filter fix JDK-8185937.
if (isIncDecKeyEvent(ke)) return;

// Fix for the regression noted in a comment in RT-29885.
// This forwards the event down into the TextField when
// the key event is actually received by the Spinner.
textField.fireEvent(ke.copyFor(textField, textField));

if (ke.getCode() == KeyCode.ENTER) return;

ke.consume();
}
});

// This event filter is to enable keyboard events being delivered to the
// spinner when the user has mouse clicked into the TextField area of the
// 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).
lh.addEventFilter(textField, KeyEvent.ANY, (ke) -> {
if (! control.isEditable() || isIncDecKeyEvent(ke)) {
control.fireEvent(ke.copyFor(control, control));
ke.consume();
// Forwards all key events arriving at the control level to the internal
// text field, if the control is editable. This forwarding is limited to the
// text field only, and so the forwarded event will NOT traverse the entire
// scene graph. The originating event is only consumed if the forwarded
// event was consumed.
lh.addEventFilter(control, KeyEvent.ANY, e -> {
// Note: due to a bug, we check if the event is consumed here. The behavior
// will consume the appropriate arrow keys, but the event is STILL delivered to this
// filter at the same level. Without the consume check, the TextField will act
// on arrow keys, moving the cursor unexpectedly.
if (!e.isConsumed() && control.isEditable()) {
Event event = textField.getEventDispatcher().dispatchEvent(e.copyFor(textField, textField), new EventDispatchChainImpl());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be cleaner to use the exiting EventUtil.fireEvent() routine.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The purpose of doing it this way is to prevent a full dispatch chain (starting from Window/Scene) to be built. EventUtil.fireEvent does something else. Perhaps a utility method is warranted if this pattern becomes commonplace to solve these kinds of issues.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it. I should have looked at that code more closely.

Your code assumes that it's sufficient to send the event to the single dispatcher associated with the node. That seems like a substantial change since it bypasses any handlers or filters installed further up the node hierarchy. But there are plenty of other instances where this happens. Anyone who tries to install a filter that tries to monitor events going to all TextFields at a global level will already see some anomalies. For example, you will never see an Escape or Enter key event that's targeted at the TextField inside a ComboBox.


if (event == null || event.isConsumed()) {
e.consume(); // consume original trigger event
}
}
});

Expand Down
2 changes: 0 additions & 2 deletions modules/javafx.controls/src/test/java/test/javafx/scene/control/SpinnerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1603,8 +1603,6 @@ public void test_jdk_8150946_testCommit_invalid_wrongType() {
stage.setWidth(200);
stage.setHeight(200);

intSpinner.setEditable(true);

Button defaultButton = new Button("OK");
defaultButton.setOnAction(arg0 -> { enterDefaultPass = true; });
defaultButton.setDefaultButton(true);
Expand Down