8288893: Popup and its subclasses cannot input text from InputMethod #1634
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,185 @@ | ||
| /* | ||
| * Copyright (c) 2024, Oracle and/or its affiliates. All rights reserved. | ||
| * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER. | ||
| * | ||
| * This code is free software; you can redistribute it and/or modify it | ||
| * under the terms of the GNU General Public License version 2 only, as | ||
| * published by the Free Software Foundation. Oracle designates this | ||
| * particular file as subject to the "Classpath" exception as provided | ||
| * by Oracle in the LICENSE file that accompanied this code. | ||
| * | ||
| * This code is distributed in the hope that it will be useful, but WITHOUT | ||
| * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or | ||
| * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License | ||
| * version 2 for more details (a copy is included in the LICENSE file that | ||
| * accompanied this code). | ||
| * | ||
| * You should have received a copy of the GNU General Public License version | ||
| * 2 along with this work; if not, write to the Free Software Foundation, | ||
| * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA. | ||
| * | ||
| * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA | ||
| * or visit www.oracle.com if you need additional information or have any | ||
| * questions. | ||
| */ | ||
|
|
||
| package com.sun.javafx.scene; | ||
|
|
||
| import com.sun.javafx.scene.SceneHelper; | ||
| import java.lang.ref.WeakReference; | ||
| import java.util.LinkedList; | ||
| import javafx.beans.value.ChangeListener; | ||
| import javafx.beans.value.WeakChangeListener; | ||
| import javafx.event.EventHandler; | ||
| import javafx.scene.Node; | ||
| import javafx.scene.Scene; | ||
| import javafx.scene.input.InputMethodEvent; | ||
| import javafx.scene.input.InputMethodRequests; | ||
|
|
||
| /** | ||
| * Used to manage a collection of scenes which must coordinate enabling input | ||
| * method events and retrieving InputMethodRequests. PopupWindows do not have | ||
| * the OS focus and rely on events being posted first to a root | ||
| * (non-popup) scene and then routed through the PopupWindow stack. If any | ||
| * PopupWindow requires input method events they must be enabled on the root | ||
| * scene and input method requests from that scene must be routed to the | ||
| * PopupWindow. | ||
| */ | ||
| public class InputMethodStateManager { | ||
| /** | ||
| * The root non-popup scene which the OS sends input method | ||
| * events to. | ||
| */ | ||
| private final WeakReference<Scene> rootScene; | ||
|
|
||
| /** | ||
| * The scene for which we enabled input method events. | ||
| */ | ||
| private Scene currentEventScene; | ||
|
|
||
| /** | ||
| * The scene stack including the root. | ||
| */ | ||
| private final LinkedList<Scene> scenes = new LinkedList<Scene>(); | ||
|
|
||
| /** | ||
| * We listen for changes to the input method requests configuration | ||
| * on every scene in the stack. | ||
| */ | ||
| private final ChangeListener<InputMethodRequests> inputMethodRequestsChangedListener = | ||
| (obs, old, current) -> updateInputMethodEventEnableState(); | ||
| private final ChangeListener<EventHandler<? super InputMethodEvent>> onInputMethodTextChangedListener = | ||
| (obs, old, current) -> updateInputMethodEventEnableState(); | ||
|
|
||
| private final WeakChangeListener<InputMethodRequests> weakInputMethodRequestsChangedListener = | ||
| new WeakChangeListener(inputMethodRequestsChangedListener); | ||
| private final WeakChangeListener<EventHandler<? super InputMethodEvent>> weakOnInputMethodTextChangedListener = | ||
| new WeakChangeListener(onInputMethodTextChangedListener); | ||
|
|
||
| /** | ||
| * Constructs a new instance. Only root (non-popup) scenes should do this. | ||
| * | ||
| * @param scene the root {@link Scene} for which input methods should be enabled and disabled | ||
| */ | ||
| public InputMethodStateManager(Scene scene) { | ||
| this.rootScene = new WeakReference<>(scene); | ||
| this.scenes.add(scene); | ||
| this.currentEventScene = scene; | ||
| } | ||
|
|
||
| /** | ||
| * Add a new Scene to the stack. | ||
| */ | ||
| public void addScene(Scene scene) { | ||
| scenes.addFirst(scene); | ||
| updateInputMethodEventEnableState(); | ||
| } | ||
|
|
||
| /** | ||
| * Remove a Scene from the stack. | ||
| */ | ||
| public void removeScene(Scene scene) { | ||
| /** | ||
| * If this scene is going away we should cleanup any composition | ||
| * state. Hiding a window doesn't ensure proper cleanup. | ||
| */ | ||
| SceneHelper.finishInputMethodComposition(rootScene.get()); | ||
|
|
||
| Node focusOwner = scene.getFocusOwner(); | ||
| if (focusOwner != null) { | ||
| focusOwner.inputMethodRequestsProperty().removeListener(weakInputMethodRequestsChangedListener); | ||
| focusOwner.onInputMethodTextChangedProperty().removeListener(weakOnInputMethodTextChangedListener); | ||
| } | ||
|
|
||
| scenes.remove(scene); | ||
| updateInputMethodEventEnableState(); | ||
| } | ||
|
|
||
| /** | ||
| * Every Scene must call this before the focusOwner changes. | ||
| */ | ||
| public void focusOwnerWillChangeForScene(Scene scene) { | ||
| /** | ||
| * Calling finishInputMethodComposition is only necessary if there's a | ||
| * node that accepts IM events. But there's a system test that | ||
| * expects finishInputMethodComposition to be called whenever the | ||
| * focusOwner changes. To satisfy this test we call | ||
| * finishInputMethodComposition even if no IM is enabled | ||
| * (so currentEventScene will be null). This will be no-op as far as | ||
| * the OS is concerned. | ||
| */ | ||
| if (scene == currentEventScene || currentEventScene == null) { | ||
| Scene root = rootScene.get(); | ||
| if (root != null) { | ||
| SceneHelper.finishInputMethodComposition(root); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Every Scene must call this when the focusOwner changes. | ||
| */ | ||
| public void focusOwnerChanged(Node oldFocusOwner, Node newFocusOwner) { | ||
| if (oldFocusOwner != null) { | ||
| oldFocusOwner.inputMethodRequestsProperty().removeListener(weakInputMethodRequestsChangedListener); | ||
| oldFocusOwner.onInputMethodTextChangedProperty().removeListener(weakOnInputMethodTextChangedListener); | ||
| } | ||
| if (newFocusOwner != null) { | ||
| newFocusOwner.inputMethodRequestsProperty().addListener(weakInputMethodRequestsChangedListener); | ||
| newFocusOwner.onInputMethodTextChangedProperty().addListener(weakOnInputMethodTextChangedListener); | ||
| } | ||
| updateInputMethodEventEnableState(); | ||
| } | ||
|
|
||
| public Scene getRootScene() { | ||
| return rootScene.get(); | ||
| } | ||
|
|
||
| private void updateInputMethodEventEnableState() { | ||
| currentEventScene = null; | ||
| // Visit Scenes in order from top to bottom. | ||
| for (Scene scene : scenes) { | ||
| Node focusOwner = scene.getFocusOwner(); | ||
| if ((focusOwner != null) && | ||
| (focusOwner.getInputMethodRequests() != null) && | ||
| (focusOwner.getOnInputMethodTextChanged() != null)) { | ||
| currentEventScene = scene; | ||
| break; | ||
| } | ||
| } | ||
| Scene root = rootScene.get(); | ||
| if (root != null) { | ||
| SceneHelper.enableInputMethodEvents(root, currentEventScene != null); | ||
| } | ||
| } | ||
|
|
||
| public InputMethodRequests getInputMethodRequests() { | ||
| if (currentEventScene != null) { | ||
| Node focusOwner = currentEventScene.getFocusOwner(); | ||
| if (focusOwner != null) { | ||
| return focusOwner.getInputMethodRequests(); | ||
| } | ||
| } | ||
| return null; | ||
| } | ||
| } | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All this feels overly complicated -
Why do we need to keep a list of scenes? Can't we get this information indirectly
from Window.getWindows()?Since the
PopupWindowhas theownerWindowProperty, could we simply listen to the focusOwner property and get the "root" window and its scene by traversing the hierarchy? Do we really need to maintain derived lists when the InputMethodRequests is only needed during the handling of a specific event?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like trying to maintain derived lists either. What you're proposing is an interesting idea but I won't have time to work out the details until next week.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to recalculate the IM state whenever a PopupWindow is shown or hidden (because the set of focusOwners changes). In this PR I'm relying on the existing logic in the PopupWindow class to track when scenes are added or removed so I can match when the PopupWindow starts and stops event monitoring. This is done via the addScene and removeScene calls in the InputMethodStateManager.
In theory I could track the coming and going of scenes by monitoring window ownership lists. This would be complicated since it's recursive. The root scene's window only owns the first popup. If another popup is shown it's owned by the popup below it. I would have to attach change listeners to the root scene's window list, scan the list for popups, and then recursively add change listeners to their window lists. I would much rather track what the PopupWindow class is already doing rather than try to implement an entirely separate mechanism.
Given that I rely on the PopupWindow code to tell me when scenes come and go it's expedient to just keep a list of the scenes around. I suppose I could remove that list and instead work my way recursively through the window ownership lists but I don't think there's much to be gained by that.
The listeners attached to the focusOwner property are triggered too late in the process. The current system updates the OS state (via finishInputMethodComposition and enableInputMethodEvents) at very specific points in time and I don't want to disturb that. In particular all of this happens before we trigger the change listeners on the focusOwner property.