Re: RFR: 8320912: IME should commit on focus change
On Mon, 29 Apr 2024 14:09:03 GMT, yosbits wrote: > I found a serious problem with this change. Thank you! I've created https://bugs.openjdk.org/browse/JDK-8331319 feel free to update / clarify the information in the ticket. - PR Comment: https://git.openjdk.org/jfx/pull/1356#issuecomment-2082942526
Re: RFR: 8320912: IME should commit on focus change
On Tue, 30 Jan 2024 20:32:36 GMT, Martin Fox wrote:
> This is a Mac only bug. If the user was in the middle of IM text composition
> and clicked on a different node the partially composed text was left in the
> old node and the IM window wasn't dismissed. This PR implements the existing
> finishInputMethodComposition call so it can commit the text and dismiss the
> IM window before focus moves away from the node where composition was taking
> place.
>
> This PR changes the implementation of `unmarkText` to match what we want and
> what Apple says it should do ("The text view should accept the marked text as
> if it had been inserted normally"). With that said I haven't found an IME
> that calls this routine.
**I found a serious problem with this change.**
After applying this change, the IME cannot be typed after the small window is
displayed.
Even basic applications that use ContextMenu, ChoiceBox, ComboBox, MenuButton,
etc. are affected.
The impact of this problem will be perceived as block level in the IME usage
environment.
This problem is no longer reproduced by reverting the changes in JDK-8320912.
However, since JDK-8301900 depends on the changes in JDK-8320912, 8301900 was
also reverted.
This problem can be reproduced with the following application
``` Java
import javafx.application.Application;
import javafx.scene.Scene;
import javafx.scene.control.ChoiceBox;
import javafx.scene.control.ComboBox;
import javafx.scene.control.Label;
import javafx.scene.control.MenuButton;
import javafx.scene.control.MenuItem;
import javafx.scene.control.TextArea;
import javafx.scene.control.TextField;
import javafx.scene.layout.HBox;
import javafx.scene.layout.VBox;
import javafx.stage.Stage;
public class ImeTest extends Application{
enum Items {
A,
B,
C
}
@Override
public void start(Stage primaryStage) throws Exception {
ChoiceBox choiceBox = new ChoiceBox<>();
ComboBox comboBox = new ComboBox<>();
MenuButton menuButton = new MenuButton();
menuButton.getItems().addAll(new MenuItem("1"), new
MenuItem("2"));
choiceBox.getItems().addAll(Items.values());
comboBox.getItems().addAll(Items.values());
VBox root = new VBox(
new Label("1. Select any of the following control
items."),
new HBox( 8, choiceBox, comboBox, menuButton),
new Label("2. Entering text with the IME."),
new HBox(8, new TextField(), new TextArea()));
Scene scene = new Scene(root, 600, 600);
primaryStage.setScene(scene);
primaryStage.show();
}
public static void main(String[] args) {
Application.launch(args);
}
}
-
PR Comment: https://git.openjdk.org/jfx/pull/1356#issuecomment-2082858747
Re: RFR: 8320912: IME should commit on focus change
On Fri, 2 Feb 2024 16:11:52 GMT, Kevin Rushforth wrote:
>> This is a Mac only bug. If the user was in the middle of IM text composition
>> and clicked on a different node the partially composed text was left in the
>> old node and the IM window wasn't dismissed. This PR implements the existing
>> finishInputMethodComposition call so it can commit the text and dismiss the
>> IM window before focus moves away from the node where composition was taking
>> place.
>>
>> This PR changes the implementation of `unmarkText` to match what we want and
>> what Apple says it should do ("The text view should accept the marked text
>> as if it had been inserted normally"). With that said I haven't found an IME
>> that calls this routine.
>
> modules/javafx.graphics/src/main/java/com/sun/glass/ui/mac/MacView.java line
> 81:
>
>> 79: @Override native protected void _exitFullscreen(long ptr, boolean
>> animate);
>> 80: @Override native protected void _enableInputMethodEvents(long ptr,
>> boolean enable);
>> 81: @Override native protected void _finishInputMethodComposition(long
>> ptr);
>
> Now that you override this method on Mac, I recommend changing the comment in
> the base class from `... needed only on Windows` to `... needed only on some
> platforms`.
I'll pick up the comment change in the PR #1351.
-
PR Review Comment: https://git.openjdk.org/jfx/pull/1356#discussion_r1476296254
Re: RFR: 8320912: IME should commit on focus change
On Tue, 30 Jan 2024 20:32:36 GMT, Martin Fox wrote:
> This is a Mac only bug. If the user was in the middle of IM text composition
> and clicked on a different node the partially composed text was left in the
> old node and the IM window wasn't dismissed. This PR implements the existing
> finishInputMethodComposition call so it can commit the text and dismiss the
> IM window before focus moves away from the node where composition was taking
> place.
>
> This PR changes the implementation of `unmarkText` to match what we want and
> what Apple says it should do ("The text view should accept the marked text as
> if it had been inserted normally"). With that said I haven't found an IME
> that calls this routine.
The code changes look good. I did some cursory testing (since both you and Andy
did extensive testing) and it fixes the bug.
I left one minor comment. If you choose to fix it, I'll reapprove.
modules/javafx.graphics/src/main/java/com/sun/glass/ui/mac/MacView.java line 81:
> 79: @Override native protected void _exitFullscreen(long ptr, boolean
> animate);
> 80: @Override native protected void _enableInputMethodEvents(long ptr,
> boolean enable);
> 81: @Override native protected void _finishInputMethodComposition(long
> ptr);
Now that you override this method on Mac, I recommend changing the comment in
the base class from `... needed only on Windows` to `... needed only on some
platforms`.
-
Marked as reviewed by kcr (Lead).
PR Review: https://git.openjdk.org/jfx/pull/1356#pullrequestreview-1859616385
PR Review Comment: https://git.openjdk.org/jfx/pull/1356#discussion_r1476258024
Re: RFR: 8320912: IME should commit on focus change
On Tue, 30 Jan 2024 20:32:36 GMT, Martin Fox wrote:
> This is a Mac only bug. If the user was in the middle of IM text composition
> and clicked on a different node the partially composed text was left in the
> old node and the IM window wasn't dismissed. This PR implements the existing
> finishInputMethodComposition call so it can commit the text and dismiss the
> IM window before focus moves away from the node where composition was taking
> place.
>
> This PR changes the implementation of `unmarkText` to match what we want and
> what Apple says it should do ("The text view should accept the marked text as
> if it had been inserted normally"). With that said I haven't found an IME
> that calls this routine.
You are right - I should have checked a native app. It works as expected then.
Thank you for fixing this bug!
-
Marked as reviewed by angorya (Reviewer).
PR Review: https://git.openjdk.org/jfx/pull/1356#pullrequestreview-1856993847
Re: RFR: 8320912: IME should commit on focus change
On Wed, 31 Jan 2024 21:49:32 GMT, Andy Goryachev wrote: >>>notice how the underline under あ is still there in the first text field. >> >> That behavior surprised me the first time I saw it but I eventually figured >> out that it's a feature, not a bug. If you switch back to the original >> window by clicking on the title bar you should pick up composition exactly >> where you left off e.g. the IM window should appear showing the same >> candidates it was showing earlier. That's the way native Mac apps like >> TextEdit work (most of the time, every now and then clicking on the title >> bar commits the text). >> >> On a technical level the focusOwner for the Scene isn't changing so JavaFX >> doesn't think there's any reason to finish composition. And apparently the >> OS agrees and relies on Glass to retain enough state that it can pick up >> where it left off. > >> e.g. the IM window should appear showing the same candidates it was showing >> earlier. > > Thank you for clarification! > The behavior is different, let me explain: > > using the MonkeyTester, open TextField page and the Native-to-ascii tool > (Tools -> Native-to-ascii). > 1. switch to Japanese input > 2. type "su" into the text field in the main window > 3. click on a "native" text area in the other window > 4. type "zu" in the native window > 5. **unexpectedly**, the text field in the main window shows the symbol still > underlined > 6. click on the main window title to focus back on the original text field. > type "zuki" > EXPECTED: > - the text field should contain 鈴木 > ACTUAL: > - the text is すずき, the last two symbols underlined: > >  > > It looks like the text was indeed committed (despite it being displayed > underlined at step 5), and getting back to it from another window does not > restore the IME to the state expected at the step 2. > > Interestingly, if instead of going to a different window of the same javafx > application, the user clicks elsewhere on some other application window and > then goes back to javafx by clicking on the title bar, the IME window is > indeed gets back to the expected state. @andy-goryachev-oracle You're right, I missed a detail and ended up testing something else. I did some more testing and it looks like the JavaFX behavior is consistent with other apps like TextEdit. If you move focus away from a window in the middle of IM composition the composed text is left underlined. If you restore focus to that window *before* typing anything the IM will pick up where it left off. But if you type something in another window before restoring focus the IM will commit the text the moment focus returns. This seems to be driven by the OS since TextEdit behaves the same way. - PR Comment: https://git.openjdk.org/jfx/pull/1356#issuecomment-1920268535
Re: RFR: 8320912: IME should commit on focus change
On Wed, 31 Jan 2024 20:55:16 GMT, Martin Fox wrote: > e.g. the IM window should appear showing the same candidates it was showing > earlier. Thank you for clarification! The behavior is different, let me explain: using the MonkeyTester, open TextField page and the Native-to-ascii tool (Tools -> Native-to-ascii). 1. switch to Japanese input 2. type "su" into the text field in the main window 3. click on a "native" text area in the other window 4. type "zu" in the native window 5. **unexpectedly**, the text field in the main window shows the symbol still underlined 6. click on the main window title to focus back on the original text field. type "zuki" EXPECTED: - the text field should contain 鈴木 ACTUAL: - the text is すずき, the last two symbols underlined:  It looks like the text was indeed committed (despite it being displayed underlined at step 5), and getting back to it from another window does not restore the IME to the state expected at the step 2. Interestingly, if instead of going to a different window of the same javafx application, the user clicks elsewhere on some other application window and then goes back to javafx by clicking on the title bar, the IME window is indeed gets back to the expected state. - PR Comment: https://git.openjdk.org/jfx/pull/1356#issuecomment-1920033399
Re: RFR: 8320912: IME should commit on focus change
On Wed, 31 Jan 2024 20:07:05 GMT, Andy Goryachev wrote: > Another problem I see, and it might be a totally separate issue, is that > sometimes when I launch the Monkey Tester and switch to Japanese input, > TextField is still receives US input, despite the IME window appearing. That behavior happens any time the focus is on anything other than a TextInput control and you start using an IM. Glass is told that the IM should be disabled but it still sends events to the input context. That's a long standing bug. The fix for that is part of PR #1351 (in keyDown: I check the IM enabled state before sending key events to the input context). - PR Comment: https://git.openjdk.org/jfx/pull/1356#issuecomment-1919949874
Re: RFR: 8320912: IME should commit on focus change
On Wed, 31 Jan 2024 18:44:55 GMT, Andy Goryachev wrote: >notice how the underline under あ is still there in the first text field. That behavior surprised me the first time I saw it but I eventually figured out that it's a feature, not a bug. If you switch back to the original window by clicking on the title bar you should pick up composition exactly where you left off e.g. the IM window should appear showing the same candidates it was showing earlier. That's the way native Mac apps like TextEdit work (most of the time, every now and then clicking on the title bar commits the text). On a technical level the focusOwner for the Scene isn't changing so JavaFX doesn't think there's any reason to finish composition. And apparently the OS agrees and relies on Glass to retain enough state that it can pick up where it left off. - PR Comment: https://git.openjdk.org/jfx/pull/1356#issuecomment-1919942747
Re: RFR: 8320912: IME should commit on focus change
On Tue, 30 Jan 2024 20:32:36 GMT, Martin Fox wrote:
> This is a Mac only bug. If the user was in the middle of IM text composition
> and clicked on a different node the partially composed text was left in the
> old node and the IM window wasn't dismissed. This PR implements the existing
> finishInputMethodComposition call so it can commit the text and dismiss the
> IM window before focus moves away from the node where composition was taking
> place.
>
> This PR changes the implementation of `unmarkText` to match what we want and
> what Apple says it should do ("The text view should accept the marked text as
> if it had been inserted normally"). With that said I haven't found an IME
> that calls this routine.
Another problem I see, and it might be a totally separate issue, is that
sometimes when I launch the Monkey Tester and switch to Japanese input,
TextField is still receives US input, despite the IME window appearing.
When this happens, the IME window appears somewhere in a corner, in another
screen. It is also difficult to reproduce. I first thought it might be
related to
[JDK-8324666](https://bugs.openjdk.org/browse/JDK-8324666) JFXPanel: Japanese
IME window initially shown in the corner (Windows)
except the latter is a JFXPanel issue on Windows.
(it's also possibly I am doing something wrong, though I am doing a clean
gradle build, followed by a full refresh and rebuild in Eclipse)
-
PR Comment: https://git.openjdk.org/jfx/pull/1356#issuecomment-1919862497
Re: RFR: 8320912: IME should commit on focus change
On Tue, 30 Jan 2024 20:32:36 GMT, Martin Fox wrote:
> This is a Mac only bug. If the user was in the middle of IM text composition
> and clicked on a different node the partially composed text was left in the
> old node and the IM window wasn't dismissed. This PR implements the existing
> finishInputMethodComposition call so it can commit the text and dismiss the
> IM window before focus moves away from the node where composition was taking
> place.
>
> This PR changes the implementation of `unmarkText` to match what we want and
> what Apple says it should do ("The text view should accept the marked text as
> if it had been inserted normally"). With that said I haven't found an IME
> that calls this routine.
Noticed two issues:
1. using the MonkeyTester, open TextField page and the Native-to-ascii tool
(Tools -> Native-to-ascii).
2. switch to Japanese input
3. type "a" into the text field in the main window
4. click on a "native" text area in the other window
PROBLEM 1:
notice how the underline under あ is still there in the first text field. the
input might have been committed, because when I click back on the first text
field, the underline disappears. It works correctly when clicking on the
second text field *in the same* window (add
op.option(new TextField());
to TextFieldPage:111 in the Monkey Tester
https://github.com/andy-goryachev-oracle/MonkeyTest
-
PR Comment: https://git.openjdk.org/jfx/pull/1356#issuecomment-1919719343
RFR: 8320912: IME should commit on focus change
This is a Mac only bug. If the user was in the middle of IM text composition
and clicked on a different node the partially composed text was left in the old
node and the IM window wasn't dismissed. This PR implements the existing
finishInputMethodComposition call so it can commit the text and dismiss the IM
window before focus moves away from the node where composition was taking place.
This PR changes the implementation of `unmarkText` to match what we want and
what Apple says it should do ("The text view should accept the marked text as
if it had been inserted normally"). With that said I haven't found an IME that
calls this routine.
-
Commit messages:
- Proper IM window cleanup when focus moves from one node to another
Changes: https://git.openjdk.org/jfx/pull/1356/files
Webrev: https://webrevs.openjdk.org/?repo=jfx=1356=00
Issue: https://bugs.openjdk.org/browse/JDK-8320912
Stats: 40 lines in 4 files changed: 36 ins; 0 del; 4 mod
Patch: https://git.openjdk.org/jfx/pull/1356.diff
Fetch: git fetch https://git.openjdk.org/jfx.git pull/1356/head:pull/1356
PR: https://git.openjdk.org/jfx/pull/1356
