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

[hjohn]

This is a proof of concept fix for the linked issue.

We'll need to discuss if using an event filter in the Behavior is the appropriate fix and how much impact this may have on current applications.

---

Progress

• Change must be properly reviewed (1 review required, with at least 1 Reviewer)
• Change must not contain extraneous whitespace
• Commit message must refer to an issue

Issue

• JDK-8337246: SpinnerSkin does not consume ENTER KeyEvent when editor ActionEvent is consumed (Bug - P4)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/1629/head:pull/1629
$ git checkout pull/1629

Update a local copy of the PR:
$ git checkout pull/1629
$ git pull https://git.openjdk.org/jfx.git pull/1629/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1629

View PR using the GUI difftool:
$ git pr show -t 1629

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/1629.diff

Using Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back jhendrikx! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

[mlbridge]

Webrevs

• 01: Full - Incremental (4e6dafca)
• 00: Full (afef5153)

[hjohn]

The failing SpinnerTest::testEnterEscapeKeysWithDefaultCancelButtons test is a bit odd; it expects that Enter in an editable Spinner should result in activating the Default Button. However, a normal TextField doesn't do this either...

In a non-editable Spinner, the default button logic works as expected.

As it is, we can't have both; the bug wants to not have Enter bubbling up, but then the default button logic is not going to work.

IMHO, Spinner's text field should work like regular text fields, so if Enter in those fields can't activate the default button, then neither should Spinner's editable field.

[beldenfox]

The failing SpinnerTest::testEnterEscapeKeysWithDefaultCancelButtons test is a bit odd; it expects that Enter in an editable Spinner should result in activating the Default Button. However, a normal TextField doesn't do this either...

Maybe I'm misunderstanding this comment but in my testing pressing Enter in an editable TextField can activate the default button as long as the TextField doesn't have an OnAction handler set. It should not be so easy to block Enter from activating the default button.

It might be helpful to write up a single test that verifies that Enter is handled correctly for TextField, ComboBox, DatePicker, and Spinner. I'm working on a PR to clean up ESC handling and it was useful to write up a single test covering all of these controls. (I want to ensure that the user can always uses ESC to close a modal dialog even if the TextField has a formatter assigned to it. This isn't working currently.)

Why doesn't Spinner work the same as ComboBox? For a ComboBox the client registers the action handler directly on the ComboBox instead of the combo box's editor.

[beldenfox]

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

[hjohn]

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.

[beldenfox]

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.

[hjohn]

The failing SpinnerTest::testEnterEscapeKeysWithDefaultCancelButtons test is a bit odd; it expects that Enter in an editable Spinner should result in activating the Default Button. However, a normal TextField doesn't do this either...

Maybe I'm misunderstanding this comment but in my testing pressing Enter in an editable TextField can activate the default button as long as the TextField doesn't have an OnAction handler set. It should not be so easy to block Enter from activating the default button.

Ah, perhaps that's the problem, I did have an action handler, although it did't consume the event (which normally should mean it's a no-op). Hm...

It might be helpful to write up a single test that verifies that Enter is handled correctly for TextField, ComboBox, DatePicker, and Spinner. I'm working on a PR to clean up ESC handling and it was useful to write up a single test covering all of these controls. (I want to ensure that the user can always uses ESC to close a modal dialog even if the TextField has a formatter assigned to it. This isn't working currently.)

Yeah, I guess I didn't look deep enough.

Why doesn't Spinner work the same as ComboBox? For a ComboBox the client registers the action handler directly on the ComboBox instead of the combo box's editor.

Very good point; when I was looking at the code I was quite confused with the way the editor property works -- it being read only seemed quite useless, except if that's the only way to register an action handler. It looks suspiciously like a poor design.

[hjohn]

So I took a look in TextFieldBehavior how it could possibly be that installing an empty Action handler could lead to the Enter event being consumed, and I found this:

    // fix of JDK-8207759: reverted logic
    // mapping not auto-consume and consume if handled by action
    if (onAction != null || actionEvent.isConsumed()) {
        event.consume();
    }

So, that if will consume the event if there is an action handler or the action event was consumed... I'm pretty sure that should be an and.

In fact, I'm unsure why the code even bothers to check if there is an action handler... just check if the Action event was consumed.

[andy-goryachev-oracle]

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

[beldenfox]

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.

[andy-goryachev-oracle]

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).

[hjohn]

@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.

[andy-goryachev-oracle]

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.

[hjohn]

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.

[andy-goryachev-oracle]

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

this is offensive and not very productive.