[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-openjfx-dev
Subject:    Re: RFR: 8242544: CMD+ENTER key event crashes the application when invoked on dialog
From:       Martin Fox <duke () openjdk ! java ! net>
Date:       2021-12-29 17:18:17
Message-ID: H-r_2qBgZA1wqocdXtjWzoG2EYrDcRnDi8HBLne7KyY=.f5c7e798-6022-4fe5-95a2-a3eec242ab10 () github ! com
[Download RAW message or body]

On Wed, 29 Dec 2021 00:44:44 GMT, Andreas Heger <duke@openjdk.java.net> wrote:

> This also solves [JDK-8205915 [macOS] Accelerator assigned to button in dialog \
> fires menuItem in owning stage](https://bugs.openjdk.java.net/browse/JDK-8205915l). \
>  I must admit that I don't have 100% insight about what actually caused the \
> problems and how the event flow completely works. 
> In MacOS, key events with a command or control modifier are handled via the \
> performKeyEquivalent method, first. If no view or window handles such a shortcut \
> key event (indicated by a return value NO in performKeyEquivalent), the key event \
> is sent a second time via the sendEvent method. Before this fix, the method  \
> **sendJavaKeyEvent** in **GlassViewDelegate.m** seemed to be called twice: first by \
> performKeyEquivalent and then by sendEvent, since no responder returned YES in \
> performKeyEquivalent. For not handling the same event twice in JFX, there seemed to \
> be a workaround in  **sendJavaKeyEvent** in **GlassViewDelegate.m**. It only \
> handled the first call. The second call checked if the given event was exactly the \
> same like in the last call. In this case it simply returned and did not forward the \
> event. This means that all key events with a CMD or CTRL modifier were handled in \
> JFX in the performKeyEquivalent event loop, even though they actually weren't used \
> by the MacOS keyEquivale
 nt functionality and all responders returned NO in performKeyEquivalent. So MacOS \
sent the event again to all responders in the sendEvent loop. I assume that the \
window has been destroyed already, when MacOS tried to send the second event to it, \
because the closing was handled in the first call. Sending an event to a no longer \
existing window probably caused the crash by an unhandled exception.
> 
> It seems that this problem existed in the past and there was a workaround in \
> **GlassWindow.m**. In the method **performKeyEquivalent**, it was checked if the \
> window was closed and if so, it returned YES. However, nowadays the window closing \
> check did not work... self->gWindow->isClosed returned NO when I debugged it, \
> although the window should be closed by the key event. Returning YES in case of a \
> closed window is not a clean solution, anyway, because YES should mean that the \
> shortcut key equivalent is handled. Another point is that Apple writes that \
> overwriting performKeyEquivalent in an NSWindow subclass is discouraged. So, I \
> completely deleted the method from **GlassWindow.m**, since it only returned the \
> value of the super function, except for the non working closing check. 
> Since the default version of performKeyEquivalent in NSWindow always returns NO, \
> only deleting the method from **GlassWindow.m** did not fix the crash. So I tried \
> to solve the problem that a shortcut key event is handled in the \
> performKeyEquivalent loop. The call of **sendJavaKeyEvent** in \
> **GlassViewDelegate.m** which is caused by a performKeyEquivalent should not be \
> handled, actually. So, I simply removed this call which is done in \
> **GlassView3D.m** **performKeyEquivalent** method. By removing the call, there is \
> also no longer any need to check if the same key event was passed to \
> **sendJavaKeyEvent** in **GlassViewDelegate.m**, so this check is also removed. 
> I'm curious about your comments and reviews and I wonder if this is a clean \
> solution. All my tests so far seemed to be fine.

I can reproduce this with a small Mac app that does nothing but set the NSWindow's \
contentView to nil while handling `processKeyEvent:`. I'm not handling the event \
twice or destroying the NSWindow. The crash happens immediately after returning from \
my implementation of `processKeyEvent:` and back into Apple's code. There's something \
about Apple's implementation of this routine that's sensitive to having the \
contentView nulled out. I don't see the same crash if I set the contentView to nil \
inside a `keyDown:` event.

I would love to get rid of our `performKeyEquivalent:` methods but we can't. JavaFX \
assumes that the focused Node will get the first opportunity to process a KeyEvent. \
For example, if a TextArea has the focus it gets the first opportunity to process \
`Cmd-A` to select all text. A KeyEvent should only bubble up to the top level menus \
if the focused node doesn't handle it. When using the system menubar on Mac this \
means we need to implement `processKeyEquivalent:` to ensure that we can hand the key \
event to the focused JavaFX node before it gets handled by the main NSMenu.

Unfortunately there's currently no way to determine that the event was consumed by \
the node so the Mac glass code hands it over to the main menu anyway. This leads to \
bugs where one event triggers multiple actions like \
[JDK-8087863](https://bugs.openjdk.java.net/browse/JDK-8087863) and \
[JDK-8088897](https://bugs.openjdk.java.net/browse/JDK-8088897) and the ones listed \
over in javafxports. Most of the changes for fixing this are in PR #694.

You're correct that the implementation of `performKeyEquivalent:` in GlassWindow.m is \
probably not necessary. The `close` selector is sent to the NSWindow asynchronously \
so the `isClosed` flag will never be set at the end of `performKeyEquivalent:`. This \
has been true since sometime in 2015, see the comments in \
`Java_com_sun_glass_ui_mac_MacWindow__1close`.

-------------

PR: https://git.openjdk.java.net/jfx/pull/704


[prev in list] [next in list] [prev in thread] [next in thread]