On Mon, 7 Oct 2024 20:17:23 GMT, Martin Fox <m...@openjdk.org> wrote:
>> macOS processes a shortcut key like Cmd+A in two phases. In the first phase
>> it’s shopped around as a “key equivalent”. If it’s not consumed as a key
>> equivalent it enters the second phase and processed as a normal keyDown
>> event. Among other things the key equivalent phase ensures the shortcut will
>> be seen by the system menu bar *before* being treated as a keyDown. This is
>> the opposite of how JavaFX works; it expects a key event to be fired at the
>> current focus node which gets first crack at the event before it works its
>> way out to the menu bar.
>>
>> We can’t really opt out of the key equivalent phase but we can get the event
>> before the system menu bar does. Our implementation of performKeyEquivalent
>> pushes the event through the JavaFX scene graph but has no way of knowing if
>> the scene graph consumed it. The result is that a consumed event is always
>> handed to the system menu bar where it can also trigger a menu item.
>>
>> This PR introduces a variant of notifyKey that returns a boolean indicating
>> whether the event was consumed or not. If the event was consumed
>> performKeyEquivalent doesn’t allow it to continue on to the system menu bar.
>>
>> I’m trying to fix this old, old problem because I’ve seen at least one
>> JavaFX app tie itself up in knots trying to work around this. Despite the
>> number of files being touched it is not a deep fix; there’s just a boolean
>> return value that needs to be plumbed through multiple layers.
>
> Martin Fox has updated the pull request with a new target base due to a merge
> or a rebase. The incremental webrev excludes the unrelated changes brought in
> by the merge/rebase. The pull request contains eight additional commits since
> the last revision:
>
> - Merge remote-tracking branch 'upstream/master' into sysmenudup
> - Added some comments
> - Removed redundant check to avoid having the same NSEvent sent into
> the scene graph twice.
> - performKeyEquivalent and keyDown now do the same thing with a guard
> to prevent double-processing.
> - Merge remote-tracking branch 'upstream/master' into sysmenudup
> - Updated robot test to run on other platforms
> - Added Robot test
> - If JavaFX processes a shortcut it is not sent on to the system menu bar
modules/javafx.graphics/src/main/java/com/sun/javafx/tk/quantum/GlassViewEventHandler.java
line 240:
> 238:
> 239: @SuppressWarnings("removal")
> 240: @Override public boolean handleKeyEvent(View view, long time, int
> type, int key,
minor: insert newline after `override`?
modules/javafx.graphics/src/main/java/com/sun/javafx/tk/quantum/GlassViewEventHandler.java
line 250:
> 248: keyNotification.modifiers = modifiers;
> 249:
> 250: final boolean consumed = QuantumToolkit.runWithoutRenderLock(()
> -> {
minor: we probably don't need the `final` here
modules/javafx.graphics/src/main/java/javafx/scene/Scene.java line 2196:
> 2194: (sceneFocusOwner != null && sceneFocusOwner.getScene()
> == Scene.this) ? sceneFocusOwner : Scene.this;
> 2195:
> 2196: if (eventTarget == null) return false;
minor: insert braces?
modules/javafx.graphics/src/main/native-glass/mac/GlassView3D.h line 57:
> 55: BOOL isHiDPIAware;
> 56:
> 57: NSEvent *lastKeyEvent;
very minor: we probably should stay away from trying to align names, especially
since preceding lines don't.
modules/javafx.graphics/src/main/native-glass/mac/GlassView3D.m line 250:
> 248: self->handlingKeyEvent = NO;
> 249: self->didCommitText = NO;
> 250:
do we need
`[lastKeyEvent release];`
here?
tests/system/src/test/java/test/robot/javafx/scene/MenuDoubleShortcutTest.java
line 58:
> 56: static CountDownLatch startupLatch = new CountDownLatch(1);
> 57:
> 58: static volatile Stage stage;
I don't think we need this field
tests/system/src/test/java/test/robot/javafx/scene/MenuDoubleShortcutTest.java
line 59:
> 57:
> 58: static volatile Stage stage;
> 59: static volatile TestApp testApp;
+1 for making this field `volatile`
-------------
PR Review Comment: https://git.openjdk.org/jfx/pull/1528#discussion_r1792389733
PR Review Comment: https://git.openjdk.org/jfx/pull/1528#discussion_r1792390373
PR Review Comment: https://git.openjdk.org/jfx/pull/1528#discussion_r1792392486
PR Review Comment: https://git.openjdk.org/jfx/pull/1528#discussion_r1792394464
PR Review Comment: https://git.openjdk.org/jfx/pull/1528#discussion_r1792396688
PR Review Comment: https://git.openjdk.org/jfx/pull/1528#discussion_r1792400400
PR Review Comment: https://git.openjdk.org/jfx/pull/1528#discussion_r1792400740
