Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8258805: Japanese characters not entered by mouse click on Windows 10 #2142

Closed
wants to merge 2 commits into from

Conversation

@dmarkov20
Copy link
Member

@dmarkov20 dmarkov20 commented Jan 19, 2021

Problem description:
The IME behaviour has changed starting from recent Windows 10 builds. In particular if the complex characters (Japanese, Chinese, etc.) are entered to some component and the focus is transferred to another component (which does not support the IM) the IM is disabled and the unconfirmed composition string gets discarded.
On previous Windows versions in the same situation the composition string is not discarded.

Fix:
It is necessary to take care of unconfirmed composition string once the IME is going to be disabled.

Testing:
All our automated regression and JCK tests passed with the proposed change.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8258805: Japanese characters not entered by mouse click on Windows 10

Reviewers

Download

$ git fetch https://git.openjdk.java.net/jdk pull/2142/head:pull/2142
$ git checkout pull/2142

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Jan 19, 2021

👋 Welcome back dmarkov! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot added the rfr label Jan 19, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jan 19, 2021

@dmarkov20 The following labels will be automatically applied to this pull request:

  • awt
  • i18n

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

@mlbridge
Copy link

@mlbridge mlbridge bot commented Jan 19, 2021

Webrevs

@mlbridge
Copy link

@mlbridge mlbridge bot commented Jan 19, 2021

Mailing list message from Ichiroh Takiguchi on i18n-dev:

Hello Dmitry.

The bugid seems to be private, so I don't know the details.
I think the current code can change the candidate string after getting
the focus.
If possible, could you show me the test instructions ?

Thanks,
Ichiroh Takiguchi

On 2021-01-19 20:16, Dmitry Markov wrote:

Problem description:
The IME behaviour has changed starting from recent Windows 10 builds.
In particular if the complex characters (Japanese, Chinese, etc.) are
entered to some component and the focus is transferred to another
component (which does not support the IM) the IM is disabled and the
unconfirmed composition string gets discarded.
On previous Windows versions in the same situation the composition
string is not discarded.

Fix:
It is necessary to take care of unconfirmed composition string once
the IME is going to be disabled.

Testing:
All our automated regression and JCK tests passed with the proposed
change.

-------------

Commit messages:
- 8258805: Japanese characters not entered by mouse click on Windows
10

Changes:
https://urldefense.proofpoint.com/v2/url?u=https-3A__git.openjdk.java.net_jdk_pull_2142_files&d=DwIBaQ&c=jf_iaSHvJObTbx-siA1ZOg&r=TWSRKjAEFvjdtcldX_CzGUCRvo92UQrQaevJPjdE9G0&m=pSPWtUQC712ERQcCWzl97FRkUYd8Rgu1S4ejmCvqfho&s=tQjpctdFBD-VNGT6Sz99HLO87e4Il0g5UPyncfk05xQ&e=
Webrev:
https://urldefense.proofpoint.com/v2/url?u=https-3A__webrevs.openjdk.java.net_-3Frepo-3Djdk-26pr-3D2142-26range-3D00&d=DwIBaQ&c=jf_iaSHvJObTbx-siA1ZOg&r=TWSRKjAEFvjdtcldX_CzGUCRvo92UQrQaevJPjdE9G0&m=pSPWtUQC712ERQcCWzl97FRkUYd8Rgu1S4ejmCvqfho&s=uS7Zv56IELOfi9JydPxkC8cn11F2sm2rQIzygpVRKGU&e=
Issue:
https://urldefense.proofpoint.com/v2/url?u=https-3A__bugs.openjdk.java.net_browse_JDK-2D8258805&d=DwIBaQ&c=jf_iaSHvJObTbx-siA1ZOg&r=TWSRKjAEFvjdtcldX_CzGUCRvo92UQrQaevJPjdE9G0&m=pSPWtUQC712ERQcCWzl97FRkUYd8Rgu1S4ejmCvqfho&s=2ceq871Y2SbUmo-W1IRN4YDFPxeXMq1Y1GSY4YgkyNY&e=
Stats: 6 lines in 1 file changed: 5 ins; 0 del; 1 mod
Patch:
https://urldefense.proofpoint.com/v2/url?u=https-3A__git.openjdk.java.net_jdk_pull_2142.diff&d=DwIBaQ&c=jf_iaSHvJObTbx-siA1ZOg&r=TWSRKjAEFvjdtcldX_CzGUCRvo92UQrQaevJPjdE9G0&m=pSPWtUQC712ERQcCWzl97FRkUYd8Rgu1S4ejmCvqfho&s=V5Hl37kt-btSEbTU2y1pD1-_e-z1N_QDDSl2tRIc6HI&e=
Fetch: git fetch
https://urldefense.proofpoint.com/v2/url?u=https-3A__git.openjdk.java.net_jdk&d=DwIBaQ&c=jf_iaSHvJObTbx-siA1ZOg&r=TWSRKjAEFvjdtcldX_CzGUCRvo92UQrQaevJPjdE9G0&m=pSPWtUQC712ERQcCWzl97FRkUYd8Rgu1S4ejmCvqfho&s=v3cOrMgmRWxELPv_w6aY_bwvvM6_qqYJY5yaIW6tADY&e=
pull/2142/head:pull/2142

PR:
https://urldefense.proofpoint.com/v2/url?u=https-3A__git.openjdk.java.net_jdk_pull_2142&d=DwIBaQ&c=jf_iaSHvJObTbx-siA1ZOg&r=TWSRKjAEFvjdtcldX_CzGUCRvo92UQrQaevJPjdE9G0&m=pSPWtUQC712ERQcCWzl97FRkUYd8Rgu1S4ejmCvqfho&s=XG8QFzRvrJv3WP3HW32IfaG7xG8WqxAlQkk2Fp3u708&e=

@mlbridge
Copy link

@mlbridge mlbridge bot commented Jan 19, 2021

Mailing list message from Dmitry Markov on i18n-dev:

Hi Ichiron,

I have updated the bug. Now the test, steps to reproduce and other details are available. Please let me know if you need anything else.

Regards,
Dmitry

On 19 Jan 2021, at 11:44, Ichiroh Takiguchi <takiguc at linux.vnet.ibm.com> wrote:

Hello Dmitry.

The bugid seems to be private, so I don't know the details.
I think the current code can change the candidate string after getting the focus.
If possible, could you show me the test instructions ?

Thanks,
Ichiroh Takiguchi

On 2021-01-19 20:16, Dmitry Markov wrote:

Problem description:
The IME behaviour has changed starting from recent Windows 10 builds.
In particular if the complex characters (Japanese, Chinese, etc.) are
entered to some component and the focus is transferred to another
component (which does not support the IM) the IM is disabled and the
unconfirmed composition string gets discarded.
On previous Windows versions in the same situation the composition
string is not discarded.
Fix:
It is necessary to take care of unconfirmed composition string once
the IME is going to be disabled.
Testing:
All our automated regression and JCK tests passed with the proposed change.
-------------
Commit messages:
- 8258805: Japanese characters not entered by mouse click on Windows 10
Changes:
https://urldefense.proofpoint.com/v2/url?u=https-3A__git.openjdk.java.net_jdk_pull_2142_files&d=DwIBaQ&c=jf_iaSHvJObTbx-siA1ZOg&r=TWSRKjAEFvjdtcldX_CzGUCRvo92UQrQaevJPjdE9G0&m=pSPWtUQC712ERQcCWzl97FRkUYd8Rgu1S4ejmCvqfho&s=tQjpctdFBD-VNGT6Sz99HLO87e4Il0g5UPyncfk05xQ&e=
Webrev:
https://urldefense.proofpoint.com/v2/url?u=https-3A__webrevs.openjdk.java.net_-3Frepo-3Djdk-26pr-3D2142-26range-3D00&d=DwIBaQ&c=jf_iaSHvJObTbx-siA1ZOg&r=TWSRKjAEFvjdtcldX_CzGUCRvo92UQrQaevJPjdE9G0&m=pSPWtUQC712ERQcCWzl97FRkUYd8Rgu1S4ejmCvqfho&s=uS7Zv56IELOfi9JydPxkC8cn11F2sm2rQIzygpVRKGU&e=
Issue:
https://urldefense.proofpoint.com/v2/url?u=https-3A__bugs.openjdk.java.net_browse_JDK-2D8258805&d=DwIBaQ&c=jf_iaSHvJObTbx-siA1ZOg&r=TWSRKjAEFvjdtcldX_CzGUCRvo92UQrQaevJPjdE9G0&m=pSPWtUQC712ERQcCWzl97FRkUYd8Rgu1S4ejmCvqfho&s=2ceq871Y2SbUmo-W1IRN4YDFPxeXMq1Y1GSY4YgkyNY&e=
Stats: 6 lines in 1 file changed: 5 ins; 0 del; 1 mod
Patch:
https://urldefense.proofpoint.com/v2/url?u=https-3A__git.openjdk.java.net_jdk_pull_2142.diff&d=DwIBaQ&c=jf_iaSHvJObTbx-siA1ZOg&r=TWSRKjAEFvjdtcldX_CzGUCRvo92UQrQaevJPjdE9G0&m=pSPWtUQC712ERQcCWzl97FRkUYd8Rgu1S4ejmCvqfho&s=V5Hl37kt-btSEbTU2y1pD1-_e-z1N_QDDSl2tRIc6HI&e=
Fetch: git fetch
https://urldefense.proofpoint.com/v2/url?u=https-3A__git.openjdk.java.net_jdk&d=DwIBaQ&c=jf_iaSHvJObTbx-siA1ZOg&r=TWSRKjAEFvjdtcldX_CzGUCRvo92UQrQaevJPjdE9G0&m=pSPWtUQC712ERQcCWzl97FRkUYd8Rgu1S4ejmCvqfho&s=v3cOrMgmRWxELPv_w6aY_bwvvM6_qqYJY5yaIW6tADY&e=
pull/2142/head:pull/2142
PR:
https://urldefense.proofpoint.com/v2/url?u=https-3A__git.openjdk.java.net_jdk_pull_2142&d=DwIBaQ&c=jf_iaSHvJObTbx-siA1ZOg&r=TWSRKjAEFvjdtcldX_CzGUCRvo92UQrQaevJPjdE9G0&m=pSPWtUQC712ERQcCWzl97FRkUYd8Rgu1S4ejmCvqfho&s=XG8QFzRvrJv3WP3HW32IfaG7xG8WqxAlQkk2Fp3u708&e=

@aivanov-jdk
Copy link
Member

@aivanov-jdk aivanov-jdk commented Jan 19, 2021

Fix:
It is necessary to take care of unconfirmed composition string once the IME is going to be disabled.

The fix commits the unconfirmed composition string. Committing is better than discarding. Is it possible to preserve the state and to leave the string uncommitted?

@dmarkov20
Copy link
Member Author

@dmarkov20 dmarkov20 commented Jan 19, 2021

Fix:
It is necessary to take care of unconfirmed composition string once the IME is going to be disabled.

The fix commits the unconfirmed composition string. Committing is better than discarding. Is it possible to preserve the
state and to leave the string uncommitted?

The fix reverts the previous (correct) behaviour back. It is unnecessary to store the state and keep the string. That activity may be quite complicated and requires additional resources especially if there are several components with active IME

@takiguc
Copy link

@takiguc takiguc commented Jan 19, 2021

Hello Dmitry.
Sorry, I should use GitHub instead of mailing list.
I tested your fix, I saw side effect by committing preedit string.
I'll give you detail information.

@takiguc
Copy link

@takiguc takiguc commented Jan 20, 2021

I tried attached testcase.

import java.awt.*;
import java.awt.event.*;

public class AWTTextTest1 extends Frame {
    AWTTextTest1() {
        setTitle("AWTTextTest1");
        setLayout(new GridLayout(0, 1));
        add(new TextField());
        add(new TextField());
        addWindowListener(new WindowAdapter() {
            public void windowClosing(WindowEvent we) {
                System.exit(0);
            }
        });
        setSize(400, 300);
        setVisible(true);
    }
    public static void main(String[] args) {
        new AWTTextTest1();
    }
}
  1. Start testcase
  2. Click upper TextField, turn on IME, type some character (In case of Japanese, type "aiu")
  3. Click lower TextField, preedit string is committed into lower TextField.

Without the fix, preedit string was canceled by above operation.

@mrserb
Copy link
Member

@mrserb mrserb commented Jan 20, 2021

How do the native components work in that case like awt textarea or external apps like notepad?

@takiguc
Copy link

@takiguc takiguc commented Jan 20, 2021

How do the native components work in that case like awt textarea or external apps like notepad?

I tested TextField+TextField, TextArea+TextArea, TextArea+TextField.
Without fix: Preedit string was cancel by input focus change.
With fix: Preedit string was committed into clicked component.

For external application, I used Sakura Editor, because it can create 2 editor panes on one window.
Preedit string was committed before moving input focus.

@dmarkov20
Copy link
Member Author

@dmarkov20 dmarkov20 commented Jan 20, 2021

I tried attached testcase.

import java.awt.*;
import java.awt.event.*;

public class AWTTextTest1 extends Frame {
    AWTTextTest1() {
        setTitle("AWTTextTest1");
        setLayout(new GridLayout(0, 1));
        add(new TextField());
        add(new TextField());
        addWindowListener(new WindowAdapter() {
            public void windowClosing(WindowEvent we) {
                System.exit(0);
            }
        });
        setSize(400, 300);
        setVisible(true);
    }
    public static void main(String[] args) {
        new AWTTextTest1();
    }
}
  1. Start testcase
  2. Click upper TextField, turn on IME, type some character (In case of Japanese, type "aiu")
  3. Click lower TextField, preedit string is committed into lower TextField.

Without the fix, preedit string was canceled by above operation.

Hello Ichiroh,

Thank you for the details. I can reproduce it. I will look into this..

Regards,
Dmitry

@aivanov-jdk
Copy link
Member

@aivanov-jdk aivanov-jdk commented Jan 20, 2021

The fix commits the unconfirmed composition string. Committing is better than discarding. Is it possible to preserve the
state and to leave the string uncommitted?

The fix reverts the previous (correct) behaviour back.

According to the bug description, it used to stay in uncommitted state.

It is unnecessary to store the state and keep the string. That activity may be quite complicated and requires additional resources especially if there are several components with active IME

I definitely agree that committing the text is better behaviour than discarding uncommitted text.

I am for accepting these changes unless we can keep the text uncommitted and allow the user to work with text when the focus moves back to the text component.

@dmarkov20
Copy link
Member Author

@dmarkov20 dmarkov20 commented Jan 21, 2021

An interesting thing is that the test (see AWTTextTest1.java) behaves like a native application if TextField is replaced with JTextField. It appears AWT and Swing components behave differently while processing input method requests. That’s a great topic to discuss but it is out of scope for this PR. We can open a separate bug for it, if any.

Going back to the current PR. I updated the fix. Now we will commit the composition string if there is an active client. That changes eliminates the issue described in JDK-8258805. Also the behaviour of AWTTextTest1 is the same as it used to be on the previous versions w/o the fix.

@aivanov-jdk
Copy link
Member

@aivanov-jdk aivanov-jdk commented Jan 21, 2021

Now we will commit the composition string if there is an active client. That changes eliminates the issue described in JDK-8258805. Also the behaviour of AWTTextTest1 is the same as it used to be on the previous versions w/o the fix.

Just to confirm: the updated behaviour is similar to what was happening in previous versions of Windows 10, that is if a compositing text isn't committed, it remains uncommitted in the text component when the focus changes instead of being committed as it was the case in the previous iteration. Is my understanding correct?

@dmarkov20
Copy link
Member Author

@dmarkov20 dmarkov20 commented Jan 21, 2021

Now we will commit the composition string if there is an active client. That changes eliminates the issue described in JDK-8258805. Also the behaviour of AWTTextTest1 is the same as it used to be on the previous versions w/o the fix.

Just to confirm: the updated behaviour is similar to what was happening in previous versions of Windows 10, that is if a compositing text isn't committed, it remains uncommitted in the text component when the focus changes instead of being committed as it was the case in the previous iteration. Is my understanding correct?

I am sorry but you didn't get it right. The behaviour, which was introduced in previous iteration, is still in place. It is required to get rid of the problem described in JDK-8258805
The purpose of of the second iteration is to eliminate negative side effect (more details here #2142 (comment) ) introduced by the first version of the fix.

@aivanov-jdk
Copy link
Member

@aivanov-jdk aivanov-jdk commented Jan 21, 2021

Now we will commit the composition string if there is an active client. That changes eliminates the issue described in JDK-8258805. Also the behaviour of AWTTextTest1 is the same as it used to be on the previous versions w/o the fix.

Just to confirm: the updated behaviour is similar to what was happening in previous versions of Windows 10, that is if a compositing text isn't committed, it remains uncommitted in the text component when the focus changes instead of being committed as it was the case in the previous iteration. Is my understanding correct?

I am sorry but you didn't get it right. The behaviour, which was introduced in previous iteration, is still in place. It is required to get rid of the problem described in JDK-8258805
The purpose of of the second iteration is to eliminate negative side effect (more details here #2142 (comment) ) introduced by the first version of the fix.

I admit I am even more confused now. To me, the description in the comment above is nearly the same as in JBS comment. Is the difference that the original test case disabled IME for the middle JTextField whereas in the test case above all JTextField support IME?

In the updated version of the fix, we always commit the text on any focus change whether the newly focused component supports IME or not.

@dmarkov20
Copy link
Member Author

@dmarkov20 dmarkov20 commented Jan 21, 2021

I admit I am even more confused now. To me, the description in the comment above is nearly the same as in JBS comment. Is the difference that the original test case disabled IME for the middle JTextField whereas in the test case above all JTextField support IME?

Well.. I think the main difference between tests is that the test attached to the bug uses JTextField (Swing) and the test provided above uses TextField (AWT). The same input method events are processed differently for Swing and AWT text components. Good example is the following test:

import java.awt.*;
import java.awt.event.*;

public class AWTTextTest1 extends Frame {
    AWTTextTest1() {
        setTitle("AWTTextTest1");
        setLayout(new GridLayout(0, 1));
        add(new TextField());
        add(new TextField());
        addWindowListener(new WindowAdapter() {
            public void windowClosing(WindowEvent we) {
                System.exit(0);
            }
        });
        setSize(400, 300);
        setVisible(true);
    }
    public static void main(String[] args) {
        new AWTTextTest1();
    }
}
  1. Run test (originally it uses TextField)
  2. Click upper TextField, turn on IME, type some character (In case of Japanese, type "aiu")
  3. Click lower TextField, the string is canceled.
  4. Replace TextField with JTextField in the test. Compile and run it again.
  5. Click upper JTextField, turn on IME, type some character (In case of Japanese, type "aiu")
  6. Click lower JTextField, the string is committed before focus transition.

In the updated version of the fix, we always commit the text on any focus change whether the newly focused component supports IME or not.

That’s not quite right. Actually we commit the text if the current IM client is “active client”. For example, JTextField is an “active client” but TextField - NOT. The status “active client” depends on the implementation of getInputMethodRequests() method.

@naotoj
Copy link
Member

@naotoj naotoj commented Jan 21, 2021

Hi,

AWT's TextComponent is a peered input client, and Swing's JTextComponent is an active input client. Thus it is OK to behave differently. I would expect that AWT's one behaves as the same as native Windows apps, and Swing's one should commit text into the component that has lost the focus.

@openjdk
Copy link

@openjdk openjdk bot commented Jan 21, 2021

@dmarkov20 This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8258805: Japanese characters not entered by mouse click on Windows 10

Reviewed-by: aivanov

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 49 new commits pushed to the master branch:

  • a8073ef: 8253478: (se) epoll Selector should use eventfd for wakeup instead of pipe
  • 34eb8b3: 8255765: Shenandoah: Isolate concurrent, degenerated and full GC
  • c3c6662: 8259954: gc/shenandoah/mxbeans tests fail with -Xcomp
  • 6ce0799: 8259851: Use boolean type for tasks in SubTasksDone
  • 4bcffeb: 8260029: aarch64: fix typo in verify_oop_array
  • e1de0bf: 8260043: Reduce allocation in sun.net.www.protocol.jar.Handler.parseURL
  • 4dfd8cc: 8259897: gtest os.dll_address_to_function_and_library_name_vm fails on AIX
  • 7f7166d: 8260035: Deproblemlist few problemlisted test
  • 5940287: 8260048: Shenandoah: ShenandoahMarkingContext asserts are unnecessary
  • f8a9602: 8260025: Missing comma in VM_Version_Ext::_family_id_amd
  • ... and 39 more: https://git.openjdk.java.net/jdk/compare/6b4732fe519a502d88b977fe293299998cc1fadc...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready label Jan 21, 2021
@dmarkov20
Copy link
Member Author

@dmarkov20 dmarkov20 commented Jan 21, 2021

Hi,

AWT's TextComponent is a peered input client, and Swing's JTextComponent is an active input client. Thus it is OK to behave differently. I would expect that AWT's one behaves as the same as native Windows apps, and Swing's one should commit text into the component that has lost the focus.

Thank you for confirmation! With the fix we have the same behaviour as you described.

@takiguc
Copy link

@takiguc takiguc commented Jan 22, 2021

Thanks, Dmitry.
I tested your new fix.
It's fine now on Swing and AWT.

@dmarkov20
Copy link
Member Author

@dmarkov20 dmarkov20 commented Jan 22, 2021

/integrate

@openjdk openjdk bot closed this Jan 22, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels Jan 22, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jan 22, 2021

@dmarkov20 Since your change was applied there have been 69 commits pushed to the master branch:

  • a887177: 8246788: ZoneRules invariants can be broken
  • 874aef4: 8259707: LDAP channel binding does not work with StartTLS extension
  • c5ad713: 8260250: Duplicate check in DebugInformationRecorder::recorders_frozen
  • bf5e801: 8259922: MethodHandles.collectArguments does not throw IAE if pos is outside the arity range
  • 0ea5862: 8260053: Optimize Tokens' use of Names
  • 18eb6d9: 8255348: NPE in PKIXCertPathValidator event logging code
  • a97f3c1: 8258853: Support separate function declaration and definition with ENABLE_IF-based SFINAE
  • 154e1d6: 8259009: G1 heap summary should be shown in "Heap Parameters" window on HSDB
  • acbcde8: 8256111: Create implementation for NSAccessibilityStaticText protocol
  • f928265: 8260009: InstanceKlass::has_as_permitted_subclass() fails if subclass was redefined
  • ... and 59 more: https://git.openjdk.java.net/jdk/compare/6b4732fe519a502d88b977fe293299998cc1fadc...master

Your commit was automatically rebased without conflicts.

Pushed as commit 53fecba.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

@dmarkov20 dmarkov20 deleted the JDK-8258805-fix branch Jan 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
5 participants