8359899: Stage.isFocused() returns invalid value when Stage fails to receive focus #1849
Conversation
|
👋 Welcome back lkostyra! A progress list of the required criteria for merging this PR into |
|
❗ This change is not yet ready to be integrated. |
Webrevs
|
|
@lukostyra This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply issue a |
|
/keepalive |
|
@lukostyra The pull request is being re-evaluated and the inactivity timeout has been reset. |
|
I’ve been testing this PR by launching the manual KeyboardTest app. The call to SetForegroundWindow fails but not before activating the window. Since it’s not the foreground window this PR correctly sets the JavaFX focused property back to false. But when I later click on the title bar to bring the window to the foreground the focused property is not updated. I’m not sure how we’re expected to detect that our HWND has come to the foreground. There’s no specific message sent when this happens and since the OS thinks the window is already active and focused we don’t get WM_ACTIVATE or WM_SETFOCUS. There’s some messages we could use heuristically (like WM_NCACTIVATE) but I couldn’t find anything more clear cut. I'll keep looking but it doesn't look like Windows makes this easy. |
|
@beldenfox thanks for checking. I remember doing a fair share of looking for a reliable answer to this and couldn't find anything specific, other that SetForegroundWindow being able to fail and having to "handle it". I'll get back to testing this starting next week as well, maybe we'll manage to track down some more reasonable solution. |
|
@lukostyra This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply issue a |
|
/touch I didn't get a chance to look into other solutions yet, but I will be investigating that soon. |
|
@lukostyra The pull request is being re-evaluated and the inactivity timeout has been reset. |
I played around with this for a bit and I cannot reproduce this behavior. When I start the app it gets brought to foreground and stage.isFocused() returns true as it should for me. Could it be the difference between Windows versions maybe? I'm running Win 11 24H2 at this point in time. |
|
Reviewers: @beldenfox @kevinrushforth /reviewers 2 |
|
@kevinrushforth |
kevinrushforth
left a comment
kevinrushforth
left a comment
There was a problem hiding this comment.
The code changes look reasonable to me. I did leave one question inline.
What is the best way to test this? Ideally, I'd like a way to locally force the case where a window cannot be made focusable and have that test check the value of Stage::isFocused, reporting it (incorrectly) as true without this fix and false with the fix. I would then like to click on the Window (as @beldenfox did), and verify that it now reports as true.
I was able to verify that I get a different failing error message when I run StageFocusTest with gradle daemon enabled. Without this fix, two test methods fail when they try to readback the color. With this fix, they report that the window doesn't have focus. So that seems to validate at least part of the fix.
| ::ShowWindow(hWnd, visible ? SW_SHOW : SW_HIDE); | ||
| ::ShowWindow(hWnd, visible ? SW_SHOWNA : SW_HIDE); |
There was a problem hiding this comment.
Is this change needed? I presume that the idea behind it is that if it is a focusable window, it will be activated anyway?
There was a problem hiding this comment.
Yes, this is part of the fix. The values in question are:
SW_SHOW- shows a window and activates it, sending theWM_ACTIVATEmessageSW_SHOWNA- shows a window but doesn't activate it - activation is postponed to us later calling::SetForegroundWindow()
By using SW_SHOWNA we can postpone the window activation and have ::SetForegroundWindow() trigger it. If ::SetForegroundWindow() succeeds, window gets activated and gains focus, and WM_ACTIVATE gets sent, which will then be captured by JavaFX message loop so we can notify Java-side that we gained focus. If ::SetForegroundWindow() fails, we see that in this function and we can notify Java-side the focus has been lost.
This is unfortunately a bit hacky, but I couldn't find another way of establishing whether we actually have focus or not, especially combining that with what ::SetForegroundWindow() returns. With SW_SHOW the WM_ACTIVATE message would go through despite ::SetForegroundWindow() failing and us ultimately not having focus.
My first attempt also considered leaving this part as-is and simply notifying Java-side if we lost focus or not based on ::SetForegroundWindow() result, but that wasn't consistent. Postponing the window activation seemed to me as the best bet here.
There was a problem hiding this comment.
On my system the SetForegroundWindow call activates the window and makes it the focus window. We get WM_ACTIVATE and WM_SETFOCUS and tell the Java side to set the focused property to true. Then SetForegroundWindow returns false and we tell the Java side to clear the focused property.
I'm also running an up-to-date version of Win 11. I can reproduce the ::SetForegroundWindow failure when launching from a MinGW terminal window but not from a Cygwin terminal. |
3 participants
This PR fixes
isFocused()returning invalid value when Stage fails to receive focus after callingStage.show(). This problem is Windows-only.In Windows the
SetForegroundWindow()API lists a set of conditions to successfully grant focus to a Window. If any of the conditions are not met, the API will return FALSE. JavaFX did not respect that and instead assumed that receivingWM_ACTIVATEwith our Window being activated is enough to assume the Window is in focus (which in some cases is not true).I first tried reacting to
WM_SETFOCUSandWM_KILLFOCUSbut it seems those messages are not sent when the window is shown for the first time (insteadWM_ACTIVATEis used).To correct this behavior, I noticed the following path is the most reliable:
ShowWindow()usingSW_SHOWNAinstead ofSW_SHOW- that makes the window visible but does NOT activate itSetForegroundWindow()- that will attempt to give the Window focus and will also activate it if it is successfulnotifyFocuscallback will be called viaWM_ACTIVATEhandlernotifyFocuscallback manually informing the upper layers the focus is lost. This establishes the correct state ofWindow.focusedproperty.With this change I observed that all tests pass as intended as long as two conditions are met (these are needed to satisfy
SetForegroundWindow()restrictions):If any of above two conditions is not met, some tests (including canary test from #1804) now timeout/fail when checking whether
Window.isFocused()is true.Manually started JavaFX apps (ex. Ensemble) run as they used to and still receive focus upon Stage showing.
Progress
Issue
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/1849/head:pull/1849$ git checkout pull/1849Update a local copy of the PR:
$ git checkout pull/1849$ git pull https://git.openjdk.org/jfx.git pull/1849/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 1849View PR using the GUI difftool:
$ git pr show -t 1849Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/1849.diff
Using Webrev
Link to Webrev Comment