Thanks, very interesting! The first thing that popped up is that FX does expose the Skin's implementation via Node.lookup() and CSS.
-andy From: openjfx-dev <openjfx-dev-r...@openjdk.org> on behalf of Tobias Oelgarte <tobias.oelga...@gmail.com> Date: Friday, November 8, 2024 at 12:47 To: openjfx-dev@openjdk.org <openjfx-dev@openjdk.org> Subject: Re: Prioritized event handlers This whole situation reminds me very much of Shadow DOM [1], an approach for components (controls/skins) with a complex internal structure that should not be visible from the outside. The handling of events could serve as a template here. [2] [1] https://developer.mozilla.org/en-US/docs/Web/API/Web_components/Using_shadow_DOM [2] https://pm.dartus.fr/posts/2021/shadow-dom-and-event-propagation/ On 07.11.24 20:06, Andy Goryachev wrote: > 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). This is a very strange rationale, in my opinion. And why talk about Controls when the Event.copyFor() is at the very fundamental level? To me, this severely complicates everything, including breaking isConsumed() logic. Sending events that are not designed to bubble up seems to run contrary to the whole event dispatching idea. Are you trying to explain the current implementation? Swing never needed to store the target in the event, and it works just fine. This brings me to another problem I am struggling with: why expose the event dispatchers?? This should have never been done, in my opinion. Swing's FwDispatcher is an implementation detail. But instead, we seem to be dealing with unnecessarily increased complexity, including misbehaving EventDispatcher implementations. May be it's just me, I don't see the beauty of this thing. -andy From: openjfx-dev <openjfx-dev-r...@openjdk.org><mailto:openjfx-dev-r...@openjdk.org> on behalf of John Hendrikx <john.hendr...@gmail.com><mailto:john.hendr...@gmail.com> Date: Thursday, November 7, 2024 at 05:35 To: openjfx-dev@openjdk.org<mailto:openjfx-dev@openjdk.org> <openjfx-dev@openjdk.org><mailto:openjfx-dev@openjdk.org> Subject: Re: Prioritized event handlers 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><mailto:openjfx-dev-r...@openjdk.org> on behalf of Michael Strauß <michaelstr...@gmail.com><mailto:michaelstr...@gmail.com> Date: Friday, October 27, 2023 at 19:41 To: openjfx-dev <openjfx-dev@openjdk.org><mailto: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><mailto: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
