John,
The issue occurs inside the event dispatchers. An event is passed into
dispatchEvent which may return a separate event. What state is carried over
from the incoming event to the outgoing event? If there are unconsumed event
handlers attached to the incoming event are they also attached to the returned
event?
There’s nothing in the EventDispatcher documentation that says that any state
from the incoming event must be carried over to the outgoing event and
currently the system doesn’t rely on any state being carried over. But as I
understand it this PR will only work if the unconsumed event handlers are
copied from the incoming event to the outgoing event.
If the dispatcher uses copyFor the state should be copied correctly. My guess
is that the current JavaFX dispatchers will preserve this state but I haven’t
take the time to review all the code (there’s a bunch of them).
Martin
> On Nov 12, 2024, at 11:06 AM, John Hendrikx <john.hendr...@gmail.com> wrote:
>
> Hi Martin,
>
> I'm still unsure how this exactly is supposed to go wrong.
>
> Whenever you dispatch a new event, you should always follow-up and check if
> it was consumed after the dispatch finishes. If it is, then you mark the
> triggering event also as consumed. If this is done consistently everywhere,
> then the consumed flag should function as expected or not?
>
> Code that does is simply wrong:
>
> e -> {
> node.fireEvent(new KeyEvent(...)); // can't check if it was
> consumed!
> // consume "e" or not?
> }
>
> The above is a bug, and needs to be fixed if it is present in FX.
>
> So the way I see it, every time you fire a new event in response to another,
> it is your responsibility to ensure that you check if the new event was
> consumed, and if it was, to mark the initial event as consumed as well.
> Following this everywhere should make the consumed flag work as expected.
>
> What am I missing?
>
> --John
>
> On 12/11/2024 19:14, Martin Fox wrote:
>> (Here’s hoping this e-mail makes it out to the list. If this is an ongoing
>> problem I might have to switch to using my gmail address.)
>>
>> I’m writing up some notes to improve the documentation on event dispatch.
>> There seems to be only one rule governing dispatchers and that is “return
>> null from dispatchEvent to end dispatch”. Beyond that anything goes.
>>
>> From what I can tell this PR will work if the event passed to the handler is
>> the original event or a copy created using copyFor() and the event returned
>> from dispatchEvent is either the same one that was passed to the handlers or
>> a copy created using copyFor(). These are all reasonable assumptions but
>> there’s nothing in the API that enforces them. In the worst case an event
>> dispatcher may receive an event, pass an entirely new event to one of its
>> handlers, and then return yet another entirely new event from dispatchEvent.
>>
>> (In the codebase there are three classes that implement EventDispatcher and
>> thirteen that extend an existing EventDispatcher. My guess is that all of
>> them behave reasonably and copy their events using copyFor but I haven’t
>> actually cracked open the code to verify that.)
>>
>> At this point I’ll resort to hand-waving and speculation. Perhaps this
>> should be divorced from events and reframed in terms of the dispatch
>> process. Instead of “do this if this event is not consumed” it’s “do this if
>> the current dispatch ends without consuming an event”. The unconsumed event
>> handlers would then be attached to the dispatch chain instead of the event.
>> And that’s about as far as I’ve thought this through.
>>
>> Martin
>>
>>> On Nov 12, 2024, at 5:02 AM, John Hendrikx <john.hendr...@gmail.com> wrote:
>>>
>>> I think `discardUnconsumedEventHandlers` is still a bit unpredictable, as
>>> it would depend on handler order.
>>>
>>> The user handler would need to be called *after* Skin/Behavior handlers for
>>> it to be able to always discard any ifUnconsumed callbacks. If the Skin is
>>> replaced, then it will re-register handlers and those will then be called
>>> last...
>>>
>>> Of course, if we change Behaviors to do their work in filters, then we
>>> could get this to work properly.
>>>
>>> --John
>>>
>>> On 10/11/2024 08:12, Michael Strauß wrote:
>>>> In JavaFX, user code and skins share the same event system. Since
>>>> invocation order is fundamentally important for events, this leads to
>>>> a lot of problems when skins add event handlers to their controls.
>>>> Assume user code adds an event handler to a control, and _then_ sets a
>>>> skin that also adds an event handler for the same event. In this case,
>>>> the user-provided handler is invoked first. If the skin is set
>>>> _first_, the skin gets the first chance to handle the event (see also
>>>> https://bugs.openjdk.org/browse/JDK-8231245).
>>>>
>>>> Prioritized event handlers might be a solution for this problem, but
>>>> they are quite difficult to get right. Instead, I think we can get
>>>> almost all of the benefits using a much simpler solution: unconsumed
>>>> event handlers.
>>>>
>>>> We add a new method to the `Event` class:
>>>>
>>>> <E extends Event> void ifUnconsumed(EventHandler<E> handler)
>>>>
>>>> When an event filter or an event handler receives an event, it calls
>>>> the `ifUnconsumed` method with another event handler. Then, after both
>>>> phases of event delivery have completed, the list of unconsumed event
>>>> handlers associated with the event is invoked in sequence. Once an
>>>> unconsumed event handler consumes the event, further propagation is
>>>> stopped.
>>>>
>>>> Skins and their behaviors would then always use the unconsumed form of
>>>> event handlers:
>>>>
>>>> // inside a skin/behavior
>>>> control.addEventHandler(
>>>> KeyEvent.KEY_PRESSED,
>>>> e -> e.ifUnconsumed(event -> {
>>>> // act on the event
>>>> }));
>>>>
>>>> This allows user code to see an event during both the capture and
>>>> bubble phases without any inteference of the skin or behavior. If user
>>>> code doesn't consume the event, the skin or behavior gets to act on
>>>> it.
>>>>
>>>> In addition to that, we add a second new method to the `Event` class:
>>>>
>>>> void discardUnconsumedEventHandlers()
>>>>
>>>> Calling this method in an event handler allows user code to discard
>>>> any unconsumed event handlers that have been registered prior. Let's
>>>> consider an example where user code wants to prevent a control skin
>>>> from acting on the ENTER key event, but it also doesn't want to
>>>> consume the event. Adding an event handler to the control, and then
>>>> calling `discardUnconsumedEventHandlers()` inside of the event handler
>>>> achieves exactly that: it allows the ENTER key event to flow freely
>>>> through the control without interacting with it, since its unconsumed
>>>> event handlers will never be called.
>>>>
>>>> Here is the PR for this proposal: https://github.com/openjdk/jfx/pull/1633
