Prioritized event handlers

Thu Nov 7 13:34:32 UTC 2024

Hi Andy,

Simply replacing a Skin can change the order of event handlers. In other 
words, if you registered your handler **after** the control was shown, 
then replace its Skin (which can be done by CSS state changes), then 
your handler is now called before the Skin/Behavior handlers.  Also 
removing your own handlers, then installing them back (in the same 
order) will not lead to the same result either.  The culprit is simply 
that two different parties (FX and the User) are sharing the same system 
-- this is mostly fine for properties, but Events are stateful, and so 
the processing order matters.

I've given this quite a lot of thought recently, and I also looked at 
the Spinner problem, and I think I do have an all encompassing solution 
for both the ordering problem and the Spinner problem.

Observations:

Controls have some trouble keeping their Skin internals hidden from the 
outside world.  For the user, the Spinner is a black box; it is the 
thing that gets the focus and accepts input, even though the actual 
implementation may be delegating this to an internal component (in the 
case of the default SpinnerSkin, a FakeFocusTextField -- a detail that 
you can see "leaking" out when registering a listener on Scene#focusOwner).

Secondly, because Skins are re-using normal controls, which only work 
when focused, they sometimes have to "fake" the focus (this is a problem 
I won't be solving in this post).  IMHO however, this should be handled 
differently altogether, perhaps with some kind of focus delegation 
mechanism as part of Behaviors -- the user observable focus should IMHO 
always be the Control, as it is a black box.  The current fake focus 
implementation tries to keep this hidden (by not firing change 
listeners) which is incredibly dubious, and also fails (you can still 
see internals being focused with Scene#focusOwner).

Analysis:

Inside SpinnerSkin, there is a lot of jumping through hoops in the form 
of filtering events, copying them, and being aware of what the Behavior 
needs (SpinnerSkin for example knows that the Behavior has need of the 
UP/DOWN/LEFT/RIGHT keys, it shouldn't...) -- all this jumping through 
hoops is to make the internal TextField component receive events, but at 
the same time, not confusing the Behavior with duplicated events (as the 
Behavior handlers are in between the event root (the window) and the 
target (the internal textfield) events flow past the Behavior's handlers).

Solution:

The Behavior event handlers are at the Control level -- as far as the 
user is concerned, this is the target and also final destination (there 
is nothing deeper).  In effect you could say that the Behavior could 
register either a Filter or a Handler, and the result would be the same 
-- filtering down from Scene -> Node -> Spinner and then bubbling up 
from Spinner -> Node -> Scene, it shouldn't matter if the Behavior 
registers a filter or a handler as they're at the same position (of 
course there are some small differences, but I think we actually WANT 
filters in behaviors, more on this later).

One of the problems is that the event handlers in the behavior trigger 
**after** the internal TextField -- but we have the perfect mechanism to 
swap this around; if the Behavior registered a filter (which as I said 
earlier is sort of at the same position conceptually) then it would 
trigger **before** the internal TextField.  Spinner is then free to 
process and consume the arrow keys it needs, and the Skin would not need 
to be aware of this (ie. if a different Behavior decides to consume the 
+/- keys, then it would work with the current Skin).  The Skin then just 
forwards any events that arrive at the Control level still (everything 
minus potential arrow keys consumed by the Behavior) to the internal 
text field.

The second part of this solution relates to how to get Events to go down 
to the internal TextField.  Events are fired at the current focus owner, 
which is the Control.  The TextField only has fake focus.  So we need to 
take events that arrive at the Control, copy them and send them to the 
internal TextField.  However, there is a small problem -- the standard 
event mechanism will make such copies filter down from Window level all 
the way to the TextField level (passing the Spinner control).  But this 
isn't want we want at all.  Spinner is a black box; when we want to fire 
an event into the internals, we don't want this event to be exposed 
either (aside from the problems that causes, with having to be careful 
not to copy the event again as it will pass your forwarding handler 
again).  In effect, we want a different event dispatch hierarchy that is 
limited to the Control only.  So, why not build a dispatch chain that 
exclusively targets the internals?  This chain in this case only 
consists of one EventDispatcher, the one from the internal TextField.  
The event copy is fired at it and the control works as expected.  It 
automatically also solves the "ENTER" problem described in 
https://bugs.openjdk.org/browse/JDK-8337246 without further hacks.

Takeaways:

I now think Behaviors should always be doing their event handling in 
filters, for the simple reason that they should get to act first before 
the Skin gets to act on an event; events arriving at the Control level 
which is the final destination can be handled by either a handler or a 
filter and there would be no discernible difference.  Not only does this 
solve the problem with the internal text field and the Skin needing to 
be aware of what keys Behavior will be handling, but it also then makes 
behaviors always act before user event handlers, creating consistency 
here (ie. replacing a Skin will not affect event handler order).  This 
frees up handlers for user purposes.  The ordering problem moves to the 
filters, so we may still want to do a solution here (see below).

Event handler order when Skins get re-added:

Handlers registered by users can change order with regards to handlers 
registered by Behaviors when the Skin change or depending on when the 
user registered their handlers.  This is problematic for events as they 
are stateful and can be consumed, and so an order change can lead to a 
functionality change. This is not user friendly, as the user expects 
that the event handler and filter infrastructure is for their use 
alone.  I can see a few things we could explore further:

- See if it is possible to add some internal API that allows Behaviors 
to register filters or handlers first/last -- note that "last" here 
means **always** last, even when user handlers are added later (first 
would be an easier option).
- See if it is possible to change how `buildEventDispatchChain` works 
for Controls to include another dispatcher level exclusively for 
Behaviors.   Where a standard chain would look like 
Window->Scene->Nodes->Control, it would become 
Window->Scene->Nodes->Control->Behavior(private) -- as this is a 
completely separate level, handlers from users and behaviors cannot 
become intertwined and so there will always be a predictable ordering

On 07/11/2024 01:03, Andy Goryachev wrote:
>
> Dear Michael:
>
> What happened to this proposal?  I would like to restart the 
> discussion, if possible.
>
> More specifically, I would like to discuss the following topics:
>
>  1. the reason the discussion was started was due to "priority
>     inversion" problem in Controls/Skins, ex.: JDK-8231245
>     <https://bugs.openjdk.org/browse/JDK-8231245> Controls' behavior
>     must not depend on sequence of handler registration.  Do we have
>     this problem elsewhere?  In other words, does it make sense to
>     change the API at the EventDispatcher level when the problem can
>     be easily solved by the InputMap at the Control level?
>  2. dispatching of events that have been consumed (as mentioned in the
>     earlier discussion)
>  3. problem of creating unnecessary clones of events via
>     Event.copyFor(), leading to ex.: JDK-8337246
>     <https://bugs.openjdk.org/browse/JDK-8337246> SpinnerSkin does not
>     consume ENTER KeyEvent when editor ActionEvent is consumed
>  4. why do we need Event.copyFor() in the first place?why does Event
>     contain the target??
>
1. You would have the same problem with other events that Behaviors 
listen to (ie. mouse events), or events they handle without going 
through the input map (TYPED, RELEASED), or events they handle with a 
parent event type (all KeyEvents, not a specific one).

2. This is IMHO a bug, it only happens for the same event type at one 
level, it should simply be fixed

3. See above, but in short, I think the problem is not the event clones, 
but the fact that these clones are traversing the entire 
Window->Scene->Node hierarchy.  Events with a restricted hierarchy solve 
this.

4. The target allows you to register the same event handler instance 
multiple times on multiple different controls without having to create a 
new lambda each time that knows which control (target) it is for. It has 
a similar function to the Observable parameter in InvalidationListeners 
and ChangeListeners.  In effect, having the Target will also make it 
possible in the future to share a single input map with all controls of 
the same type (currently there is a ton of memory being wasted here 
duplicating input maps, especially for controls with large input maps 
like TextFields).

--John

> Too many topics, yes, we might want to split the discussion into 
> separate threads.
>
> -andy
>
> *From: *openjfx-dev <openjfx-dev-retn at openjdk.org> on behalf of 
> Michael Strauß <michaelstrau2 at gmail.com>
> *Date: *Friday, October 27, 2023 at 19:41
> *To: *openjfx-dev <openjfx-dev at openjdk.org>
> *Subject: *Re: Prioritized event handlers
>
> Here is the proposal:
> https://gist.github.com/mstr2/4bde9c97dcf608a0501030ade1ae7dc1
>
> Comments are welcome.
>
>
> On Fri, Oct 27, 2023 at 8:21 PM Andy Goryachev
> <andy.goryachev at oracle.com> wrote:
> >
> > Would it be possible to create a proposal in the JEP format 
> outlining the proposed public API?
> >
> >
> >
> > Thank you
> >
> > -andy
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/openjfx-dev/attachments/20241107/c60730e7/attachment-0001.htm>