8301900: TextArea: Committing text with ENTER in an IME window inserts newline #1351

beldenfox · 2024-01-26T17:32:02Z

In the Mac glass code the presence of "marked" text (which is tracked in the nsAttrBuffer) signals that an IME is active. In this state the current code assumes that when NSTextInputContext handles a keyDown: it will either generate a call to insertText:replacementRange: or one of the routines that manipulates the marked (composed) text. But this bug shows that sometimes the IME acts on the event without generating any calls back into glass at all.

In this PR the logic is simplified: if the NSTextInputContext handles the keyDown: and there's marked (composed) text we don't generate a KeyEvent. Otherwise we do. This PR removes the shouldProcessKeyEvent flag since it no longer assumes we can catch callbacks to update it correctly.

The existing code also assumes that the composition phase ends when the NSTextInputContext calls insertText:replacementRange to commit the text. This is true but if the user tries to use a dead-key sequence to generate a non-existent character (like an accented 'q') the context will call insertText twice while handling one key down event. In that case we can't exit the composition mode upon seeing the first insertText call since it will cause us to mis-handle the second one. This PR defers exiting composition mode until the end of keyDown:.

I also updated a few deprecated constants so this file no longer generates compiler warnings.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed (2 reviews required, with at least 1 Reviewer, 1 Author)

Issues

JDK-8301900: TextArea: Committing text with ENTER in an IME window inserts newline (Bug - P4)
JDK-8088172: Mac: On German keyboard, pressing <+><q> inserts two apostrophes instead of one (Bug - P4)
JDK-8089803: [Mac, TextArea] Japanese IME, caret moves to the next line when pressing Return to select a candidate. (Bug - P4)

Reviewers

Kevin Rushforth (@kevinrushforth - Reviewer)
Andy Goryachev (@andy-goryachev-oracle - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jfx.git pull/1351/head:pull/1351
$ git checkout pull/1351

Update a local copy of the PR:
$ git checkout pull/1351
$ git pull https://git.openjdk.org/jfx.git pull/1351/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 1351

View PR using the GUI difftool:
$ git pr show -t 1351

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jfx/pull/1351.diff

Webrev

Link to Webrev Comment

bridgekeeper · 2024-01-26T17:32:19Z

👋 Welcome back mfox! 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.

beldenfox · 2024-01-26T17:33:37Z

/issue JDK-8088172

openjdk · 2024-01-26T17:34:15Z

@beldenfox
Adding additional issue to issue list: 8088172: Mac: On German keyboard, pressing <+><q> inserts two apostrophes instead of one.

mlbridge · 2024-01-26T17:37:47Z

Webrevs

beldenfox · 2024-01-26T17:46:10Z

/issue JDK-8089803

openjdk · 2024-01-26T17:46:35Z

@beldenfox
Adding additional issue to issue list: 8089803: [Mac, TextArea] Japanese IME, caret moves to the next line when pressing Return to select a candidate..

kevinrushforth · 2024-01-26T18:15:36Z

Reviewers: @andy-goryachev-oracle @kevinrushforth

/reviewers 2

openjdk · 2024-01-26T18:16:56Z

@kevinrushforth
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 1 Reviewer, 1 Author).

kevinrushforth

Code changes look good. All my testing looks good.

beldenfox · 2024-01-27T19:15:01Z

I need to amend this PR. If the IM is disabled in the middle of composition we don't dismiss the IM window. In the past that was sort of OK because we ignored the enabled stated and kept sending events to the NSTextInputContext. With this PR we honor the state and stop talking to the context so the IM window just freezes and never goes away. I'll be adding a line of code to ensure we get rid of the IM window when the enabled state changes (which we should have been doing all along).

This condition can arise if the user starts composing and then clicks on a control that doesn't accept IM input (like a button). It can also happen with JDK-8087477 which is a bug I'm still investigating.

andy-goryachev-oracle · 2024-01-29T19:02:07Z

@beldenfox
are you going to update this PR with a fix for dismissing the IME window you alluded in the previous comment?

I don't know whether it's the side effect of that scenario, or a new issue, but try this:

using a simple app with a TextArea with pre-existing text (I am using the MonkeyTester, but any simple app would do)
switch to japanese/romaji input
type in "ariga". you'll see the IME window with some candidates (for some reason, the IME window disappears on "ari" - I guess it's expected since the native TextEdit app exhibits the same behavior)
click elsewhere. the IME window disappears
PROBLEM 1: the inline candidate remains underlined
switch to German input
click on some other position in the text
type in + q (from JDK-8088172)
PROBLEM 2: the text gets inserted into a different position, replacing that underlined Japanese candidate created earlier

edit: some of the scenarios are described in https://bugs.openjdk.org/browse/JDK-8320912

andy-goryachev-oracle · 2024-01-29T19:04:10Z

... and scenarios described in JDK-8088172 and JDK-8089803 seem to have been fixed.

andy-goryachev-oracle · 2024-01-29T19:05:03Z

modules/javafx.graphics/src/main/native-glass/mac/GlassView3D.m

+        didCommitText = NO;
+        nsAttrBuffer = [nsAttrBuffer initWithString: @""];
+    }
+    else if (!inputContextHandledEvent || (nsAttrBuffer.length == 0)) {


} else if { ?

beldenfox · 2024-01-30T00:57:53Z

@andy-goryachev-oracle

@beldenfox are you going to update this PR with a fix for dismissing the IME window you alluded in the previous comment?

I did that with the most recent commit (Jan 27th).

click elsewhere. the IME window disappears

That indicates that you got my most recent commit, otherwise the IME window would not disappear immediately.

I'm really glad you caught this. The old code for disabling the IME just called [self unmarkText]. I added the code for dismissing the IME window but didn't notice that unmarkText doesn't seem to work. Currently it commits a zero-length string (which appears to be a no-op) and leaves the composition buffer untouched. That combination doesn't make much sense and doesn't match Apple's spec. I need to dig around in the code history to see why it's behaving this way. This also explains some of the bad behavior you noticed in JDK-8320912.

beldenfox · 2024-01-30T01:16:16Z

Currently it commits a zero-length string (which appears to be a no-op) and leaves the composition buffer untouched.

Correcting myself: it does clear the composition buffer. But the zero-length commit doesn't seem to be doing what it needs to.

beldenfox · 2024-01-30T15:01:42Z

@andy-goryachev-oracle The behavior you're seeing when clicking on another control is a manifestation of https://bugs.openjdk.org/browse/JDK-8320912. I put together a fix for that bug but decided not to roll it into this PR. If it would help testing I could do that.

The current code to disable the IME happens after focus has already moved to the node that doesn't want IME input. So the commit that's fired off by unmarkText targets the wrong node. The fix is to implement finishInputMethodComposition so we do the cleanup before the focus changes.

andy-goryachev-oracle · 2024-01-30T15:56:04Z

JDK-8320912. I put together a fix for that bug but decided not to roll it into this PR.

Are these two bugs independent? Should JDK-8320912 be resolved first?

beldenfox · 2024-01-30T17:21:37Z

JDK-8320912. I put together a fix for that bug but decided not to roll it into this PR.

Are these two bugs independent? Should JDK-8320912 be resolved first?

They are independent. This bug has to do with how we divide keystrokes between KeyEvents and InputMethodEvents. JDK-8320912 is about cleaning up during focus changes. The order in which they get resolved doesn't matter.

andy-goryachev-oracle · 2024-01-30T17:33:06Z

The order in which they get resolved doesn't matter.

would it be possible to get JDK-8320912 integrated first then? It seems the scenario it addresses is narrower, and it will be easier to review this bug.

beldenfox · 2024-01-30T20:43:33Z

@andy-goryachev-oracle I submitted PR #1356.

andy-goryachev-oracle · 2024-01-30T20:44:20Z

I submitted PR

thank you!

kevinrushforth · 2024-02-02T16:26:39Z

Once #1356 is integrated I'll re-review.

andy-goryachev-oracle · 2024-02-02T17:10:48Z

@beldenfox could you sync up with the latest master branch please?
Eclipse reports a conflict in GlassView3D.m around lines 722, I am surprised there are no conflicts in github...

andy-goryachev-oracle · 2024-02-02T17:12:47Z

Even though it says "This branch has conflicts that must be resolved" there is a green checkmark next to it in the Pull Requests view:

https://github.com/openjdk/jfx/pulls?q=is%3Apr+is%3Aopen+sort%3Aupdated-desc+label%3Arfr

kevinrushforth · 2024-02-02T17:27:42Z

I'm surprised Skara hasn't noticed that there is a merge conflict -- when it does, it should add a merge-conflict label. Maybe it needs something other than a PR comment to wake it up?

beldenfox · 2024-02-02T17:47:25Z

Skara has noted the merge conflict. I'll get that patched up. I also need to tweak the implementation of setEnabled, it's trying to handle focus moving from node to node but the logic is wrong.

kevinrushforth · 2024-02-02T17:49:27Z

Skara hasn't labeled it, so I'm restating the requirement of 2 reviewers to see if that wakes up the bot to check for the merge conflict.

/reviewers 2

openjdk · 2024-02-02T17:49:54Z

@kevinrushforth
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 1 Reviewer, 1 Author).

openjdk · 2024-02-02T17:50:17Z

@beldenfox this pull request can not be integrated into master due to one or more merge conflicts. To resolve these merge conflicts and update this pull request you can run the following commands in the local repository for your personal fork:

git checkout macimefixes
git fetch https://git.openjdk.org/jfx.git master
git merge FETCH_HEAD
# resolve conflicts and follow the instructions given by git merge
git commit -m "Merge master"
git push

beldenfox · 2024-02-02T18:47:52Z

Two additions to the code. I was not handling the case where the keystroke didn't commit text but did clear the marked text (getting us out of composition mode). You can do this by repeatedly pressing ESC.

When the enabled state changes we shouldn't have any marked text but if we do it should just be discarded. The old code was trying to deal with focusOwner changes but that cleanup has to happen before the enabled state changes instead of after (which is why finishInputMethodComposition exists).

andy-goryachev-oracle

As far as I can tell, the new behavior matches the native app one.

有難う

openjdk · 2024-02-23T19:44:28Z

@beldenfox 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:

8301900: TextArea: Committing text with ENTER in an IME window inserts newline
8088172: Mac: On German keyboard, pressing <+><q> inserts two apostrophes instead of one
8089803: [Mac, TextArea] Japanese IME, caret moves to the next line when pressing Return to select a candidate.

Reviewed-by: kcr, angorya

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 14 new commits pushed to the master branch:

afa206b: 8321603: Bump minimum JDK version for JavaFX to JDK 21
e0b88bc: 8278021: Fix warnings in macOS glass native code and treat warnings as errors
9a06bf9: 8322748: Caret blinking in JavaFX should only stop when caret moves
ee8633c: 8089373: Translation from character to key code is not sufficient
1fb56e3: 8323880: Caret rendered at wrong position in case of a click event on RTL text
2754939: 8309374: Accessibility Focus Rectangle on ListItem is not drawn when ListView is shown for first time
48be7d3: 8324182: Deprecate for removal SimpleSelector and CompoundSelector classes
49d7d52: 8325798: Spinner throws uncatchable exception on tab out from garbled text
f379eae: 8325873: Update JDK_DOCS property to point to JDK 21 docs
e2f42c5: 8325154: resizeColumnToFitContent is slower than it needs to be
... and 4 more: https://git.openjdk.org/jfx/compare/17dfab066b1f5db3d34b130cade3acd09ee21a70...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.

beldenfox · 2024-02-23T22:38:13Z

/integrate

openjdk · 2024-02-23T22:39:58Z

Going to push as commit d9263ab.
Since your change was applied there have been 14 commits pushed to the master branch:

afa206b: 8321603: Bump minimum JDK version for JavaFX to JDK 21
e0b88bc: 8278021: Fix warnings in macOS glass native code and treat warnings as errors
9a06bf9: 8322748: Caret blinking in JavaFX should only stop when caret moves
ee8633c: 8089373: Translation from character to key code is not sufficient
1fb56e3: 8323880: Caret rendered at wrong position in case of a click event on RTL text
2754939: 8309374: Accessibility Focus Rectangle on ListItem is not drawn when ListView is shown for first time
48be7d3: 8324182: Deprecate for removal SimpleSelector and CompoundSelector classes
49d7d52: 8325798: Spinner throws uncatchable exception on tab out from garbled text
f379eae: 8325873: Update JDK_DOCS property to point to JDK 21 docs
e2f42c5: 8325154: resizeColumnToFitContent is slower than it needs to be
... and 4 more: https://git.openjdk.org/jfx/compare/17dfab066b1f5db3d34b130cade3acd09ee21a70...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-02-23T22:40:07Z

@beldenfox Pushed as commit d9263ab.

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

beldenfox added 4 commits January 19, 2024 07:26

Modified method of determing which keyDowns are sent as KeyEvents.

c06de95

Merge remote-tracking branch 'upstream/master' into macimefixes

6a1b33d

Comment tweaks

6ea5c81

Merge remote-tracking branch 'upstream/master' into macimefixes

c47a3b4

openjdk bot added the rfr Ready for review label Jan 26, 2024

kevinrushforth self-requested a review January 26, 2024 18:15

kevinrushforth mentioned this pull request Jan 26, 2024

8278021: Fix warnings in macOS glass native code and treat warnings as errors #687

Closed

3 tasks

kevinrushforth approved these changes Jan 27, 2024

View reviewed changes

When IM enabled state changes we dismiss the IM window.

4909848

andy-goryachev-oracle reviewed Jan 29, 2024

View reviewed changes

beldenfox mentioned this pull request Jan 31, 2024

8320912: IME should commit on focus change #1356

Closed

3 tasks

kevinrushforth self-requested a review February 2, 2024 13:51

kevinrushforth requested a review from andy-goryachev-oracle February 2, 2024 16:26

openjdk bot added the merge-conflict Pull request has merge conflict with target branch label Feb 2, 2024

beldenfox added 2 commits February 2, 2024 09:59

Merge remote-tracking branch 'upstream/master' into macimefixes

fb29ffd

Now handling ESC ending the composition state. Minor setEnabled cleanup.

e654db7

openjdk bot removed the merge-conflict Pull request has merge conflict with target branch label Feb 2, 2024

andy-goryachev-oracle approved these changes Feb 6, 2024

View reviewed changes

kevinrushforth approved these changes Feb 23, 2024

View reviewed changes

openjdk bot added the ready Ready to be integrated label Feb 23, 2024

openjdk bot added the integrated Pull request has been integrated label Feb 23, 2024

openjdk bot closed this Feb 23, 2024

openjdk bot removed ready Ready to be integrated rfr Ready for review labels Feb 23, 2024

beldenfox deleted the macimefixes branch July 23, 2024 18:58

8301900: TextArea: Committing text with ENTER in an IME window inserts newline #1351

8301900: TextArea: Committing text with ENTER in an IME window inserts newline #1351

Uh oh!

Conversation

beldenfox commented Jan 26, 2024 • edited by openjdk bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Progress

Issues

Reviewers

Reviewing

Webrev

Uh oh!

bridgekeeper bot commented Jan 26, 2024

Uh oh!

beldenfox commented Jan 26, 2024

Uh oh!

openjdk bot commented Jan 26, 2024

Uh oh!

mlbridge bot commented Jan 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Webrevs

Uh oh!

beldenfox commented Jan 26, 2024

Uh oh!

openjdk bot commented Jan 26, 2024

Uh oh!

kevinrushforth commented Jan 26, 2024

Uh oh!

openjdk bot commented Jan 26, 2024

Uh oh!

kevinrushforth left a comment

Choose a reason for hiding this comment

Uh oh!

beldenfox commented Jan 27, 2024

Uh oh!

andy-goryachev-oracle commented Jan 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

andy-goryachev-oracle commented Jan 29, 2024

Uh oh!

andy-goryachev-oracle Jan 29, 2024

Choose a reason for hiding this comment

Uh oh!

beldenfox commented Jan 30, 2024

Uh oh!

beldenfox commented Jan 30, 2024

Uh oh!

beldenfox commented Jan 30, 2024

Uh oh!

andy-goryachev-oracle commented Jan 30, 2024

Uh oh!

beldenfox commented Jan 30, 2024

Uh oh!

andy-goryachev-oracle commented Jan 30, 2024

Uh oh!

beldenfox commented Jan 30, 2024

Uh oh!

andy-goryachev-oracle commented Jan 30, 2024

Uh oh!

kevinrushforth commented Feb 2, 2024

Uh oh!

andy-goryachev-oracle commented Feb 2, 2024

Uh oh!

andy-goryachev-oracle commented Feb 2, 2024

Uh oh!

kevinrushforth commented Feb 2, 2024

Uh oh!

beldenfox commented Feb 2, 2024

Uh oh!

kevinrushforth commented Feb 2, 2024

Uh oh!

openjdk bot commented Feb 2, 2024

Uh oh!

openjdk bot commented Feb 2, 2024

Uh oh!

beldenfox commented Feb 2, 2024

Uh oh!

andy-goryachev-oracle left a comment

Choose a reason for hiding this comment

Uh oh!

beldenfox commented Jan 26, 2024 •

edited by openjdk bot

Loading

mlbridge bot commented Jan 26, 2024 •

edited

Loading

andy-goryachev-oracle commented Jan 29, 2024 •

edited

Loading