On Tue, 12 Nov 2024 15:08:11 GMT, Martin Fox <m...@openjdk.org> wrote:
>> Input methods don’t work for text controls inside Popups. This PR fixes that.
>>
>> Some background:
>>
>> A PopupWindow always has an owner. The owner of the first Popup is a
>> standard Window; I’ll refer to that as the root window. But a popup can show
>> another popup (for example, a popup may contain a ComboBox which opens a
>> menu which is also a Popup) resulting in a stack of PopupWindows.
>>
>> Under the hood each PopupWindow has its own scene (this is not visible in
>> the API). So if there’s a stack of PopupWindows there’s also a stack of
>> scenes with the root window’s scene at the bottom.
>>
>> The OS focus always stays with the root window (in part because JavaFX can’t
>> move the OS focus when it’s embedded). This means a KeyEvent is initially
>> fired at the focusOwner in the root window’s scene. Each PopupWindow in the
>> stack uses an EventRedirector to refire that key event at its own
>> focusOwner. In effect KeyEvents start processing at the top-most scene in
>> the stack and work their way down to the root scene.
>>
>> There are several reasons why Input Methods aren’t currently working for
>> Popups.
>>
>> - InputMethodEvents are not being redirected. This PR extends the
>> EventRedirector to refire InputMethod events in the same way it refires
>> KeyEvents.
>>
>> - If a PopupWindow contains a TextInput control it will enable input method
>> events on its scene which has no effect since that scene doesn’t have OS
>> focus. If a PopupWindow wants to enable IM events it needs to do so on the
>> root window’s scene. Put another way IM events should be enabled on the root
>> scene if and only if one of the focusOwners in the scene stack requires IM
>> events.
>>
>> - The OS always retrieves the InputMethodRequests from the root window’s
>> scene. InputMethodRequests should be retrieved from whichever focusOwner in
>> the scene stack is processing InputMethodEvents.
>>
>> In this PR the root scene creates an InputMethodStateManager object and
>> shares it with all of the Popup scenes in the stack. Any time the focusOwner
>> changes in a scene it tells the InputMethodStateManager so it can determine
>> whether IM events should be enabled on the root scene. The root scene also
>> calls on the InputMethodStateManager to retrieve InputMethodRequests so it
>> can grab them from the appropriate Node in the scene stack.
>>
>> This PR also fixes JDK-8334586. Currently the scene only enabled or disables
>> IM events when the focusOwner changes. If a node’s skin is installed after
>> it becomes focusOwner the scene won’t notice the cha...
>
> Martin Fox has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Tweak to satisfy system test.
This PR does fix the issue at hand (merged the latest master and tested on
macOS 15.1.1)
It still feels a bit heavy (re: ordered scene list), but I don't have a good
suggestion, so I am ok with this approach.
left some minor comments - will re-approve if you decide to make changes.
modules/javafx.graphics/src/main/java/com/sun/javafx/scene/InputMethodStateManager.java
line 95:
> 93: public void addScene(Scene scene) {
> 94: if (scenes.contains(scene)) {
> 95: System.err.println("Popup scene already present");
what is the point of outputting this message? is user supposed to do anything
in response to it?
modules/javafx.graphics/src/main/java/javafx/stage/PopupWindow.java line 563:
> 561:
> 562: startMonitorOwnerEvents(ownerWindowValue);
> 563: Scene scene = getScene();
very minor: this code and L577 can be moved before L548
-------------
Marked as reviewed by angorya (Reviewer).
PR Review: https://git.openjdk.org/jfx/pull/1634#pullrequestreview-2500497302
PR Review Comment: https://git.openjdk.org/jfx/pull/1634#discussion_r1882697050
PR Review Comment: https://git.openjdk.org/jfx/pull/1634#discussion_r1882699944
