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

[andy-goryachev-oracle]

Enable backpropagation of isConsumed flag to the ancestor(s) of events cloned via Event.copyFor().

This change has a minimal API impact and allows for a fine-grained control of when to propagate and when not to propagate.

The proposed change could make JDK-8303209 unnecessary.

---

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/1523/head:pull/1523
$ git checkout pull/1523

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1523

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

Using diff file

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

Webrev

Link to Webrev Comment

[bridgekeeper]

👋 Welcome back angorya! 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.

[openjdk]

@andy-goryachev-oracle has indicated that a compatibility and specification (CSR) request is needed for this pull request.

@andy-goryachev-oracle please create a CSR request for issue JDK-8337246 with the correct fix version. This pull request cannot be integrated until the CSR request is approved.

[mlbridge]

Webrevs

• 01: Full - Incremental (48022d6b)
• 00: Full (551ed976)

[mstr2]

I don't think that we should introduce functionality to the event system that is only accessible to controls shipped with JavaFX, but not to third-party controls. Either this is an important feature for control authors, then it should be accessible for third-party controls as well. Or it is not important, then it shouldn't be added in the first place.

In my opinion, javafx.controls shouldn't be a special citizen, but just a consumer of JavaFX APIs. I know that that is not the case right now, but let's not make the situation more entrenched than it currently is.

[andy-goryachev-oracle]

but not to third-party controls

Does it mean you propose to make the Event.propagateConsume() method public?

[beldenfox]

This PR assumes that during dispatch events are copied using copyFor. But the documentation states that an EventDispatcher can do whatever it wants in dispatchEvent. A developer could provide a dispatcher that creates entirely new events rather than just copy existing ones and that would break this code. The only thing that’s guaranteed is that the dispatcher will return null to stop event dispatch and that's all that the solution in JDK-8303209 relies on.

[mstr2]

but not to third-party controls

Does it mean you propose to make the Event.propagateConsume() method public?

If we agree that this is the right API, then yes.

[andy-goryachev-oracle]

This PR assumes that during dispatch events are copied using copyFor. But the documentation states that an EventDispatcher can do whatever it wants in dispatchEvent. A developer could provide a dispatcher that creates entirely new events rather than just copy existing ones and that would break this code. The only thing that’s guaranteed is that the dispatcher will return null to stop event dispatch and that's all that the solution in JDK-8303209 relies on.

a) If the EventDispatcher does something different like creating the new events, it is up to that ED to deal with consequences. It means the developer wants to create new events and not to have the link between the events.

b) I am not convinced that returning null from a dispatcher is a good solution. We already have an API for that: Event.isConsumed(). Adding a new, parallel paradigm, in my opinion is unnecessary.

The bigger issue I see, as I mentioned earlier, is that

1. the Event has the target field which is absolutely, totally unnecessary (the target already knows who the target is, it is the target) and is an anti-pattern because
2. it forces the dispatcher to create new event instances for new targets, breaking the isConsumed logic

I don't think we'll ever change the event dispatching mechanism and remove the target field from Event. So the only remaining solutions are little hacks and workarounds here and there.

I don't think adding return value to the dispatcher is the right solution.

[beldenfox]

b) I am not convinced that returning null from a dispatcher is a good solution. We already have an API for that: Event.isConsumed(). Adding a new, parallel paradigm, in my opinion is unnecessary.

We would not be adding anything. Consuming an event is how a developer communicates to one EventDispatcher that the event was dealt with. But this information is propagated through the dispatch chain by returning null from dispatchEvent. That's all in place already.

There's a variant of EventUtil.fireEvent that returns the result of dispatchEvent so a client can check for null to see if the event was consumed. And there are already places in the code where this happens. JDK-8303209 calls for this to be cleaned up and made public.

[andy-goryachev-oracle]

@beldenfox :
EventUtil is not a public API. Checking the return value of the dispatchEvent is, in my opinion, an anti-pattern. The right pattern is this (TabPaneBehavior:85):

    public boolean canCloseTab(Tab tab) {
        Event event = new Event(tab,tab,Tab.TAB_CLOSE_REQUEST_EVENT);
        Event.fireEvent(tab, event);
        return ! event.isConsumed();
    }

Using your own argument, a dev can write a dispatcher which returns garbage from its dispatchEvent method, breaking the thing. So I am still not convinced.

[beldenfox]

@andy-goryachev-oracle I agree that propagating the consumed flag is clearer and would lead to less surprises. Once upon a time I also thought that I could call fireEvent and then check the event's consumed flag afterward. I had to dig around a bit to figure out that all these copies were being made. It is confusing.

For that matter you can't really infer that an event was consumed just because the dispatch chain returned null. The EventDispatcher documentation is a little hazy on this but in theory a dispatcher could return null for other reasons. It seems to be mostly a matter of convention that null indicates that an event was consumed.

With that said in this PR changing the consumed flag in one event can change the consumed state of another event and that also seems like unexpected and surprising behavior.

[andy-goryachev-oracle]

Yeah, that's why I tried to hide this from the public. I see the whole create-multiple-events idea as a bad design (tm), so the proposed change is a hack^H^H^H workaround to be used internally by FX in a few specific cases (Note1). The current PR also makes the fix localized to the specific scenario, further reducing the regression risk.

I don't think this functionality should be made public, contrary to what @mstr2 and @kleopatra say (sorry!). I don't have any better ideas, and let's admit it, the current event dispatching paradigm will never be changed.

Note 1:

There might be more scenarios where FX needs to know whether any of the secondary events got consumed. It's unlikely that trivial use of FX triggers the issue, but we can look through the bug list to see if we have any more mentioning isConsumed().

[mstr2]

For that matter you can't really infer that an event was consumed just because the dispatch chain returned null. The EventDispatcher documentation is a little hazy on this but in theory a dispatcher could return null for other reasons. It seems to be mostly a matter of convention that null indicates that an event was consumed.

It seems much more likely that we could clean this up by explicitly specifying this behavior.

Here's what I would do:

1. Implement JDK-8303209
2. Specify that dispatchEvent must only return null if the event was consumed.
3. Explicitly document that Event.isConsumed() can only be used inside of an event handler, but not to detect whether an event was consumed at the place where the event is fired.

This would be sufficient to solve the problem at hand, and will also enable third-party controls to do the same.

Going further, I question whether we need the isConsumed() method at all. Consumed events shouldn't be propagated, and the current behavior is very strange to say the least (events on the same level are propagated even though they're consumed). If consumed events are not propagated, there should be no need to check whether an event was consumed.

[andy-goryachev-oracle]

• Implement JDK-8303209
• Specify that dispatchEvent must only return null if the event was consumed.
• Explicitly document that Event.isConsumed() can only be used inside of an event handler, but not to detect whether an event was consumed at the place where the event is fired.

I am afraid this makes no sense to me. I still think the root cause of the issue(s) is the cloning of events, where the cloning code ignores the possible consumption of the secondary events.

I don't think we should redefine semantics of Event.isConsumed(), this method worked well in AWT/Swing and elsewhere (QT).

If anything, the code that clones an event for the purposes of changing its target needs to look at the isConsumed() flag and propagate it to the original event. The problem is that Event.copyFor() is also used for other purposes as well.

Looks like we've reached an impasse (again).

[andy-goryachev-oracle]

Actually, no CSR is required if I revert the final keyword changes.

[beldenfox]

The cloning of an event is necessary in order to update the target when the event is fired and the source when the event is passed to a handler. The alternative would be to modify those fields in-place which sounds like a bookkeeping nightmare particularly if an event is re-fired (something this PR does). I can see why the JavaFX designers decided to make copies instead.

@andy-goryachev-oracle wrote:

a) If the EventDispatcher does something different like creating the new events, it is up to that ED to deal with consequences. It means the developer wants to create new events and not to have the link between the events.

An EventDispatcher that creates a new event without linking it to the original will break this PR. But that's a consequence you're creating. You can't say the developer is making a choice about event linking when the very notion is new with this PR.

I agree with @mstr2, the dispatch machinery has an existing publicly accessible mechanism for communicating whether an event was consumed and it works fine. The documentation might be fuzzy around the edges but that can be solved.

[andy-goryachev-oracle]

/csr unneeded

[openjdk]

@andy-goryachev-oracle determined that a CSR request is not needed for this pull request.

[andy-goryachev-oracle]

Removed 'final' keyword from existing public APIs.

The changes are limited to TextFieldBehavior and SpinnerSkin.

[andy-goryachev-oracle]

An EventDispatcher that creates a new event without linking it to the original will break this PR. But that's a consequence you're creating. You can't say the developer is making a choice about event linking when the very notion is new with this PR.

Actually, this statement is not correct: the consumed flag will be propagated, but only if propagateConsume was set by the helper, which is being done in a very specific case.

[beldenfox]

Actually, this statement is not correct: the consumed flag will be propagated, but only if propagateConsume was set by the helper, which is being done in a very specific case.

If a perfectly legal, properly written dispatcher in the chain creates a new event without using copyFor the propagateConsume link will be broken and this PR will not work correctly. That's solely because this PR is trying to create a new way of communicating whether an event is consumed or not instead of using the existing mechanism.

[andy-goryachev-oracle]

If a perfectly legal, properly written dispatcher in the chain creates a new event

As I mentioned earlier, this is the problem of the dispatcher. Anyone can fire an event.

But this PR does not link the two events generally, but addresses one specific scenario.

[mstr2]

/csr unneeded

You will need a CSR, because you propose to make the public consumed field private (and change its type).

[beldenfox]

But this PR does not link the two events generally, but addresses one specific scenario.

The event is being fired which means it may pass through any number of dispatchers some of which may be written by outside developers.

As I mentioned earlier, this is the problem of the dispatcher. Anyone can fire an event.

There are rules for how a dispatcher works. A dispatcher that follows those rules can still break this PR. This suggests that the problem is with the PR and not the dispatcher. And there is a way to write this PR that works with the existing rules governing dispatchers.

[andy-goryachev-oracle]

you are right, csr is needed.

/csr

[openjdk]

@andy-goryachev-oracle has indicated that a compatibility and specification (CSR) request is needed for this pull request.

@andy-goryachev-oracle please create a CSR request for issue JDK-8337246 with the correct fix version. This pull request cannot be integrated until the CSR request is approved.

[kevinrushforth]

You will need a CSR, because you propose to make the public consumed field private (and change its type).

Thanks for pointing this out. I haven't had time to look at this PR yet, but this aspect of the change is a breaking API change, and is not something we would generally do. There would need to be an extremely compelling reason to do this. I am unconvinced that there is.

I haven't formed an opinion on the approach itself, since I haven't read through all of the discussion. If we do end up going this route, there are behavioral aspects that need to be captured in a CSR as well.

[andy-goryachev-oracle]

There are rules for how a dispatcher works. A dispatcher that follows those rules can still break this PR.

Here are the rules (in EventDispatcher interface):

    /**
     * Dispatches the specified event by this {@code EventDispatcher}. Does
     * any required event processing. Both the event and its further path can
     * be modified in this method. If the event is not handled / consumed during
     * the capturing phase, it should be dispatched to the rest of the chain
     * ({@code event = tail.dispatch(event);}).
     *
     * @param event the event do dispatch
     * @param tail the rest of the chain to dispatch event to
     * @return the return event or {@code null} if the event has been handled /
     *      consumed
     */

I don't see where it talks about creating new events, handling of cloned events and their ::isConsumed in the context of EH. The null return value, in my opinion, is only significant in the context of further event dispatching: null means no more dispatch.

If the dispatcher does something outside of the scope of event dispatching, it's the responsibility of the said dispatcher to deal with the consequences.

I don't see how this relates to the narrow scope of this PR.

[bridgekeeper]

@andy-goryachev-oracle This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

[andy-goryachev-oracle]

converting to Draft to investigate a solution that will not require public API changes

[andy-goryachev-oracle]

/csr unneeded

[openjdk]

@andy-goryachev-oracle determined that a CSR request is not needed for this pull request.