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-r...@openjdk.org> on behalf of
Michael Strauß <michaelstr...@gmail.com>
*Date: *Friday, October 27, 2023 at 19:41
*To: *openjfx-dev <openjfx-dev@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.goryac...@oracle.com> wrote:
>
> Would it be possible to create a proposal in the JEP format
outlining the proposed public API?
>
>
>
> Thank you
>
> -andy
