8288893: Popup and its subclasses cannot input text from InputMethod #1634
Conversation
… Popups and their root Scene.
|
👋 Welcome back mfox! A progress list of the required criteria for merging this PR into |
|
❗ This change is not yet ready to be integrated. |
Webrevs
|
|
Converting to Draft. My testing macros weren't setup correctly and I missed a test. |
|
/reviewers 2 |
|
@kevinrushforth |
|
I'll take a look at it after Thanksgiving. Somewhat related questions (sorry if totally unrelated): Q1. shouldn't the input method be tied to the |
|
@andy-goryachev-oracle I'll answer your questions in reverse order.
It's better to think that there's a separate IM associated with each operating system window but the OS only shows one at any time. That's the way the operating system API's work. There hasn't been any reason to track this inside JavaFX. Normally the OS tracks which Scene has focus and we track which Node is the focusOwner in that Scene and that's all the bookkeeping we need. Things only get complicated with PopupWindows.
Normally we do connect the input method with the Scene.focusOwner. But when PopupWindows are open keyboard events from one peer window are distributed across multiple Scenes so there's more than one focusOwner we could connect the input method to. It's up to JavaFX to figure out which Scene's focusOwner is relevant. |
|
Running the branch (with the latest master merged) on macOS 14.7.1, noticed that IME window does not show reliably:
It gets eventually shown if I commit and keep typing. Also, you may want to merge the latest master, this branch has been out for quite some time. edit: please ignore, it actually works as expected. I was typing "su" expecting a popup, but did not realized that "su" alone produced no candidates. One needs to type "suzuki" to get the candidate list displayed, or start with "a". |
| // This may generate events so it must be done while we're | ||
| // still monitoring owner events. | ||
| Scene scene = getScene(); | ||
| SceneHelper.getInputMethodStateManager(scene).removeScene(scene); |
There was a problem hiding this comment.
Window.scene is a property, so theoretically the application code can set a new Scene at any moment. How will that be handled?
There was a problem hiding this comment.
In a PopupWindow the scene there is no API to set the scene. It's managed internally by the system and should not change over the lifetime of the popup window. I will take a second look at that code next week to verify.
There was a problem hiding this comment.
One can extend PopupWindow and call the protected setScene(), so this is a valid scenario, I think.
There was a problem hiding this comment.
PopupWindow already specifies that subclasses should not do this:
Note to subclasses: the scene used by PopupWindow is very specifically managed by PopupWindow. This method is overridden to throw UnsupportedOperationException. You cannot specify your own scene.
So no, it is not a valid use case.
There was a problem hiding this comment.
More to the point, the setScene method is final in PopupWindow and will unconditionally throw UnsupportedOperationException
| * scene and input method requests from that scene must be routed to the | ||
| * PopupWindow. | ||
| */ | ||
| public class InputMethodStateManager { |
There was a problem hiding this comment.
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 PopupWindow has the ownerWindowProperty, 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.
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.
Why do we need to keep a list of scenes? Can't we get this information indirectly
from Window.getWindows()?
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.
Since the
PopupWindowhas theownerWindowProperty, could we simply listen to the focusOwner property and get the "root" window and its scene by traversing the hierarchy?
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.
There was a problem hiding this comment.
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.
| */ | ||
| public void addScene(Scene scene) { | ||
| if (scenes.contains(scene)) { | ||
| System.err.println("Popup scene already present"); |
There was a problem hiding this comment.
what is the point of outputting this message? is user supposed to do anything in response to it?
| @@ -559,6 +560,8 @@ private void doVisibleChanged(boolean visible) { | |||
| rootWindow = getRootWindow(ownerWindowValue); | |||
|
|
|||
| startMonitorOwnerEvents(ownerWindowValue); | |||
| Scene scene = getScene(); | |||
There was a problem hiding this comment.
very minor: this code and L577 can be moved before L548
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 change. In this PR the InputMethodStateManager installs listeners on the focusOwner to detect changes in the IM configuration.
Progress
Issue
Reviewers
Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/1634/head:pull/1634$ git checkout pull/1634Update a local copy of the PR:
$ git checkout pull/1634$ git pull https://git.openjdk.org/jfx.git pull/1634/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 1634View PR using the GUI difftool:
$ git pr show -t 1634Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/1634.diff
Using Webrev
Link to Webrev Comment